sh: clk: Fix clk_enable() to return 0 on NULL clk

Commit Message

Geert Uytterhoeven Feb. 2, 2023, 4:20 p.m. UTC

On SH, devm_clk_get_optional_enabled() fails with -EINVAL if the clock
is not found.  This happens because __devm_clk_get() assumes it can pass
a NULL clock pointer (as returned by clk_get_optional()) to the init()
function (clk_prepare_enable() in this case), while the SH
implementation of clk_enable() considers that an error.

Fix this by making the SH clk_enable() implementation return zero
instead, like the Common Clock Framework does.

Reported-by: John Paul Adrian Glaubitz <[e-mail]>
Signed-off-by: Geert Uytterhoeven <[e-mail]>
---
Exposed by commit 599566c1c3692052 ("r8169: use
devm_clk_get_optional_enabled() to simplify the code"), cfr.
https://lore.kernel.org/all/[e-mail]

Boot-tested on qemu/rts7751r2d, which did not show the problem though.
---
 drivers/sh/clk/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

John Paul Adrian Glaubitz Feb. 2, 2023, 4:28 p.m. UTC | #1

Hi Geert!

On Thu, 2023-02-02 at 17:20 +0100, Geert Uytterhoeven wrote:
> On SH, devm_clk_get_optional_enabled() fails with -EINVAL if the clock
> is not found.  This happens because __devm_clk_get() assumes it can pass
> a NULL clock pointer (as returned by clk_get_optional()) to the init()
> function (clk_prepare_enable() in this case), while the SH
> implementation of clk_enable() considers that an error.
> 
> Fix this by making the SH clk_enable() implementation return zero
> instead, like the Common Clock Framework does.
> 
> Reported-by: John Paul Adrian Glaubitz <[e-mail]>
> Signed-off-by: Geert Uytterhoeven <[e-mail]>
> ---
> Exposed by commit 599566c1c3692052 ("r8169: use
> devm_clk_get_optional_enabled() to simplify the code"), cfr.
> https://lore.kernel.org/all/[e-mail]
> 
> Boot-tested on qemu/rts7751r2d, which did not show the problem though.
> ---
>  drivers/sh/clk/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
> index d996782a710642cd..7a73f5e4a1fc70cc 100644
> --- a/drivers/sh/clk/core.c
> +++ b/drivers/sh/clk/core.c
> @@ -295,7 +295,7 @@ int clk_enable(struct clk *clk)
>  	int ret;
>  
>  	if (!clk)
> -		return -EINVAL;
> +		return 0;
>  
>  	spin_lock_irqsave(&clock_lock, flags);
>  	ret = __clk_enable(clk);

I can confirm that this patch makes the r8169 driver work again!

Tested-by: John Paul Adrian Glaubitz <[e-mail]>

Adrian

Stephen Boyd Feb. 2, 2023, 11:17 p.m. UTC | #2

Quoting Geert Uytterhoeven (2023-02-02 08:20:55)
> On SH, devm_clk_get_optional_enabled() fails with -EINVAL if the clock
> is not found.  This happens because __devm_clk_get() assumes it can pass
> a NULL clock pointer (as returned by clk_get_optional()) to the init()
> function (clk_prepare_enable() in this case), while the SH
> implementation of clk_enable() considers that an error.
> 
> Fix this by making the SH clk_enable() implementation return zero
> instead, like the Common Clock Framework does.
> 
> Reported-by: John Paul Adrian Glaubitz <[e-mail]>
> Signed-off-by: Geert Uytterhoeven <[e-mail]>
> ---

Acked-by: Stephen Boyd <[e-mail]>

Patch

diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
index d996782a710642cd..7a73f5e4a1fc70cc 100644
--- a/drivers/sh/clk/core.c
+++ b/drivers/sh/clk/core.c
@@ -295,7 +295,7 @@  int clk_enable(struct clk *clk)
 	int ret;
 
 	if (!clk)
-		return -EINVAL;
+		return 0;
 
 	spin_lock_irqsave(&clock_lock, flags);
 	ret = __clk_enable(clk);