lkml.org 
[lkml]   [2015]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RESEND PATCH] clk: at91: keep slow clk enabled to prevent system hang
On Mon, 12 Jan 2015 15:44:24 -0800
Mike Turquette <mturquette@linaro.org> wrote:

> Quoting Boris Brezillon (2015-01-12 07:12:50)
> > All slow clk users are not properly requesting it (get + prepare + enable)
> > before using it.
> > If all users properly requesting this clock decide that they don't need it
> > anymore (or are removed), this lead to this clock being disabled while
> > faulty users are still requiring it, which in turn hangs the system.
>
> The correct fix is for all users to claim the clock and enable it. Is
> there a plan to implement that any time soon?

Yes, hopefully for the next release, but this requires identifying all
the offending drivers, fixing them and testing all the changes.

>
> >
> > Prevent slow oscillator clock from being disabled until all users are
> > properly requesting it.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Reported-by: Bo Shen <voice.shen@atmel.com>
> > Cc: stable@vger.kernel.org
> > ---
> > Hi Mike,
> >
> > Sorry for the noise, but I forgot to add the LKML and LAKML in Cc.
> >
> > Can you have a look at this fix and let me know if this is how you want this
> > problem addressed ?
> > I can also request (get + prepare + enable) the clk in the pmc probe function,
> > so that it can never be disabled.
> >
> > If you're fine with the approach, can you queue it for the next -rc ?
>
> I can queue something for the next -rc, but maybe not this fix.

Great.

>
> >
> > Best Regards,
> >
> > Boris
> >
> > drivers/clk/at91/clk-slow.c | 28 ++++++++++++++++++++++++++--
> > 1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/at91/clk-slow.c b/drivers/clk/at91/clk-slow.c
> > index 32f7c1b..effe3b0 100644
> > --- a/drivers/clk/at91/clk-slow.c
> > +++ b/drivers/clk/at91/clk-slow.c
> > @@ -96,7 +96,19 @@ static void clk_slow_osc_unprepare(struct clk_hw *hw)
> > if (tmp & AT91_SCKC_OSC32BYP)
> > return;
> >
> > - writel(tmp & ~AT91_SCKC_OSC32EN, sckcr);
> > + /*
> > + * FIXME: All slow clk users are not properly requesting it (get +
> > + * prepare + enable) before using it.
> > + * If all users properly requesting this clock decide that they don't
> > + * need it anymore (or are removed), this lead to this clock being
> > + * disabled while faulty users are still requiring it, which in turn
> > + * hangs the system.
> > + * Prevent this clock from being disabled until all users are properly
> > + * requesting it.
> > + * Once this is done we should re-introduce this line:
> > + *
> > + * writel(tmp & ~AT91_SCKC_OSC32EN, sckcr);
> > + */
>
> The main problem here is that the clk prepare and enable usecounts are a
> lie. The use counts may be zero but the clock signal is still active. I
> think a better fix is for the clock driver to get, prepare & enable this
> clock in its probe function as you described above. If someone stumbles
> across this clock signal and wonders why it won't quiesce, at least the
> debug values will be accurate.

Yep, that makes sense.
I'll rework my patch.

Thanks,

Boris


--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


\
 
 \ /
  Last update: 2015-01-13 11:21    [W:0.057 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site