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

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.

[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?

[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?

[ ... ]

[ ... ]

[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?

[ ... ]

[ ... ]

[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?

[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()?

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

[ ... ]

+	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