[PATCH 1/2] clk: imx: do not sleep if IRQ's are still disabled

Thomas Gleixner
2016-06-09T22:55:26Z

On Thu, 9 Jun 2016, Stefan Agner wrote:
> On 2016-06-09 13:08, Thomas Gleixner wrote:
> > On Tue, 7 Jun 2016, Dong Aisheng wrote:
> >> Then it may need introduce a lot changes and increase many new core APIs.
> >> Is that a problem?
> >
> > No. That's all better than each driver having broken workarounds. It's a
> > common problem so it wants to be addressed at the core level. There you have a
> > central point to do this and you can still catch abusers which call stuff from
> > the wrong context. The hacks in the drivers don't allow that because they look
> > at the context, i.e. irq disabled, instead of checking the system state.
>
> IMHO, the hacky part of my patch was how I detected whether to use sleep
> or delay. That said I am ok with API extension too, I guess it is fairly
> common use case... I found at least 6 clock prepare functions with sleep
> in it (and some udelays, all between 1-100).
>
> Your proposed solution uses "early_boot_or_suspend_resume" which I did
> not found as a convenient function in the wild :-)
>
> How would you implement that?

Early boot is simple. Supsend/resume is not that hard either. We have a patch
in RT which does exactly what you need. See below.

Then the state check simply becomes:

system_state != SYSTEM_RUNNING

Thanks,

tglx

8<---------------------------

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 350dfb08aee3..5e63b681f58e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -473,6 +473,7 @@ extern enum system_states {
SYSTEM_HALT,
SYSTEM_POWER_OFF,
SYSTEM_RESTART,
+ SYSTEM_SUSPEND,
} system_state;

#define TAINT_PROPRIETARY_MODULE 0
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index b7342a24f559..bfd9e0982f15 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -285,6 +285,8 @@ static int create_image(int platform_mode)

local_irq_disable();

+ system_state = SYSTEM_SUSPEND;
+
error = syscore_suspend();
if (error) {
printk(KERN_ERR "PM: Some system devices failed to power down, "
@@ -314,6 +316,7 @@ static int create_image(int platform_mode)
syscore_resume();

Enable_irqs:
+ system_state = SYSTEM_RUNNING;
local_irq_enable();

Enable_cpus:
@@ -437,6 +440,7 @@ static int resume_target_kernel(bool platform_mode)
goto Enable_cpus;

local_irq_disable();
+ system_state = SYSTEM_SUSPEND;

error = syscore_suspend();
if (error)
@@ -470,6 +474,7 @@ static int resume_target_kernel(bool platform_mode)
syscore_resume();

Enable_irqs:
+ system_state = SYSTEM_RUNNING;
local_irq_enable();

Enable_cpus:
@@ -555,6 +560,7 @@ int hibernation_platform_enter(void)
goto Enable_cpus;

local_irq_disable();
+ system_state = SYSTEM_SUSPEND;
syscore_suspend();
if (pm_wakeup_pending()) {
error = -EAGAIN;
@@ -567,6 +573,7 @@ int hibernation_platform_enter(void)

Power_up:
syscore_resume();
+ system_state = SYSTEM_RUNNING;
local_irq_enable();

Enable_cpus:
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index f9fe133c13e2..80ebc0726290 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -359,6 +359,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
arch_suspend_disable_irqs();
BUG_ON(!irqs_disabled());

+ system_state = SYSTEM_SUSPEND;
+
error = syscore_suspend();
if (!error) {
*wakeup = pm_wakeup_pending();
@@ -375,6 +377,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
syscore_resume();
}

+ system_state = SYSTEM_RUNNING;
+
arch_suspend_enable_irqs();
BUG_ON(irqs_disabled());