[PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Masahiro Yamada]

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.

Thanks,


Changes in v2:
  - Rebase on Linux 4.6-rc1

 arch/arm/mach-ep93xx/clock.c     |  2 +-
 arch/arm/mach-mmp/clock.c        |  3 +++
 arch/arm/mach-sa1100/clock.c     | 15 ++++++++-------
 arch/arm/mach-w90x900/clock.c    |  3 +++
 arch/blackfin/mach-bf609/clock.c |  3 +++
 arch/m68k/coldfire/clk.c         |  4 ++++
 arch/mips/bcm63xx/clk.c          |  3 +++
 arch/mips/lantiq/clk.c           |  3 +++
 drivers/sh/clk/core.c            |  2 +-
 9 files changed, 29 insertions(+), 9 deletions(-)

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);
-- 
1.9.1

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Stephen Boyd]

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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Masahiro Yamada]

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.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Ralf Baechle]

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

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Masahiro Yamada]

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?


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Masahiro Yamada]

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.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project