net/iavf: check PTP capabilities during init

Message ID 20251121-jk-dpdk-iavf-rx-timestamping-fix-v1-1-21c9a337a6f2@intel.com (mailing list archive)
State New
Delegated to: Bruce Richardson
Headers
Series net/iavf: check PTP capabilities during init |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-unit-arm64-testing pending Testing pending
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/aws-unit-testing success Unit Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-intel-Functional fail Functional Testing issues
ci/iol-mellanox-Functional success Functional Testing PASS

Commit Message

Keller, Jacob E Nov. 21, 2025, 11:39 p.m. UTC
Commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
added a check against whether the PF has actually enabled Rx
timestamping in iavf_dev_info_get(). Unfortunately, this function may be
called prior to the PTP capabilities being exchanged, which results in
Rx timestamping not being supported.

Fix this by checking the PF PTP capabilities near the end of
iavf_dev_init(). This ensures the VF knows the capabilities at the point
where the iavf_dev_info_get() function can be called. Doing the check at
init is better than inside the info callback, as the info callback is
called many times.

The capability exchange in iavf_dev_start() is kept to ensure that
capabilities are updated after resets.

Fixes: d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
My recent fix to prevent enabling Rx timestamping on PFs which do support
PTP capability but do not report Rx timestamping accidentally broke PFs
which *do* support Rx timestamping. This is because the driver did not
exchange capability before reporting its device info. Fix this by checking
PF capabilities during iavf_dev_init().
---
 drivers/net/intel/iavf/iavf_ethdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)


---
base-commit: ef98b88455bf4a7c8b7aa3106a761c9e9270d6a3
change-id: 20251121-jk-dpdk-iavf-rx-timestamping-fix-abdcb42f0197

Best regards,
--  
Jacob Keller <jacob.e.keller@intel.com>
  

Comments

Bruce Richardson Nov. 24, 2025, 12:09 p.m. UTC | #1
On Fri, Nov 21, 2025 at 03:39:37PM -0800, Jacob Keller wrote:
> Commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
> added a check against whether the PF has actually enabled Rx
> timestamping in iavf_dev_info_get(). Unfortunately, this function may be
> called prior to the PTP capabilities being exchanged, which results in
> Rx timestamping not being supported.
> 
> Fix this by checking the PF PTP capabilities near the end of
> iavf_dev_init(). This ensures the VF knows the capabilities at the point
> where the iavf_dev_info_get() function can be called. Doing the check at
> init is better than inside the info callback, as the info callback is
> called many times.
> 
> The capability exchange in iavf_dev_start() is kept to ensure that
> capabilities are updated after resets.
> 
> Fixes: d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> My recent fix to prevent enabling Rx timestamping on PFs which do support
> PTP capability but do not report Rx timestamping accidentally broke PFs
> which *do* support Rx timestamping. This is because the driver did not
> exchange capability before reporting its device info. Fix this by checking
> PF capabilities during iavf_dev_init().
> ---
>  drivers/net/intel/iavf/iavf_ethdev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 3ef766de4704..9b07b11a6b51 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -2887,6 +2887,14 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>  		}
>  	}
>  
> +	/* Get PTP caps early to verify device capabilities */
> +	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) {
> +		if (iavf_get_ptp_cap(adapter)) {
> +			PMD_INIT_LOG(ERR, "Failed to get ptp capability");
> +			goto security_init_err;
> +		}
> +	}
> +

With this code added here, do we still need to keep - or should we keep -
the existing call in iavf_dev_start()? I would have expected the call to
iavf_get_ptp_cap to be moved rather than duplicated. Is there a reason
to keep the existing call?

/Bruce
  
Patrick Robb Nov. 24, 2025, 3:45 p.m. UTC | #2
Hi. There are some DTS failures on this testbed due to misconfiguration
applied by us at UNH last week. I'm fixing it and rerunning testing which
will update the CI checks for your patchseries.

On Fri, Nov 21, 2025 at 6:39 PM Jacob Keller <jacob.e.keller@intel.com>
wrote:

> Commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
> added a check against whether the PF has actually enabled Rx
> timestamping in iavf_dev_info_get(). Unfortunately, this function may be
> called prior to the PTP capabilities being exchanged, which results in
> Rx timestamping not being supported.
>
> Fix this by checking the PF PTP capabilities near the end of
> iavf_dev_init(). This ensures the VF knows the capabilities at the point
> where the iavf_dev_info_get() function can be called. Doing the check at
> init is better than inside the info callback, as the info callback is
> called many times.
>
> The capability exchange in iavf_dev_start() is kept to ensure that
> capabilities are updated after resets.
>
> Fixes: d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> My recent fix to prevent enabling Rx timestamping on PFs which do support
> PTP capability but do not report Rx timestamping accidentally broke PFs
> which *do* support Rx timestamping. This is because the driver did not
> exchange capability before reporting its device info. Fix this by checking
> PF capabilities during iavf_dev_init().
> ---
>  drivers/net/intel/iavf/iavf_ethdev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c
> b/drivers/net/intel/iavf/iavf_ethdev.c
> index 3ef766de4704..9b07b11a6b51 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -2887,6 +2887,14 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>                 }
>         }
>
> +       /* Get PTP caps early to verify device capabilities */
> +       if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) {
> +               if (iavf_get_ptp_cap(adapter)) {
> +                       PMD_INIT_LOG(ERR, "Failed to get ptp capability");
> +                       goto security_init_err;
> +               }
> +       }
> +
>         iavf_default_rss_disable(adapter);
>
>         iavf_dev_stats_reset(eth_dev);
>
> ---
> base-commit: ef98b88455bf4a7c8b7aa3106a761c9e9270d6a3
> change-id: 20251121-jk-dpdk-iavf-rx-timestamping-fix-abdcb42f0197
>
> Best regards,
> --
> Jacob Keller <jacob.e.keller@intel.com>
>
>
  
Keller, Jacob E Nov. 24, 2025, 9:34 p.m. UTC | #3
On 11/24/2025 4:09 AM, Bruce Richardson wrote:
> On Fri, Nov 21, 2025 at 03:39:37PM -0800, Jacob Keller wrote:
>> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
>> index 3ef766de4704..9b07b11a6b51 100644
>> --- a/drivers/net/intel/iavf/iavf_ethdev.c
>> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
>> @@ -2887,6 +2887,14 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>>  		}
>>  	}
>>  
>> +	/* Get PTP caps early to verify device capabilities */
>> +	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) {
>> +		if (iavf_get_ptp_cap(adapter)) {
>> +			PMD_INIT_LOG(ERR, "Failed to get ptp capability");
>> +			goto security_init_err;
>> +		}
>> +	}
>> +
> 
> With this code added here, do we still need to keep - or should we keep -
> the existing call in iavf_dev_start()? I would have expected the call to
> iavf_get_ptp_cap to be moved rather than duplicated. Is there a reason
> to keep the existing call?
> 
> /Bruce

So, I considered this, but I am not sure. In principle, we need to make
sure that we re-check capabilities after a reset. It is unlikely but it
is possible that a device reset could cause the PF to change its
decision on capabilities. I didn't see logic to call this during the
reset handler. If my understanding is right, thats because the start
function would get called after a reset, at which point we'd recheck
during the device start function.

I think its harmless to re-check the values as they should either be the
same or if they did somehow change we should get the updated value. The
intent of checking earlier is really just to make sure we try not to
report that we support timestamps when we definitely don't.

In particular, I wanted to get the current case with the upstream ice
driver and the DPDK code to fail closed instead of attempting to enable
timestamps but doing so incorrectly. (As outlined in my earlier cover
letter, we have a compatibility issue with the in-tree Linux kernel
drivers and the out-of-tree drivers regarding the capability struct layout).

That setup was trying to turn on timestamps but the PF did not actually
enable timestamps due to the capability layout issue, and thus the
timestamps were bogus. Instead, the new situation should be that DPDK
reports it can't enable timestamps, until we figure out the solution for
compatibility across these different variants of virtchnl. I'm working
on that but don't have an ETA or known time where we'll have a good
solution.

Thanks,
Jake
  
Hore, Soumyadeep Nov. 25, 2025, 10:30 a.m. UTC | #4
Commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support") added a check against whether the PF has actually enabled Rx timestamping in iavf_dev_info_get(). Unfortunately, this function may be called prior to the PTP capabilities being exchanged, which results in Rx timestamping not being supported.

Fix this by checking the PF PTP capabilities near the end of iavf_dev_init(). This ensures the VF knows the capabilities at the point where the iavf_dev_info_get() function can be called. Doing the check at init is better than inside the info callback, as the info callback is called many times.

The capability exchange in iavf_dev_start() is kept to ensure that capabilities are updated after resets.

Fixes: d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
My recent fix to prevent enabling Rx timestamping on PFs which do support PTP capability but do not report Rx timestamping accidentally broke PFs which *do* support Rx timestamping. This is because the driver did not exchange capability before reporting its device info. Fix this by checking PF capabilities during iavf_dev_init().
---
 drivers/net/intel/iavf/iavf_ethdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 3ef766de4704..9b07b11a6b51 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -2887,6 +2887,14 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
 		}
 	}
 
