Re: [PATCH net-next v11 07/13] dpll: sit9531x: implement input pin state on a DPLL

[Ali Rouhi <arouhi@sitime.com>]

On Mon, 5 Oct 2026, netdev-bot+sashiko@kernel.org wrote:

Replies inline, in the order of the summary list. The first, second and
seventh items are one question and are answered together.

> [Severity: Medium]
> Is the active selection ever looked at again once a higher-priority input
> recovers?
>
> [...]
>
> Doesn't that leave the PLL on the lower-priority input indefinitely,
> while sit9531x_dpll_mode_get() reports DPLL_MODE_AUTOMATIC? The commit
> message says the selection goes to "the highest-priority valid input,
> which is how the DPLL interface defines automatic mode".

> [Severity: Medium]
> Can this keep a PLL on a lower-priority source while a higher-priority
> one is healthy?
>
> [...]
>
> sit9531x_dpll_mode_get() always reports DPLL_MODE_AUTOMATIC. The uAPI
> defines that as the highest priority input pin being auto selected by the
> dpll. The commit message describes this keep-in-place policy, but it also
> says the driver follows the highest-priority valid input. How do the two
> fit together?

> [Severity: Medium]
> Has the clock monitor had time to qualify a newly enabled input by the
> time the selection is picked?
>
> [...]
>
> The device only leaves its selection on LOS, and no poll path rewrites
> CLK_ACTIVESEL. Wouldn't the PLL stay on the lower-priority source after
> the new input qualifies?

That is the intended policy and the commit message now says it more
plainly. The device re-runs its own selection only when the source it
follows loses signal; it does not notice a table rewrite. Moving
CLK_ACTIVESEL on every commit was the first version of this fix. Each
move is a re-selection, which takes the PLL through holdover and unlocks
it for about ten seconds, so a priority change further down the table, or
the removal of a source the PLL is not on, must not move it. The rule
became: move only when the highest-priority live source is a different
one than before, and otherwise stay on the current source while it is
listed and has signal.

What that leaves open is the case all three items describe: the preferred
source recovers, or a newly enabled receiver qualifies after the pick
ran, and nothing re-selects. The device's own revertive switching returns
only to the source CLK_ACTIVESEL names, so the driver would have to
re-pick from the poll when the highest-priority live source differs from
the one in use. That is a re-selection, with the unlock it costs, on a
timer rather than on a request, and whether to take it under AUTOMATIC or
to document the behavior as non-revertive is still open. The commit
message states the rule as implemented and the open point; the mechanism
follows in a later revision.

> [Severity: Medium]
> Should empty come from the table the device ends up holding, not from the
> requested one?
>
> [...]
>
> The release loop then clears HO_FORCE. Wouldn't the device be left with
> an empty table, no forced holdover, and an ACTIVESEL naming the
> disconnected source?

Fixed: the decision is made on the table the device holds after the
refresh, not on the request. A failed write on a PLL whose table was
empty keeps the hold, and so does an empty request whose latch failed.

> [Severity: Medium]
> What happens if the table write and latch succeed, but all
> SIT9531X_HO_CLEAR_TRIES attempts here fail?
>
> [...]
>
> Doesn't the retry then report success while the PLL stays in forced
> holdover? The poll does not retry the release either, so the PLL seems to
> stay there until some unrelated table edit runs the full sequence again.

Fixed: the release is owed and the poll retries it every tick until it
lands, logging when it does. The request still returns the error, since
the PLL is not tracking when it returns.

> [Severity: Medium]
> Should cfg_prio[] and cfg_known be restored when
> sit9531x_prio_table_apply() fails?
>
> [...]
>
> The poll sees a changed priority and sends a notification for a
> priority that was refused.

Fixed: the configured priority and its known flag are saved before the
apply and put back when it fails, so nothing reports a priority the
device never took and the next rebuild does not use it.

> [Severity: Medium]
> Should chan->ho_valid be checked before holdover is kept forced here?
>
> [...]
>
> Doesn't that keep the device on a holdover estimate it never marked
> valid, while userspace is told it is in holdover?

The force stays: it is the only way the device follows no input at all,
and a PLL whose table is empty must not keep running on a source every
pin reports as disconnected.

What changes is the report. When the driver itself forced holdover for an
empty table and the device has not marked its holdover value valid,
lock_status_get() now reports UNLOCKED, as the uAPI text asks; HOLDOVER
is reported only when the device says the estimate is valid.

> [Severity: Medium]
> Is it safe to go ahead with the pick when sit9531x_input_mon_fetch()
> fails?
>
> [...]
>
> The comment in sit9531x_prio_activesel_pick() says such a write sends the
> PLL back to the dead source and it unlocks. Wouldn't that happen here,
> with the request reported as a success?

Fixed: a monitor read that fails rolls the commit back and returns the
error. The selection is never picked from a loss-of-signal state that may
be a poll period old.

> [Severity: Low]
> Can this early return leave HO_FORCE asserted?
>
> [...]
>
> Should this path also try to clear the bit before returning?

> [Severity: Low]
> Is the old value of HO_FORCE meant to be thrown away?
>
> [...]
>
> Say the loaded profile or an external tool left a PLL in forced
> holdover. Any SELECTABLE or DISCONNECTED change on that PLL, or a later
> priority set, would then quietly release it and let it lock to a
> reference.

Fixed, in two parts. A force that failed to write is released all the
same, because the write may have landed: the failure goes to the release
path rather than returning. And a hold the driver did not set -- one
found set with a non-empty table and no release owed, so placed by the
loaded configuration or by a tool -- is left in place by a table write;
only the hold the driver set for an empty table is released. The force is
written as the register was read with the bit set, so the
read-modify-write the finding points at is gone.

> [Severity: Low]
> Now that operstate_on_dpll_get is added, does anything send a
> dpll_pin_change_ntf() when the operstate changes?
>
> [...]
>
> The gap seems to exist only at this commit.

That is the state of this patch; the next one, which adds priority,
extends the comparison to operational state and priority, and that is
where the series ends up. Moving the comparison into this patch would
compare an attribute the patch does not yet report.

> [Severity: Low]
> What happens to seen_srcs when the commit and this read-back both fail?
>
> [...]
>
> Doesn't that overwrite priorities set through sit9531x_input_prio_set()
> with slot positions from a table nobody asked for? The reseed check
> cannot tell a failed write by the driver apart from an outside rewrite.

Right: two consecutive bus failures make the next poll adopt whatever
table the device holds, and the configured priorities become the slot
positions of that table. Adopting the device's table is the designed
recovery for an outside rewrite, and the driver cannot tell the two apart
from the table alone. Remembering that its own commit failed, so that the
next poll refreshes seen_srcs without re-seeding the configured
priorities, is a small flag and is noted for a later revision. The
priorities the poll reports in the meantime are the ones actually in the
device.

> [Severity: Low]
> Does the rollback skip the register whose write failed?
>
> [...]
>
> Later in the series, sit9531x_output_divo_write() restores "the byte
> whose write reported the error" (j <= written). Should this sequence do
> the same?

Fixed: the register whose write failed is restored too, from the byte
read before the write, which is what the output divider's rollback
already did.