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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dpll: self-deadlock in sit9531x_dpll_input_pin_prio_set via dpll_pin_change_ntf
- [Medium] dpll: absent inputs alias with lowest priority in sit9531x_input_prio_get
- [Medium] dpll: priority table cache corrupted by failed writes in sit9531x_prio_table_commit
--

commit e60b7c983f9c78bdbd8985a082cc6489f1c9b61d
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

    dpll: sit9531x: add support to get and set priority on input pins
    
    The priority table is ordered: the slot an input occupies is its
    priority, and the device selects the lowest occupied slot whose signal is
    qualified.  Getting the priority is therefore a search of the table for
    the input, and setting it moves the input to the requested slot.
    
    An input absent from the table has no priority to report, and the core is
    told so rather than handed a made-up number.

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

[Severity: Medium]
Is this statement accurate?

Looking at sit9531x_input_prio_get(), when an input is absent (!slot), it maps
to SIT9531X_PRIO_MAX_SLOTS (11), yielding a priority of 10:

    if (!slot)
        slot = SIT9531X_PRIO_MAX_SLOTS;

    *prio = slot - 1;

This aliases an absent input with an input legitimately placed in the lowest
priority slot. The core is handed this constructed priority rather than an
error, which seems to contradict the description.

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
[ ... ]
@@ -570,16 +570,30 @@ 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;
[ ... ]

[Severity: Medium]
Could this corrupt the priority table cache when an I2C write fails?

This function populates the newly added cache fields (prio_srcs and prio_last)
based on the provided srcs array. However, looking at the caller
sit9531x_prio_table_commit():

drivers/dpll/sit9531x/core.c:sit9531x_prio_table_commit() {
    ...
    for (slot = 0; slot < SIT9531X_PRIO_MAX_SLOTS; slot++) {
        ...
        rc = sit9531x_write_u8(sitdev, reg, val);
        if (rc)
            goto commit;
    }

commit:
    ...
    sit9531x_prio_mask_build(sitdev, pll_idx, srcs);
    ...
}

If an I2C write fails midway, sit9531x_prio_mask_build() still caches the fully
intended state from srcs. This masks the failure from subsequent getters, which
will return the intended but uncommitted state until the background polling
thread eventually corrects it.

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;
 }
 
[ ... ]
+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];
[ ... ]
+	if (rc == -EINVAL) {
+		NL_SET_ERR_MSG(extack,
+			       "Pin is not a reference of this DPLL; connect it first");
+		return rc;
+	}
+	if (rc) {
+		NL_SET_ERR_MSG(extack, "Failed to set input priority");
+		return rc;
+	}
+
+	while (changed_cnt--)
+		dpll_pin_change_ntf(changed[changed_cnt]);
+
+	return 0;
+}

[Severity: High]
Does this code introduce a self-deadlock?

The driver attempts to notify userspace of priority shifts using
dpll_pin_change_ntf(). This function tries to unconditionally acquire the
non-recursive dpll_lock. However, the DPLL framework already holds dpll_lock
when dispatching the prio_set callback (e.g., from dpll_pin_pre_doit()).

Should this use the lockless __dpll_pin_change_ntf() instead to avoid hanging
the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915000015.80480-1-arouhi@sitime.com?part=7