[2/5] clk: prevent spurious parent rate propagation

Message ID 1336367310-5140-3-git-send-email-mturquette@linaro.org
State Accepted
Commit 63f5c3b2b18dcaca0fc8983b52a3f5d4d70a0590
Headers show

Commit Message

Mike Turquette May 7, 2012, 5:08 a.m.
Patch 'clk: always pass parent_rate into .round_rate' made a subtle
change to the semantics of .round_rate.  It is now expected for the
parent's rate to always be passed in, simplifying the implemenation of
various .round_rate callback definitions.

However the patch also introduced a bug in clk_calc_new_rates whereby a
clock without the CLK_SET_RATE_PARENT flag set could still propagate a
rate change up to a parent clock if the the .round_rate callback
modified the &best_parent_rate value in any way.

This patch fixes the issue at the framework level (in
clk_calc_new_rates) by specifically handling the case where the
CLK_SET_RATE_PARENT flag is not set.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Reported-by: Sascha Hauer<s.hauer@pengutronix.de>
---
 drivers/clk/clk.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

Comments

Sascha Hauer May 7, 2012, 7:58 a.m. | #1
On Sun, May 06, 2012 at 10:08:27PM -0700, Mike Turquette wrote:
> Patch 'clk: always pass parent_rate into .round_rate' made a subtle
> change to the semantics of .round_rate.  It is now expected for the
> parent's rate to always be passed in, simplifying the implemenation of
> various .round_rate callback definitions.
> 
> However the patch also introduced a bug in clk_calc_new_rates whereby a
> clock without the CLK_SET_RATE_PARENT flag set could still propagate a
> rate change up to a parent clock if the the .round_rate callback
> modified the &best_parent_rate value in any way.
> 
> This patch fixes the issue at the framework level (in
> clk_calc_new_rates) by specifically handling the case where the
> CLK_SET_RATE_PARENT flag is not set.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Reported-by: Sascha Hauer<s.hauer@pengutronix.de>

I think a warning might be useful when a clk driver tries to change the
parent rate even if it is not allowed to. This can be added in a later
patch, so

Acked-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8149764..7ceca0e 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -774,12 +774,18 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
>  	if (IS_ERR_OR_NULL(clk))
>  		return NULL;
>  
> +	/* save parent rate, if it exists */
> +	if (clk->parent)
> +		best_parent_rate = clk->parent->rate;
> +
>  	/* never propagate up to the parent */
>  	if (!(clk->flags & CLK_SET_RATE_PARENT)) {
>  		if (!clk->ops->round_rate) {
>  			clk->new_rate = clk->rate;
>  			return NULL;
>  		}
> +		new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
> +		goto out;
>  	}
>  
>  	/* need clk->parent from here on out */
> @@ -795,7 +801,6 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
>  		goto out;
>  	}
>  
> -	best_parent_rate = clk->parent->rate;
>  	new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
>  
>  	if (best_parent_rate != clk->parent->rate) {
> -- 
> 1.7.5.4
> 
>
Mike Turquette May 7, 2012, 5:34 p.m. | #2
On Mon, May 7, 2012 at 12:58 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Sun, May 06, 2012 at 10:08:27PM -0700, Mike Turquette wrote:
>> Patch 'clk: always pass parent_rate into .round_rate' made a subtle
>> change to the semantics of .round_rate.  It is now expected for the
>> parent's rate to always be passed in, simplifying the implemenation of
>> various .round_rate callback definitions.
>>
>> However the patch also introduced a bug in clk_calc_new_rates whereby a
>> clock without the CLK_SET_RATE_PARENT flag set could still propagate a
>> rate change up to a parent clock if the the .round_rate callback
>> modified the &best_parent_rate value in any way.
>>
>> This patch fixes the issue at the framework level (in
>> clk_calc_new_rates) by specifically handling the case where the
>> CLK_SET_RATE_PARENT flag is not set.
>>
>> Signed-off-by: Mike Turquette <mturquette@linaro.org>
>> Reported-by: Sascha Hauer<s.hauer@pengutronix.de>
>
> I think a warning might be useful when a clk driver tries to change the
> parent rate even if it is not allowed to. This can be added in a later
> patch, so
>

A fine idea.

> Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
>

Thanks,
Mike

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8149764..7ceca0e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -774,12 +774,18 @@  static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
 	if (IS_ERR_OR_NULL(clk))
 		return NULL;
 
+	/* save parent rate, if it exists */
+	if (clk->parent)
+		best_parent_rate = clk->parent->rate;
+
 	/* never propagate up to the parent */
 	if (!(clk->flags & CLK_SET_RATE_PARENT)) {
 		if (!clk->ops->round_rate) {
 			clk->new_rate = clk->rate;
 			return NULL;
 		}
+		new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
+		goto out;
 	}
 
 	/* need clk->parent from here on out */
@@ -795,7 +801,6 @@  static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
 		goto out;
 	}
 
-	best_parent_rate = clk->parent->rate;
 	new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
 
 	if (best_parent_rate != clk->parent->rate) {