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

[Ali Rouhi <arouhi@sitime.com>]

On Wed, 14 Oct 2026, sashiko-bot@kernel.org wrote:

Reply inline.  Taking the three High findings together first, since
they rest on the same premise, then the two Medium.

> [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.

> [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.

> [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?

An output the loaded configuration routes to no PLL is never
registered as a pin, so neither setter can be called for one.
out[].routed is set only by sit9531x_out_state_fetch() at probe, and
sit9531x_dpll_pin_is_registrable() requires it before a pin object is
created.  The pin callbacks are the only callers of
sit9531x_output_freq_set() and sit9531x_output_phase_adjust_set().
pll_idx 0 is therefore never reached on behalf of an unrouted output,
and no flush is issued on PLL A for one.

The asymmetry is real as a matter of style: the getters test routed
and the setters rely on registration instead.  Adding the test to the
setters has no caller that can exercise it, so it is left for a later
series rather than added here untested.

> [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.

> [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?

Agreed on both, and v14 sets flush_pending on both paths, as the phase
setter already did.

On the failed flush, v14 also stores the delay that was committed
instead of marking the cache stale.  A stale mark would have the
getter publish the new delay anyway, after which the core would treat
an identical retry as a request for the value the driver had just
reported, and the realignment would never run.