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

[netdev-bot+sashiko@kernel.org]

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.

> +
> +	return rc;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011061211.448373-1-ivecera%40redhat.com