Re: [PATCH net-next v14 06/12] dpll: sit9531x: implement input pin state on a DPLL

[netdev-bot+sashiko@kernel.org]

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

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

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] sit9531x_prio_table_commit() decides whether an existing
  forced holdover belongs to someone else with `ho_foreign = (val &…
- [Medium] Qualification failure is ignored both when choosing a source
  and when reporting ACTIVE.
- [Medium] After a failed commit, sit9531x_prio_table_commit() decides at
  `release:` whether to keep or release the hold by reading…
- [Medium] sit9531x_input_prio_set() rolls cfg_prio/cfg_known back on any
  error from sit9531x_prio_table_apply(), including the case where the…
- [Medium] When sit9531x_prio_table_commit() cannot release its own
  forced holdover, it sets chan->ho_release_pending, and only the poll…
- [Medium] ref->pll_mask alone decides when a shared input receiver is
  forced off, and it counts only the four PLLs' priority tables…
- [Medium] sit9531x_prio_src_live() treats the inter-PLL sync net (code
  6) as always having signal: hw_src_input() yields…
- [Medium] On a failed table write, sit9531x_prio_table_commit() rolls
  back and then latches unconditionally.
- [Low] operstate_on_dpll_get reports a value the device changes on its
  own, but at this commit sit9531x_dpll_changes_check() polls only…
- [Low] sit9531x_chan_routed_ref_read() writes the unlock key
  (SIT9531X_PLL_DEBUG_UNLOCK) to PLL-page 0xBD and returns immediately…

--

dpll: sit9531x: implement input pin state on a DPLL

This adds state_on_dpll_get/set and operstate_on_dpll_get for the sit9531x
input pins.  An input's state comes from whether it is in each PLL's
priority table.  The table is rewritten under a forced holdover with a
newly chosen active selection.  Operstate comes from the routed source on
the debug status bus and from each lane's clock monitor.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 78a7a1def2137..1aad598930d72 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> @@ -433,26 +527,595 @@ static u8 sit9531x_prio_slot_get(u8 val, u8 slot)

[ ... ]

> +static bool sit9531x_prio_src_live(const struct sit9531x_dev *sitdev, u8 src)
> +{
> +	u8 index = sit9531x_hw_src_input(src);
> +
> +	if (index >= sitdev->info->num_inputs)
> +		return true;

[Severity: Medium]
Does this treat the inter-PLL sync net as always having signal?  For
SIT9531X_PRIO_SRC_INTSYNC, sit9531x_hw_src_input() returns
SIT9531X_INTSYNC_PIN_ID.  That is >= num_inputs, so this returns true.

The net only carries a clock while another PLL drives it
(sitdev->intsync_src).  INTSYNC can end up listed above live inputs while
no PLL drives it, for example after a prio_set gives it priority 0, or
when a profile ranks it first.  In that case sit9531x_prio_top_live()
returns it, and sit9531x_prio_activesel_pick() writes it into
CLK_ACTIVESEL.

The comment on sit9531x_prio_activesel_pick() says a selection naming a
dead source sends the PLL to it and it unlocks.  Could that happen here
once the hold is released?

Later in the series, the dpll.c side treats an undriven net as NO_SIGNAL
(sit9531x_dpll_selection_no_signal()), but this picker does not.

> +
> +	return !sitdev->ref[index].los;
> +}

[ ... ]

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

[ ... ]

> +	rc = sit9531x_read_pll_u8(sitdev, pll_idx, SIT9531X_PLL_REG_HO_CTRL,
> +				  &val);
> +	if (rc)
> +		return rc;
> +
> +	ho_foreign = (val & BIT(SIT9531X_PLL_HO_FORCE_BIT)) &&
> +		     chan->prio_mask && !chan->ho_release_pending;

[Severity: Medium]
Can this tell a hold the driver set from one it did not?  Any forced hold
found while the cached table is empty is treated as the driver's own.
Nothing records whether the driver actually set it.

Path 1: the loaded profile leaves a PLL with an empty table and HO_CTRL
bit 4 set.  Probe gives prio_mask == 0, and a user sets an input
SELECTABLE.  ho_foreign is false, prio_mask becomes non-zero, and the
release loop clears a force bit the driver never set.

Path 2: a tool forces holdover while the table lists sources, and the
user removes the last source.  ho_foreign is true, but the
if (!chan->prio_mask) branch after release: returns before ho_foreign is
checked, and nothing remembers that the hold was foreign.  On the next
add, prio_mask == 0 makes ho_foreign false and the hold is lifted.

The commit message says:

    A holdover the driver
    did not force -- set by the loaded configuration or by a tool -- is left
    in place by such a write; only the hold the driver itself set for an
    empty table is released.

To make that hold, would the driver need an explicit flag that records
when it owns the hold?

[ ... ]

> +rollback:
> +	if (rc && written) {

[ ... ]

> +		for (slot = 0; slot < written; slot += 2) {
> +			const u8 *prev = &chan->prio_srcs[slot];
> +			u8 old;
> +
> +			if (slot + 1 < SIT9531X_PRIO_MAX_SLOTS) {
> +				old = sit9531x_prio_slot_set(0, slot, prev[0]);
> +				old = sit9531x_prio_slot_set(old, slot + 1,
> +							     prev[1]);
> +			} else {
> +				old = last_old;
> +			}

[Severity: Medium]
Can this rollback move a PLL off a healthy fallback?  Say the PLL has
failed over from A to B and A is still in LOS.  The selection nibble in
last_old then still names A.

If the last register is in the rollback range, it is restored to
last_old.  On an earlier failure, such as sit9531x_input_mon_fetch()
failing, it is never touched.  Either way, the unconditional
sit9531x_prio_prg_commit() below latches a selection that names the dead
A.

The comment on sit9531x_prio_activesel_pick() says that in this
situation "the next table write sends the PLL back to it and it
unlocks".  Once the hold is released, would a failed request then unlock
a PLL that was locked to B, instead of leaving the device as the request
found it?

[ ... ]

> +	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: Medium]
What happens if this read-back also fails?  prio_mask then still holds
the table from before the write.  The release: block below uses that
stale mask to decide whether to keep or release the hold.

Case A, adding the first source to an empty table: the force write and
every slot write succeed, then sit9531x_prio_prg_commit() fails and so
does the read-back.  prio_mask stays 0, so the if (!chan->prio_mask)
branch keeps the hold and clears ho_release_pending.

The next poll reads the table, which now lists the source, and prio_mask
becomes non-zero.  sit9531x_ho_release_retry() does nothing because
pending is false.  A retried SELECTABLE returns 0 early in
sit9531x_input_prio_add():

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

so the PLL stays in forced holdover while the pin reports SELECTABLE.

Case B, removing the last source with the latch and the read-back both
failing: the old non-empty prio_mask stays.  The hold is then released
over an empty hardware table, and the PLL follows the source the
selection nibble still names while every pin reports DISCONNECTED.

Should the mask be marked unknown here, so that the release decision and
the pending flag do not depend on it?

> +
> +release:

[ ... ]

> +	if (!chan->prio_mask) {
> +		dev_dbg(sitdev->dev,
> +			"PLL%c: no source listed, holdover kept\n",
> +			'A' + pll_idx);
> +		chan->ho_release_pending = false;
> +		return rc;
> +	}

[ ... ]

> @@ -479,6 +1142,155 @@ 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)
> +{

[ ... ]

> +	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;
> +	}

[Severity: Medium]
Does this roll back a priority the device actually accepted?  Suppose
sit9531x_prio_table_commit() writes and latches the table, and only the
holdover release fails after SIT9531X_HO_CLEAR_TRIES attempts.  By then
the commit has already run:

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

and it then returns ho_rc.  The restore here puts the old cfg_prio back
while the hardware holds the new order.

The table matches seen_srcs, so the poll never reseeds.  prio_get (wired
up in the next patch) then reports the old value, and the next rebuild
for any other reason writes the old order back.

The next patch's commit message says "A priority the table write refused
is not kept", but in this case the device accepted it.

[ ... ]

> @@ -661,6 +1474,73 @@ static int sit9531x_pll_is_active(struct sit9531x_dev *sitdev, u8 pll_idx,

[ ... ]

> +static int sit9531x_chan_routed_ref_read(struct sit9531x_dev *sitdev,
> +					 u8 pll_idx, u8 *ref)
> +{
> +	int rc, lock_rc;
> +	u8 v, i;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	rc = sit9531x_write_pll_u8(sitdev, pll_idx, SIT9531X_PLL_REG_DEBUG,
> +				   SIT9531X_PLL_DEBUG_UNLOCK);
> +	if (rc)
> +		return rc;

[Severity: Low]
Should this path also go through relock:?  Elsewhere in this patch, a
write that reports an error is assumed to possibly have reached the part.
The failed force write still goes to release, and slot writes are counted
before they are issued.

If the unlock key landed despite the error, the debug page stays open.
The comment at relock: says that leaves it "open to a stray write".

The poll relocks it only by calling this function again, and it does
that only while the PLL is tracking.  Could a PLL that becomes unlocked,
frozen or free-running keep its debug page unlocked indefinitely?

[ ... ]

> @@ -976,6 +1871,71 @@ static void sit9531x_dev_ref_states_update(struct sit9531x_dev *sitdev)

[ ... ]

> +static void sit9531x_ho_release_retry(struct sit9531x_dev *sitdev, u8 pll_idx)
> +{
> +	struct sit9531x_chan *chan = &sitdev->chan[pll_idx];
> +
> +	if (!chan->ho_release_pending)
> +		return;

[Severity: Medium]
Is the owed release lost on unbind?  The poll is the only place an owed
release is retried, and sit9531x_dev_stop() only cancels the worker and
unregisters the DPLLs:

static void sit9531x_dev_stop(struct sit9531x_dev *sitdev)
{
	...
	kthread_cancel_delayed_work_sync(&sitdev->work);
	...
}

so the force bit can outlive the driver instance.

On rebind, ho_release_pending starts false and the table read gives a
non-zero prio_mask.  So in sit9531x_prio_table_commit():

	ho_foreign = (val & BIT(SIT9531X_PLL_HO_FORCE_BIT)) &&
		     chan->prio_mask && !chan->ho_release_pending;

is true, and every later table write leaves the hold in place.  Would the
PLL then stay in forced holdover until a power cycle or a manual register
write?

[ ... ]

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

[ ... ]

> @@ -241,6 +266,124 @@ const struct dpll_device_ops sit9531x_dpll_device_ops = {

[ ... ]

> +static void
> +sit9531x_dpll_selection_operstate_get(struct sit9531x_dev *sitdev,
> +				      const struct sit9531x_dpll *sitdpll,
> +				      u8 pin_id,
> +				      enum dpll_pin_operstate *operstate)
> +{
> +	const struct sit9531x_ref *ref;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	if (sit9531x_dpll_selection_active(sitdev, sitdpll, pin_id)) {
> +		*operstate = DPLL_PIN_OPERSTATE_ACTIVE;
> +		return;
> +	}
> +
> +	if (pin_id < sitdev->info->num_inputs) {
> +		ref = sit9531x_ref_state_get(sitdev, pin_id);
> +		if (ref->los) {
> +			*operstate = DPLL_PIN_OPERSTATE_NO_SIGNAL;
> +			return;
> +		}
> +		if (ref->qual_fail) {
> +			*operstate = DPLL_PIN_OPERSTATE_QUAL_FAILED;
> +			return;
> +		}
> +	}

[Severity: Medium]
Should a failed qualification rule out ACTIVE here?
sit9531x_dpll_selection_active() does not check ref->qual_fail.  So when
the PLL is routed to and locked on a drifting lane, this returns ACTIVE
before it reaches the qual_fail branch.

include/uapi/linux/dpll.h defines:

 * @DPLL_PIN_OPERSTATE_ACTIVE: pin is qualified and actively used by the DPLL
 * @DPLL_PIN_OPERSTATE_QUAL_FAILED: pin signal failed qualification (e.g.
 *   frequency or phase monitor)

There is a related question on the selection side.
sit9531x_prio_src_live() only tests !sitdev->ref[index].los, so
sit9531x_prio_top_live() and sit9531x_prio_activesel_pick() can pick a
drifting input over a qualified lower-priority one.  One example is
removing the current source from a table [cur, drifted, healthy].  Is it
known whether the device itself rejects drifted sources?

[ ... ]

> @@ -253,8 +396,163 @@ sit9531x_dpll_input_pin_direction_get(const struct dpll_pin *pin,

[ ... ]

> +	switch (state) {
> +	case DPLL_PIN_STATE_DISCONNECTED:
> +		rc = sit9531x_input_prio_remove(sitdev, sitdpll->id, hw_src);

[ ... ]

> +		ref->pll_mask &= ~pll_bit;
> +		/*
> +		 * The receiver is shared, so the last DPLL to let go turns it
> +		 * off.  That has to happen even when the table rewrite
> +		 * reported an error, or the input stays powered with nothing
> +		 * tracking it; the first error is the one returned.
> +		 */
> +		if (!ref->pll_mask) {
> +			int off_rc = sit9531x_input_disable(sitdev, dpin->id);

[Severity: Medium]
Can this cut the reference of a PLL in manual active select?
ref->pll_mask counts only priority tables, both here and in
sit9531x_ref_pll_mask_rebuild():

	u16 mask = sitdev->chan[pll_idx].prio_mask;

A profile can put a PLL in manual active select (PLL_CONFIG1F bit 6).
The PLL then follows CLK_ACTIVESEL, or the manual input select when
MISCINNER bit 5 is set, rather than its table.
sit9531x_manual_sel_report() only warns about this.

Suppose such a PLL runs on INx and its table does not list INx.
Disconnecting INx from the last PLL whose table lists it drops pll_mask
to 0.  sit9531x_input_disable() then sets the force bit and clears the
state bit for a receiver that the manual-select PLL is still locked to.

[ ... ]

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