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

[Ivan Vecera]

On 10/12/26 08:13, netdev-bot+sashiko@xxxxxxxxxx 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