| Message ID | 1459821083-28116-1-git-send-email-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 04/05, Masahiro Yamada wrote: > The clk_disable() in the common clock framework (drivers/clk/clk.c) > returns immediately if a given clk is NULL or an error pointer. It > allows clock consumers to call clk_disable() without IS_ERR_OR_NULL > checking if drivers are only used with the common clock framework. > > Unfortunately, NULL/error checking is missing from some of non-common > clk_disable() implementations. This prevents us from completely > dropping NULL/error checking from callers. Let's make it tree-wide > consistent by adding IS_ERR_OR_NULL(clk) to all callees. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Acked-by: Greg Ungerer <gerg@uclinux.org> > Acked-by: Wan Zongshun <mcuos.com@gmail.com> > --- > > Stephen, > > This patch has been unapplied for a long time. > > Please let me know if there is something wrong with this patch. > I'm mostly confused why we wouldn't want to encourage people to call clk_disable or unprepare on a clk that's an error pointer. Typically an error pointer should be dealt with, instead of silently ignored, so why wasn't it dealt with by passing it up the probe() path?
Hi Stephen, 2016-04-08 9:33 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: > On 04/05, Masahiro Yamada wrote: >> The clk_disable() in the common clock framework (drivers/clk/clk.c) >> returns immediately if a given clk is NULL or an error pointer. It >> allows clock consumers to call clk_disable() without IS_ERR_OR_NULL >> checking if drivers are only used with the common clock framework. >> >> Unfortunately, NULL/error checking is missing from some of non-common >> clk_disable() implementations. This prevents us from completely >> dropping NULL/error checking from callers. Let's make it tree-wide >> consistent by adding IS_ERR_OR_NULL(clk) to all callees. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> Acked-by: Greg Ungerer <gerg@uclinux.org> >> Acked-by: Wan Zongshun <mcuos.com@gmail.com> >> --- >> >> Stephen, >> >> This patch has been unapplied for a long time. >> >> Please let me know if there is something wrong with this patch. >> > > I'm mostly confused why we wouldn't want to encourage people to > call clk_disable or unprepare on a clk that's an error pointer. > Typically an error pointer should be dealt with, instead of > silently ignored, so why wasn't it dealt with by passing it up > the probe() path? > This makes our driver programming life easier. For example, let's see drivers/tty/serial/8250/8250_of.c The "clock-frequency" DT property takes precedence over "clocks" property. So, it is valid to probe the driver with a NULL pointer for info->clk. if (of_property_read_u32(np, "clock-frequency", &clk)) { /* Get clk rate through clk driver if present */ info->clk = devm_clk_get(&ofdev->dev, NULL); if (IS_ERR(info->clk)) { dev_warn(&ofdev->dev, "clk or clock-frequency not defined\n"); return PTR_ERR(info->clk); } ret = clk_prepare_enable(info->clk); if (ret < 0) return ret; clk = clk_get_rate(info->clk); } As a result, we need to make sure the clk pointer is valid before calling clk_disable_unprepare(). If we could support pointer checking in callees, we would be able to clean-up lots of clock consumers.
On Thu, Apr 07, 2016 at 05:33:28PM -0700, Stephen Boyd wrote: > On 04/05, Masahiro Yamada wrote: > > The clk_disable() in the common clock framework (drivers/clk/clk.c) > > returns immediately if a given clk is NULL or an error pointer. It > > allows clock consumers to call clk_disable() without IS_ERR_OR_NULL > > checking if drivers are only used with the common clock framework. > > > > Unfortunately, NULL/error checking is missing from some of non-common > > clk_disable() implementations. This prevents us from completely > > dropping NULL/error checking from callers. Let's make it tree-wide > > consistent by adding IS_ERR_OR_NULL(clk) to all callees. > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > Acked-by: Greg Ungerer <gerg@uclinux.org> > > Acked-by: Wan Zongshun <mcuos.com@gmail.com> > > --- > > > > Stephen, > > > > This patch has been unapplied for a long time. > > > > Please let me know if there is something wrong with this patch. > > > > I'm mostly confused why we wouldn't want to encourage people to > call clk_disable or unprepare on a clk that's an error pointer. > Typically an error pointer should be dealt with, instead of > silently ignored, so why wasn't it dealt with by passing it up > the probe() path? While your argument makes perfect sense, Many clk_disable implementations are already doing similar checks, for example: arch/arm/mach-davinci/clock.c: void clk_disable(struct clk *clk) { unsigned long flags; if (clk == NULL || IS_ERR(clk)) return; [...] arch/arm/mach-omap1/clock.c void clk_disable(struct clk *clk) { unsigned long flags; if (clk == NULL || IS_ERR(clk)) return; [...] arch/avr32/mach-at32ap/clock.c void clk_disable(struct clk *clk) { unsigned long flags; if (IS_ERR_OR_NULL(clk)) return; [...] arch/mips/lantiq/clk.c: static inline int clk_good(struct clk *clk) { return clk && !IS_ERR(clk); } [...] void clk_disable(struct clk *clk) { if (unlikely(!clk_good(clk))) return; if (clk->disable) [...] So should we go and weed out these checks? Ralf
2016-04-08 19:06 GMT+09:00 Ralf Baechle <ralf@linux-mips.org>: > On Thu, Apr 07, 2016 at 05:33:28PM -0700, Stephen Boyd wrote: > >> On 04/05, Masahiro Yamada wrote: >> > The clk_disable() in the common clock framework (drivers/clk/clk.c) >> > returns immediately if a given clk is NULL or an error pointer. It >> > allows clock consumers to call clk_disable() without IS_ERR_OR_NULL >> > checking if drivers are only used with the common clock framework. >> > >> > Unfortunately, NULL/error checking is missing from some of non-common >> > clk_disable() implementations. This prevents us from completely >> > dropping NULL/error checking from callers. Let's make it tree-wide >> > consistent by adding IS_ERR_OR_NULL(clk) to all callees. >> > >> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> > Acked-by: Greg Ungerer <gerg@uclinux.org> >> > Acked-by: Wan Zongshun <mcuos.com@gmail.com> >> > --- >> > >> > Stephen, >> > >> > This patch has been unapplied for a long time. >> > >> > Please let me know if there is something wrong with this patch. >> > >> >> I'm mostly confused why we wouldn't want to encourage people to >> call clk_disable or unprepare on a clk that's an error pointer. >> Typically an error pointer should be dealt with, instead of >> silently ignored, so why wasn't it dealt with by passing it up >> the probe() path? > > While your argument makes perfect sense, Many clk_disable implementations > are already doing similar checks, for example: > > arch/arm/mach-davinci/clock.c: > > void clk_disable(struct clk *clk) > { > unsigned long flags; > > if (clk == NULL || IS_ERR(clk)) > return; > [...] > > arch/arm/mach-omap1/clock.c > > void clk_disable(struct clk *clk) > { > unsigned long flags; > > if (clk == NULL || IS_ERR(clk)) > return; > [...] > > arch/avr32/mach-at32ap/clock.c > > void clk_disable(struct clk *clk) > { > unsigned long flags; > > if (IS_ERR_OR_NULL(clk)) > return; > [...] > > arch/mips/lantiq/clk.c: > > static inline int clk_good(struct clk *clk) > { > return clk && !IS_ERR(clk); > } > > [...] > > void clk_disable(struct clk *clk) > { > if (unlikely(!clk_good(clk))) > return; > > if (clk->disable) > [...] > > So should we go and weed out these checks? > > Ralf Please help me understand your thought clearly. [1] Should calling clk_unprepare/disable() with a NULL pointer be allowed? [2] Should calling clk_unprepare/disable() with an error pointer be allowed?
On 04/08, Masahiro Yamada wrote: > > > This makes our driver programming life easier. > > > For example, let's see drivers/tty/serial/8250/8250_of.c > > > The "clock-frequency" DT property takes precedence over "clocks" property. > So, it is valid to probe the driver with a NULL pointer for info->clk. > > > if (of_property_read_u32(np, "clock-frequency", &clk)) { > > /* Get clk rate through clk driver if present */ > info->clk = devm_clk_get(&ofdev->dev, NULL); > if (IS_ERR(info->clk)) { > dev_warn(&ofdev->dev, > "clk or clock-frequency not defined\n"); > return PTR_ERR(info->clk); > } > > ret = clk_prepare_enable(info->clk); > if (ret < 0) > return ret; > > clk = clk_get_rate(info->clk); > } > > > As a result, we need to make sure the clk pointer is valid > before calling clk_disable_unprepare(). > > > If we could support pointer checking in callees, we would be able to > clean-up lots of clock consumers. > > I'm not sure if you meant to use that example for the error pointer case? It bails out if clk_get() returns an error pointer. I'm all for a no-op in clk_disable()/unprepare() when the pointer is NULL. But when it's an error pointer the driver should be handling it and bail out before it would ever call enable/prepare on it or disable/unprepare.
On 04/08, Ralf Baechle wrote: > > While your argument makes perfect sense, Many clk_disable implementations > are already doing similar checks, for example: > > arch/arm/mach-davinci/clock.c: > [...] > > So should we go and weed out these checks? Yes, it would be nice to at least make the differing implementations of the clk API consistent. Of course, we should really put our efforts towards getting rid of the non-CCF implementations instead so that there's less confusion overall.
Hi Stephen, 2016-04-14 9:33 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: > On 04/08, Masahiro Yamada wrote: >> >> >> This makes our driver programming life easier. >> >> >> For example, let's see drivers/tty/serial/8250/8250_of.c >> >> >> The "clock-frequency" DT property takes precedence over "clocks" property. >> So, it is valid to probe the driver with a NULL pointer for info->clk. >> >> >> if (of_property_read_u32(np, "clock-frequency", &clk)) { >> >> /* Get clk rate through clk driver if present */ >> info->clk = devm_clk_get(&ofdev->dev, NULL); >> if (IS_ERR(info->clk)) { >> dev_warn(&ofdev->dev, >> "clk or clock-frequency not defined\n"); >> return PTR_ERR(info->clk); >> } >> >> ret = clk_prepare_enable(info->clk); >> if (ret < 0) >> return ret; >> >> clk = clk_get_rate(info->clk); >> } >> >> >> As a result, we need to make sure the clk pointer is valid >> before calling clk_disable_unprepare(). >> >> >> If we could support pointer checking in callees, we would be able to >> clean-up lots of clock consumers. >> >> > > I'm not sure if you meant to use that example for the error > pointer case? It bails out if clk_get() returns an error pointer. > > I'm all for a no-op in clk_disable()/unprepare() when the pointer > is NULL. But when it's an error pointer the driver should be > handling it and bail out before it would ever call enable/prepare > on it or disable/unprepare. Let me explain my original idea. We do various initialization in a probe method, so we (OK, I) sometimes want to split init code into some helper function(s) like this: static int foo_clk_init(struct platform_device *pdev, struct foo_priv *priv) { int ret; priv->clk = devm_clk_get(&pdev->dev, NULL); /* case 1 */ if (IS_ERR(priv->clk)) { dev_err(&pdev->dev, "falied to get clk\n"); return PTR_ERR(priv->clk); } ret = clk_prepare_enable(priv->clk); /* case 2 */ if (ret < 0) { dev_err(&pdev->dev, "falied to enable clk\n"); return ret; } priv->clk_rate = clk_get_rate(priv->clk); /* case 3 */ if (!priv->clk_rate) { dev_err(&pdev->dev, "clk rate should not be zero\n"); return -EINVAL; } [ do something ] return 0; } static int foo_probe(struct platform_device *pdev) { [memory allocation, OF parse, various init.... ] ret = foo_clk_init(pdev, priv); if (ret < 0) goto err; ret = foo_blahblah_init(pdev, priv) /* case 4 */ ir (ret < 0) goto err; [ more initialization ... ] return 0; err: clk_disable_unprepare(priv->clk); return ret; } There are some failure paths in this example. [1] If case 1 fails, priv->clk contains an error pointer. We should not do clk_disable_unprepare(). [2] If case 2 fails, priv->clk contains a valid pointer, but we should not do clk_disable_unprepare(). [3] If case 3 fails, priv->clk contains a valid pointer, and we should do clk_disable_unprepare(). [4] If case 4 fails, priv->clk contains a valid pointer, and we should do clk_disable_unprepare(). My difficulty is that [1]-[3] are contained in one helper function. (A real example is drivers/i2c/busses/i2c-uniphier.c) If foo_clk_init() fails for reason [1], I want clk_disable_unprepare() to just return. (This is my original intention of this patch.) If foo_clk_init() fails for reason [3], I want clk_disable_unprepare() to do its job. OK, now I notice another problem in my code; if foo_clk_init() fails for reason [2], clk_disable() WARN's due to zero enable_count. if (WARN_ON(core->enable_count == 0)) return; Perhaps, I got screwed up by splitting clock init stuff into a helper function.
On 04/14, Masahiro Yamada wrote: > > OK, now I notice another problem in my code; > if foo_clk_init() fails for reason [2], > clk_disable() WARN's due to zero enable_count. > > if (WARN_ON(core->enable_count == 0)) > return; > > > > Perhaps, I got screwed up by splitting clock init stuff > into a helper function. Yep! Can't we just split the enable/disable out into another function separate from the clk_get/put part? That would make things more symmetric and avoid this problem.
diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c index 39ef3b6..4e11f7d 100644 --- a/arch/arm/mach-ep93xx/clock.c +++ b/arch/arm/mach-ep93xx/clock.c @@ -293,7 +293,7 @@ void clk_disable(struct clk *clk) { unsigned long flags; - if (!clk) + if (IS_ERR_OR_NULL(clk)) return; spin_lock_irqsave(&clk_lock, flags); diff --git a/arch/arm/mach-mmp/clock.c b/arch/arm/mach-mmp/clock.c index ac6633d..39ad3896 100644 --- a/arch/arm/mach-mmp/clock.c +++ b/arch/arm/mach-mmp/clock.c @@ -67,6 +67,9 @@ void clk_disable(struct clk *clk) { unsigned long flags; + if (IS_ERR_OR_NULL(clk)) + return; + WARN_ON(clk->enabled == 0); spin_lock_irqsave(&clocks_lock, flags); diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c index cbf53bb..ea103fd 100644 --- a/arch/arm/mach-sa1100/clock.c +++ b/arch/arm/mach-sa1100/clock.c @@ -85,13 +85,14 @@ void clk_disable(struct clk *clk) { unsigned long flags; - if (clk) { - WARN_ON(clk->enabled == 0); - spin_lock_irqsave(&clocks_lock, flags); - if (--clk->enabled == 0) - clk->ops->disable(clk); - spin_unlock_irqrestore(&clocks_lock, flags); - } + if (IS_ERR_OR_NULL(clk)) + return; + + WARN_ON(clk->enabled == 0); + spin_lock_irqsave(&clocks_lock, flags); + if (--clk->enabled == 0) + clk->ops->disable(clk); + spin_unlock_irqrestore(&clocks_lock, flags); } EXPORT_SYMBOL(clk_disable); diff --git a/arch/arm/mach-w90x900/clock.c b/arch/arm/mach-w90x900/clock.c index 2c371ff..90ec250 100644 --- a/arch/arm/mach-w90x900/clock.c +++ b/arch/arm/mach-w90x900/clock.c @@ -46,6 +46,9 @@ void clk_disable(struct clk *clk) { unsigned long flags; + if (IS_ERR_OR_NULL(clk)) + return; + WARN_ON(clk->enabled == 0); spin_lock_irqsave(&clocks_lock, flags); diff --git a/arch/blackfin/mach-bf609/clock.c b/arch/blackfin/mach-bf609/clock.c index 3783058..fed8015 100644 --- a/arch/blackfin/mach-bf609/clock.c +++ b/arch/blackfin/mach-bf609/clock.c @@ -97,6 +97,9 @@ EXPORT_SYMBOL(clk_enable); void clk_disable(struct clk *clk) { + if (IS_ERR_OR_NULL(clk)) + return; + if (clk->ops && clk->ops->disable) clk->ops->disable(clk); } diff --git a/arch/m68k/coldfire/clk.c b/arch/m68k/coldfire/clk.c index fddfdcc..eb0e8c1 100644 --- a/arch/m68k/coldfire/clk.c +++ b/arch/m68k/coldfire/clk.c @@ -101,6 +101,10 @@ EXPORT_SYMBOL(clk_enable); void clk_disable(struct clk *clk) { unsigned long flags; + + if (IS_ERR_OR_NULL(clk)) + return; + spin_lock_irqsave(&clk_lock, flags); if ((--clk->enabled == 0) && clk->clk_ops) clk->clk_ops->disable(clk); diff --git a/arch/mips/bcm63xx/clk.c b/arch/mips/bcm63xx/clk.c index 6375652..d6a39bf 100644 --- a/arch/mips/bcm63xx/clk.c +++ b/arch/mips/bcm63xx/clk.c @@ -326,6 +326,9 @@ EXPORT_SYMBOL(clk_enable); void clk_disable(struct clk *clk) { + if (IS_ERR_OR_NULL(clk)) + return; + mutex_lock(&clocks_mutex); clk_disable_unlocked(clk); mutex_unlock(&clocks_mutex); diff --git a/arch/mips/lantiq/clk.c b/arch/mips/lantiq/clk.c index a0706fd..c8d87b1 100644 --- a/arch/mips/lantiq/clk.c +++ b/arch/mips/lantiq/clk.c @@ -130,6 +130,9 @@ EXPORT_SYMBOL(clk_enable); void clk_disable(struct clk *clk) { + if (IS_ERR_OR_NULL(clk)) + return; + if (unlikely(!clk_good(clk))) return; diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c index 92863e3..9978fa4 100644 --- a/drivers/sh/clk/core.c +++ b/drivers/sh/clk/core.c @@ -252,7 +252,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);