[PATCH] ARM: mach-shmobile: fix Oops during system wake up

[PATCH] ARM: mach-shmobile: fix Oops during system wake up

[Guennadi Liakhovetski]

During system resume the clock subsystem is restoring all clock rates,
if sound has not been used before suspend, the rate remains 0 and
the wake up Oopses with division by zero.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Simon Horman <horms@verge.net.au>
Cc: Magnus Damm <damm@opensource.se>
---
 arch/arm/mach-shmobile/clock-sh7372.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
index e9731b5..fb29d89 100644
--- a/arch/arm/mach-shmobile/clock-sh7372.c
+++ b/arch/arm/mach-shmobile/clock-sh7372.c
@@ -458,6 +458,9 @@ static int fsidiv_set_rate(struct clk *clk, unsigned long rate)
 {
 	int idx;
 
+	if (!rate)
+		return -EDOM;
+
 	idx = (clk->parent->rate / rate) & 0xffff;
 	if (idx < 2)
 		return -EINVAL;
-- 
1.7.2.5

Re: [PATCH] ARM: mach-shmobile: fix Oops during system wake up

[Guennadi Liakhovetski]

On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:

> During system resume the clock subsystem is restoring all clock rates,
> if sound has not been used before suspend, the rate remains 0 and
> the wake up Oopses with division by zero.

Right, I see now. Magnus had a more general solution for this problem, 
which I accidentally dropped in my tests, which caused this Oops. So, 
maybe his patch to check rate in clks_core_resume() is a better fix for 
this problem. Feel free to ignore this then.

Thanks
Guennadi

> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Magnus Damm <damm@opensource.se>
> ---
>  arch/arm/mach-shmobile/clock-sh7372.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
> index e9731b5..fb29d89 100644
> --- a/arch/arm/mach-shmobile/clock-sh7372.c
> +++ b/arch/arm/mach-shmobile/clock-sh7372.c
> @@ -458,6 +458,9 @@ static int fsidiv_set_rate(struct clk *clk, unsigned long rate)
>  {
>  	int idx;
>  
> +	if (!rate)
> +		return -EDOM;
> +
>  	idx = (clk->parent->rate / rate) & 0xffff;
>  	if (idx < 2)
>  		return -EINVAL;
> -- 
> 1.7.2.5
> 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] ARM: mach-shmobile: fix Oops during system wake up

[Simon Horman]

On Fri, Apr 15, 2011 at 08:37:55PM +0200, Guennadi Liakhovetski wrote:
> On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> 
> > During system resume the clock subsystem is restoring all clock rates,
> > if sound has not been used before suspend, the rate remains 0 and
> > the wake up Oopses with division by zero.
> 
> Right, I see now. Magnus had a more general solution for this problem, 
> which I accidentally dropped in my tests, which caused this Oops. So, 
> maybe his patch to check rate in clks_core_resume() is a better fix for 
> this problem. Feel free to ignore this then.

Hi Guennadi,

do you have a link/title for Magnus's patch?

Re: [PATCH] ARM: mach-shmobile: fix Oops during system wake up

[Paul Mundt]

On Fri, Apr 15, 2011 at 08:37:55PM +0200, Guennadi Liakhovetski wrote:
> On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> 
> > During system resume the clock subsystem is restoring all clock rates,
> > if sound has not been used before suspend, the rate remains 0 and
> > the wake up Oopses with division by zero.
> 
> Right, I see now. Magnus had a more general solution for this problem, 
> which I accidentally dropped in my tests, which caused this Oops. So, 
> maybe his patch to check rate in clks_core_resume() is a better fix for 
> this problem. Feel free to ignore this then.
> 
Having a solution that doesn't exist in the list archives is effectively
the same as not having a solution at all.

From your description I imagine you mean something like checking if
clkp->rate is set prior to calling in to ->set_rate, however this
probably needs a bit more of a think. Your example stipulates that we're
resuming clocks that haven't been "used", which suggests that we're
probably better off simply not touching the clock at all in the resume
path. I expect if you were to set the rate via the recalc path instead it
also wouldn't have the desired effect for your case.

In any event, it's quite obvious that clks_core_resume() is going to need
to be reworked a bit.

Re: [PATCH] ARM: mach-shmobile: fix Oops during system wake up

[Magnus Damm]

On Mon, Apr 18, 2011 at 6:09 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Fri, Apr 15, 2011 at 08:37:55PM +0200, Guennadi Liakhovetski wrote:
>> On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
>>
>> > During system resume the clock subsystem is restoring all clock rates,
>> > if sound has not been used before suspend, the rate remains 0 and
>> > the wake up Oopses with division by zero.
>>
>> Right, I see now. Magnus had a more general solution for this problem,
>> which I accidentally dropped in my tests, which caused this Oops. So,
>> maybe his patch to check rate in clks_core_resume() is a better fix for
>> this problem. Feel free to ignore this then.
>>
> Having a solution that doesn't exist in the list archives is effectively
> the same as not having a solution at all.

Sure. I suppose Guennadi is referring to a hunk in a local patch that
doesn't exist in list archives.

> From your description I imagine you mean something like checking if
> clkp->rate is set prior to calling in to ->set_rate, however this
> probably needs a bit more of a think. Your example stipulates that we're
> resuming clocks that haven't been "used", which suggests that we're
> probably better off simply not touching the clock at all in the resume
> path. I expect if you were to set the rate via the recalc path instead it
> also wouldn't have the desired effect for your case.
>
> In any event, it's quite obvious that clks_core_resume() is going to need
> to be reworked a bit.

I ran across this, not sure if it's the right way to fix this.

--- 0001/drivers/sh/clk/core.c
+++ work/drivers/sh/clk/core.c  2011-04-18 08:46:43.000000000 +0900
@@ -641,7 +641,7 @@ static void clks_core_resume(void)
                        if (likely(clkp->ops->set_parent))
                                clkp->ops->set_parent(clkp,
                                        clkp->parent);
-                       if (likely(clkp->ops->set_rate))
+                       if (rate && likely(clkp->ops->set_rate))
                                clkp->ops->set_rate(clkp, rate);
                        else if (likely(clkp->ops->recalc))
                                clkp->rate = clkp->ops->recalc(clkp);

This does not trigger on mainline SH-Mobile ARM since we don't support
system wide suspend.

/ magnus