+	/* Get PTP caps early to verify device capabilities */
+	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) {
+		if (iavf_get_ptp_cap(adapter)) {
+			PMD_INIT_LOG(ERR, "Failed to get ptp capability");
+			goto security_init_err;
+		}
+	}
+
 	iavf_default_rss_disable(adapter);
 
 	iavf_dev_stats_reset(eth_dev);

---
base-commit: ef98b88455bf4a7c8b7aa3106a761c9e9270d6a3
change-id: 20251121-jk-dpdk-iavf-rx-timestamping-fix-abdcb42f0197

Best regards,
--
Jacob Keller <jacob.e.keller@intel.com>

Hi Jacob,

Currently PTP features are not enabled in DPDK. We only have the Rx timestamp API in place. Typically the change that you want needs to be implemented in ethdev_ops.timesync_enable(), which is not implemented.
  
Bruce Richardson Nov. 25, 2025, 1:12 p.m. UTC | #5
On Tue, Nov 25, 2025 at 10:30:37AM +0000, Hore, Soumyadeep wrote:
> Commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support") added a check against whether the PF has actually enabled Rx timestamping in iavf_dev_info_get(). Unfortunately, this function may be called prior to the PTP capabilities being exchanged, which results in Rx timestamping not being supported.
> 
> Fix this by checking the PF PTP capabilities near the end of iavf_dev_init(). This ensures the VF knows the capabilities at the point where the iavf_dev_info_get() function can be called. Doing the check at init is better than inside the info callback, as the info callback is called many times.
> 
> The capability exchange in iavf_dev_start() is kept to ensure that capabilities are updated after resets.
> 
> Fixes: d21c2fe6e5a1 ("net/iavf: fix check for PF Rx timestamp support")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> My recent fix to prevent enabling Rx timestamping on PFs which do support PTP capability but do not report Rx timestamping accidentally broke PFs which *do* support Rx timestamping. This is because the driver did not exchange capability before reporting its device info. Fix this by checking PF capabilities during iavf_dev_init().
> ---
>  drivers/net/intel/iavf/iavf_ethdev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 3ef766de4704..9b07b11a6b51 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -2887,6 +2887,14 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>  		}
>  	}
>  
> +	/* Get PTP caps early to verify device capabilities */
> +	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) {
> +		if (iavf_get_ptp_cap(adapter)) {
> +			PMD_INIT_LOG(ERR, "Failed to get ptp capability");
> +			goto security_init_err;
> +		}
> +	}
> +
>  	iavf_default_rss_disable(adapter);
>  
>  	iavf_dev_stats_reset(eth_dev);
> 
> ---
> base-commit: ef98b88455bf4a7c8b7aa3106a761c9e9270d6a3
> change-id: 20251121-jk-dpdk-iavf-rx-timestamping-fix-abdcb42f0197
> 
> Best regards,
> --
> Jacob Keller <jacob.e.keller@intel.com>
> 
> Hi Jacob,
> 
> Currently PTP features are not enabled in DPDK. We only have the Rx timestamp API in place. Typically the change that you want needs to be implemented in ethdev_ops.timesync_enable(), which is not implemented.
> 
At this stage we are post final RC for 25.11, so taking this patch would be
risky anyway. Let's target 26.03 for this, and then look to  backport to
25.11.1

Thanks,
/Bruce
  
Keller, Jacob E Nov. 25, 2025, 10:07 p.m. UTC | #6
On 11/25/2025 2:30 AM, Hore, Soumyadeep wrote:
> Hi Jacob,
> 
> Currently PTP features are not enabled in DPDK. We only have the Rx timestamp API in place. Typically the change that you want needs to be implemented in ethdev_ops.timesync_enable(), which is not implemented.
> 

I'm not sure I follow you here. For iAVF Rx timestamping is considered
part of the PTP capabilities. We shouldn't be reporting Rx timestamp
offload if Rx timestamping is not available. There are some
circumstances where the PF will not report Rx timestampding (and will
not enable it) because of capabilities mismatch. However, DPDK driver is
saying Rx timestamp offload works, and trying (and failing) to enable it
in those cases.

However, the check for where we report Rx timestamp offload was before
we negotiated properly with the PF, which is what this change is trying
to fix.

This has nothing to do with other PTP features.

Thanks,
Jake
  
Keller, Jacob E Nov. 25, 2025, 11:57 p.m. UTC | #7
On 11/25/2025 5:12 AM, Bruce Richardson wrote:
> On Tue, Nov 25, 2025 at 10:30:37AM +0000, Hore, Soumyadeep wrote:
>> Hi Jacob,
>>
>> Currently PTP features are not enabled in DPDK. We only have the Rx timestamp API in place. Typically the change that you want needs to be implemented in ethdev_ops.timesync_enable(), which is not implemented.
>>
> At this stage we are post final RC for 25.11, so taking this patch would be
> risky anyway. Let's target 26.03 for this, and then look to  backport to
> 25.11.1
> 
> Thanks,
> /Bruce

