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

[Ivan Vecera <ivecera@redhat.com>]

On 10/10/26 21:27, netdev-bot+sashiko@kernel.org wrote:
> 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.

Yes, I will move the !freq check into this patch.

Thanks,
Ivan
---
pw-bot: cr