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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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] In N-div signal formats, setting the P-pin frequency can change
  the N-pin frequency without notifying the N-pin.
- [Low] The commit message overstates the locking change.

--

dpll: zl3073x: notify sibling pin when shared output config changes

This patch adds zl3073x_dpll_output_pin_sibling_get(). Frequency, esync
and phase adjust set callbacks use it to notify the other pin of an
output pair when shared output state changes. It also takes zldpll->lock
around the registration and unregistration changes to zldpll->pins.

> The sibling lookup walks the zldpll->pins list, so serialize all
> accesses to that list with zldpll->lock: take it around list_add() and
> list_del() in the pin (un)registration paths and assert it is held in
> the list walkers.

[Severity: Low]
Is this description accurate? After this patch, several walkers of
zldpll->pins still don't follow it.

zl3073x_dpll_changes_check() walks the list again after dropping the lock:

unlock:
	mutex_unlock(&zldpll->lock);
	...
	list_for_each_entry(pin, &zldpll->pins, list) {
		...
			dpll_pin_change_ntf(pin->dpll_pin);
	}

zl3073x_dpll_ref_sync_pairs_register() walks the list without the lock
and without an assertion. zl3073x_dpll_ptp_adjtime_phase_step() has no
lockdep_assert_held(), although its caller zl3073x_dpll_ptp_adjtime()
does hold the lock.

Also, zl3073x_dpll_pins_unregister() takes the lock around
list_splice_init(), not list_del(). list_del() runs outside the lock on
the spliced private list.

I don't see a live race here. The monitor work is only queued after
zl3073x_dpll_register() in zl3073x_dev_start(). It is cancelled
synchronously in zl3073x_dev_stop() before unregistration.
zl3073x_dpll_ref_sync_pairs_register() runs in the registration thread.

Could the message be narrowed to the lookup helpers? Alternatively, the
remaining walkers could take the lock, with assertions added where the
lock is already held.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 9eec27331fe3d..d71471d64aec4 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -1001,7 +1053,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>  		}
>  
>  		/* 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)) {
> @@ -1013,8 +1074,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;
> -		if (!out.esync_n_period)
> -			return -EINVAL;
> +		if (!out.esync_n_period) {
> +			rc = -EINVAL;
> +			goto unlock;
> +		}
>  
>  		/* Update the output divisor */
>  		out.div = new_div;

[Severity: High]
In N-div mode, can setting the P-pin frequency change the N-pin
frequency without any notification for the N-pin?

This branch rewrites out.div, which the N-pin shares. It also recomputes
esync_n_period with truncating division, and only a zero result is
rejected. zl3073x_dev_output_pin_freq_get() reports the N-pin frequency
as:

	freq = zl3073x_synth_freq_get(synth) / out->div;

	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
		freq /= out->esync_n_period;

So if old_div * old_period is not a multiple of new_div, the N-pin
output changes.

For example, take a 600 MHz synth with div=60 and period=10, so P is
10 MHz and N is 1 MHz. Setting P to 2.5 MHz gives new_div=240 and
period = 600 / 240 = 2, so N becomes 1.25 MHz.

zl3073x_pin_check_freq() only checks that the requested frequency
divides the synth frequency, so this request is accepted.
zl3073x_out_state_set() then commits the new values to hardware.

The sibling is only looked up in the !zl3073x_out_is_ndiv() branch.
This path therefore reaches the unlock label with sibling == NULL, and
dpll_pin_freq_set() in the core only notifies the requested pin.

The commit message excludes N-div mode on the assumption that the N-pin
frequency is kept, but the arithmetic doesn't guarantee that.

Would it work to compare the sibling's effective frequency before and
after the commit and notify on change? Another option is to reject P
frequencies that can't keep the N frequency exact.

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