Re: [PATCH net v4 5/5] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera <ivecera@redhat.com>]

On 10/10/26 21:27, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Pins that are on the list but not registered get passed to
>    __dpll_pin_change_ntf(), and the new NCO comment is wrong.
> 
> --
> 
> dpll: zl3073x: notify sibling pin when shared output config changes
> 
> This patch adds zl3073x_dpll_output_pin_sibling_get(). It sends a change
> notification for the sibling P/N pin from frequency_set(), esync_set() and
> phase_adjust_set() after zldpll->lock is dropped. Pins are now added to
> zldpll->pins before dpll_pin_register() and removed after
> dpll_pin_unregister(), so a registered sibling can always be found.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 6d9a6d21d30b7..1d61fe5fe5862 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> @@ -132,11 +134,42 @@ 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 in the pin list
>> + * of this DPLL. A pin is on the list from just before its registration
>> + * until just after its unregistration, so a registered sibling is always
>> + * found, but the returned pin may also be one that is not (yet or any
>> + * longer) registered.
>> + *
>> + * Return: pointer to sibling pin, or NULL if it is not on the pin list
>> + */
> 
> [ ... ]
> 
>> @@ -937,9 +974,24 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
>>   
>>   	/* Enable 1PPS eSync for this pin frequency */
>>   	zl3073x_out_esync_enable(&out, synth_freq / out.div);
>> -
>> +commit:
>>   	/* 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 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.
>> +	 */
>> +	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
>> +unlock:
>> +	mutex_unlock(&zldpll->lock);
>> +
>> +	if (!rc && sibling)
>> +		__dpll_pin_change_ntf(sibling->dpll_pin);
> 
> [Severity: Low]
> 
> The kernel-doc for __dpll_pin_change_ntf() in drivers/dpll/dpll_netlink.c
> says its argument is "@pin: registered pin pointer". The kernel-doc for
> zl3073x_dpll_output_pin_sibling_get() above says the returned pin "may
> also be one that is not (yet or any longer) registered".
> 
> Should the sibling's registration state be checked before it is passed to
> __dpll_pin_change_ntf() here? The same question applies to the matching
> calls in zl3073x_dpll_output_pin_frequency_set() and
> zl3073x_dpll_output_pin_phase_adjust_set().
> 
> __dpll_pin_change_ntf() runs the notifier chain before the
> dpll_pin_available() check in dpll_pin_event_send():
> 
> int __dpll_pin_change_ntf(struct dpll_pin *pin)
> {
> 	lockdep_assert_held(&dpll_lock);
> 	dpll_pin_notify(pin, pin->clock_id, DPLL_PIN_CHANGED);
> 	return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin);
> }
> 
> This means a notifier subscriber can get DPLL_PIN_CHANGED for a pin
> before it got DPLL_PIN_CREATED, or after it got DPLL_PIN_DELETED.
> 
> It looks like this can happen during probe:
> 
> - zl3073x_dpll_device_register() runs before zl3073x_dpll_pins_register().
> - The P pin (even id) is registered before its N sibling.
> - A userspace PIN_SET on the registered P pin holds dpll_lock.
> - That PIN_SET can find N on the list after list_add(), while
>    dpll_pin_register(N) is still waiting for dpll_lock.
> 
> list_add() now runs before dpll_pin_register() for every pin type. So the
> existing callers of zl3073x_dpll_nco_pin_get() and
> zl3073x_dpll_pin_get_by_ref() can also get a pin that is not registered
> yet and pass it to __dpll_pin_change_ntf(). Before this patch, that could
> only happen during teardown, between dpll_pin_unregister() and list_del().
> 
> As the commit message says, no netlink message is sent for such pins, and
> ice_dpll_pin_notify() ignores DPLL_PIN_CHANGED. So nothing visibly breaks
> today.
> 
> Would a per-pin registered flag be cleaner? It could be set and cleared
> under zldpll->lock around dpll_pin_register() and dpll_pin_unregister(),
> and checked before notifying. Another option is to relax the core
> kernel-doc and have dpll_pin_notify() check registration.

Such a pin gets no netlink notification and the only in-tree notifier
ignores DPLL_PIN_CHANGED, so I would rather not add a registered flag
in a net fix.

> Separately, the new comment in zl3073x_dpll_nco_pin_register() says:
> 
> 	/* Add the pin to the list before registering it with the DPLL core so
> 	 * that the list reflects the DPLL registration state.
> 	 */
> 
> Is this accurate? With the early list_add(), the list holds every
> registered pin and also pins that are not registered yet.

Right, will fix the comment in v5.

Thanks,
Ivan