clk: at91: keep slow clk enabled to prevent system hang
Subsystems (3)
PATCH RESEND 1/1 clk: at91: keep slow clk enabled to prevent system hang
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 slow oscillator clock from being disabled until all users are properly requesting it.
---
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 ?
Best Regards,
Boris
drivers/clk/at91/clk-slow.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
drivers/clk/at91/clk-slow.c +26 -2
@@ -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); +*/ }static int clk_slow_osc_is_prepared (struct clk_hw *hw)
@@ -211,7 +223,19 @@ static void clk_slow_rc_osc_unprepare(struct clk_hw *hw)struct clk_slow_rc_osc *osc =to_clk_slow_rc_osc (hw);void __iomem *sckcr = osc->sckcr; -writel (readl (sckcr) & ~AT91_SCKC_RCEN , 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(readl(sckcr) & ~AT91_SCKC_RCEN, sckcr); +*/ }static int clk_slow_rc_osc_is_prepared (struct clk_hw *hw)
Signature · 1 line
1.9.1
PATCH RESEND 1/1 clk: at91: keep slow clk enabled to prevent system hang
Quoting Boris Brezillon (2015-01-12 07:12:50)
Quoted 5 lines
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?
Quoted 10 lines
Prevent slow oscillator clock from being disabled until all users are properly requesting it.
Signed-off-by Boris Brezillon <[email protected]>Reported-by Bo Shen <[email protected]>Cc stable at vger.kernel.org---
Hi Mike,
Quoted 8 lines
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.
Quoted 20 lines
Best Regards,
Boris
drivers/clk/at91/clk-slow.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)drivers/clk/at91/clk-slow.c +3
-1@@ -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.
drivers/clk/at91/clk-slow.c static void clk_slow_osc_unprepare(struct clk_hw *hw) · line 111
Quoted 10 lines
drivers/clk/at91/clk-slow.c +10
-0+* 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.
Regards,
Mike
Quoted 27 lines
drivers/clk/at91/clk-slow.c +13
-1}static int clk_slow_osc_is_prepared (struct clk_hw *hw)@@ -211,7 +223,19 @@ static void clk_slow_rc_osc_unprepare(struct clk_hw *hw)struct clk_slow_rc_osc *osc =to_clk_slow_rc_osc (hw);void __iomem *sckcr = osc->sckcr; -writel (readl (sckcr) & ~AT91_SCKC_RCEN , 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(readl(sckcr) & ~AT91_SCKC_RCEN, sckcr); +*/ }static int clk_slow_rc_osc_is_prepared (struct clk_hw *hw)Signature · 1 line
1.9.1
Quoted 9 lines · On Mon, 12 Jan 2015 15:44:24 -0800 Mike Turquette <[email protected]> wrote:
Quoting Boris Brezillon (2015-01-12 07:12:50)
Quoted 5 lines
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.
Quoted 13 lines
Quoted 13 lines
Prevent slow oscillator clock from being disabled until all users are properly requesting it.
Signed-off-by Boris Brezillon <[email protected]>Reported-by Bo Shen <[email protected]>Cc stable at vger.kernel.org---
Hi Mike,Sorry for the noise, but I forgot to add the LKML and LAKML in Cc.
Quoted 8 lines
Quoted 6 lines
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.
Quoted 30 lines
Quoted 30 lines
Best Regards,
Boris
drivers/clk/at91/clk-slow.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)drivers/clk/at91/clk-slow.c +12
-1@@ -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);
Quoted 8 lines
drivers/clk/at91/clk-slow.c static void clk_slow_osc_unprepare(struct clk_hw *hw) · line 111
Quoted 1 line
drivers/clk/at91/clk-slow.c +1
-0+*/ 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
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com