Re: [PATCH net-next 1/6] dpll: zl3073x: notify sibling pin when shared output config changes
From: Ivan Vecera
Date: Wed Sep 30 2026 - 03:50:45 EST
On 9/30/26 8:57 AM, netdev-bot+sashiko@xxxxxxxxxx wrote:
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The patch adds zl3073x_dpll_output_pin_sibling_get()
(drivers/dpll/zl3073x/dpll.c:146).
- [Medium] The commit says it closes the case where the sibling pin's
frequency changes without a notification.
- [Low] The commit message presents the missing sibling notification as a
user-visible defect: userspace listening on the sibling pin "is never…
--
dpll: zl3073x: notify sibling pin when shared output config changes
This patch adds zl3073x_dpll_output_pin_sibling_get(), which finds the
other pin of a P/N output pair. The esync, frequency and phase adjust
setters use it to send a pin change notification for the sibling after
the shared output registers are written, and they now use explicit
mutex_lock()/mutex_unlock() so the notification goes out after
zldpll->lock is dropped.
Changing one of these settings through one pin's dpll_pin therefore
also changes the other (sibling) pin's effective configuration, but
only the pin the change was requested on gets a dpll_pin_change_ntf()
notification - userspace listening on the sibling pin is never told
its frequency, esync configuration or phase adjustment changed.
[Severity: Low]
This describes a user-visible defect, but the trailers only have
Tested-by: and Signed-off-by:.
The dpll core only calls __dpll_pin_change_ntf() for the pin in the
request (for example in dpll_pin_freq_set()). The sibling has not been
notified since the output setters were added.
Should this carry Fixes: tags such as these?
Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Fixes: 6287262f761e ("dpll: zl3073x: Add support to adjust phase")
Agreed, this is a pre-existing user-visible defect. I will add the three
Fixes: tags in v2:
Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Fixes: 6287262f761e ("dpll: zl3073x: Add support to adjust phase")
Jakub, a process question: this patch is both the fix and a prerequisite
for the rest of the series - it introduces
zl3073x_dpll_output_pin_sibling_get() and the notify-outside-lock
handling that the output enable/disable and PTP perout patches build on.
The defect itself is only a missing change notification (no crash, no
data corruption). Is it OK to keep it as patch 1 of this net-next series,
or would you prefer it split out and sent to net first?
diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index f2e137475b40..2c6de4dab8b4 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -132,6 +132,30 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
return NULL;
}
+/**
+ * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
+ * @pin: output pin whose sibling is sought
+ *
+ * Output pin ids are allocated in P/N pairs (P even, N odd) that share a
+ * single HW output. Looks up the other pin of the pair, if it is
+ * registered as a dpll_pin on this DPLL.
+ *
+ * Return: pointer to sibling pin, or NULL if it is not registered
+ */
+static struct zl3073x_dpll_pin *
+zl3073x_dpll_output_pin_sibling_get(struct zl3073x_dpll_pin *pin)
+{
+ struct zl3073x_dpll_pin *sibling;
+
+ list_for_each_entry(sibling, &pin->dpll->pins, list) {
+ if (!zl3073x_dpll_is_input_pin(sibling) &&
+ sibling->id == (pin->id ^ 1))
+ return sibling;
+ }
+
+ return NULL;
+}
+
[ ... ]
@@ -934,27 +962,44 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
[ ... ]
+ /* Commit output configuration */
+ rc = zl3073x_out_state_set(zldev, out_id, &out);
+ if (rc)
+ goto unlock;
+
+ /* The clock type, esync period and esync width are all shared by
+ * both pins of the output pair, so the sibling pin's esync
+ * configuration changes too and userspace has to be notified.
*/
- out.esync_n_width = out.div / 2;
+ sibling = zl3073x_dpll_output_pin_sibling_get(pin);
- /* Commit output configuration */
- return zl3073x_out_state_set(zldev, out_id, &out);
+ mutex_unlock(&zldpll->lock);
+
+ if (sibling)
+ __dpll_pin_change_ntf(sibling->dpll_pin);
[Severity: High]
Can sibling->dpll_pin be NULL, or the sibling already freed, by the time
it is dereferenced here?
zl3073x_dpll_output_pin_sibling_get() returns any output pin on
zldpll->pins with the matching id. It does not check that
sibling->dpll_pin is still set. The lookup runs under zldpll->lock, but
the dereference happens after mutex_unlock().
The teardown path does not take zldpll->lock:
drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_pins_unregister() {
list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
zl3073x_dpll_pin_unregister(pin);
list_del(&pin->list);
zl3073x_dpll_pin_free(pin);
}
}
zl3073x_dpll_pin_unregister() ends with:
dpll_pin_put(pin->dpll_pin, &pin->tracker);
pin->dpll_pin = NULL;
and dpll_pin_put() has already dropped dpll_lock at that point.
zl3073x_dpll_pins_register() uses list_add(), so the N pin (2k+1) sits
before its P pin (2k) on the list. That means N is torn down while P is
still registered and reachable over netlink.
One possible interleaving:
CPU1 (devlink reload, flash_prepare or unbind)
zl3073x_dev_stop()
zl3073x_dpll_unregister()
zl3073x_dpll_pins_unregister()
dpll_pin_put(N) /* dpll_lock released */
CPU0 (DPLL_CMD_PIN_SET on P, dpll_lock held)
dpll_pin_freq_set()
zl3073x_dpll_output_pin_frequency_set(P)
mutex_lock(&zldpll->lock)
sibling = zl3073x_dpll_output_pin_sibling_get(P) /* finds N */
mutex_unlock(&zldpll->lock)
CPU1
N->dpll_pin = NULL
list_del(&N->list)
zl3073x_dpll_pin_free(N)
CPU0
__dpll_pin_change_ntf(sibling->dpll_pin)
In that case, would __dpll_pin_change_ntf() get NULL and oops on
pin->id and pin->clock_id in dpll_pin_notify()? Or would CPU0 read the
freed zl3073x_dpll_pin?
The list walk in zl3073x_dpll_output_pin_sibling_get() can also run
while list_del() is in progress, because the writer never takes
zldpll->lock.
zl3073x_dpll_output_pin_frequency_set() and
zl3073x_dpll_output_pin_phase_adjust_set() have the same pattern.
The later patch "dpll: zl3073x: add PTP periodic output support" also
calls zl3073x_dpll_output_pin_sibling_get() from
zl3073x_dpll_ptp_enable() without holding zldpll->lock at all.
This cannot happen; the notification is already serialized against pin
teardown.
The output setters (esync_set / frequency_set / phase_adjust_set) run
with dpll_lock held: the dpll core invokes the pin ops under dpll_lock,
and they use __dpll_pin_change_ntf(), whose contract is exactly "caller
must hold dpll_lock" (dpll_netlink.c: lockdep_assert_held(&dpll_lock),
"suitable for use inside pin callbacks which are already invoked under
dpll_lock"). So sibling_get() and the __dpll_pin_change_ntf() call - even
though it runs after mutex_unlock(&zldpll->lock) - are all covered by
dpll_lock.
Pin teardown frees the sibling via zl3073x_dpll_pin_unregister() ->
dpll_pin_unregister(), which takes dpll_lock. While a setter holds
dpll_lock for the whole callback, teardown cannot even reach
dpll_pin_unregister(N), let alone the following list_del()/kfree(). The
proposed interleaving (CPU1 freeing N while CPU0 notifies) is therefore
impossible: CPU0 holds dpll_lock throughout.
The PTP enable() path (zl3073x_dpll_ptp_enable(), added in patch 6) does
call sibling_get() outside dpll_lock, but it is serialized differently:
zl3073x_dpll_unregister() unregisters the PTP clock *before* the pins
(zl3073x_dpll_ptp_unregister() then zl3073x_dpll_pins_unregister()), and
ptp_clock_unregister() quiesces in-flight enable() callbacks. No
enable() can run while the pins are being freed.
[ ... ]
@@ -1010,7 +1058,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
out.width = new_div;
/* Commit output configuration */
- return zl3073x_out_state_set(zldev, out_id, &out);
+ rc = zl3073x_out_state_set(zldev, out_id, &out);
+ if (rc)
+ goto unlock;
+
+ /* The other pin's frequency changed too - it has to be
+ * notified about the change.
+ */
+ sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+
+ goto unlock;
}
if (zl3073x_dpll_is_p_pin(pin)) {
@@ -1022,8 +1079,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
* Update divisor for N-pin to keep N-pin frequency.
*/
out.esync_n_period = (out.esync_n_period * out.div) / new_div;
[Severity: Medium]
Does the N pin also need a notification on this path?
In the N-div formats OUTPUT_DIV is still shared. The N-pin frequency is
computed from both values:
drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
...
freq = zl3073x_synth_freq_get(synth) / out->div;
if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
freq /= out->esync_n_period;
...
}
The rescale above truncates and only rejects 0. zl3073x_pin_check_freq()
only requires the new P frequency to divide the synth frequency.
For example, take a 500 MHz synth with P = 10 MHz (div 50) and
N = 2 MHz (period 5). Setting P to 25 MHz gives new_div 20 and period
250 / 20 = 12. N then becomes 500M / (20 * 12), about 2.083 MHz.
This path reaches the unlock label with sibling still NULL, so the N
pin gets no DPLL_CMD_PIN_CHANGE_NTF. The commit message says the sibling
is notified "whenever the shared HW state actually changes". Should this
case be covered too?
The later commit "dpll: zl3073x: factor out output pin frequency helper"
does not change this. The truncation moves into
zl3073x_dpll_output_pin_freq_set(), and frequency_set still calls
zl3073x_dpll_output_pin_sibling_get() only when
!zl3073x_out_is_ndiv(&out).
By design the N-pin frequency is meant to be preserved when the P-pin
frequency changes - that is the purpose of rescaling esync_n_period. With
an exact rescale, div_new * period_new == div_old * period_old, so
N = synth / (div * period) is unchanged and no sibling notification is
needed.
The only thing that can actually perturb N here is the truncation in that
rescale, which is the separate issue Sashiko raised on patch 5 ("does this
rescale really keep the N-pin frequency unchanged?"). I will fix it there
by rejecting an inexact rescale with -EINVAL (i.e. when div_old * period_old
is not divisible by div_new), so N is guaranteed to stay unchanged. With that
fix there is nothing to notify on this path, so no sibling notification is added
here.
Thanks,
Ivan