[PATCH] usb: dwc3: core: fix clk_get_rate returning negative value
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 {
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 ^
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 ^
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);
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 )
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.
participants (2)
-
Eugen Hristev -
Marek Vasut