Re: [PATCH v10 11/14] dpll: sit9531x: add support to get phase offset on the connected input pin

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dpll: sit9531x: permanent debug tap state corruption on I2C read failure
--

commit 86796ea4e61e041e1adcf2a7bb6d9a9966596b9a
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

    dpll: sit9531x: add support to get phase offset on the connected input pin

This patch adds support for reporting the phase difference between a PLL's
reference and the PLL itself in the sit9531x driver. It achieves this by
reading the on-chip time-to-digital converter through the debug window.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index c8c3cd6a64baf..8f6f8ffc8dc13 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +/*
> + * sit9531x_phase_offset_read - read phase difference via TDC
> + * @phase_ps:	output phase difference in picoseconds
> + *
> + * Reads the Time-to-Digital Converter (TDC) signed 35-bit code from the
> + * PLL page registers, then converts to picoseconds using the VCO
> + * frequency: phase_diff = tdc_code / fvco.
> + *
> + * Caller must hold sitdev->multiop_lock.
> + */
> +int sit9531x_phase_offset_read(struct sit9531x_dev *sitdev, u8 pll_idx,
> +			       s64 *phase_ps)
> +{

[ ... ]

> +	/* Unlock the debug page so the TDC registers are accessible. */
> +	rc = sit9531x_write_pll_u8(sitdev, pll_idx,
> +				   SIT9531X_PLL_REG_DEBUG,
> +				   SIT9531X_PLL_DEBUG_UNLOCK);
> +	if (rc)
> +		goto relock;
> +
> +	/*
> +	 * Remember the tap selection so it can be put back.  The key
> +	 * register is re-locked below, but the mux is not part of the key:
> +	 * leaving it parked on the TDC with a slow sampling clock selected
> +	 * is a state change the caller did not ask for, and the next reader
> +	 * of a different tap would have to know to undo it.
> +	 */
> +	if (!sit9531x_read_pll_u8(sitdev, pll_idx,
> +				  SIT9531X_PLL_REG_DBG_WRITE_CODE,
> +				  &old_write_code) &&
> +	    !sit9531x_read_pll_u8(sitdev, pll_idx,
> +				  SIT9531X_PLL_REG_DBG_READ_CODE,
> +				  &old_read_code))
> +		have_old = true;

[Severity: Medium]
What happens if the initial I2C reads fail here?

If a transient I2C read failure occurs when attempting to save the write
and read codes in sit9531x_phase_offset_read(), have_old remains false but
the function does not abort.

The function then proceeds to overwrite the registers, and eventually skips
the restoration during cleanup because have_old is false:

> +relock:
> +	if (have_old) {
> +		sit9531x_write_pll_u8(sitdev, pll_idx,
> +				      SIT9531X_PLL_REG_DBG_READ_CODE,
> +				      old_read_code);
> +		sit9531x_write_pll_u8(sitdev, pll_idx,
> +				      SIT9531X_PLL_REG_DBG_WRITE_CODE,
> +				      old_write_code);
> +	}

Could this permanently leave the debug tap state corrupted (parked on the TDC
with a slow clock enabled) and break subsequent reads of other taps?

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