[PATCH 2/3] power: clock_ops.c: check return of clk_enable() in pm_clk_resume()

[PATCH 2/3] power: clock_ops.c: check return of clk_enable() in pm_clk_resume()

[Ben Dooks]

The clk_enable() call in the pm_clk_resume() call returns an error
that is not being checked. If clk_enable() fails then we should
not set the state of the clock to PCE_STATUS_ENABLED.

Note, the issue of warning the user if this fails has not been
addressed in this patch as this is not the only place the driver
calls clk_enable().

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Reviewed-by: Ian Molton <ian.molton@codethink.co.uk>
---
 drivers/base/power/clock_ops.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index b9dd8fa..9bb95ab 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -252,6 +252,7 @@ int pm_clk_resume(struct device *dev)
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
 	unsigned long flags;
+	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -262,8 +263,9 @@ int pm_clk_resume(struct device *dev)
 
 	list_for_each_entry(ce, &psd->clock_list, node) {
 		if (ce->status < PCE_STATUS_ERROR) {
-			clk_enable(ce->clk);
-			ce->status = PCE_STATUS_ENABLED;
+			ret = clk_enable(ce->clk);
+			if (ret == 0)
+				ce->status = PCE_STATUS_ENABLED;
 		}
 	}
 
-- 
1.8.5.2

Re: [PATCH 2/3] power: clock_ops.c: check return of clk_enable() in pm_clk_resume()

[Rafael J. Wysocki]

On Saturday, January 11, 2014 01:05:39 PM Ben Dooks wrote:
> The clk_enable() call in the pm_clk_resume() call returns an error
> that is not being checked. If clk_enable() fails then we should
> not set the state of the clock to PCE_STATUS_ENABLED.
> 
> Note, the issue of warning the user if this fails has not been
> addressed in this patch as this is not the only place the driver
> calls clk_enable().
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Reviewed-by: Ian Molton <ian.molton@codethink.co.uk>
> ---
>  drivers/base/power/clock_ops.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index b9dd8fa..9bb95ab 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -252,6 +252,7 @@ int pm_clk_resume(struct device *dev)
>  	struct pm_subsys_data *psd = dev_to_psd(dev);
>  	struct pm_clock_entry *ce;
>  	unsigned long flags;
> +	int ret;
>  
>  	dev_dbg(dev, "%s()\n", __func__);
>  
> @@ -262,8 +263,9 @@ int pm_clk_resume(struct device *dev)
>  
>  	list_for_each_entry(ce, &psd->clock_list, node) {
>  		if (ce->status < PCE_STATUS_ERROR) {
> -			clk_enable(ce->clk);
> -			ce->status = PCE_STATUS_ENABLED;
> +			ret = clk_enable(ce->clk);
> +			if (ret == 0)

Can you please use if (!ret) here?

And analogously in patch [3/3]?

> +				ce->status = PCE_STATUS_ENABLED;
>  		}
>  	}
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/3] power: clock_ops.c: check return of clk_enable() in pm_clk_resume()

[Ben Dooks]

On 13/01/14 19:55, Rafael J. Wysocki wrote:
> On Saturday, January 11, 2014 01:05:39 PM Ben Dooks wrote:

