Re: [PATCH net v4 1/5] dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · 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).

- [Medium] At this commit, the subject says 0 Hz is no longer offered,
  but zl3073x_pin_props_get() (drivers/dpll/zl3073x/prop.c) only filters…

--

dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

This patch stops zl3073x_pin_props_get() from putting the current pin
frequency into the supported list when it reads back as 0 Hz. If the
list ends up empty, it is freed and not published, because the DPLL
core rejects an empty non-NULL list.

> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index ac9d41d0f978e..18a1bf3103326 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c
> @@ -295,13 +295,20 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
>  		goto err_alloc_ranges;
>  	}
>  
> -	/* Start with current frequency at index 0 */
> -	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
> +	/* Start with current frequency at index 0. A sub-Hz frequency is
> +	 * read back as 0 Hz and cannot be set, so it is not offered.
> +	 */
> +	j = 0;
> +	if (curr_freq) {
> +		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);
> +
> +		ranges[j++] = freq;
> +	}
>  
>  	/* Add frequencies from firmware node, skipping current frequency
>  	 * and filtering out frequencies not representable by device
>  	 */
> -	for (i = 0, j = 1; i < num_freqs; i++) {
> +	for (i = 0; i < num_freqs; i++) {
>  		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(freqs[i]);
>  
>  		if (freqs[i] == curr_freq)

[Severity: Medium]
Can 0 Hz still end up offered through the firmware list?

The subject says 0 Hz is no longer offered. However, only a 0 Hz current
frequency is filtered.

Suppose the "supported-frequencies-hz" property has a 0 entry and
curr_freq is nonzero. That entry is not skipped by the
freqs[i] == curr_freq check. It goes on to zl3073x_pin_check_freq(),
which has no zero check at this commit:

drivers/dpll/zl3073x/prop.c:zl3073x_pin_check_freq() {
	if (freq > U32_MAX)
		goto err_inv_freq;
    ...
		/* Check the frequency divides synth frequency */
		if (synth_freq % (u32)freq)
			goto err_inv_freq;
    ...
}

On an output pin, does this divide by zero during probe?

On an input pin, zl3073x_ref_freq_factorize(0, ...) succeeds on the first
base frequency of 1. Here 0 / 1 = 0 <= U16_MAX and 0 % 1 == 0, so 0 Hz is
published as supported. It could then be programmed with base 1 and
multiplier 0.

The dpll-pin.yaml binding sets no minimum for supported-frequencies-hz,
so the schema does not reject 0.

A later patch in this series, "dpll: zl3073x: reject output frequencies
with too small divisor", changes the first check in
zl3073x_pin_check_freq() to:

	if (!freq || freq > U32_MAX)
		goto err_inv_freq;

That fixes this by the end of the series. At this intermediate commit,
though, the 0 Hz case is still reachable from firmware.

Would it make sense to move the !freq check into this patch? Bisection
would then not land on a kernel that can still divide by zero here.

[ ... ]

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