Re: [PATCH net-next 1/6] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera <ivecera@redhat.com>]

On 9/30/26 8:57 AM, netdev-bot+sashiko@kernel.org 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