| Message ID | 20220523091633.5217-1-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh: clk: Extend valid clk ptr checks using IS_ERR_OR_NULL | expand |
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.)
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
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
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)) {
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(-)