| Message ID | 1377020063-30213-2-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 20, 2013 at 07:34:20PM +0200, Sylwester Nawrocki wrote: > +int __clk_get(struct clk *clk) > +{ > + if (WARN_ON((!clk))) > + return 0; This changes the behaviour of clk_get() > + > + if (!try_module_get(clk->owner)) > + return 0; If you want this to be safe against NULL pointers, just do this: if (clk && !try_module_get(clk->owner)) return 0; > + > + return 1; > +} > +EXPORT_SYMBOL(__clk_get); > + > +void __clk_put(struct clk *clk) > +{ > + if (!clk || IS_ERR(clk)) > + return; > + > + module_put(clk->owner); Calling clk_put() with an error-pointer should be a Bad Thing and something that shouldn't be encouraged, so trapping it is probably unwise. So, just do here: if (clk) module_put(clk->owner); If we do have some callers of this with ERR pointers, then we could add: if (WARN_ON_ONCE(IS_ERR(clk))) return; and remove it after a full kernel cycle or so.
On 08/20/2013 10:30 PM, Russell King - ARM Linux wrote: > On Tue, Aug 20, 2013 at 07:34:20PM +0200, Sylwester Nawrocki wrote: >> +int __clk_get(struct clk *clk) >> +{ >> + if (WARN_ON((!clk))) >> + return 0; > > This changes the behaviour of clk_get() My bad, will remove that. >> + >> + if (!try_module_get(clk->owner)) >> + return 0; > > If you want this to be safe against NULL pointers, just do this: > > if (clk && !try_module_get(clk->owner)) > return 0; Ok, that should work too. >> + >> + return 1; >> +} >> +EXPORT_SYMBOL(__clk_get); >> + >> +void __clk_put(struct clk *clk) >> +{ >> + if (!clk || IS_ERR(clk)) >> + return; >> + >> + module_put(clk->owner); > > Calling clk_put() with an error-pointer should be a Bad Thing and something > that shouldn't be encouraged, so trapping it is probably unwise. So, just > do here: > > if (clk) > module_put(clk->owner); > > If we do have some callers of this with ERR pointers, then we could add: > > if (WARN_ON_ONCE(IS_ERR(clk))) > return; > > and remove it after a full kernel cycle or so. I wouldn't be surprised to see some callers with ERR pointers, since clk_put() has been mostly a no op. I'm inclined to leave such a check temporarily, let's see if it catches any issues. Thanks for review of the other patches.
diff --git a/arch/arm/include/asm/clkdev.h b/arch/arm/include/asm/clkdev.h index 80751c1..4e8a4b2 100644 --- a/arch/arm/include/asm/clkdev.h +++ b/arch/arm/include/asm/clkdev.h @@ -14,12 +14,14 @@ #include <linux/slab.h> +#ifndef CONFIG_COMMON_CLK #ifdef CONFIG_HAVE_MACH_CLKDEV #include <mach/clkdev.h> #else #define __clk_get(clk) ({ 1; }) #define __clk_put(clk) do { } while (0) #endif +#endif static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size) { diff --git a/arch/blackfin/include/asm/clkdev.h b/arch/blackfin/include/asm/clkdev.h index 9053bed..7ac2436 100644 --- a/arch/blackfin/include/asm/clkdev.h +++ b/arch/blackfin/include/asm/clkdev.h @@ -8,7 +8,9 @@ static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size) return kzalloc(size, GFP_KERNEL); } +#ifndef CONFIG_COMMON_CLK #define __clk_put(clk) #define __clk_get(clk) ({ 1; }) +#endif #endif diff --git a/arch/mips/include/asm/clkdev.h b/arch/mips/include/asm/clkdev.h index 2624754..1b3ad7b 100644 --- a/arch/mips/include/asm/clkdev.h +++ b/arch/mips/include/asm/clkdev.h @@ -14,8 +14,10 @@ #include <linux/slab.h> +#ifndef CONFIG_COMMON_CLK #define __clk_get(clk) ({ 1; }) #define __clk_put(clk) do { } while (0) +#endif static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size) { diff --git a/arch/sh/include/asm/clkdev.h b/arch/sh/include/asm/clkdev.h index 6ba9186..c419014 100644 --- a/arch/sh/include/asm/clkdev.h +++ b/arch/sh/include/asm/clkdev.h @@ -25,7 +25,9 @@ static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size) return kzalloc(size, GFP_KERNEL); } +#ifndef CONFIG_COMMON_CLK #define __clk_put(clk) #define __clk_get(clk) ({ 1; }) +#endif #endif /* __CLKDEV_H__ */ diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 56a00db..0e0eb31 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1851,6 +1851,30 @@ void devm_clk_unregister(struct device *dev, struct clk *clk) } EXPORT_SYMBOL_GPL(devm_clk_unregister); +/* + * clkdev helpers + */ +int __clk_get(struct clk *clk) +{ + if (WARN_ON((!clk))) + return 0; + + if (!try_module_get(clk->owner)) + return 0; + + return 1; +} +EXPORT_SYMBOL(__clk_get); + +void __clk_put(struct clk *clk) +{ + if (!clk || IS_ERR(clk)) + return; + + module_put(clk->owner); +} +EXPORT_SYMBOL(__clk_put); + /*** clk rate change notifiers ***/ /** diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h index dd7adff..b7c0b58 100644 --- a/include/linux/clk-private.h +++ b/include/linux/clk-private.h @@ -25,10 +25,13 @@ #ifdef CONFIG_COMMON_CLK +struct module; + struct clk { const char *name; const struct clk_ops *ops; struct clk_hw *hw; + struct module *owner; struct clk *parent; const char **parent_names; struct clk **parents; diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h index a6a6f60..94bad77 100644 --- a/include/linux/clkdev.h +++ b/include/linux/clkdev.h @@ -43,4 +43,9 @@ int clk_add_alias(const char *, const char *, char *, struct device *); int clk_register_clkdev(struct clk *, const char *, const char *, ...); int clk_register_clkdevs(struct clk *, struct clk_lookup *, size_t); +#ifdef CONFIG_COMMON_CLK +int __clk_get(struct clk *clk); +void __clk_put(struct clk *clk); +#endif + #endif