Re: [PATCH net-next v9 07/15] dpll: sit9531x: add support to get and set priority on input pins
From: netdev-bot+sashiko
Date: Thu Sep 17 2026 - 11:22:33 EST
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