On Tue, Apr 07, 2026 at 08:14:45AM -0700, Stephen Hemminger wrote: > On Thu, 12 Mar 2026 11:57:33 +0000 > Bruce Richardson <[email protected]> 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 <[email protected]> > > > --- > > > 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
