Re: [PATCH net-next v13 10/12] 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: operations on unrouted outputs disrupt PLL 0 by flushing its phase
- [Medium] dpll: sit9531x: missed flush_pending update strands unaligned output
--

commit 0ba6c14ce491dafce857ce27044cf9f711bbf95f
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to adjust output phase

This commit adds support to adjust the output phase for the SiT9531x DPLL
driver. It allows shifting an output in time against others driven by the same
PLL by configuring the programmable reset delay registers.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 9aa0d327b6bf6..e15070ea2ae69 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -2294,8 +2748,22 @@ int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
>  	ret = sit9531x_prg_commit(sitdev);
>  	if (ret && !rc)
>  		rc = ret;
> -	if (rc)
> +	if (rc) {
> +		/*
> +		 * The divider may have changed all the same: a failed update
> +		 * can have reached the part, a loop lock can fail after it
> +		 * took effect, and a failed rollback leaves whatever landed.
> +		 * An advance is held as T_out - |advance| against the period
> +		 * that was, and the re-timed bytes may or may not be in, so
> +		 * the cache no longer says what the output does.  Mark it for
> +		 * a read-back: the core drops a retry of either request,
> +		 * the rate because it reads the new one first and the phase
> +		 * because it would match the stale cache.
> +		 */
> +		if (sitdev->out[out_idx].phase_armed)
> +			sitdev->out[out_idx].phase_stale = true;

[Severity: Medium]
When prg_commit() fails, the driver marks phase_stale = true but omits setting
flush_pending = true (unlike the equivalent error path in
sit9531x_output_phase_adjust_set()).

Could this leave the output divider permanently unaligned? If the driver state
tracking loses the pending flush requirement, a subsequent manual phase set
request could be falsely treated as a no-op and dropped, preventing recovery
without setting a dummy phase first.

>  		return rc;
> +	}
>  
>  	/*
>  	 * Step 5: flush the PLL's output phase so the new DIVO starts
> @@ -2308,19 +2776,31 @@ int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
>  	 * divider on its old phase, which is a realignment that did not
>  	 * happen rather than a rate that did not change -- and reporting a
>  	 * failure would be doubly wrong, because the core asks for the
> -	 * current rate first and would drop an identical retry.
> +	 * current rate first and would drop an identical retry.  A re-timed
> +	 * delay is then committed but not applied either; mark it for a
> +	 * read-back rather than cache a value the flush did not realize.
>  	 */
>  	rc = sit9531x_output_phase_flush(sitdev, pll_idx);

[Severity: High]
Does this trigger an unintended phase flush on PLL 0 when a frequency
change is applied to an unrouted pin?

When an output pin is not routed to any PLL by the boot configuration,
sit9531x_out_state_fetch() initializes it with routed = false and pll_idx = 0.
Since sit9531x_output_freq_set() does not check if the pin is routed,
pll_idx = 0 is passed to sit9531x_output_phase_flush(), which will forcefully
restart the dividers for all active outputs driven by PLL 0.

>  	if (rc) {
>  		dev_warn(sitdev->dev,
>  			 "out%u: rate changed but the divider phase was not realigned (%d)\n",
>  			 out_idx, rc);
> +		if (retime)
> +			sitdev->out[out_idx].phase_stale = true;

[Severity: Medium]
Similarly to the commit failure above, if sit9531x_output_phase_flush()
fails, the cache is marked stale but the pending flush requirement is not
recorded. 

Will a subsequent retry with the same target phase incorrectly hit the cache
path in sit9531x_output_phase_adjust_set() and skip the required flush?

>  		rc = 0;
> +	} else {
> +		/* Whatever flush was owed from before has now run. */
> +		sitdev->out[out_idx].flush_pending = false;
> +		if (retime) {
> +			sitdev->out[out_idx].phase_adj = phase_adj;
> +			sitdev->out[out_idx].phase_armed = phase_adj != 0;
> +			sitdev->out[out_idx].phase_stale = false;
> +		}
>  	}
>  
[ ... ]
> @@ -2374,27 +2854,140 @@ int sit9531x_output_freq_get(struct sit9531x_dev *sitdev, u8 out_idx,
>  	return 0;
>  }
>  
> +int sit9531x_output_phase_adjust_set(struct sit9531x_dev *sitdev,
> +				     u8 out_idx, s32 phase_ps)
> +{
> +	u8 old_bytes[SIT9531X_OUT_PRG_BYTES], new_bytes[SIT9531X_OUT_PRG_BYTES];
> +	const struct sit9531x_chip_info *info = sitdev->info;
> +	u64 fvco, divo, coarse;
> +	u8 pll_idx, fine;
> +	s32 phase_adj;
> +	int rc, ret;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	if (out_idx >= info->num_outputs)
> +		return -EINVAL;
> +
> +	pll_idx = sitdev->out[out_idx].pll_idx;
> +	if (pll_idx >= SIT9531X_NUM_PLLS)
> +		return -EINVAL;
> +

[Severity: High]
Does this logic fail to verify sitdev->out[out_idx].routed before
proceeding with pll_idx = 0?

Getter functions like sit9531x_output_phase_read() correctly check
!sitdev->out[out_idx].routed and abort with -ENODATA, but this function
proceeds to calculate and write register values against PLL 0 for unrouted
pins.

> +	rc = sit9531x_get_fvco(sitdev, pll_idx, &fvco);
> +	if (rc)
> +		return rc == -ENODATA ? -ENODEV : rc;
[ ... ]
> +flush:
> +	/*
> +	 * Restart the output divider phase so the freshly programmed delay is
> +	 * applied against a known edge instead of the divider's arbitrary
> +	 * running phase.
> +	 */
> +	rc = sit9531x_output_phase_flush(sitdev, pll_idx);

[Severity: High]
Since the hardware lacks a per-output flush mechanism, triggering this phase
flush with pll_idx = 0 for an unrouted output will forcefully restart the
dividers for all active outputs legitimately driven by PLL 0.

Could a user interacting with an unrouted, disconnected DPLL pin trigger
visible edge jumps and hardware disruption across unrelated active outputs?

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