>> @@ -262,8 +263,9 @@ int pm_clk_resume(struct device *dev)
>>
>>   	list_for_each_entry(ce, &psd->clock_list, node) {
>>   		if (ce->status < PCE_STATUS_ERROR) {
>> -			clk_enable(ce->clk);
>> -			ce->status = PCE_STATUS_ENABLED;
>> +			ret = clk_enable(ce->clk);
>> +			if (ret == 0)
>
> Can you please use if (!ret) here?
>
> And analogously in patch [3/3]?

I will fix and re-send today, thanks.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 1/3] power: clock_ops.c: fixup clk prepare/unprepare count

[Ben Dooks]

The drivers/base/power/clock_ops.c file is causing warnings from
the clock driver (as shown below) due to failing to do a clk_prepare()
call before enabling a clock. It also fails to check the balance of
prepare/unprepare as __pm_clk_remove() do clk_disable_unprepare() call.

This bug has probably been in since commit b2476490e ("clk: introduce
the common clock framework") as the warning was part of the original
commit. It is strange that it has not been noticed (although this has
also been coupled with a failure for certain SH builds to not build the
necessary glue to use this method of controlling the clocks).

In summary, this is probably needed in several stable branches but need
advice on which ones.

On the Renesas Lager board, this causes numerous warnings of the following
and even worse the clock system will not enable clocks, causing drivers
that are in development to fail to work:

WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883 __clk_enable+0x2c/0xa0()

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Reviewed-by: Ian Molton <ian.molton@codethink.co.uk>
---
 drivers/base/power/clock_ops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 9d8fde7..b9dd8fa 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -43,6 +43,7 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 	if (IS_ERR(ce->clk)) {
 		ce->status = PCE_STATUS_ERROR;
 	} else {
+		clk_prepare(ce->clk);
 		ce->status = PCE_STATUS_ACQUIRED;
 		dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
 	}
@@ -99,10 +100,12 @@ static void __pm_clk_remove(struct pm_clock_entry *ce)
 
 	if (ce->status < PCE_STATUS_ERROR) {
 		if (ce->status == PCE_STATUS_ENABLED)
-			clk_disable_unprepare(ce->clk);
+			clk_disable(ce->clk);
 
-		if (ce->status >= PCE_STATUS_ACQUIRED)
+		if (ce->status >= PCE_STATUS_ACQUIRED) {
+			clk_unprepare(ce->clk);
 			clk_put(ce->clk);
+		}
 	}
 
 	kfree(ce->con_id);
-- 
1.8.5.2

[PATCH 2/3] power: clock_ops.c: check return of clk_enable() in pm_clk_resume()

[Ben Dooks]

The clk_enable() call in the pm_clk_resume() call returns an error
that is not being checked. If clk_enable() fails then we should
not set the state of the clock to PCE_STATUS_ENABLED.

Note, the issue of warning the user if this fails has not been
addressed in this patch as this is not the only place the driver
calls clk_enable().

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Reviewed-by: Ian Molton <ian.molton@codethink.co.uk>
---
 drivers/base/power/clock_ops.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index b9dd8fa..cad7190 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -252,6 +252,7 @@ int pm_clk_resume(struct device *dev)
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
 	unsigned long flags;
+	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -262,8 +263,9 @@ int pm_clk_resume(struct device *dev)
 
 	list_for_each_entry(ce, &psd->clock_list, node) {
 		if (ce->status < PCE_STATUS_ERROR) {
-			clk_enable(ce->clk);
-			ce->status = PCE_STATUS_ENABLED;
+			ret = clk_enable(ce->clk);
+			if (!ret)
+				ce->status = PCE_STATUS_ENABLED;
 		}
 	}
 
-- 
1.8.5.2

[PATCH 3/3] power: clock_ops.c: report clock errors from clk_enable()

[Ben Dooks]

If clk_enable() fails, then print a message so that the user can see
what is happening instead of silently failing to enable the clock.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Reviewed-by: Ian Molton <ian.molton@codethink.co.uk>
---
 drivers/base/power/clock_ops.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index cad7190..e870bbe 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -33,6 +33,21 @@ struct pm_clock_entry {
 };
 
 /**
+ * pm_clk_enable - Enable a clock, reporting any errors
+ * @dev: The device for the given clock
+ * @clk: The clock being enabled.
+ */
+static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
+{
+	int ret = clk_enable(clk);
+	if (ret)
+		dev_err(dev, "%s: failed to enable clk %p, error %d\n",
+			__func__, clk, ret);
+
+	return ret;
+}
+
+/**
  * pm_clk_acquire - Acquire a device clock.
  * @dev: Device whose clock is to be acquired.
  * @ce: PM clock entry corresponding to the clock.
@@ -263,7 +278,7 @@ int pm_clk_resume(struct device *dev)
 
 	list_for_each_entry(ce, &psd->clock_list, node) {
 		if (ce->status < PCE_STATUS_ERROR) {
-			ret = clk_enable(ce->clk);
+			ret = __pm_clk_enable(dev, ce->clk);
 			if (!ret)
 				ce->status = PCE_STATUS_ENABLED;
 		}
@@ -381,7 +396,7 @@ int pm_clk_resume(struct device *dev)
 	spin_lock_irqsave(&psd->lock, flags);
 
 	list_for_each_entry(ce, &psd->clock_list, node)
-		clk_enable(ce->clk);
+		__pm_clk_enable(dev, ce->clk);
 
 	spin_unlock_irqrestore(&psd->lock, flags);
 
-- 
1.8.5.2

Re: [PATCH 1/3] power: clock_ops.c: fixup clk prepare/unprepare count

[Rafael J. Wysocki]

On Tuesday, January 14, 2014 12:23:40 PM Ben Dooks wrote:
> The drivers/base/power/clock_ops.c file is causing warnings from
> the clock driver (as shown below) due to failing to do a clk_prepare()
> call before enabling a clock. It also fails to check the balance of
> prepare/unprepare as __pm_clk_remove() do clk_disable_unprepare() call.
> 
> This bug has probably been in since commit b2476490e ("clk: introduce
> the common clock framework") as the warning was part of the original
> commit. It is strange that it has not been noticed (although this has
> also been coupled with a failure for certain SH builds to not build the
> necessary glue to use this method of controlling the clocks).
> 
> In summary, this is probably needed in several stable branches but need
> advice on which ones.
> 
> On the Renesas Lager board, this causes numerous warnings of the following
> and even worse the clock system will not enable clocks, causing drivers
> that are in development to fail to work:
> 
> WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883 __clk_enable+0x2c/0xa0()

All three queued up for 3.14, thanks!

> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Reviewed-by: Ian Molton <ian.molton@codethink.co.uk>
> ---
>  drivers/base/power/clock_ops.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index 9d8fde7..b9dd8fa 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -43,6 +43,7 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>  	if (IS_ERR(ce->clk)) {
>  		ce->status = PCE_STATUS_ERROR;
>  	} else {
> +		clk_prepare(ce->clk);
>  		ce->status = PCE_STATUS_ACQUIRED;
>  		dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
>  	}
> @@ -99,10 +100,12 @@ static void __pm_clk_remove(struct pm_clock_entry *ce)
>  
>  	if (ce->status < PCE_STATUS_ERROR) {
>  		if (ce->status == PCE_STATUS_ENABLED)
> -			clk_disable_unprepare(ce->clk);
> +			clk_disable(ce->clk);
>  
> -		if (ce->status >= PCE_STATUS_ACQUIRED)
> +		if (ce->status >= PCE_STATUS_ACQUIRED) {
> +			clk_unprepare(ce->clk);
>  			clk_put(ce->clk);
> +		}
>  	}
>  
>  	kfree(ce->con_id);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.