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

[Ivan Vecera <ivecera@redhat.com>]

On 10/12/26 08:13, 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] The patch passes pins that may not be registered to
>    __dpll_pin_change_ntf(), whose kernel-doc requires a "registered pin…
> 
> --
> 
> dpll: zl3073x: notify sibling pin when shared output config changes
> 
> This commit adds zl3073x_dpll_output_pin_sibling_get(). The output
> frequency, esync and phase adjust set callbacks now use it to send a
> change notification for the other pin of the P/N pair. So that the
> sibling can always be found, pins are added to zldpll->pins before
> dpll_pin_register() and removed only after dpll_pin_unregister().
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 6d9a6d21d30b7..1cdadb8e46fb2 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.
> 
> [ ... ]
> 
>> @@ -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]
> Is it ok to pass a sibling that may not be registered to
> __dpll_pin_change_ntf()? The kernel-doc for that function asks for a
> registered pin:
> 
> drivers/dpll/dpll_netlink.c:
> /**
>   * __dpll_pin_change_ntf - notify that the pin has been changed
>   * @pin: registered pin pointer
>   ...
> 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);
> }
> 
> dpll_pin_event_send() checks dpll_pin_available(), so no netlink message
> is sent. But dpll_pin_notify() runs before that check, so the in-kernel
> notifier chain still gets DPLL_PIN_CHANGED for the unregistered pin.
> 
> This window looks reachable during probe. zl3073x_dpll_register()
> registers the dpll device before its pins, and pins are registered in
> index order. That means P is already registered when N is list_add()ed
> in zl3073x_dpll_pin_register():
> 
>      mutex_lock(&zldpll->lock);
>      list_add(&pin->list, &zldpll->pins);
>      mutex_unlock(&zldpll->lock);
> 
>      /* Register the pin */
>      rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
> 
> Suppose a DPLL_CMD_PIN_SET on P holds dpll_lock from dpll_pin_pre_doit().
> N's dpll_pin_register() then waits on dpll_lock. Meanwhile P's callback
> finds N on the list and sends DPLL_PIN_CHANGED for N before N's
> DPLL_PIN_CREATED.
> 
> Teardown is the mirror case. list_add() inserts at the head, so N is
> unregistered before P. N also stays on the list until after
> dpll_pin_unregister() returns in zl3073x_dpll_pin_unregister(). A
> PIN_SET on P during that window would send CHANGED for N after its
> DPLL_PIN_DELETED.
> 
> zl3073x_dpll_output_pin_frequency_set() and
> zl3073x_dpll_output_pin_phase_adjust_set() have the same issue.
> 
> zl3073x_dpll_nco_pin_register() now also adds the NCO pin to the list
> before dpll_pin_register(). Because of that, the existing callers that
> notify the result of zl3073x_dpll_nco_pin_get() (the input pin
> state_on_dpll_set and mode_set paths) hit the same window.
> 
> As the commit message notes, nothing breaks in-tree today, because
> ice_dpll_pin_notify() ignores every action except DPLL_PIN_CREATED and
> DPLL_PIN_DELETED.
> 
> Would it be better to skip the notification when the sibling is not
> registered? Alternatively, the __dpll_pin_change_ntf() kernel-doc and
> dpll_pin_notify() could be updated to accept unregistered pins
> explicitly.
Such a pin gets no netlink notification and the only in-tree notifier
ignores DPLL_PIN_CHANGED, so I would rather not add a registration
check in a net fix.

Thanks,
Ivan