|SUCCESS| [v2] net/idpf: harden PTP frequency adjustment

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162887

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-07

commit 3f4d8e9a1b2c (fabricated for review purposes)
Author: Soumyadeep Hore <soumyadeep.hore at intel.com>

    net/idpf: harden PTP frequency adjustment

This patch modifies the PTP frequency adjustment calculation in
idpf_timesync_adjust_freq() to use 128-bit arithmetic instead of the
previous multi-step overflow prevention approach.

> 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;
>  

The variable declaration style uses __int128 which is a compiler-specific
extension. Is this type portable across all compilers DPDK supports? The
DPDK coding standards typically prefer standard C types. Does DPDK have a
policy on using __int128?

Additionally, incval changes from int64_t to uint64_t. Does this change
affect signedness handling elsewhere in the function or in
idpf_ptp_adj_dev_clk_fine()? Can ptp->base_incval be negative?

[ ... ]

> +	/* 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;

Does this calculation handle the full range of possible ppm values
correctly? The original code checked for overflow with rte_log2_u64().
With __int128, what happens if diff exceeds INT64_MAX or INT64_MIN after
the division? Can the cast to int64_t and subsequent addition to incval
(now uint64_t) cause unexpected behavior?

Also, the divisor in the original code was uint64_t, but here 1000000LL is
int64_t. Does this affect the calculation?

>  
>  	ret = idpf_ptp_adj_dev_clk_fine(adapter, incval);
>  	if (ret) {