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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] dpll: sit9531x: sit9531x_prio_activesel_pick() latches
  lower-priority source
- [Medium] dpll: sit9531x: stale live_top after table commit causes
  spurious reselect
- [Medium] dpll: sit9531x: sit9531x_reselect() leaves PLL stuck in
  holdover
- [Medium] dpll: sit9531x: manual PLL priority table entries prevent
  disabling inputs
--

commit ace42e1765151d695e38e18abcdcf755dae0588e
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: implement input pin state on a DPLL

This commit implements getting and setting DPLL input pin state and
operational state by managing the per-PLL priority tables, input receiver
enables, and reference re-selection from holdover.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 209671e7917e2..af6d7918958dc 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> @@ -381,20 +486,460 @@ static u8 sit9531x_prio_slot_get(u8 val, u8 slot)

[ ... ]

> +/*
> + * Pick the active selection to latch; after a latch the PLL uses that source
> + * until it loses signal.
> + */
> +static u8 sit9531x_prio_activesel_pick(struct sit9531x_dev *sitdev,
> +				       const u8 *old, const u8 *srcs, u8 cur)
> +{
> +	u8 top, i;
> +
> +	top = sit9531x_prio_top_live(sitdev, srcs);
> +	if (top != SIT9531X_PRIO_SRC_NONE &&
> +	    top != sit9531x_prio_top_live(sitdev, old))
> +		return top;
> +
> +	cur = sit9531x_prio_src_canon(sitdev, cur & SIT9531X_PRIO_NIBBLE_MASK);
> +
> +	if (sit9531x_prio_src_usable(cur) &&
> +	    sit9531x_prio_src_live(sitdev, cur))
> +		for (i = 0; i < SIT9531X_PRIO_MAX_SLOTS; i++)
> +			if (srcs[i] == cur)
> +				return cur;
> +
> +	if (top != SIT9531X_PRIO_SRC_NONE)
> +		return top;
> +
> +	for (i = 0; i < SIT9531X_PRIO_MAX_SLOTS; i++)
> +		if (sit9531x_prio_src_usable(srcs[i]))
> +			return srcs[i];
> +
> +	return srcs[0];
> +}

[Severity: Medium]
Can sit9531x_prio_activesel_pick() re-latch a lower-priority source instead
of top when called from sit9531x_reselect()?

sit9531x_reselect() calls sit9531x_prio_table_commit(sitdev, pll_idx,
chan->prio_srcs), which passes old == chan->prio_srcs and srcs ==
chan->prio_srcs.  Because old and srcs are the same array (and
sit9531x_prio_top_live() evaluates both against the freshly fetched
sitdev->ref[].los state), top != sit9531x_prio_top_live(sitdev, old) is
always false.

If the previous CLK_ACTIVESEL_PLL nibble (cur) names a lower-priority entry
in srcs[] whose loss-of-signal bit is clear (such as OCXO, or a second source
that recovered in the same poll interval) while the PLL is not tracking it,
sit9531x_prio_activesel_pick() returns cur instead of top.  sit9531x_reselect()
then sets chan->live_top = top, leaving the PLL locked to the lower-priority
source without re-selecting top on later polls.

In addition, cur is canonicalized with sit9531x_prio_src_canon(), whereas
chan->prio_srcs[] holds uncanonicalized nibbles when read from hardware in
sit9531x_prio_mask_build(), so srcs[i] == cur and the fallback return srcs[i]
do not canonicalize differential N-lane codes.

[ ... ]

