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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dpll: phase adjust on unrouted output causes spurious flush on PLL 0
--

commit 110382f4bcb6a9f0c30f287863026dd1cb412fb3
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to adjust output phase

This commit introduces support for output phase adjustment in the SiT9531x
driver. It computes coarse and fine delays and implements the hardware
programming sequence, including divider realignments.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> +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 code need to check if the output is actually routed?

For unrouted outputs, the initial state fetch assigns a default placeholder
pll_idx = 0. Without verifying sitdev->out[out_idx].routed is true, this
extracts pll_idx = 0 and computes phase bytes against PLL 0's Fvco when
userspace requests a phase adjustment on a disconnected pin.

[ ... ]
> +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]
Will this cause a spurious flush on PLL 0 for unrouted pins?

If the request proceeded with the placeholder pll_idx = 0, this flush
restarts the divider phase for every output driven by PLL 0. This could
subject active, properly routed outputs on PLL 0 to an unintended phase
jump and loss of lock, simply because userspace issued a valid DPLL netlink
request on a disconnected pin.

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