Re: [PATCH net-next v14 07/12] dpll: sit9531x: add support to get and set priority on input pins

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 1

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] A priority that the device has latched can be rolled back in
  the driver's records and reported to userspace as refused.
- [Medium] A priority change confined to standby inputs can move the PLL
  off a healthy fallback reference.
- [Low] The default priority for a source a PLL has never listed is
  SIT9531X_PRIO_MAX_SLOTS - 1 = 10.

--

dpll: sit9531x: add support to get and set priority on input pins

This patch adds prio_get and prio_set callbacks for the sit9531x input
pins. Both work on a per-PLL configured priority, and the Page 1 table is
rebuilt in priority order when a listed input's priority changes. The
poll now also watches priority and operational state, and the baseline
for that comparison is taken when each pin is registered.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 1aad598930d72..f3dbe949f219b 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -671,6 +671,44 @@ bool sit9531x_input_prio_present(struct sit9531x_dev *sitdev, u8 pll_idx,
>  	return !!(sitdev->chan[pll_idx].prio_mask & BIT(input_idx));
>  }
>  
> +/*
> + * sit9531x_input_prio_get - read an input's priority for a PLL
> + * @input_idx:	input source in hardware encoding (see
> + *		sit9531x_input_hw_src())
> + * @prio:	output priority (lower is preferred)
> + *
> + * Reports the priority configured for the source on this PLL, which is
> + * kept whether or not the source is currently in the table: state and
> + * priority are separate attributes, so disconnecting an input and
> + * connecting it again must not change the priority it reports.  The
> + * value is seeded from the hardware table, and re-seeded whenever the
> + * read-back shows the table was rewritten by something other than this
> + * driver.  A source that was never listed reports the lowest slot.

[ ... ]

> +	chan = &sitdev->chan[pll_idx];
> +	if (chan->cfg_known & BIT(input_idx))
> +		*prio = chan->cfg_prio[input_idx];
> +	else
> +		*prio = SIT9531X_PRIO_MAX_SLOTS - 1;
> +
> +	return 0;
> +}

[Severity: Low]
Is "the lowest slot" still accurate now that priorities from 0 to 255 are
accepted?

The default for a source the PLL has never listed is
SIT9531X_PRIO_MAX_SLOTS - 1, which is 10. sit9531x_prio_cfg_seed() seeds
it the same way:

	chan->cfg_prio[src] = SIT9531X_PRIO_MAX_SLOTS - 1;
	chan->cfg_known |= BIT(src);

The commit message says the same thing:

  "An input the PLL has never listed reports the lowest slot, the
  priority it would be connected at."

However, sit9531x_dpll_input_pin_prio_set() accepts values up to U8_MAX:

	if (prio > U8_MAX) {
		NL_SET_ERR_MSG(extack, "Priority out of range (0-255)");

sit9531x_prio_table_build() sorts members by cfg_prio and uses
prio_last only to break ties:

	kc = (chan->cfg_known & BIT(cur)) ?
	     chan->cfg_prio[cur] : U8_MAX;

Here is an example. IN0 is listed and is set to 50. IN1 has never been
listed and keeps the default of 10. Connecting IN1 then goes through
sit9531x_input_prio_add()->sit9531x_prio_table_apply()->
sit9531x_prio_table_build(), which puts IN1 in slot 0, ahead of IN0.
sit9531x_prio_activesel_pick() may then move the PLL to it.

Should the default follow the real lowest priority (U8_MAX, for example),
or should the comments and commit message stop promising "lowest"?

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 4c7cae4ce93ec..0400444d91a55 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> +static int
> +sit9531x_dpll_input_pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
> +				 const struct dpll_device *dpll,
> +				 void *dpll_priv, u32 prio,
> +				 struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_input_prio_set(sitdev, sitdpll->id,
> +				     sit9531x_input_hw_src(dpin->id),
> +				     (u8)prio);

[Severity: Medium]
Can a priority change that only touches standby inputs move the PLL off
a healthy fallback reference?

sit9531x_prio_table_commit() takes the current source from the
CLK_ACTIVESEL nibble it reads back, not from the reference actually
routed to the PLL:

		u8 sel = sit9531x_prio_slot_get(val, slot + 1);
		...
		sel = sit9531x_prio_activesel_pick(sitdev, chan->prio_srcs,
						   srcs, sel);

The driver's comments say that when the selected source A loses its
signal, the device fails over to B on its own and leaves the selection
naming A. Suppose A then recovers while the PLL is still on B.

Now a prio_set that only reorders inputs below A and B rewrites the
table. In sit9531x_prio_activesel_pick(), A is the top live source in
both the old and the new table, so the function falls through to:

	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;

It returns A, and latching that selection moves the PLL from B to A.

Doesn't that contradict "a change further down the table leaves the PLL
where it is" in the commit message? The comment above the pick also
says "a change further down the table must not pull a PLL off a healthy
reference".

The selection logic came with the previous patch, for state changes.
This patch adds prio_set as a new way to reach it. When the top is
unchanged, would keeping the routed reference (chan->routed_ref) rather
than the stale selection nibble avoid the switch?

> +	if (!rc)
> +		dpin->prio = prio;
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (rc) {
> +		NL_SET_ERR_MSG(extack, "Failed to set input priority");
> +		return rc;
> +	}

[Severity: Medium]
Can the driver's record of this priority end up disagreeing with what
the device latched?

In sit9531x_prio_table_commit(), the slot writes and the latch can
succeed. The new table is then recorded as the read-back baseline:

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

The holdover release runs after that. If all SIT9531X_HO_CLEAR_TRIES
attempts fail:

	if (ho_rc) {
		...
		if (!rc)
			rc = ho_rc;
	}
	chan->ho_release_pending = !!ho_rc;

sit9531x_ho_release_retry() clears HO_FORCE later from the poll, so the
PLL ends up running the new order.

Meanwhile sit9531x_input_prio_set() sees rc != 0 and reverts:

	rc = sit9531x_prio_table_apply(sitdev, pll_idx, chan->prio_mask);
	if (rc) {
		chan->cfg_prio[input_idx] = old_prio;
		chan->cfg_known = old_known;
	}

sit9531x_dpll_input_pin_prio_set() then returns the error and leaves
dpin->prio as it was.

sit9531x_chan_state_fetch() re-seeds only when the read-back differs
from seen_srcs:

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

seen_srcs already holds the new table, so the poll never repairs this.
The getter and the poll snapshot keep reporting the old priority while
the device holds the new order, and no notification is sent.

The next sit9531x_prio_table_apply() for some unrelated reason, such as a
sibling's state change, builds from the reverted cfg_prio. That reorders
the device back without anyone asking, and
sit9531x_prio_activesel_pick() may move the PLL off its reference.

A latch failure in sit9531x_prio_prg_commit(), or a rollback that fails
part-way, looks like it ends in the same split. In both cases seen_srcs
comes from the read-back table while cfg_prio has been reverted.

The commit message says:

  "A priority the table write refused is not kept: the configured
  value goes back to what it was, so nothing reports a priority the
  device never took."

On this path, isn't the driver reporting the opposite of what the device
holds? Could the rollback be based on what the read-back table actually
encodes, rather than on the errno?

[ ... ]

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