Sure. In that case i think we need to revert the other change that
checked the RX_TIMESTAMP flag as it corrected one issue but caused a
regression in Rx timestamp offload when it *is* functional.

I think its commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx
timestamp support"). That commit without this fix will incorrectly
prevent enabling Rx timestamps.

Thanks,
Jake
  
Bruce Richardson Nov. 26, 2025, 5:01 p.m. UTC | #8
On Tue, Nov 25, 2025 at 03:57:10PM -0800, Jacob Keller wrote:
> 
> 
> On 11/25/2025 5:12 AM, Bruce Richardson wrote:
> > On Tue, Nov 25, 2025 at 10:30:37AM +0000, Hore, Soumyadeep wrote:
> >> Hi Jacob,
> >>
> >> Currently PTP features are not enabled in DPDK. We only have the Rx timestamp API in place. Typically the change that you want needs to be implemented in ethdev_ops.timesync_enable(), which is not implemented.
> >>
> > At this stage we are post final RC for 25.11, so taking this patch would be
> > risky anyway. Let's target 26.03 for this, and then look to  backport to
> > 25.11.1
> > 
> > Thanks,
> > /Bruce
> 
> Sure. In that case i think we need to revert the other change that
> checked the RX_TIMESTAMP flag as it corrected one issue but caused a
> regression in Rx timestamp offload when it *is* functional.
> 
> I think its commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx
> timestamp support"). That commit without this fix will incorrectly
> prevent enabling Rx timestamps.
> 
I've sent a revert patch now[1]. Please verify that this revert is correct
and ok for inclusion as a last-minute fix in final 25.11 release.

Thanks,
/Bruce

[1] https://patches.dpdk.org/project/dpdk/patch/20251126165943.1536164-1-bruce.richardson@intel.com/
  
Keller, Jacob E Dec. 1, 2025, 10:49 p.m. UTC | #9
> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Wednesday, November 26, 2025 9:01 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Hore, Soumyadeep <soumyadeep.hore@intel.com>; dev@dpdk.org;
> Greenwalt, Paul <paul.greenwalt@intel.com>; Medvedkin, Vladimir
> <vladimir.medvedkin@intel.com>; Jiale, SongX <songx.jiale@intel.com>
> Subject: Re: [PATCH] net/iavf: check PTP capabilities during init
> 
> On Tue, Nov 25, 2025 at 03:57:10PM -0800, Jacob Keller wrote:
> >
> >
> > On 11/25/2025 5:12 AM, Bruce Richardson wrote:
> > > On Tue, Nov 25, 2025 at 10:30:37AM +0000, Hore, Soumyadeep wrote:
> > >> Hi Jacob,
> > >>
> > >> Currently PTP features are not enabled in DPDK. We only have the Rx
> timestamp API in place. Typically the change that you want needs to be
> implemented in ethdev_ops.timesync_enable(), which is not implemented.
> > >>
> > > At this stage we are post final RC for 25.11, so taking this patch would be
> > > risky anyway. Let's target 26.03 for this, and then look to  backport to
> > > 25.11.1
> > >
> > > Thanks,
> > > /Bruce
> >
> > Sure. In that case i think we need to revert the other change that
> > checked the RX_TIMESTAMP flag as it corrected one issue but caused a
> > regression in Rx timestamp offload when it *is* functional.
> >
> > I think its commit d21c2fe6e5a1 ("net/iavf: fix check for PF Rx
> > timestamp support"). That commit without this fix will incorrectly
> > prevent enabling Rx timestamps.
> >
> I've sent a revert patch now[1]. Please verify that this revert is correct
> and ok for inclusion as a last-minute fix in final 25.11 release.
> 
> Thanks,
> /Bruce
> 
> [1] https://patches.dpdk.org/project/dpdk/patch/20251126165943.1536164-1-
> bruce.richardson@intel.com/

Looks good to me, and thanks for fixing my mistake quickly!

Regards,
Jake
  

Patch

diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 3ef766de4704..9b07b11a6b51 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -2887,6 +2887,14 @@  iavf_dev_init(struct rte_eth_dev *eth_dev)
 		}
 	}
 
+	/* Get PTP caps early to verify device capabilities */
+	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) {
+		if (iavf_get_ptp_cap(adapter)) {
+			PMD_INIT_LOG(ERR, "Failed to get ptp capability");
+			goto security_init_err;
+		}
+	}
+
 	iavf_default_rss_disable(adapter);
 
 	iavf_dev_stats_reset(eth_dev);