Re: [PATCH net-next v9 07/15] 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 7 potential
issue(s) to consider:

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

- [High] Self-deadlock: sit9531x_dpll_input_pin_prio_set()
  (drivers/dpll/sit9531x/dpll.c) ends with `while (changed_cnt--)…
- [High] Use-after-free / NULL dereference window:
  sit9531x_dpll_input_pin_prio_set() is the first pin op that walks the…
- [Medium] sit9531x_input_prio_get() (drivers/dpll/sit9531x/core.c)
  collapses the documented 'absent' encoding (chan->prio_last[src] == 0,…
- [Medium] Silent no-op priority set: sit9531x_input_prio_set()
  (drivers/dpll/sit9531x/core.c:671) locates only the FIRST slot holding…
- [Low] The new struct member sit9531x_chan.prio_srcs[] (core.h:152) is
  written by sit9531x_prio_mask_build() ('chan->prio_srcs[slot] = src;')…
- [Low] sit9531x_dpll_input_pin_prio_set() notifies the pin the request
  was made for, in addition to the shifted siblings: the traversal…
- [Low] sit9531x_prio_table_commit() rebuilds the newly extended cache
  (chan->prio_srcs / chan->prio_last, alongside prio_mask) from the…

--

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

This adds .prio_get and .prio_set to the sit9531x input pin ops, backed by a
new cached view of the hardware priority table (prio_srcs[] and prio_last[])
that is refreshed whenever the table is written or polled.  Getting a
priority looks up the cached slot for the pin's source code; setting one
moves the source to the requested slot through sit9531x_input_prio_set().

A few questions below.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 25418485e9757..484c94def578c 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -521,21 +521,81 @@ 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
> + * cached from the hardware table read at startup and refreshed after every
> + * table write and poll read-back, so pin-get reflects hardware state without
> + * issuing synchronous register reads per pin.  A source with no known slot
> + * falls back to the lowest-priority valid slot.
> + *
> + * Caller must hold sitdev->multiop_lock.
> + */
> +int sit9531x_input_prio_get(struct sit9531x_dev *sitdev, u8 pll_idx,
> +			    u8 input_idx, u8 *prio)
> +{
> +	const struct sit9531x_chan *chan;
> +	u8 slot;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	if (pll_idx >= SIT9531X_NUM_PLLS)
> +		return -EINVAL;
> +	input_idx = sit9531x_prio_src_canon(sitdev, input_idx);
> +	if (input_idx >= SIT9531X_PRIO_NUM_SRC)
> +		return -EINVAL;
> +
> +	chan = &sitdev->chan[pll_idx];
> +	slot = chan->prio_last[input_idx];
> +	if (!slot)
> +		slot = SIT9531X_PRIO_MAX_SLOTS;
> +
> +	*prio = slot - 1;
> +
> +	return 0;
> +}

[Severity: Medium]
The commit message says:

  "An input absent from the table has no priority to report, and the core is
   told so rather than handed a made-up number."

Does sit9531x_input_prio_get() actually tell the core that?  The absent
encoding documented for prio_last[] in core.h (0 = the source is not in the
table) is turned into SIT9531X_PRIO_MAX_SLOTS and the function returns 0:

	slot = chan->prio_last[input_idx];
	if (!slot)
		slot = SIT9531X_PRIO_MAX_SLOTS;

	*prio = slot - 1;

Since SIT9531X_PRIO_MAX_SLOTS is 11, *prio becomes 10, which is a valid
slot.  dpll_msg_add_pin_prio() puts whatever prio_get returns into
DPLL_A_PIN_PRIO, and dpll_msg_add_pin_dplls() calls it for every pin/dpll
reference regardless of connection state.

Can userspace then tell "input not in the priority table" apart from "input
programmed at slot 10"?

The value also does not round-trip: writing 10 back reaches
sit9531x_input_prio_set(), which returns -EINVAL when the source is not
found in the table, so
sit9531x_dpll_input_pin_prio_set() answers with "Pin is not a reference of
this DPLL; connect it first".

Would it be clearer either to return an error from prio_get for an absent
source, or to reword the changelog to match the fallback the code
implements?

> +
> +/*
> + * 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().

[Severity: Low]
sit9531x_prio_table_commit() calls sit9531x_prio_mask_build() with the
intended srcs[] array even when a slot read or write failed mid-sequence
(the "goto commit" path).  With this patch prio_last[] becomes the source of
truth for the priority reported by the new .prio_get op, so a get issued
after a failed set can report a slot the device never accepted, and after a
partial shift-write the device may hold the source in two slots and select
it at a higher priority than the driver reports.

The existing comment at the rebuild site documents this choice, and
sit9531x_chan_state_fetch() calls sit9531x_prio_table_read() plus
sit9531x_prio_mask_build() on every poll, so the window is bounded by the
poll period.  Is it worth noting in the kernel-doc for
sit9531x_input_prio_get() that the cached slot may be an intended rather
than a confirmed one until the next poll?

> + *
> + * 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)
>  {
> +	struct sit9531x_chan *chan = &sitdev->chan[pll_idx];
> +	u8 first[SIT9531X_PRIO_NUM_SRC] = { 0 };
>  	u16 mask = 0;
> -	u8 slot;
> +	u8 slot, src, src_canon;
>  
> -	for (slot = 0; slot < SIT9531X_PRIO_MAX_SLOTS; slot++)
> -		mask |= BIT(srcs[slot] & SIT9531X_PRIO_NIBBLE_MASK);
> +	for (slot = 0; slot < SIT9531X_PRIO_MAX_SLOTS; slot++) {
> +		src = srcs[slot] & SIT9531X_PRIO_NIBBLE_MASK;
> +		chan->prio_srcs[slot] = src;

[ ... ]

> diff --git a/drivers/dpll/sit9531x/core.h b/drivers/dpll/sit9531x/core.h
> index f628bd5b7a975..1ab8ebfd78da1 100644
> --- a/drivers/dpll/sit9531x/core.h
> +++ b/drivers/dpll/sit9531x/core.h
> @@ -129,6 +129,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
[ ... ]
> @@ -143,6 +149,8 @@ struct sit9531x_chan {
>  	bool		inner_lol;
>  	bool		ho_freeze;
>  	bool		ho_valid;
> +	u8		prio_srcs[SIT9531X_PRIO_MAX_SLOTS];

[Severity: Low]
Is prio_srcs[] read anywhere?  Grepping drivers/dpll/sit9531x/ turns up only
this declaration and the single store in sit9531x_prio_mask_build()
("chan->prio_srcs[slot] = src;"), both at this commit and at the end of the
series.  sit9531x_input_prio_get() reads prio_last[] only.

The kernel-doc above ("so priority reads generate no register traffic") and
the comment on sit9531x_prio_mask_build() ("the per-slot copy and the
last-slot-seen array are what priority get answers from") describe a
consumer that does not exist.  Was a user of this array dropped, or can the
member and those comment lines go?

> +	u8		prio_last[SIT9531X_PRIO_NUM_SRC];
>  	u16		prio_mask;
>  };
>  
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index ab9aa7db1839c..e2ff21adfdb23 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -469,10 +469,112 @@ sit9531x_dpll_input_pin_state_on_dpll_set(const struct dpll_pin *pin,
>  	return rc;
>  }
>  
[ ... ]
> +/*
> + * sit9531x_dpll_input_pin_prio_set - set input pin priority
> + *
> + * writes input priority table on Page 1 via
> + * core.c sit9531x_input_prio_set().  Forces holdover during update.
> + */
> +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)
> +{
> +	struct dpll_pin *changed[SIT9531X_MAX_INPUTS + 1];
> +	struct sit9531x_dpll_pin *sibling;
> +	struct sit9531x_dpll_pin *dpin = pin_priv;
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	struct sit9531x_dev *sitdev = sitdpll->dev;
> +	u8 changed_cnt = 0, hw_src, slot;
> +	int get_rc, rc;
[ ... ]
> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_input_prio_set(sitdev, sitdpll->id,
> +				     sit9531x_input_hw_src(dpin->id),
> +				     (u8)prio);

[Severity: Medium]
This is the first caller of sit9531x_input_prio_set(); in the parent commit
the helper has none.  Can a set through it report success without changing
the effective priority?

sit9531x_input_prio_set() locates only the first slot holding the source:

	for (from = 0; from < SIT9531X_PRIO_MAX_SLOTS; from++)
		if (sit9531x_prio_src_canon(sitdev, srcs[from]) == input_idx)
			break;
	...
	srcs[prio] = input_idx;
	if (!memcmp(srcs, orig_srcs, sizeof(srcs)))
		return 0;

Duplicated source codes are the driver's own representation of an unused
slot -- sit9531x_input_prio_remove() backfills the freed tail:

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

So for a table like [A,B,B,B,...,B], moving B down to slot 5 shifts
duplicates onto each other, the array comes out bit-identical, the memcmp
shortcut returns 0 and no register is written.  The device keeps selecting B
at its old, higher slot, while sit9531x_prio_mask_build() records the first
occurrence, so a following get reports the old slot and not the one just
written.

The changelog says "setting it moves the input to the requested slot" --
should sit9531x_input_prio_set() clear or account for the other slots
holding the same source code?

> +	if (!rc) {
> +		list_for_each_entry(sibling, &sitdpll->pins, list) {
> +			if (!sit9531x_dpll_is_input_pin(sibling) ||
> +			    sit9531x_dpll_is_xo_pin(sibling))
> +				continue;

[Severity: High]
This is the first pin op that walks the other pins of the DPLL, and
sibling->dpll_pin is collected into changed[] without a NULL check.  Can it
race with teardown?

sit9531x_dpll_pins_unregister() in core.c does, per pin:

	list_for_each_entry_safe(pin, next, &sitdpll->pins, list) {
		sit9531x_dpll_pin_unregister(pin);
		list_del(&pin->list);
		kfree(pin);
	}

sit9531x_dpll_pin_unregister() calls dpll_pin_unregister(), which takes and
then drops dpll_lock:

drivers/dpll/dpll_core.c:dpll_pin_unregister() {
	mutex_lock(&dpll_lock);
	__dpll_pin_unregister(dpll, pin, ops, priv, NULL);
	mutex_unlock(&dpll_lock);
}

and then sets pin->dpll_pin = NULL, while list_del() and kfree() run with
dpll_lock dropped.  In that gap a netlink pin-set on a still-registered
sibling holds dpll_lock and walks &sitdpll->pins, so it can store a NULL
dpll_pin into changed[] and later hand it to dpll_pin_change_ntf() ->
__dpll_pin_change_ntf() -> dpll_pin_notify(pin, pin->clock_id, ...), or
follow a list node that is being list_del()'d and kfree()'d.

The pre-existing poll worker does a similar traversal, but
kthread_cancel_delayed_work_sync() in sit9531x_dev_stop() runs before any
unregister.  Is anything serializing this new traversal against
sit9531x_dpll_pins_unregister()?

> +
> +			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;
> +			if (changed_cnt < ARRAY_SIZE(changed))
> +				changed[changed_cnt++] = sibling->dpll_pin;

[Severity: Low]
The traversal filter skips only non-input and XO pins, so dpin itself is
collected here too.  The core already emits a notification for that pin
after a successful callback:

drivers/dpll/dpll_netlink.c:dpll_pin_prio_set() {
	ret = ops->prio_set(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
			    dpll_priv(dpll), prio, extack);
	if (ret)
		return ret;
	__dpll_pin_change_ntf(pin);
}

Does one priority change end up sending two DPLL_CMD_PIN_CHANGE_NTF
messages for the requested pin?  Skipping sibling == dpin in the loop would
leave that notification to the core.

> +		}
> +	}
> +	mutex_unlock(&sitdev->multiop_lock);
[ ... ]
> +	while (changed_cnt--)
> +		dpll_pin_change_ntf(changed[changed_cnt]);

[Severity: High]
Can this deadlock on dpll_lock?  The .prio_set callback is invoked with
dpll_lock already held:

dpll_pin_pre_doit()
	mutex_lock(&dpll_lock);
	...
dpll_pin_set_from_nlattr()
  dpll_pin_prio_set()
	ret = ops->prio_set(...);
	if (ret)
		return ret;
	__dpll_pin_change_ntf(pin);	/* lockdep_assert_held(&dpll_lock) */

and dpll_pin_change_ntf() is the wrapper that takes the same non-recursive
mutex again:

drivers/dpll/dpll_netlink.c:dpll_pin_change_ntf() {
	mutex_lock(&dpll_lock);
	ret = __dpll_pin_change_ntf(pin);
	mutex_unlock(&dpll_lock);
}

changed_cnt is non-zero in the ordinary case: sibling->prio starts at 0 from
the kzalloc_obj() in sit9531x_dpll_pin_alloc() and is never seeded, and the
loop does not exclude the pin being set, so the first "pin-set ... prio N"
with N different from the cached value queues at least one entry.

Should this use __dpll_pin_change_ntf(), the way the core does from this
context, instead of dpll_pin_change_ntf()?

>  static const struct dpll_pin_ops sit9531x_dpll_input_pin_ops = {
[ ... ]

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