Re: [PATCH v1] net/idpf: harden PTP frequency adjustment

Re: [PATCH v1] net/idpf: harden PTP frequency adjustment

[Bruce Richardson]

On Thu, Mar 12, 2026 at 02:43:44PM -0400, Soumyadeep Hore wrote:
> Normalize ppm to an unsigned magnitude before using it in the
> increment value scaling path.
> 
> This avoids negating INT64_MIN and also prevents subtracting 62
> from the reduced log sum unless the sum is still above the
> overflow threshold reported by Coverity.
> 
> Coverity issue: 501832
> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>  drivers/net/intel/idpf/idpf_ethdev.c | 32 +++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/intel/idpf/idpf_ethdev.c b/drivers/net/intel/idpf/idpf_ethdev.c
> index 5e57a45775..1c5bd2ee12 100644
> --- a/drivers/net/intel/idpf/idpf_ethdev.c
> +++ b/drivers/net/intel/idpf/idpf_ethdev.c
> @@ -1007,7 +1007,7 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
>  	struct idpf_ptp *ptp = adapter->ptp;
>  	int64_t incval, diff = 0;
>  	bool negative = false;
> -	uint64_t div, rem;
> +	uint64_t abs_ppm, div, rem;
>  	uint64_t divisor = 1000000ULL << 16;
>  	int shift;
>  	int ret;
> @@ -1016,26 +1016,34 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
>  
>  	if (ppm < 0) {
>  		negative = true;
> -		ppm = -ppm;
> +		abs_ppm = ppm == INT64_MIN ? (uint64_t)INT64_MAX + 1 :
> +			(uint64_t)(-ppm);
> +	} else {
> +		abs_ppm = (uint64_t)ppm;
>  	}
>  
>  	/* can incval * ppm overflow ? */
> -	if (rte_log2_u64(incval) + rte_log2_u64(ppm) > 62) {
> -		rem = ppm % divisor;
> -		div = ppm / divisor;
> +	if (rte_log2_u64(incval) + rte_log2_u64(abs_ppm) > 62) {
> +		rem = abs_ppm % divisor;
> +		div = abs_ppm / divisor;
>  		diff = div * incval;
> -		ppm = rem;
> +		abs_ppm = rem;
> +
> +		if (abs_ppm != 0) {
> +			uint32_t log_sum;
>  
> -		shift = rte_log2_u64(incval) + rte_log2_u64(ppm) - 62;
> -		if (shift > 0) {
> -			/* drop precision */
> -			ppm >>= shift;
> -			divisor >>= shift;
> +			log_sum = rte_log2_u64(incval) + rte_log2_u64(abs_ppm);
> +			if (log_sum > 62) {
> +				shift = log_sum - 62;
> +				/* drop precision */
> +				abs_ppm >>= shift;
> +				divisor >>= shift;
> +			}
>  		}
>  	}
>  
>  	if (divisor)
> -		diff = diff + incval * ppm / divisor;
> +		diff = diff + incval * abs_ppm / divisor;
>  

Asking Claude AI about this code and how to simplify it a bit while also
avoiding the INT64_MIN issue, it suggests using __int128 type, giving much
shorter code:

	__int128 diff;
	int ret;

	/*
	 * ppm is scaled ppm: 1 ppm = 2^16 units (matching Linux adjfine).
	 * Use __int128 to avoid overflow without complex split/shift logic,
	 * and to correctly handle ppm == INT64_MIN.
	 */
	incval = ptp->base_incval;
	diff = ((__int128)incval * ppm) / (1000000LL << 16);
	incval += (int64_t)diff;

	ret = idpf_ptp_adj_dev_clk_fine(adapter, incval);
	if (ret != 0)
		PMD_DRV_LOG(ERR, "PTP failed to set incval, err %d", ret);
	return ret;

Maybe worth considering?

/Bruce

[PATCH v1] net/idpf: harden PTP frequency adjustment

[Soumyadeep Hore]

Normalize ppm to an unsigned magnitude before using it in the
increment value scaling path.

This avoids negating INT64_MIN and also prevents subtracting 62
from the reduced log sum unless the sum is still above the
overflow threshold reported by Coverity.

Coverity issue: 501832

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/intel/idpf/idpf_ethdev.c | 32 +++++++++++++++++-----------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/net/intel/idpf/idpf_ethdev.c b/drivers/net/intel/idpf/idpf_ethdev.c
index 5e57a45775..1c5bd2ee12 100644
--- a/drivers/net/intel/idpf/idpf_ethdev.c
+++ b/drivers/net/intel/idpf/idpf_ethdev.c
@@ -1007,7 +1007,7 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
 	struct idpf_ptp *ptp = adapter->ptp;
 	int64_t incval, diff = 0;
 	bool negative = false;
-	uint64_t div, rem;
+	uint64_t abs_ppm, div, rem;
 	uint64_t divisor = 1000000ULL << 16;
 	int shift;
 	int ret;
@@ -1016,26 +1016,34 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
 
 	if (ppm < 0) {
 		negative = true;
-		ppm = -ppm;
+		abs_ppm = ppm == INT64_MIN ? (uint64_t)INT64_MAX + 1 :
+			(uint64_t)(-ppm);
+	} else {
+		abs_ppm = (uint64_t)ppm;
 	}
 
 	/* can incval * ppm overflow ? */
-	if (rte_log2_u64(incval) + rte_log2_u64(ppm) > 62) {
-		rem = ppm % divisor;
-		div = ppm / divisor;
+	if (rte_log2_u64(incval) + rte_log2_u64(abs_ppm) > 62) {
+		rem = abs_ppm % divisor;
+		div = abs_ppm / divisor;
 		diff = div * incval;
-		ppm = rem;
+		abs_ppm = rem;
+
+		if (abs_ppm != 0) {
+			uint32_t log_sum;
 
-		shift = rte_log2_u64(incval) + rte_log2_u64(ppm) - 62;
-		if (shift > 0) {
-			/* drop precision */
-			ppm >>= shift;
-			divisor >>= shift;
+			log_sum = rte_log2_u64(incval) + rte_log2_u64(abs_ppm);
+			if (log_sum > 62) {
+				shift = log_sum - 62;
+				/* drop precision */
+				abs_ppm >>= shift;
+				divisor >>= shift;
+			}
 		}
 	}
 
 	if (divisor)
-		diff = diff + incval * ppm / divisor;
+		diff = diff + incval * abs_ppm / divisor;
 
 	if (negative)
 		incval -= diff;
-- 
2.47.1

Re: [PATCH v1] net/idpf: harden PTP frequency adjustment

[Stephen Hemminger]

On Thu, 12 Mar 2026 11:57:33 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Mar 12, 2026 at 02:43:44PM -0400, Soumyadeep Hore wrote:
> > Normalize ppm to an unsigned magnitude before using it in the
> > increment value scaling path.
> > 
> > This avoids negating INT64_MIN and also prevents subtracting 62
> > from the reduced log sum unless the sum is still above the
> > overflow threshold reported by Coverity.
> > 
> > Coverity issue: 501832
> > 
> > Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> > ---
> >  drivers/net/intel/idpf/idpf_ethdev.c | 32 +++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/intel/idpf/idpf_ethdev.c b/drivers/net/intel/idpf/idpf_ethdev.c
> > index 5e57a45775..1c5bd2ee12 100644
> > --- a/drivers/net/intel/idpf/idpf_ethdev.c
> > +++ b/drivers/net/intel/idpf/idpf_ethdev.c
> > @@ -1007,7 +1007,7 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
> >  	struct idpf_ptp *ptp = adapter->ptp;
> >  	int64_t incval, diff = 0;
> >  	bool negative = false;
> > -	uint64_t div, rem;
> > +	uint64_t abs_ppm, div, rem;
> >  	uint64_t divisor = 1000000ULL << 16;
> >  	int shift;
> >  	int ret;
> > @@ -1016,26 +1016,34 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
> >  
> >  	if (ppm < 0) {
> >  		negative = true;
> > -		ppm = -ppm;
> > +		abs_ppm = ppm == INT64_MIN ? (uint64_t)INT64_MAX + 1 :
> > +			(uint64_t)(-ppm);
> > +	} else {
> > +		abs_ppm = (uint64_t)ppm;
> >  	}
> >  
> >  	/* can incval * ppm overflow ? */
> > -	if (rte_log2_u64(incval) + rte_log2_u64(ppm) > 62) {
> > -		rem = ppm % divisor;
> > -		div = ppm / divisor;
> > +	if (rte_log2_u64(incval) + rte_log2_u64(abs_ppm) > 62) {
> > +		rem = abs_ppm % divisor;
> > +		div = abs_ppm / divisor;
> >  		diff = div * incval;
> > -		ppm = rem;
> > +		abs_ppm = rem;
> > +
> > +		if (abs_ppm != 0) {
> > +			uint32_t log_sum;
> >  
> > -		shift = rte_log2_u64(incval) + rte_log2_u64(ppm) - 62;
> > -		if (shift > 0) {
> > -			/* drop precision */
> > -			ppm >>= shift;
> > -			divisor >>= shift;
> > +			log_sum = rte_log2_u64(incval) + rte_log2_u64(abs_ppm);
> > +			if (log_sum > 62) {
> > +				shift = log_sum - 62;
> > +				/* drop precision */
> > +				abs_ppm >>= shift;
> > +				divisor >>= shift;
> > +			}
> >  		}
> >  	}
> >  
> >  	if (divisor)
> > -		diff = diff + incval * ppm / divisor;
> > +		diff = diff + incval * abs_ppm / divisor;
> >    
> 
> Asking Claude AI about this code and how to simplify it a bit while also
> avoiding the INT64_MIN issue, it suggests using __int128 type, giving much
> shorter code:
> 
> 	__int128 diff;
> 	int ret;
> 
> 	/*
> 	 * ppm is scaled ppm: 1 ppm = 2^16 units (matching Linux adjfine).
> 	 * Use __int128 to avoid overflow without complex split/shift logic,
> 	 * and to correctly handle ppm == INT64_MIN.
> 	 */
> 	incval = ptp->base_incval;
> 	diff = ((__int128)incval * ppm) / (1000000LL << 16);
> 	incval += (int64_t)diff;
> 
> 	ret = idpf_ptp_adj_dev_clk_fine(adapter, incval);
> 	if (ret != 0)
> 		PMD_DRV_LOG(ERR, "PTP failed to set incval, err %d", ret);
> 	return ret;
> 
> Maybe worth considering?
> 
> /Bruce

Claude AI review of this patch spotted some possible math issues.



Normalize ppm to an unsigned magnitude before using it in the
increment value scaling path.
This avoids negating INT64_MIN and also prevents subtracting 62
from the reduced log sum unless the sum is still above the
overflow threshold reported by Coverity.

The simplification is a nice improvement and the __int128 approach
is correct for realistic hardware values.

One concern:
Warning: drivers/net/intel/idpf/idpf_ethdev.c
The cast (int64_t)diff silently truncates if diff does not fit in
64 bits.  The __int128 multiplication itself never overflows (both
UINT64_MAX × INT64_MAX and UINT64_MAX × INT64_MIN fit within signed
__int128), but after dividing by 1000000LL << 16 the quotient can
still exceed INT64_MAX when incval is large and ppm is extreme.
For realistic PTP hardware (base_incval around 2^32) this is fine,
but the API accepts the full int64_t range without documented bounds.
Adding an explicit clamp or assertion before the cast would preserve
the defensive character of the code being replaced:

Re: [PATCH v1] net/idpf: harden PTP frequency adjustment

[Bruce Richardson]

On Tue, Apr 07, 2026 at 08:14:45AM -0700, Stephen Hemminger wrote:
> On Thu, 12 Mar 2026 11:57:33 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Thu, Mar 12, 2026 at 02:43:44PM -0400, Soumyadeep Hore wrote:
> > > Normalize ppm to an unsigned magnitude before using it in the
> > > increment value scaling path.
> > > 
> > > This avoids negating INT64_MIN and also prevents subtracting 62
> > > from the reduced log sum unless the sum is still above the
> > > overflow threshold reported by Coverity.
> > > 
> > > Coverity issue: 501832
> > > 
> > > Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> > > ---
> > >  drivers/net/intel/idpf/idpf_ethdev.c | 32 +++++++++++++++++-----------
> > >  1 file changed, 20 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/net/intel/idpf/idpf_ethdev.c b/drivers/net/intel/idpf/idpf_ethdev.c
> > > index 5e57a45775..1c5bd2ee12 100644
> > > --- a/drivers/net/intel/idpf/idpf_ethdev.c
> > > +++ b/drivers/net/intel/idpf/idpf_ethdev.c
> > > @@ -1007,7 +1007,7 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
> > >  	struct idpf_ptp *ptp = adapter->ptp;
> > >  	int64_t incval, diff = 0;
> > >  	bool negative = false;
> > > -	uint64_t div, rem;
> > > +	uint64_t abs_ppm, div, rem;
> > >  	uint64_t divisor = 1000000ULL << 16;
> > >  	int shift;
> > >  	int ret;
> > > @@ -1016,26 +1016,34 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
> > >  
> > >  	if (ppm < 0) {
> > >  		negative = true;
> > > -		ppm = -ppm;
> > > +		abs_ppm = ppm == INT64_MIN ? (uint64_t)INT64_MAX + 1 :
> > > +			(uint64_t)(-ppm);
> > > +	} else {
> > > +		abs_ppm = (uint64_t)ppm;
> > >  	}
> > >  
> > >  	/* can incval * ppm overflow ? */
> > > -	if (rte_log2_u64(incval) + rte_log2_u64(ppm) > 62) {
> > > -		rem = ppm % divisor;
> > > -		div = ppm / divisor;
> > > +	if (rte_log2_u64(incval) + rte_log2_u64(abs_ppm) > 62) {
> > > +		rem = abs_ppm % divisor;
> > > +		div = abs_ppm / divisor;
> > >  		diff = div * incval;
> > > -		ppm = rem;
> > > +		abs_ppm = rem;
> > > +
> > > +		if (abs_ppm != 0) {
> > > +			uint32_t log_sum;
> > >  
> > > -		shift = rte_log2_u64(incval) + rte_log2_u64(ppm) - 62;
> > > -		if (shift > 0) {
> > > -			/* drop precision */
> > > -			ppm >>= shift;
> > > -			divisor >>= shift;
> > > +			log_sum = rte_log2_u64(incval) + rte_log2_u64(abs_ppm);
> > > +			if (log_sum > 62) {
> > > +				shift = log_sum - 62;
> > > +				/* drop precision */
> > > +				abs_ppm >>= shift;
> > > +				divisor >>= shift;
> > > +			}
> > >  		}
> > >  	}
> > >  
> > >  	if (divisor)
> > > -		diff = diff + incval * ppm / divisor;
> > > +		diff = diff + incval * abs_ppm / divisor;
> > >    
> > 
> > Asking Claude AI about this code and how to simplify it a bit while also
> > avoiding the INT64_MIN issue, it suggests using __int128 type, giving much
> > shorter code:
> > 
> > 	__int128 diff;
> > 	int ret;
> > 
> > 	/*
> > 	 * ppm is scaled ppm: 1 ppm = 2^16 units (matching Linux adjfine).
> > 	 * Use __int128 to avoid overflow without complex split/shift logic,
> > 	 * and to correctly handle ppm == INT64_MIN.
> > 	 */
> > 	incval = ptp->base_incval;
> > 	diff = ((__int128)incval * ppm) / (1000000LL << 16);
> > 	incval += (int64_t)diff;
> > 
> > 	ret = idpf_ptp_adj_dev_clk_fine(adapter, incval);
> > 	if (ret != 0)
> > 		PMD_DRV_LOG(ERR, "PTP failed to set incval, err %d", ret);
> > 	return ret;
> > 
> > Maybe worth considering?
> > 
> > /Bruce
> 
> Claude AI review of this patch spotted some possible math issues.
> 
> 
> 
> Normalize ppm to an unsigned magnitude before using it in the
> increment value scaling path.
> This avoids negating INT64_MIN and also prevents subtracting 62
> from the reduced log sum unless the sum is still above the
> overflow threshold reported by Coverity.
> 
> The simplification is a nice improvement and the __int128 approach
> is correct for realistic hardware values.
> 
> One concern:
> Warning: drivers/net/intel/idpf/idpf_ethdev.c
> The cast (int64_t)diff silently truncates if diff does not fit in
> 64 bits.  The __int128 multiplication itself never overflows (both
> UINT64_MAX × INT64_MAX and UINT64_MAX × INT64_MIN fit within signed
> __int128), but after dividing by 1000000LL << 16 the quotient can
> still exceed INT64_MAX when incval is large and ppm is extreme.
> For realistic PTP hardware (base_incval around 2^32) this is fine,
> but the API accepts the full int64_t range without documented bounds.
> Adding an explicit clamp or assertion before the cast would preserve
> the defensive character of the code being replaced:

Sadly, the CI is also complaining. It seems that __int128 doesn't seem to
be available for 32-bit ARM. :-( Not sure what the best approach here is,
whether to add a more complicated fallback path for that case, or just
disallow the timesync on the unsupported platforms for now. [32-bit x86
doesn't seem to have issues, for example]

/Bruce

[PATCH v2] net/idpf: harden PTP frequency adjustment

[Soumyadeep Hore]

Normalize ppm to an unsigned magnitude before using it in the
increment value scaling path.

This avoids negating INT64_MIN and also prevents subtracting 62
from the reduced log sum unless the sum is still above the
overflow threshold reported by Coverity.

Coverity issue: 501832

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
v2:
-Addressed Bruce's comments
---
 drivers/net/intel/idpf/idpf_ethdev.c | 38 ++++------------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

diff --git a/drivers/net/intel/idpf/idpf_ethdev.c b/drivers/net/intel/idpf/idpf_ethdev.c
index 5e57a45775..23a2af6332 100644
--- a/drivers/net/intel/idpf/idpf_ethdev.c
+++ b/drivers/net/intel/idpf/idpf_ethdev.c
@@ -1005,42 +1005,14 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
 	struct idpf_vport *vport = dev->data->dev_private;
 	struct idpf_adapter *adapter = vport->adapter;
 	struct idpf_ptp *ptp = adapter->ptp;
-	int64_t incval, diff = 0;
-	bool negative = false;
-	uint64_t div, rem;
-	uint64_t divisor = 1000000ULL << 16;
-	int shift;
+	uint64_t incval;
+	__int128 diff;
 	int ret;
 
+	/* ppm is scaled by 2^16 to match Linux adjfine. */
 	incval = ptp->base_incval;
-
-	if (ppm < 0) {
-		negative = true;
-		ppm = -ppm;
-	}
-
-	/* can incval * ppm overflow ? */
-	if (rte_log2_u64(incval) + rte_log2_u64(ppm) > 62) {
-		rem = ppm % divisor;
-		div = ppm / divisor;
-		diff = div * incval;
-		ppm = rem;
-
-		shift = rte_log2_u64(incval) + rte_log2_u64(ppm) - 62;
-		if (shift > 0) {
-			/* drop precision */
-			ppm >>= shift;
-			divisor >>= shift;
-		}
-	}
-
-	if (divisor)
-		diff = diff + incval * ppm / divisor;
-
-	if (negative)
-		incval -= diff;
-	else
-		incval += diff;
+	diff = ((__int128)incval * ppm) / (1000000LL << 16);
+	incval += (int64_t)diff;
 
 	ret = idpf_ptp_adj_dev_clk_fine(adapter, incval);
 	if (ret) {
-- 
2.47.1