[PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[Eugen Hristev]

Unlike it's Linux counterpart, clk_get_rate can return a negative value, -ve.
The driver does not take that into account, stores the rate into
an unsigned long, and if clk_get_rate fails, it will take into consideration
the actual value and wrongly program the hardware.
E.g. on error -2 (no such entry), the rate will be 18446744073709551614
and this will be subsequently used by the driver to program the DWC3
To fix this, exit the function if the value is negative.

Fixes: 6bae0eb5b8bd ("usb: dwc3: Calculate REFCLKPER based on reference clock")
Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
---
 drivers/usb/dwc3/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 49f6a1900b01..5a8c29424578 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -138,7 +138,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
 
 	if (dwc->ref_clk) {
 		rate = clk_get_rate(dwc->ref_clk);
-		if (!rate)
+		if (!rate || (long)rate < 0)
 			return;
 		period = NSEC_PER_SEC / rate;
 	} else {
-- 
2.34.1

Re: [PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[Marek Vasut]

On 6/15/23 16:59, Eugen Hristev wrote:
> Unlike it's Linux counterpart, clk_get_rate can return a negative value, -ve.
> The driver does not take that into account, stores the rate into
> an unsigned long, and if clk_get_rate fails, it will take into consideration
> the actual value and wrongly program the hardware.
> E.g. on error -2 (no such entry), the rate will be 18446744073709551614
> and this will be subsequently used by the driver to program the DWC3
> To fix this, exit the function if the value is negative.
> 
> Fixes: 6bae0eb5b8bd ("usb: dwc3: Calculate REFCLKPER based on reference clock")
> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
> ---
>   drivers/usb/dwc3/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 49f6a1900b01..5a8c29424578 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -138,7 +138,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>   
>   	if (dwc->ref_clk) {
>   		rate = clk_get_rate(dwc->ref_clk);

This ^ assigns to unsigned long .

Please just change the data type of the $rate to 'long' (drop the unsigned).

> -		if (!rate)
> +		if (!rate || (long)rate < 0)

if (rate <= 0) will work too ^

Re: [PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[Eugen Hristev]

On 6/15/23 23:59, Marek Vasut wrote:
> On 6/15/23 16:59, Eugen Hristev wrote:
>> Unlike it's Linux counterpart, clk_get_rate can return a negative 
>> value, -ve.
>> The driver does not take that into account, stores the rate into
>> an unsigned long, and if clk_get_rate fails, it will take into 
>> consideration
>> the actual value and wrongly program the hardware.
>> E.g. on error -2 (no such entry), the rate will be 18446744073709551614
>> and this will be subsequently used by the driver to program the DWC3
>> To fix this, exit the function if the value is negative.
>>
>> Fixes: 6bae0eb5b8bd ("usb: dwc3: Calculate REFCLKPER based on 
>> reference clock")
>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>> ---
>>   drivers/usb/dwc3/core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 49f6a1900b01..5a8c29424578 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -138,7 +138,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>       if (dwc->ref_clk) {
>>           rate = clk_get_rate(dwc->ref_clk);
> 
> This ^ assigns to unsigned long .

That's right, because clk_get_rate returns an unsigned long
> 
> Please just change the data type of the $rate to 'long' (drop the 
> unsigned).

clk_get_rate returns unsigned long, so it's not normal to assign it to a 
signed long.

It is not normal that clk_get_rate returns negative values in an 
unsigned, but oh well, that's how the clock API is in U-boot

> 
>> -        if (!rate)
>> +        if (!rate || (long)rate < 0)
> 
> if (rate <= 0) will work too ^

Re: [PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[Marek Vasut]

On 6/16/23 09:31, Eugen Hristev wrote:
> On 6/15/23 23:59, Marek Vasut wrote:
>> On 6/15/23 16:59, Eugen Hristev wrote:
>>> Unlike it's Linux counterpart, clk_get_rate can return a negative 
>>> value, -ve.
>>> The driver does not take that into account, stores the rate into
>>> an unsigned long, and if clk_get_rate fails, it will take into 
>>> consideration
>>> the actual value and wrongly program the hardware.
>>> E.g. on error -2 (no such entry), the rate will be 18446744073709551614
>>> and this will be subsequently used by the driver to program the DWC3
>>> To fix this, exit the function if the value is negative.
>>>
>>> Fixes: 6bae0eb5b8bd ("usb: dwc3: Calculate REFCLKPER based on 
>>> reference clock")
>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>>> ---
>>>   drivers/usb/dwc3/core.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 49f6a1900b01..5a8c29424578 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -138,7 +138,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>       if (dwc->ref_clk) {
>>>           rate = clk_get_rate(dwc->ref_clk);
>>
>> This ^ assigns to unsigned long .
> 
> That's right, because clk_get_rate returns an unsigned long
>>
>> Please just change the data type of the $rate to 'long' (drop the 
>> unsigned).
> 
> clk_get_rate returns unsigned long, so it's not normal to assign it to a 
> signed long.
> 
> It is not normal that clk_get_rate returns negative values in an 
> unsigned, but oh well, that's how the clock API is in U-boot

Ah, I see. Then why not do this diff, and fix any clock driver which 
returns negative from clk_get_rate() instead ?

diff --git a/include/clk.h b/include/clk.h
index d91285235f7..4e5c1fc90f6 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -453,8 +453,7 @@ void clk_free(struct clk *clk);
   * @clk:       A clock struct that was previously successfully 
requested by
   *             clk_request/get_by_*().
   *
- * Return: clock rate in Hz on success, 0 for invalid clock, or -ve 
error code
- *        for other errors.
+ * Return: clock rate in Hz on success, 0 for invalid clock.
   */
  ulong clk_get_rate(struct clk *clk);

Re: [PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[Eugen Hristev]

On 6/16/23 12:22, Marek Vasut wrote:
> On 6/16/23 09:31, Eugen Hristev wrote:
>> On 6/15/23 23:59, Marek Vasut wrote:
>>> On 6/15/23 16:59, Eugen Hristev wrote:
>>>> Unlike it's Linux counterpart, clk_get_rate can return a negative 
>>>> value, -ve.
>>>> The driver does not take that into account, stores the rate into
>>>> an unsigned long, and if clk_get_rate fails, it will take into 
>>>> consideration
>>>> the actual value and wrongly program the hardware.
>>>> E.g. on error -2 (no such entry), the rate will be 18446744073709551614
>>>> and this will be subsequently used by the driver to program the DWC3
>>>> To fix this, exit the function if the value is negative.
>>>>
>>>> Fixes: 6bae0eb5b8bd ("usb: dwc3: Calculate REFCLKPER based on 
>>>> reference clock")
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>>>> ---
>>>>   drivers/usb/dwc3/core.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 49f6a1900b01..5a8c29424578 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -138,7 +138,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>       if (dwc->ref_clk) {
>>>>           rate = clk_get_rate(dwc->ref_clk);
>>>
>>> This ^ assigns to unsigned long .
>>
>> That's right, because clk_get_rate returns an unsigned long
>>>
>>> Please just change the data type of the $rate to 'long' (drop the 
>>> unsigned).
>>
>> clk_get_rate returns unsigned long, so it's not normal to assign it to 
>> a signed long.
>>
>> It is not normal that clk_get_rate returns negative values in an 
>> unsigned, but oh well, that's how the clock API is in U-boot
> 
> Ah, I see. Then why not do this diff, and fix any clock driver which 
> returns negative from clk_get_rate() instead ?
> 
> diff --git a/include/clk.h b/include/clk.h
> index d91285235f7..4e5c1fc90f6 100644
> --- a/include/clk.h
> +++ b/include/clk.h
> @@ -453,8 +453,7 @@ void clk_free(struct clk *clk);
>    * @clk:       A clock struct that was previously successfully 
> requested by
>    *             clk_request/get_by_*().
>    *
> - * Return: clock rate in Hz on success, 0 for invalid clock, or -ve 
> error code
> - *        for other errors.
> + * Return: clock rate in Hz on success, 0 for invalid clock.
>    */
>   ulong clk_get_rate(struct clk *clk);
> 

Because clk_get_rate really returns negative numbers, I cannot update 
the documentation for it when in fact it still can return errors.

This patch which I sent, fixes the driver from the perspective of 
'clk_get_rate is like this, driver should cope with it'

If you disagree with my patch, the the only way forward that I see right 
now, is to modify the clk_get_rate to no longer return negative numbers 
as errors. But this would mean modifying the clk subsystem, and it's no 
longer a simple fix rather a change in the API. (a simple fix which I 
intended with this patch )

Re: [PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[Marek Vasut]

On 6/16/23 11:30, Eugen Hristev wrote:
> On 6/16/23 12:22, Marek Vasut wrote:
>> On 6/16/23 09:31, Eugen Hristev wrote:
>>> On 6/15/23 23:59, Marek Vasut wrote:
>>>> On 6/15/23 16:59, Eugen Hristev wrote:
>>>>> Unlike it's Linux counterpart, clk_get_rate can return a negative 
>>>>> value, -ve.
>>>>> The driver does not take that into account, stores the rate into
>>>>> an unsigned long, and if clk_get_rate fails, it will take into 
>>>>> consideration
>>>>> the actual value and wrongly program the hardware.
>>>>> E.g. on error -2 (no such entry), the rate will be 
>>>>> 18446744073709551614
>>>>> and this will be subsequently used by the driver to program the DWC3
>>>>> To fix this, exit the function if the value is negative.
>>>>>
>>>>> Fixes: 6bae0eb5b8bd ("usb: dwc3: Calculate REFCLKPER based on 
>>>>> reference clock")
>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>>>>> ---
>>>>>   drivers/usb/dwc3/core.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 49f6a1900b01..5a8c29424578 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -138,7 +138,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>>       if (dwc->ref_clk) {
>>>>>           rate = clk_get_rate(dwc->ref_clk);
>>>>
>>>> This ^ assigns to unsigned long .
>>>
>>> That's right, because clk_get_rate returns an unsigned long
>>>>
>>>> Please just change the data type of the $rate to 'long' (drop the 
>>>> unsigned).
>>>
>>> clk_get_rate returns unsigned long, so it's not normal to assign it 
>>> to a signed long.
>>>
>>> It is not normal that clk_get_rate returns negative values in an 
>>> unsigned, but oh well, that's how the clock API is in U-boot
>>
>> Ah, I see. Then why not do this diff, and fix any clock driver which 
>> returns negative from clk_get_rate() instead ?
>>
>> diff --git a/include/clk.h b/include/clk.h
>> index d91285235f7..4e5c1fc90f6 100644
>> --- a/include/clk.h
>> +++ b/include/clk.h
>> @@ -453,8 +453,7 @@ void clk_free(struct clk *clk);
>>    * @clk:       A clock struct that was previously successfully 
>> requested by
>>    *             clk_request/get_by_*().
>>    *
>> - * Return: clock rate in Hz on success, 0 for invalid clock, or -ve 
>> error code
>> - *        for other errors.
>> + * Return: clock rate in Hz on success, 0 for invalid clock.
>>    */
>>   ulong clk_get_rate(struct clk *clk);
>>
> 
> Because clk_get_rate really returns negative numbers, I cannot update 
> the documentation for it when in fact it still can return errors.

Let me make it clear, let's not hack around bugs by introducing known 
defective code into the USB subsystem.

NAK

> This patch which I sent, fixes the driver from the perspective of 
> 'clk_get_rate is like this, driver should cope with it'

No, it just adds a broken workaround which hides the real issue. The 
real issue should be addressed.

> If you disagree with my patch, the the only way forward that I see right 
> now, is to modify the clk_get_rate to no longer return negative numbers 
> as errors.

Yes please. Just align the API with Linux, that would be even better.

> But this would mean modifying the clk subsystem, and it's no 
> longer a simple fix rather a change in the API. (a simple fix which I 
> intended with this patch )

Try and add 'ccflags-y += -Werror -Wsign-conversion' into 
drivers/clk/Makefile , then run buildman to get all the signed 
conversion issues, and just fix them. That should be rather a mechanical 
task.