Re: [PATCH net v5 2/5] dpll: zl3073x: reject inexact frequencies for N-divided outputs

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] Concern: zl3073x_pin_props_get() always lists the current
  (truncated) frequency as supported.

Pre-existing issues:
- [Low] Concern: userspace never sees the three new extack messages added
  in `zl3073x_dpll_output_pin_frequency_set()` ("must be at most half",…

--

dpll: zl3073x: reject inexact frequencies for N-divided outputs

This commit tightens zl3073x_dpll_output_pin_frequency_set() for
N-divided outputs. It computes esync_n_period in 64 bits and requires it
to be at least 2 and to fit into 32 bits. It also rejects any request
whose division leaves a remainder, and reports each failure case through
extack.

[ ... ]

[ ... ]

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
will userspace ever see this message, or the two added below it?

When ops->frequency_set() fails, the DPLL core overwrites the driver's
extack text:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
    ...
	ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
				 dpll, dpll_priv(dpll), freq, extack);
	if (ret) {
		NL_SET_ERR_MSG_FMT(extack,
				   "frequency set failed for dpll_id:%u",
				   dpll->id);
		return ret;
	}
    ...
}

NL_SET_ERR_MSG_FMT() always writes into extack->_msg_buf and resets
_msg. The "must be at most half", "is too low" and "must divide" messages
are therefore replaced by the generic text, and they can only be seen
through the netlink_extack tracepoint. The -EINVAL return is still
correct.

[Severity: Low]
Can this check reject a frequency that zl3073x_pin_props_get() lists as
supported?

zl3073x_pin_props_get() always puts the current frequency into the
supported list. For an N-pin, that value comes from a truncating
division:

drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
    ...
	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
		freq /= out->esync_n_period;
    ...
}

Also, zl3073x_out_state_fetch() only refuses esync_n_period == 0, so a
boot config with esync_n_period == 1 is accepted.

A request for the current value never reaches the driver, because
dpll_pin_freq_set() returns early when freq == old_freq. If the pin is
first moved to another frequency, though, a later request for the
advertised boot value can fail.

Take a 600 MHz synth, div=60 and a boot esync_n_period of 7. The N-pin
is advertised as 1428571 Hz. After N is moved to 1 MHz, a request for
1428571 gives rem=180 and returns -EINVAL.

In the same way, a P-pin frequency that would bring back
esync_n_period == 1 now fails the n_period < 2 check. According to the
series commit "dpll: zl3073x: reject output frequencies with too small
divisor", that state gave no N-pin output.

This looks consistent with the goal of rejecting inexact N frequencies.
A later comment in prop.c in this series also says the supported list
for N-divided outputs is only a necessary condition. The old code
already rejected some advertised values, such as N-pin frequencies at or
above the P-pin frequency.

Is it intended that a user cannot go back to an inexact boot-time N-pin
setting by asking for its advertised value?

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