Re: [PATCH v10 07/14] dpll: sit9531x: add support to get and set priority on input pins

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpll: sit9531x: add support to get and set priority on input pins

This adds prio_get and prio_set callbacks for the sit9531x input pins.
They are backed by a cached copy of each PLL's priority table, which
sit9531x_prio_mask_build() now maintains. prio_set moves an input to the
requested slot and notifies the other pins whose slots shifted, and
sit9531x_prio_table_commit() gains a rollback of partially written slots.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 84c1f86e6d4cf..ac184c93258cb 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -543,34 +543,87 @@ bool sit9531x_input_prio_present(struct sit9531x_dev *sitdev, u8 pll_idx,
>  }
>  
>  /*
> - * Rebuild a PLL's membership mask from the source codes of its priority
> - * table.  The mask is what the pin state getters test, so it is refreshed
> - * from exactly the values the table holds -- here after a write, and once
> - * per poll from the read-back in sit9531x_chan_state_fetch().
> + * sit9531x_input_prio_get - read an input's priority slot for a PLL
> + * @input_idx:	input source in hardware encoding (see
> + *		sit9531x_input_hw_src())
> + * @prio:	output slot position (0 = highest)
> + *
> + * Reports the last slot this source occupied on this PLL.  The value is

[Severity: Low]
This isn't a bug, but is "last slot" accurate here?

sit9531x_prio_mask_build() fills prio_last from first[], which holds the
first (highest priority) slot a source occupies among its duplicates. It
also clears the entry to 0 as soon as the source leaves the table:

		if (!first[src_canon])
			first[src_canon] = slot + 1;

The "last-slot-seen array" wording in the sit9531x_prio_mask_build()
comment below has the same problem. The @prio_last description in core.h
does match the code.

[ ... ]

> +/*
> + * Refresh a PLL's cached view of its priority table from the source codes
> + * the table holds -- here after a write, and once per poll from the
> + * read-back in sit9531x_chan_state_fetch().
> + *
> + * The membership mask is what the pin state getters test; the per-slot
> + * copy and the last-slot-seen array are what priority get answers from,
> + * so neither costs a register read per pin.
>   */
>  static void sit9531x_prio_mask_build(struct sit9531x_dev *sitdev, u8 pll_idx,
>  				     const u8 *srcs, u8 written)
>  {
> +	struct sit9531x_chan *chan = &sitdev->chan[pll_idx];
> +	u8 first[SIT9531X_PRIO_NUM_SRC] = { 0 };
>  	u16 mask = 0;
> -	u8 slot;
> -
> -	/*
> -	 * A table written only in part is not described by the values the
> -	 * request carried, and there is nothing here to describe it with
> -	 * instead, so the mask it had stands until the next poll reads the
> -	 * table back.
> -	 */
> -	if (written < SIT9531X_PRIO_MAX_SLOTS)
> -		return;
> +	u8 slot, src, src_canon;
>  
>  	for (slot = 0; slot < SIT9531X_PRIO_MAX_SLOTS; slot++) {
> -		u8 src = srcs[slot] & SIT9531X_PRIO_NIBBLE_MASK;
> +		/*
> +		 * A slot the caller could not write still holds what it
> +		 * held before, so take that rather than the value the
> +		 * request wanted to put there.
> +		 */
> +		src = slot < written ? srcs[slot] : chan->prio_srcs[slot];
> +		src &= SIT9531X_PRIO_NIBBLE_MASK;
> +		chan->prio_srcs[slot] = src;
> +		src_canon = sit9531x_prio_src_canon(sitdev, src);
> +		if (!sit9531x_prio_src_usable(src))
> +			continue;
>  
> -		if (sit9531x_prio_src_usable(src))
> -			mask |= BIT(src);
> +		mask |= BIT(src_canon);
> +		if (!first[src_canon])
> +			first[src_canon] = slot + 1;
>  	}

[Severity: Low]
This isn't a bug, but this hunk changes sit9531x_prio_mask_build() in two
ways that the commit message doesn't mention. Both affect existing users:
the pin state getter, sit9531x_input_prio_present(),
sit9531x_ref_pll_mask_rebuild() and the periodic poll.

First, the mask is now built from BIT(src_canon) instead of BIT(src).
Before this patch, when a differential pair's table entry held the N-lane
code, the P-lane pin was reported as disconnected and prio_present()
returned false. Now the same entry reports the pin as listed.

Second, the early return on a partial write is gone. The requested values
for the written slots are now merged with chan->prio_srcs for the rest.
That changes what prio_present() tells the rollback branches of
sit9531x_dpll_input_pin_state_on_dpll_set() after a failed commit.

Should these changes be described in the commit message, or moved into
the commit that introduced sit9531x_prio_mask_build()?

[ ... ]

> @@ -629,9 +683,34 @@ static int sit9531x_prio_table_commit(struct sit9531x_dev *sitdev, u8 pll_idx,
>  
>  	written = SIT9531X_PRIO_MAX_SLOTS;
>  
> +	if (rc && written) {
> +		/*
> +		 * Put the slots that did reach the device back the way they
> +		 * were.  Latching a table that is neither the previous order
> +		 * nor the requested one hands the reference selection loop
> +		 * a priority list nobody asked for.  The cache is the table
> +		 * as last read, which is what those slots held.
> +		 */
> +		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;
> +	}

[Severity: Medium]
Can this block ever run? Every failing register access earlier in
sit9531x_prio_table_commit() jumps straight to the label:

		rc = sit9531x_write_u8(sitdev, reg, val);
		if (rc)
			goto commit;

The read and the write of the last register use the same goto. The only
way to reach "if (rc && written)" is to fall through after the last write
succeeded, and rc is 0 at that point.

So when a table write fails partway through prio_set, prio_add or
prio_remove, sit9531x_prio_prg_commit() still latches the half-written
table and the forced holdover is still released.

For example, take [A,B,C,D,E,F,G,H,X,I,J] and move X to slot 0. If the
write for slots 4 and 5 fails, the device latches
[X,A,B,C,E,F,G,H,X,I,J], and D drops out of this PLL's selection list.

The partial latch itself comes from the earlier "dpll: sit9531x: implement
input pin state on a DPLL" commit. The comment here, though, says this
block prevents it.

If the gotos were pointed at this block, would a few more things need
fixing?

With written == SIT9531X_PRIO_MAX_SLOTS (11), the loop reaches slot 10 and
reads chan->prio_srcs[11], one past the end of the 11-entry array.

The last register shares its other nibble with CLK_ACTIVESEL. This block
rewrites that register from a 0 base, not with the read-modify-write the
forward path uses.

After "written = restored", sit9531x_prio_mask_build(sitdev, pll_idx,
srcs, written) would cache the requested srcs[] for exactly the slots that
were just put back to their old values.

The commit message also doesn't mention this change to a helper shared by
prio_add, prio_remove and prio_set.

[ ... ]

> diff --git a/drivers/dpll/sit9531x/core.h b/drivers/dpll/sit9531x/core.h
> index 033e1059bde9b..2c5d0100b450e 100644
> --- a/drivers/dpll/sit9531x/core.h
> +++ b/drivers/dpll/sit9531x/core.h
> @@ -126,6 +126,12 @@ struct sit9531x_out {
>   * @ho_freeze:		holdover freeze active
>   * @ho_valid:		holdover memory acquired, i.e. the holdover window
>   *			holds a valid estimate to fall back on
> + * @prio_srcs:		cached copy of the priority table, one source code
> + *			per slot; refreshed together with @prio_mask, so
> + *			priority reads generate no register traffic
> + * @prio_last:		slot each source occupies, plus one (0 = the source
> + *			is not in the table); refreshed from the same scan
> + *			as @prio_mask, so the two never disagree
>   * @prio_mask:		bit per hardware source code present in this PLL's
>   *			priority table, i.e. the sources it may select.  Read

[Severity: Low]
This isn't a bug, but the @prio_mask description still says "bit per
hardware source code".

sit9531x_prio_mask_build() now sets BIT(src_canon). An N-lane code in the
table therefore sets the P-lane bit and leaves its own bit clear.

Could this be updated to say the mask is indexed by the canonical source?

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 4437c95f578f8..56a8213dee077 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -489,10 +489,139 @@ sit9531x_dpll_input_pin_state_on_dpll_set(const struct dpll_pin *pin,

[ ... ]

> +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);
> +	if (!rc) {

[Severity: Low]
If sit9531x_input_prio_set() returns an error after the table has already
changed, is the sibling update skipped entirely?

In sit9531x_prio_table_commit(), the table is latched and
sit9531x_prio_mask_build() refreshes prio_last before the holdover release
is retried. A failure there still becomes the return value:

	if (ho_rc) {
		...
		if (!rc)
			rc = ho_rc;

A partially written table is also latched and cached on the write failure
paths.

In both cases this callback returns an error, so dpll_pin_prio_set() in
the core also skips __dpll_pin_change_ntf() for the named pin. Several
pins' reported priorities can then change with no PIN_CHANGE_NTF for any
of them, and sibling->prio stays stale.

> +		list_for_each_entry(sibling, &sitdpll->pins, list) {
> +			if (!sit9531x_dpll_is_input_pin(sibling) ||
> +			    sit9531x_dpll_is_xo_pin(sibling))
> +				continue;
> +
> +			hw_src = sit9531x_input_hw_src(sibling->id);
> +			get_rc = sit9531x_input_prio_get(sitdev, sitdpll->id,
> +							 hw_src, &slot);
> +			if (get_rc)
> +				continue;
> +
> +			if (sibling->prio == slot)
> +				continue;
> +
> +			sibling->prio = slot;
> +
> +			/*
> +			 * The core notifies the pin the request named, so
> +			 * only the others are collected here.  A pin whose
> +			 * dpll_pin is already NULL is mid-unregister: that
> +			 * runs with the device lock dropped between the
> +			 * unregister and the free, so it can be seen from
> +			 * here, and notifying through it would follow a
> +			 * pointer that is on its way out.
> +			 */
> +			if (sibling == dpin || !sibling->dpll_pin)
> +				continue;
> +
> +			if (changed_cnt < ARRAY_SIZE(changed))
> +				changed[changed_cnt++] = sibling->dpll_pin;
> +		}
> +	}
> +	mutex_unlock(&sitdev->multiop_lock);

[Severity: High]
Is it safe to walk sitdpll->pins and read sibling->dpll_pin here while
holding only multiop_lock and the core's dpll_lock? The teardown path
takes neither lock for the list or for this field:

drivers/dpll/sit9531x/core.c:sit9531x_dpll_pin_unregister() {
	...
	dpll_pin_unregister(sitdpll->dpll_dev, pin->dpll_pin, ops, pin);
	dpll_pin_put(pin->dpll_pin, &pin->tracker);
	pin->dpll_pin = NULL;
	...
}

drivers/dpll/sit9531x/core.c:sit9531x_dpll_pins_unregister() {
	...
	list_for_each_entry_safe(pin, next, &sitdpll->pins, list) {
		sit9531x_dpll_pin_unregister(pin);
		list_del(&pin->list);
		kfree(pin);
	}
}

dpll_pin_unregister() and dpll_pin_put() each take and drop dpll_lock.
The NULL store only happens after both.

Suppose a PIN_SET prio request arrives for a pin that is still registered
on the same DPLL while sit9531x_dev_stop() or a probe error path is
running. Could this loop pick up a sibling's dpll_pin after the last
dpll_pin_put() has freed it, and pass it to __dpll_pin_change_ntf()?

Could the loop also be standing on a sit9531x_dpll_pin that another CPU
is list_del()'ing and kfree()'ing? It would then write sibling->prio into
freed memory or follow LIST_POISON1.

sit9531x_dpll_pins_register() has a similar problem. It does
list_add(&pin->list, &sitdpll->pins) without the lock, after earlier pins
are already reachable from netlink.

The !sibling->dpll_pin check added in v10 reads the pointer without any
lock that orders it against teardown. It doesn't seem to close the window
the comment describes.

[ ... ]

> +	/*
> +	 * The core notifies only the pin the request named, so the ones whose
> +	 * slots moved are notified here.  This runs inside a pin op, where
> +	 * the core already holds the lock the notification needs, so it is
> +	 * the underscore helper rather than the wrapper that takes it.
> +	 */
> +	while (changed_cnt--)
> +		__dpll_pin_change_ntf(changed[changed_cnt]);
> +
> +	return 0;
> +}

[Severity: Medium]
Sibling notifications are only sent from this prio_set path. What about
sit9531x_dpll_input_pin_state_on_dpll_set() with
DPLL_PIN_STATE_DISCONNECTED?

That path calls sit9531x_input_prio_remove(). It rebuilds the table from
the sources it keeps and backfills the tail:

	} else {
		/* Backfill freed tail slots with the lowest-priority src */
		while (count < SIT9531X_PRIO_MAX_SLOTS) {
			kept[count] = kept[count - 1];
			count++;
		}
	}

Every later source moves up a slot, and sit9531x_prio_mask_build() updates
prio_last, which is what prio_get now reports. For example, removing A
from [A,B,C,...] moves B from 1 to 0 and C from 2 to 1.

dpll_pin_state_set() in the core only notifies the named pin. The
disconnect path neither notifies the siblings nor refreshes their
sibling->prio.

sit9531x_dpll_changes_check() doesn't catch this later either, because it
only compares pin state:

		if (state != pin->pin_state) {
			...
			pin->pin_state = state;
			dpll_pin_change_ntf(pin->dpll_pin);
		}

Would userspace listening for PIN_CHANGE_NTF miss the priority changes of
the other inputs after a disconnect?

[ ... ]