[PATCH] sh: clk: Extend valid clk ptr checks using IS_ERR_OR_NULL

[PATCH] sh: clk: Extend valid clk ptr checks using IS_ERR_OR_NULL

[Phil Edworthy]

In order to allow all drivers to call clk functions with an invalid clk
ptr, ensure we check not only for a NULL clk ptr, but also for errors
before using it.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
Note this has not been tested at all, I don't have any SH boards.
---
 drivers/sh/clk/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
index d996782a7106..b843c99b3604 100644
--- a/drivers/sh/clk/core.c
+++ b/drivers/sh/clk/core.c
@@ -253,7 +253,7 @@ void clk_disable(struct clk *clk)
 {
 	unsigned long flags;
 
-	if (!clk)
+	if (IS_ERR_OR_NULL(clk))
 		return;
 
 	spin_lock_irqsave(&clock_lock, flags);
@@ -294,7 +294,7 @@ int clk_enable(struct clk *clk)
 	unsigned long flags;
 	int ret;
 
-	if (!clk)
+	if (IS_ERR_OR_NULL(clk))
 		return -EINVAL;
 
 	spin_lock_irqsave(&clock_lock, flags);
@@ -470,7 +470,7 @@ void clk_enable_init_clocks(void)
 
 unsigned long clk_get_rate(struct clk *clk)
 {
-	if (!clk)
+	if (IS_ERR_OR_NULL(clk))
 		return 0;
 
 	return clk->rate;
@@ -482,7 +482,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	int ret = -EOPNOTSUPP;
 	unsigned long flags;
 
-	if (!clk)
+	if (IS_ERR_OR_NULL(clk))
 		return 0;
 
 	spin_lock_irqsave(&clock_lock, flags);
@@ -513,7 +513,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	unsigned long flags;
 	int ret = -EINVAL;
 
-	if (!parent || !clk)
+	if (!parent || IS_ERR_OR_NULL(clk))
 		return ret;
 	if (clk->parent == parent)
 		return 0;
@@ -542,7 +542,7 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
 
 struct clk *clk_get_parent(struct clk *clk)
 {
-	if (!clk)
+	if (IS_ERR_OR_NULL(clk))
 		return NULL;
 
 	return clk->parent;
@@ -551,7 +551,7 @@ EXPORT_SYMBOL_GPL(clk_get_parent);
 
 long clk_round_rate(struct clk *clk, unsigned long rate)
 {
-	if (!clk)
+	if (IS_ERR_OR_NULL(clk))
 		return 0;
 
 	if (likely(clk->ops && clk->ops->round_rate)) {
-- 
2.34.1

Re: [PATCH] sh: clk: Extend valid clk ptr checks using IS_ERR_OR_NULL

[Rob Landley]

On 5/23/22 04:16, Phil Edworthy wrote:
> In order to allow all drivers to call clk functions with an invalid clk
> ptr, ensure we check not only for a NULL clk ptr, but also for errors
> before using it.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> Note this has not been tested at all, I don't have any SH boards.

Tested-by: Rob Landley <rob@landley.net>

Rob

(Not _extensively_ tested because I dunno what I'm looking for, but it compiled
and booted to a shell prompt and I could wget a file.)

Re: [PATCH] sh: clk: Extend valid clk ptr checks using IS_ERR_OR_NULL

[Geert Uytterhoeven]

Hi Phil,

On Mon, May 23, 2022 at 11:16 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> In order to allow all drivers to call clk functions with an invalid clk
> ptr, ensure we check not only for a NULL clk ptr, but also for errors
> before using it.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Thanks for your patch!

> --- a/drivers/sh/clk/core.c
> +++ b/drivers/sh/clk/core.c
> @@ -294,7 +294,7 @@ int clk_enable(struct clk *clk)
>         unsigned long flags;
>         int ret;
>
> -       if (!clk)
> +       if (IS_ERR_OR_NULL(clk))
>                 return -EINVAL;

drivers/clk/clk.c:clk_enable() only checks for NULL, so I think this
part should be dropped.

>
>         spin_lock_irqsave(&clock_lock, flags);
> @@ -470,7 +470,7 @@ void clk_enable_init_clocks(void)
>
>  unsigned long clk_get_rate(struct clk *clk)
>  {
> -       if (!clk)
> +       if (IS_ERR_OR_NULL(clk))
>                 return 0;

Same here.

>
>         return clk->rate;
> @@ -482,7 +482,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>         int ret = -EOPNOTSUPP;
>         unsigned long flags;
>
> -       if (!clk)
> +       if (IS_ERR_OR_NULL(clk))
>                 return 0;

Same here.

>
>         spin_lock_irqsave(&clock_lock, flags);
> @@ -513,7 +513,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>         unsigned long flags;
>         int ret = -EINVAL;
>
> -       if (!parent || !clk)
> +       if (!parent || IS_ERR_OR_NULL(clk))
>                 return ret;

Same here.

>         if (clk->parent == parent)
>                 return 0;
> @@ -542,7 +542,7 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
>
>  struct clk *clk_get_parent(struct clk *clk)
>  {
> -       if (!clk)
> +       if (IS_ERR_OR_NULL(clk))
>                 return NULL;

Same here.

>
>         return clk->parent;
> @@ -551,7 +551,7 @@ EXPORT_SYMBOL_GPL(clk_get_parent);
>
>  long clk_round_rate(struct clk *clk, unsigned long rate)
>  {
> -       if (!clk)
> +       if (IS_ERR_OR_NULL(clk))
>                 return 0;

Same here.

>
>         if (likely(clk->ops && clk->ops->round_rate)) {

So it's just clk_disable() that needs the improved checking, so you can
always call it in cleanup code, regardless of failing to get the clock.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] sh: clk: Extend valid clk ptr checks using IS_ERR_OR_NULL

[Phil Edworthy]

Hi Geert,

On 08 July 2022 08:54 Geert Uytterhoeven wrote:
> On Mon, May 23, 2022 at 11:16 AM Phil Edworthy wrote:
> > In order to allow all drivers to call clk functions with an invalid clk
> > ptr, ensure we check not only for a NULL clk ptr, but also for errors
> > before using it.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/sh/clk/core.c
> > +++ b/drivers/sh/clk/core.c
> > @@ -294,7 +294,7 @@ int clk_enable(struct clk *clk)
> >         unsigned long flags;
> >         int ret;
> >
> > -       if (!clk)
> > +       if (IS_ERR_OR_NULL(clk))
> >                 return -EINVAL;
> 
> drivers/clk/clk.c:clk_enable() only checks for NULL, so I think this
> part should be dropped.
> 
...
> >
> >         if (likely(clk->ops && clk->ops->round_rate)) {
> 
> So it's just clk_disable() that needs the improved checking, so you can
> always call it in cleanup code, regardless of failing to get the clock.

Ok, I see now. NULL is a valid clk ptr, hence the SH driver needs to
check for it, whereas errors need to be caught before trying to actually
use the other clock functions.

Thanks
Phil