Re: [PATCH net-next v9 10/15] dpll: sit9531x: add support to adjust output phase

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dpll: sit9531x: stale hardware delay causes spurious phase shift on frequency change
- [High] dpll: sit9531x: integer overflow in phase_adj cache corrupts phase on frequency change
--

commit 71a00c5eb4f21406ee20e81e9a6aad131896b711
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to adjust output phase

This commit adds support to shift output in time against others driven by
the same PLL, allowing adjustment of output phase. It handles coarse and fine
delay steps and folds delays modulo the output period.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index bba42fe302a3b..8d857f1a0c890 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -1641,7 +1641,17 @@ int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
>  
>  	sitdev->out[out_idx].freq = div64_u64(fvco, divo);
>  
> -	return 0;
> +	/*
> +	 * The programmed reset delay counts VCO cycles against the output
> +	 * period in force when it was written, so a rate change silently
> +	 * re-times a previously requested phase adjust.  Re-encode the
> +	 * cached picosecond request against the new rate.
> +	 */
> +	if (sitdev->out[out_idx].phase_adj)
> +		rc = sit9531x_output_phase_adjust_set(sitdev, out_idx,
> +						      sitdev->out[out_idx].phase_adj);

[Severity: High]
Does this logic fail to clear stale delay registers if the cached offset was
rounded to exactly one output period?

If a user previously requested a very small negative phase offset (e.g., -1 ps),
hardware quantization could round the required delay up to exactly one output
period (t_out_ps). The hardware registers would receive this non-zero physical
delay (which functionally equals 0 degrees), and the driver correctly caches
phase_adj = 0.

However, when the frequency is later changed, sit9531x_output_freq_set() skips
calling sit9531x_output_phase_adjust_set() because phase_adj evaluates to 0.
Does this leave the hardware delay registers retaining the old t_out_ps delay,
which is no longer one full period at the new frequency, resulting in an
arbitrary and spurious phase shift?

[ ... ]

> @@ -1731,6 +1741,206 @@ int sit9531x_output_freq_get(struct sit9531x_dev *sitdev, u8 out_idx,
[ ... ]
> +	/*
> +	 * Cache what the registers realize, and only once every step has
> +	 * succeeded: the core drops a repeated request with the same value,
> +	 * so a cache updated by a failed call would make the retry a no-op.
> +	 *
> +	 * Quantizing to whole VCO cycles plus 30 ps steps can land a few
> +	 * picoseconds past the end of the period, which would wrap the
> +	 * subtraction below; one period is the most a delay can be.
> +	 */
> +	coarse_ps = mul_u64_u64_div_u64(coarse, 1000000000000ULL, fvco);
> +	abs_ps = coarse_ps + (u64)fine * SIT9531X_OUT_PRG_FINE_STEP_PS;
> +	if (abs_ps > t_out_ps)
> +		abs_ps = t_out_ps;
> +	if (phase_norm_ps < 0)
> +		sitdev->out[out_idx].phase_adj =
> +			abs_ps ? -(s32)(t_out_ps - abs_ps) : 0;
> +	else
> +		sitdev->out[out_idx].phase_adj = (s32)abs_ps;

[Severity: High]
Can this downcast to s32 overflow and corrupt the cached phase adjustment?

If a user requests a phase offset near INT_MAX (2,147,483,647 ps) or INT_MIN,
hardware quantization in 30 ps steps could round the absolute delay (abs_ps)
up by up to 15 ps, pushing it over the 32-bit signed integer limit (e.g., to
2,147,483,660).

When caching this value, the direct (s32) cast truncates the upper bits and
flips the sign (e.g., to -2,147,483,636). When the frequency is later changed,
the driver feeds this corrupted opposite-sign value back into
sit9531x_output_phase_adjust_set(), programming the hardware with an incorrect
physical delay. Should there be saturation bounds checking before this cast?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915000015.80480-1-arouhi@sitime.com?part=10