> +	if (!rc) {
> +		sit9531x_prio_mask_build(sitdev, pll_idx, srcs);
> +		memcpy(chan->seen_srcs, srcs, sizeof(chan->seen_srcs));
> +		chan->seen_valid = true;
> +		chan->table_pending = false;
> +	} else if (pending || pending_before) {

[Severity: Medium]
Does committing a new priority table leave chan->live_top stale?

When a user adds or removes an input pin or changes priorities via
sit9531x_prio_table_apply() -> sit9531x_prio_table_commit(), chan->live_top
is not updated to match the newly latched table.  If the next poll tick runs
while the PLL is still acquiring lock (chan->locked is false),
sit9531x_reselect() sees top != chan->live_top and calls
sit9531x_prio_table_commit() a second time, forcing the PLL back into
holdover and restarting lock acquisition.

[ ... ]

> @@ -851,34 +1615,211 @@ static int sit9531x_dev_state_fetch(struct sit9531x_dev *sitdev)

[ ... ]

> +/*
> + * Rebuild pll_mask from the cached priority tables and the manual-select
> + * inputs. Caller must hold sitdev->multiop_lock.
> + */
> +static void sit9531x_ref_pll_mask_rebuild(struct sit9531x_dev *sitdev)
> +{
> +	u8 pll_idx, src, index;
> +
> +	for (index = 0; index < sitdev->info->num_inputs; index++)
> +		sitdev->ref[index].pll_mask = 0;
> +
> +	for (pll_idx = 0; pll_idx < SIT9531X_NUM_PLLS; pll_idx++) {
> +		u16 mask = sitdev->chan[pll_idx].prio_mask;
> +
> +		for (src = 0; src < SIT9531X_PRIO_NUM_SRC; src++) {
> +			if (!(mask & BIT(src)))
> +				continue;
> +
> +			index = sit9531x_hw_src_input(src);
> +			if (index < sitdev->info->num_inputs)
> +				sitdev->ref[index].pll_mask |= BIT(pll_idx);
> +		}
> +	}
> +
> +	sit9531x_manual_claims_add(sitdev);
> +}

[Severity: Medium]
Should sit9531x_ref_pll_mask_rebuild() and sit9531x_ref_pll_mask_fetch() skip
PLLs with chan->manual_input set?

A manual-select PLL ignores its priority table and registers
sit9531x_dpll_input_pin_manual_ops without state_on_dpll_set, while
sit9531x_manual_claims_add() claims chan->manual_ref.  Because
sit9531x_ref_pll_mask_rebuild() (and sit9531x_ref_pll_mask_fetch(), which
also runs before sit9531x_chan_select_fetch() in sit9531x_dev_state_fetch())
folds chan[pll_idx].prio_mask into ref->pll_mask for manual PLLs too, any
input left in a manual PLL's unused priority table keeps BIT(pll_idx) set in
ref->pll_mask and cannot be disabled by sit9531x_input_disable() when
disconnected from all automatic PLLs.

[ ... ]

> +/*
> + * Rewrite the priority table when a listed source returns to a PLL without a
> + * reference; the latch restarts acquisition. Caller must hold
> + * sitdev->multiop_lock.
> + */
> +static void sit9531x_reselect(struct sit9531x_dev *sitdev, u8 pll_idx)
> +{
> +	struct sit9531x_chan *chan = &sitdev->chan[pll_idx];
> +	u8 top;
> +	int rc;
> +
> +	/* A PLL on manual input select uses no priority table */
> +	if (chan->manual_input || !chan->prio_mask || chan->mode ||
> +	    chan->ho_release_pending || chan->table_pending)
> +		return;
> +
> +	top = sit9531x_prio_top_live(sitdev, chan->prio_srcs);
> +	if (chan->reselect_armed) {
> +		if (top == SIT9531X_PRIO_SRC_NONE || !chan->ho_freeze) {
> +			chan->reselect_armed = false;
> +			chan->live_top = top;
> +			return;
> +		}
> +	} else {
> +		if (top == chan->live_top)
> +			return;
> +		/*
> +		 * Tracking is locked and not frozen; the two bits are read
> +		 * independently.
> +		 */
> +		if (top == SIT9531X_PRIO_SRC_NONE ||
> +		    (chan->locked && !chan->ho_freeze)) {
> +			chan->live_top = top;
> +			return;
> +		}
> +	}
> +
> +	rc = sit9531x_prio_table_commit(sitdev, pll_idx, chan->prio_srcs);

[Severity: Medium]
Can returning early on top == chan->live_top leave the PLL stuck in holdover
when a listed source is live?

If the PLL is locked to a fallback source IN1 when a higher-priority source
IN0 regains signal, sit9531x_reselect() sees chan->locked && !chan->ho_freeze,
records chan->live_top = IN0, and returns without re-latching.  If IN1 later
loses signal and the PLL enters holdover while IN0 is still live, subsequent
polls see top == chan->live_top (both IN0) and return before committing the
table, leaving the PLL in holdover indefinitely.  The same happens if the
active top source briefly drops signal (setting chan->ho_freeze) and recovers
between two polls so that top matches chan->live_top when polled.

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