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

[netdev-bot+sashiko@kernel.org]

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")

> 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.

[ ... ]

> @@ -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).

> -		if (!out.esync_n_period)
> -			return -EINVAL;
> +		if (!out.esync_n_period) {
> +			rc = -EINVAL;
> +			goto unlock;
> +		}

[ ... ]

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