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

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

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 zl3073x_dpll_output_pin_frequency_set()
  (drivers/dpll/zl3073x/dpll.c), the N-div case is excluded from the new…
- [Medium] Pins are on zldpll->pins only for part of the time they are
  registered with the DPLL core, so the new sibling lookup can miss a…

--

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

This commit adds zl3073x_dpll_output_pin_sibling_get(). The output pin
frequency, esync and phase adjust setters use it to send a change
notification for the other pin of a P/N output pair. The setters move from
guard(mutex) to explicit lock/unlock so the notification is sent after
zldpll->lock is dropped, and all accesses to zldpll->pins are now
serialized with zldpll->lock.

[ ... ]

[Severity: High]
Can the N-pin's frequency still change on this path without a
notification?

In N-div mode, zl3073x_dev_output_pin_freq_get() computes the N-pin
frequency from both the shared divisor and esync_n_period:

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

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

When the P-pin frequency is set, the esync_n_period rescale above
truncates whenever old_period * old_div is not a multiple of new_div.

For example, take a 1 GHz synth with div = 10 (P = 100 MHz) and period = 4
(N = 25 MHz). Setting P to 40 MHz gives new_div = 25 and period = 40 / 25 =
1, so N becomes 40 MHz. Both frequencies pass zl3073x_pin_check_freq()
because they divide the synth frequency.

zl3073x_out_state_set() then commits the new DIV and ESYNC_PERIOD values.
However, sibling is only assigned in the non-N-div branch, so it is still
NULL at the tail of the function:

	if (!rc && sibling)
		__dpll_pin_change_ntf(sibling->dpll_pin);

No notification is sent for the N-pin, even though it is registered in
N-div mode.

The truncation itself predates this patch. The missing notification,
though, is the case this patch sets out to fix. The commit message says:

    share a single HW output and, outside N-pin divide mode, share the
    output's divisor, ...

and:

    ... to notify the sibling pin, if it is registered, whenever the
    shared HW state actually changes.

Is that accurate, given that the divisor is also part of the N-pin
frequency in N-div mode?

Should this branch either notify the N-pin when its effective frequency
changes, or reject a rescale that is not exact?

[ ... ]

[Severity: Medium]
Can zl3073x_dpll_output_pin_sibling_get() miss a sibling that is already
registered with the DPLL core?

zl3073x_dpll_pin_register()->dpll_pin_register() publishes the pin. It
takes and drops dpll_lock and sends PIN_CREATE before the pin is added to
zldpll->pins here. P (even id) is registered before N (odd id), so this
window exists:

CPU1 (probe)
zl3073x_dpll_pins_register()
  zl3073x_dpll_pin_register(N)
    dpll_pin_register()
      N is now visible, PIN_CREATE sent

CPU2 (netlink PIN_SET on P, dpll_lock held)
zl3073x_dpll_output_pin_phase_adjust_set()
  zl3073x_out_state_set()
  zl3073x_dpll_output_pin_sibling_get()
    N is not on zldpll->pins yet, returns NULL

CPU1
  list_add(&pin->list, &zldpll->pins);

At that point userspace still has the CREATE values for N, but the shared
div, esync or phase_comp has already changed, and no change notification
is sent for N.

Teardown has the opposite window. zl3073x_dpll_pins_unregister() empties
the list before any dpll_pin_unregister() call:

	mutex_lock(&zldpll->lock);
	list_splice_init(&zldpll->pins, &pin_list);
	mutex_unlock(&zldpll->lock);

A PIN_SET on a P-pin that is still registered therefore also skips an
N-pin that is still registered. A DELETE notification follows in that
case.

This cannot cause a use-after-free. The sibling is freed only after
dpll_pin_unregister(), which needs dpll_lock, and the PIN_SET path holds
dpll_lock.

However, list membership does not match DPLL registration state, so the
notification this patch adds can still be lost. Should the lookup be tied
to registration state instead of list membership?

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