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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 8 · Low: 5

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The driver's highest-priority automatic selection only takes
  effect when the table is written.
- [Medium] Even when a table write gives sit9531x_prio_activesel_pick()
  the chance to move to the highest-priority healthy source, it keeps a…
- [Medium] In sit9531x_prio_table_commit(), whether forced holdover is
  kept or released depends on `empty =…
- [Medium] If the table write and latch succeed but all
  SIT9531X_HO_CLEAR_TRIES attempts to clear HO_FORCE fail,…
- [Medium] sit9531x_input_prio_set() writes chan->cfg_prio[input_idx] and
  cfg_known before calling sit9531x_prio_table_apply(), and does not…
- [Medium] sit9531x_prio_table_commit() forces holdover on every table
  write, and keeps it forced indefinitely when the table becomes empty…
- [Medium] In sit9531x_dpll_input_pin_state_on_dpll_set() SELECTABLE, a
  powered-down receiver is enabled with sit9531x_input_enable() and…
- [Medium] In sit9531x_prio_table_commit(), a failed
  sit9531x_input_mon_fetch() only triggers a ratelimited warning…
- [Low] sit9531x_prio_table_commit() sets the PLL's HO_CTRL force-
  holdover bit (PLL page reg 0x6F, bit 4) with sit9531x_update_pll_u8()…
- [Low] Concern: this patch adds operstate_on_dpll_get for input pins,
  but sit9531x_dpll_changes_check() compares only state_on_dpll_get…
- [Low] When a table commit fails and the read-back in
  sit9531x_prio_table_commit() (`else if…
- [Low] sit9531x_prio_table_commit() sets HO_FORCE through
  sit9531x_update_pll_u8(), a regmap_update_bits() read-modify-write,…
- [Low] The rollback in sit9531x_prio_table_commit() covers only
  registers whose writes succeeded: `written = slot + 2` advances after…

--

dpll: sit9531x: implement input pin state on a DPLL

This commit adds state_on_dpll_get/set and operstate_on_dpll_get for
sit9531x input pins. SELECTABLE and DISCONNECTED map to membership in each
PLL's hardware priority table. A table rewrite forces the PLL into
holdover, writes every slot, picks the active selection from the clock
monitor state, latches, and releases holdover again.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index bf6817b9d5656..231e2d71dd6fb 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> @@ -433,26 +526,537 @@ static u8 sit9531x_prio_slot_get(u8 val, u8 slot)

[ ... ]

> +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;
> +
> +	/*
> +	 * The table is built from canonical codes, so compare in the same
> +	 * terms: a differential pair selected through its N-lane code is the
> +	 * P-lane entry.
> +	 */
> +	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;

[Severity: Medium]
Can this keep a PLL on a lower-priority source while a higher-priority
one is healthy?

Take a table of A, B, C where A and B are live. CLK_ACTIVESEL names B
because an earlier pick ran while A had LOS. Disconnecting C builds A, B.

sit9531x_prio_top_live() returns A for both the old and the new table, so
the early return is skipped. cur is B, which is usable, live and listed,
so B is written again.

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?

> +
> +	if (top != SIT9531X_PRIO_SRC_NONE)
> +		return top;
> +
> +	return srcs[0];
> +}

[Severity: Medium]
Is the active selection ever looked at again once a higher-priority input
recovers?

The comment above this function says the PLL leaves the source named by
CLK_ACTIVESEL only when that source loses signal. If a table write
happens while the top-priority member has LOS, the pick chooses a
lower-priority live member.

When the preferred input comes back, sit9531x_dev_periodic_work() only
refreshes caches through sit9531x_dev_ref_states_update() and
sit9531x_dev_chan_states_update(). sit9531x_prio_activesel_pick() is
called only from sit9531x_prio_table_commit().

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

> +
> +static int sit9531x_prio_table_commit(struct sit9531x_dev *sitdev, u8 pll_idx,
> +				      const u8 *srcs)
> +{

[ ... ]

> +	empty = !sit9531x_prio_src_usable(srcs[0]);

[Severity: Medium]
Should empty come from the table the device ends up holding, not from the
requested one?

Start with a PLL whose last input was disconnected. The empty table was
written, and the "if (empty && !rc) return 0;" exit kept HO_FORCE set.
The ACTIVESEL nibble still names the old source.

A later SELECTABLE request goes through
sit9531x_input_prio_add()->sit9531x_prio_table_apply()->
sit9531x_prio_table_commit() with a non-empty table, so empty is false.

Suppose the first slot write fails, so written stays 0. Or a later write
fails and the rollback restores the empty table. Either way the read-back
rebuilds prio_mask from a table that is still empty.

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? The comment further down says the PLL then keeps
following that source "for as long as it has signal", while every pin
reads DISCONNECTED.

The mirror case looks similar. An empty request whose latch fails ends up
with rc != 0, so it also falls through to the release loop.

> +
> +	rc = sit9531x_update_pll_u8(sitdev, pll_idx, SIT9531X_PLL_REG_HO_CTRL,
> +				    BIT(SIT9531X_PLL_HO_FORCE_BIT),
> +				    BIT(SIT9531X_PLL_HO_FORCE_BIT));
> +	if (rc)
> +		return rc;

[Severity: Low]
Can this early return leave HO_FORCE asserted?

sit9531x_update_pll_u8() is a regmap_update_bits() read-modify-write, so
an error does not prove the bit stayed clear. Later in the series,
sit9531x_output_divo_write() assumes that a write which reported an error
may still have reached the part.

Every other error path in this function goes through the release loop.
This one returns without trying to clear the bit. The callers then see
unchanged membership through sit9531x_input_prio_present() and treat the
request as having had no effect.

Should this path also try to clear the bit before returning?

> +
> +	usleep_range(10000, 12000);

[ ... ]

> +	for (slot = 0; slot + 1 < SIT9531X_PRIO_MAX_SLOTS; slot += 2) {
> +		reg = sit9531x_prio_reg(pll_idx, slot);
> +
> +		val = sit9531x_prio_slot_set(0, slot, srcs[slot]);
> +		val = sit9531x_prio_slot_set(val, slot + 1, srcs[slot + 1]);
> +
> +		rc = sit9531x_write_u8(sitdev, reg, val);
> +		if (rc)
> +			goto rollback;
> +
> +		written = slot + 2;
> +	}

[ ... ]

> +	reg = sit9531x_prio_reg(pll_idx, slot);
> +
> +	rc = sit9531x_read_u8(sitdev, reg, &val);
> +	if (rc)
> +		goto rollback;
> +
> +	val = sit9531x_prio_slot_set(val, slot, srcs[slot]);
> +
> +	if (!empty) {
> +		u8 sel = sit9531x_prio_slot_get(val, slot + 1);
> +
> +		if (sit9531x_input_mon_fetch(sitdev))
> +			dev_warn_ratelimited(sitdev->dev,
> +					     "PLL%c: input monitor not read; choosing the selection without it\n",
> +					     'A' + pll_idx);
> +		sel = sit9531x_prio_activesel_pick(sitdev, chan->prio_srcs,
> +						   srcs, sel);
> +		val = sit9531x_prio_slot_set(val, slot + 1, sel);
> +	}

[Severity: Medium]
Is it safe to go ahead with the pick when sit9531x_input_mon_fetch()
fails?

If a register read fails, sit9531x_input_mon_fetch() returns before it
updates any ref->los. The pick then runs on values from the last poll,
which can be up to SIT9531X_STATUS_POLL_MS (500 ms) old. The comment
above says the pick "needs the signal state now, not as of the last
poll".

Say A was live at the last poll and has since lost signal. The device has
fallen back to B, but the nibble still names A. With the monitor read
failing, A still looks live, so cur = A is kept and written, and the
latch succeeds.

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?

> +
> +	rc = sit9531x_write_u8(sitdev, reg, val);
> +	if (rc)
> +		goto rollback;
> +
> +	written = SIT9531X_PRIO_MAX_SLOTS;
> +
> +rollback:
> +	if (rc && written) {

[ ... ]

> +		for (slot = 0; slot < written; slot += 2) {
> +			u8 old;
> +
> +			old = sit9531x_prio_slot_set(0, slot,
> +						     chan->prio_srcs[slot]);
> +			old = sit9531x_prio_slot_set(old, slot + 1,
> +						     chan->prio_srcs[slot + 1]);
> +			if (sit9531x_write_u8(sitdev,
> +					      sit9531x_prio_reg(pll_idx, slot),
> +					      old))
> +				break;
> +
> +			restored = slot + 2;
> +		}
> +		written = restored;
> +	}
> +

[ ... ]

> +	prg_rc = sit9531x_prio_prg_commit(sitdev);
> +	if (prg_rc && !rc)
> +		rc = prg_rc;

[Severity: Low]
Does the rollback skip the register whose write failed?

written only advances after a successful write, and the restore loop runs
while slot < written. The register that returned the error is never
restored, even though its write may have reached the device. The latch
then runs unconditionally.

If the last register fails, written is 10. That register holds slot 10
and the CLK_ACTIVESEL nibble. Slots 0-9 are restored, but a possibly
changed slot 10 and selection are latched. sit9531x_prio_table_read()
does not read ACTIVESEL, so the read-back would not catch it.

Later in the series, sit9531x_output_divo_write() restores "the byte
whose write reported the error" (j <= written). Should this sequence do
the same?

> +
> +	/*
> +	 * Refresh the cache so a get that follows a set does not have to

[ ... ]

> +	if (!rc) {
> +		sit9531x_prio_mask_build(sitdev, pll_idx, srcs);
> +		memcpy(chan->seen_srcs, srcs, sizeof(chan->seen_srcs));
> +		chan->seen_valid = true;
> +	} else if (!sit9531x_prio_table_read(sitdev, pll_idx, now)) {
> +		sit9531x_prio_mask_build(sitdev, pll_idx, now);
> +		memcpy(chan->seen_srcs, now, sizeof(chan->seen_srcs));
> +		chan->seen_valid = true;
> +	}

[Severity: Low]
What happens to seen_srcs when the commit and this read-back both fail?

chan->seen_srcs then keeps the old table, while the device may hold a
partly written or partly restored one. On the next poll,
sit9531x_chan_state_fetch() sees the mismatch:

	if (!chan->seen_valid ||
	    memcmp(srcs, chan->seen_srcs, sizeof(chan->seen_srcs)))
		sit9531x_prio_cfg_seed(sitdev, pll_idx, srcs);

sit9531x_prio_cfg_seed() then sets cfg_prio[src] = slot for every listed
source.

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.

> +
> +	/*
> +	 * A table that names no source keeps the PLL in the holdover forced

[ ... ]

> +	if (empty && !rc) {
> +		dev_dbg(sitdev->dev,
> +			"PLL%c: no source listed, holdover kept\n",
> +			'A' + pll_idx);
> +		return 0;
> +	}

[Severity: Medium]
Should chan->ho_valid be checked before holdover is kept forced here?

For DPLL_LOCK_STATUS_HOLDOVER, the uAPI in include/uapi/linux/dpll.h says
holdover forced by disconnecting all the pins is "possible only when dpll
lock-state was already DPLL_LOCK_STATUS_LOCKED_HO_ACQ". Otherwise "the
dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED".

Here, disconnecting the last input still forces holdover on a PLL that
has not acquired holdover memory. sit9531x_dpll_lock_status_get() then
reports DPLL_LOCK_STATUS_HOLDOVER from chan->ho_freeze, whatever
ho_valid says.

Doesn't that keep the device on a holdover estimate it never marked
valid, while userspace is told it is in holdover?

> +
> +	/*
> +	 * Release the forced holdover.  Apart from an empty table, nothing

[ ... ]

> +	for (attempt = 0; attempt < SIT9531X_HO_CLEAR_TRIES; attempt++) {
> +		ho_rc = sit9531x_update_pll_u8(sitdev, pll_idx,
> +					       SIT9531X_PLL_REG_HO_CTRL,
> +					       BIT(SIT9531X_PLL_HO_FORCE_BIT),
> +					       0);
> +		if (!ho_rc)
> +			break;
> +		usleep_range(1000, 2000);
> +	}
> +	if (ho_rc) {
> +		dev_err(sitdev->dev, "PLL%c left in forced holdover: %d\n",
> +			'A' + pll_idx, ho_rc);
> +		if (!rc)
> +			rc = ho_rc;
> +	}
> +
> +	return rc;
>  }

[Severity: Medium]
What happens if the table write and latch succeed, but all
SIT9531X_HO_CLEAR_TRIES attempts here fail?

rc was still 0 at that point, so sit9531x_prio_mask_build() has already
recorded the new membership, and the function returns ho_rc. In the
SELECTABLE handler sit9531x_input_prio_present() is now true, so the
claim is taken and the error is returned to userspace.

If the same SELECTABLE request is retried after the bus recovers,
sit9531x_input_prio_add() returns early:

	if (chan->prio_mask & BIT(input_idx))
		return 0;

Retrying DISCONNECTED short-circuits the same way, and so does setting an
equal priority (the memcmp() in sit9531x_prio_table_apply()).

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.

[Severity: Low]
Is the old value of HO_FORCE meant to be thrown away?

The bit is set at the top of sit9531x_prio_table_commit() without saving
its previous value, and it is cleared here unconditionally. 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.

The commit message describes the sequence as "forcing the PLL into
holdover, waiting for it to take, writing every slot and releasing
holdover again". It does not say that an existing forced holdover is
lost.

[ ... ]

> @@ -479,6 +1083,137 @@ static int sit9531x_prio_table_read(struct sit9531x_dev *sitdev, u8 pll_idx,

[ ... ]

> +int sit9531x_input_prio_set(struct sit9531x_dev *sitdev, u8 pll_idx,
> +			    u8 input_idx, u8 prio)
> +{

[ ... ]

> +	chan = &sitdev->chan[pll_idx];
> +	chan->cfg_prio[input_idx] = prio;
> +	chan->cfg_known |= BIT(input_idx);
> +
> +	if (!(chan->prio_mask & BIT(input_idx)))
> +		return 0;
> +
> +	return sit9531x_prio_table_apply(sitdev, pll_idx, chan->prio_mask);
> +}

[Severity: Medium]
Should cfg_prio[] and cfg_known be restored when
sit9531x_prio_table_apply() fails?

For example, if the first HO_FORCE update in sit9531x_prio_table_commit()
fails, no table register is written, but the new priority stays recorded.

Later in the series ("dpll: sit9531x: add support to get and set priority
on input pins") this is wired to netlink. On failure,
sit9531x_dpll_input_pin_prio_set() returns the error and leaves
dpin->prio alone, but sit9531x_input_prio_get() now reports the new
value. The poll sees a changed priority and sends a notification for a
priority that was refused.

The next unrelated add or remove also rebuilds the table with the
refused priority. That can change the preferred source as a side effect.

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 924386aec4d88..f961b7af28fed 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> @@ -231,8 +350,163 @@ sit9531x_dpll_input_pin_direction_get(const struct dpll_pin *pin,

[ ... ]

> +	case DPLL_PIN_STATE_SELECTABLE:

[ ... ]

> +		if (!ref->enabled) {
> +			rc = sit9531x_input_enable(sitdev, dpin->id);
> +			if (rc)
> +				break;
> +			enabled_here = true;
> +		}
> +		rc = sit9531x_input_prio_add(sitdev, sitdpll->id, hw_src);

[Severity: Medium]
Has the clock monitor had time to qualify a newly enabled input by the
time the selection is picked?

When the receiver was off, sit9531x_input_enable() powers it up and
sit9531x_input_prio_add() runs right away. The commit only samples the
monitors with sit9531x_input_mon_fetch() after the 10-12 ms holdover
sleep, and sit9531x_prio_activesel_pick() decides from ref->los.

A low-rate reference such as 1 PPS cannot be qualified in that window. So
the lane still shows LOSS, and the pick skips it even if it is now the
highest-priority member.

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?

[ ... ]

>  static const struct dpll_pin_ops sit9531x_dpll_input_pin_ops = {
>  	.direction_get		= sit9531x_dpll_input_pin_direction_get,
> +	.state_on_dpll_get	= sit9531x_dpll_input_pin_state_on_dpll_get,
> +	.state_on_dpll_set	= sit9531x_dpll_input_pin_state_on_dpll_set,
> +	.operstate_on_dpll_get	= sit9531x_dpll_input_pin_operstate_on_dpll_get,
>  };

[Severity: Low]
Now that operstate_on_dpll_get is added, does anything send a
dpll_pin_change_ntf() when the operstate changes?

At this commit sit9531x_dpll_changes_check() compares only the pin state:

	changed = pin->seen && state != pin->pin_state;

So LOS, qual-fail and active-reference changes are not notified.

The next patch in the series, "dpll: sit9531x: add support to get and
set priority on input pins", adds the operstate comparison to
sit9531x_dpll_changes_check(). The gap seems to exist only at this
commit.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930233714.87679-1-arouhi%40sitime.com