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

[Dong Aisheng]

On Thu, Jun 09, 2016 at 10:08:01PM +0200, 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.
> 

Okay, thanks.
If you wouldn't mind i could send out a patch based on your
suggestion for further discussion.

Thanks

Regards
Dong Aisheng

> Thanks,
> 
>   tglx

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

[Dong Aisheng]

On Thu, Jun 09, 2016 at 10:08:01PM +0200, 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.
> 

Okay, thanks.
If you wouldn't mind i could send out a patch based on your
suggestion for further discussion.

Thanks

Regards
Dong Aisheng

> Thanks,
> 
>   tglx

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

[Thomas Gleixner]

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());
 

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

[Thomas Gleixner]

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());
 

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

[Stefan Agner]

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?

--
Stefan

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

[Stefan Agner]

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?

--
Stefan

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

[Thomas Gleixner]

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.

Thanks,

tglx

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

[Thomas Gleixner]

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.

Thanks,

tglx

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

[Dong Aisheng]

On Mon, Jun 06, 2016 at 03:20:03PM +0200, Thomas Gleixner wrote:
> On Thu, 2 Jun 2016, Dong Aisheng wrote:
> > On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:
> > > Calling a function which might sleep _BEFORE_ kernel_init() is wrong. 
> > > Don't
> > > try to work around such an issue by doing magic irq_disabled() checks and 
> > > busy
> > > loops. Fix the call site and be done with it.
> > > 
> > 
> > IRQ chip and clocksource are also initialised before kernel_init()
> > which may call clk APIs like clk_prepare_enable().
> > We may not be able to guarantee those clocks are unsleepable.
> > 
> > e.g.
> > For IRQ chip, the arm,gic documents the optional clocks property in
> > Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt.
> > Although there seems no user currently, not sure there might be
> > a exception in the future.
> > 
> > For clocksource driver, it's quite common to call clk APIs to
> > enable and get clock rate which may also cause sleep.
> > e.g. ARM twd clock in arch/arm/kernel/smp_twd.c.
> > 
> > If we have to avoid the possible sleep caused by these clocks,
> > we may need to manually check it and change the related clocks
> > (e.g PLLs) from sleepable to busy loops.
> > But that is not a good solution and may also not stable when
> > clock varies.
> > 
> > It looks like not easy to find a generic solution for them.
> > What's your suggestion?
> 
> I think we should split the prepare callbacks up and move the handling logic
> into the core.
> 
> clk_ops.prepare() Legacy callback
> clk_ops.prepare_hw()  Callback which writes to the hardware
> clk_ops.prepare_done()Callback which checks whether the prepare is 
> completed
> 
> So now the core can do:
> 
> clk_prepare()
> {
>   /* Legacy code should go away */
>   if (ops->prepare) {
> ops->prepare();
> return;
>   }
> 
>   if (ops->prepare_hw)
> ops->prepare_hw();
> 
>   if (!ops->prepare_done())
>   return;
> 
>   if (early_boot_or_suspend_resume()) {
>   /*
>* Busy loop when we can't schedule in early boot,
>* suspend and resume.
>*/
>   while (!ops->prepare_done())
> ;
>   } else {
>   while (!ops->prepare_done())
> usleep(clk->prepare_delay);
>   }
> }
> 
> As a side effect that allows us to remove the gazillion of
> 
>   while (!hw_ready)
> usleep();
> 
> copies all over the place and we have a single point of control where we allow
> the clocks to busy loop.
> 
> Toughts?
> 

Great, that looks like a possible solution.
If we doing this way there's one more question that
should we do it for other clk APIs hold by prepare_lock which
all may sleep too in theory?
e.g. clk_{set|get}_rate/clk_{set|get}_parent. (possible more)
(clk_recalc_rate/clk_round_rate may also need be considered due to
it may be called within above function.)

And it seems many exist platforms doing that work in
CLK_OF_DECLARE init function in time_init().
e.g.
drivers/clk/imx/clk-imx6ul.c
drivers/clk/rockchip/clk-rk3188.c
drivers/clk/ti/clk-44xx.c
...

Then it may need introduce a lot changes and increase many new core APIs.
Is that a problem?

> Thanks,
> 
>   tglx

Regards
Dong Aisheng

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

[Dong Aisheng]

On Mon, Jun 06, 2016 at 03:20:03PM +0200, Thomas Gleixner wrote:
> On Thu, 2 Jun 2016, Dong Aisheng wrote:
> > On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:
> > > Calling a function which might sleep _BEFORE_ kernel_init() is wrong. 
> > > Don't
> > > try to work around such an issue by doing magic irq_disabled() checks and 
> > > busy
> > > loops. Fix the call site and be done with it.
> > > 
> > 
> > IRQ chip and clocksource are also initialised before kernel_init()
> > which may call clk APIs like clk_prepare_enable().
> > We may not be able to guarantee those clocks are unsleepable.
> > 
> > e.g.
> > For IRQ chip, the arm,gic documents the optional clocks property in
> > Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt.
> > Although there seems no user currently, not sure there might be
> > a exception in the future.
> > 
> > For clocksource driver, it's quite common to call clk APIs to
> > enable and get clock rate which may also cause sleep.
> > e.g. ARM twd clock in arch/arm/kernel/smp_twd.c.
> > 
> > If we have to avoid the possible sleep caused by these clocks,
> > we may need to manually check it and change the related clocks
> > (e.g PLLs) from sleepable to busy loops.
> > But that is not a good solution and may also not stable when
> > clock varies.
> > 
> > It looks like not easy to find a generic solution for them.
> > What's your suggestion?
> 
> I think we should split the prepare callbacks up and move the handling logic
> into the core.
> 
> clk_ops.prepare() Legacy callback
> clk_ops.prepare_hw()  Callback which writes to the hardware
> clk_ops.prepare_done()Callback which checks whether the prepare is 
> completed
> 
> So now the core can do:
> 
> clk_prepare()
> {
>   /* Legacy code should go away */
>   if (ops->prepare) {
> ops->prepare();
> return;
>   }
> 
>   if (ops->prepare_hw)
> ops->prepare_hw();
> 
>   if (!ops->prepare_done())
>   return;
> 
>   if (early_boot_or_suspend_resume()) {
>   /*
>* Busy loop when we can't schedule in early boot,
>* suspend and resume.
>*/
>   while (!ops->prepare_done())
> ;
>   } else {
>   while (!ops->prepare_done())
> usleep(clk->prepare_delay);
>   }
> }
> 
> As a side effect that allows us to remove the gazillion of
> 
>   while (!hw_ready)
> usleep();
> 
> copies all over the place and we have a single point of control where we allow
> the clocks to busy loop.
> 
> Toughts?
> 

Great, that looks like a possible solution.
If we doing this way there's one more question that
should we do it for other clk APIs hold by prepare_lock which
all may sleep too in theory?
e.g. clk_{set|get}_rate/clk_{set|get}_parent. (possible more)
(clk_recalc_rate/clk_round_rate may also need be considered due to
it may be called within above function.)

And it seems many exist platforms doing that work in
CLK_OF_DECLARE init function in time_init().
e.g.
drivers/clk/imx/clk-imx6ul.c
drivers/clk/rockchip/clk-rk3188.c
drivers/clk/ti/clk-44xx.c
...

Then it may need introduce a lot changes and increase many new core APIs.
Is that a problem?

> Thanks,
> 
>   tglx

Regards
Dong Aisheng

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

[Thomas Gleixner]

On Thu, 2 Jun 2016, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:
> > Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
> > try to work around such an issue by doing magic irq_disabled() checks and 
> > busy
> > loops. Fix the call site and be done with it.
> > 
> 
> IRQ chip and clocksource are also initialised before kernel_init()
> which may call clk APIs like clk_prepare_enable().
> We may not be able to guarantee those clocks are unsleepable.
> 
> e.g.
> For IRQ chip, the arm,gic documents the optional clocks property in
> Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt.
> Although there seems no user currently, not sure there might be
> a exception in the future.
> 
> For clocksource driver, it's quite common to call clk APIs to
> enable and get clock rate which may also cause sleep.
> e.g. ARM twd clock in arch/arm/kernel/smp_twd.c.
> 
> If we have to avoid the possible sleep caused by these clocks,
> we may need to manually check it and change the related clocks
> (e.g PLLs) from sleepable to busy loops.
> But that is not a good solution and may also not stable when
> clock varies.
> 
> It looks like not easy to find a generic solution for them.
> What's your suggestion?

I think we should split the prepare callbacks up and move the handling logic
into the core.

clk_ops.prepare()   Legacy callback
clk_ops.prepare_hw()Callback which writes to the hardware
clk_ops.prepare_done()  Callback which checks whether the prepare is completed

So now the core can do:

clk_prepare()
{
/* Legacy code should go away */
if (ops->prepare) {
  ops->prepare();
  return;
}

if (ops->prepare_hw)
  ops->prepare_hw();

if (!ops->prepare_done())
return;

if (early_boot_or_suspend_resume()) {
/*
 * Busy loop when we can't schedule in early boot,
 * suspend and resume.
 */
while (!ops->prepare_done())
  ;
} else {
while (!ops->prepare_done())
  usleep(clk->prepare_delay);
}
}

As a side effect that allows us to remove the gazillion of

while (!hw_ready)
  usleep();

copies all over the place and we have a single point of control where we allow
the clocks to busy loop.

Toughts?

Thanks,

tglx

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

[Thomas Gleixner]

On Thu, 2 Jun 2016, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:
> > Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
> > try to work around such an issue by doing magic irq_disabled() checks and 
> > busy
> > loops. Fix the call site and be done with it.
> > 
> 
> IRQ chip and clocksource are also initialised before kernel_init()
> which may call clk APIs like clk_prepare_enable().
> We may not be able to guarantee those clocks are unsleepable.
> 
> e.g.
> For IRQ chip, the arm,gic documents the optional clocks property in
> Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt.
> Although there seems no user currently, not sure there might be
> a exception in the future.
> 
> For clocksource driver, it's quite common to call clk APIs to
> enable and get clock rate which may also cause sleep.
> e.g. ARM twd clock in arch/arm/kernel/smp_twd.c.
> 
> If we have to avoid the possible sleep caused by these clocks,
> we may need to manually check it and change the related clocks
> (e.g PLLs) from sleepable to busy loops.
> But that is not a good solution and may also not stable when
> clock varies.
> 
> It looks like not easy to find a generic solution for them.
> What's your suggestion?

I think we should split the prepare callbacks up and move the handling logic
into the core.

clk_ops.prepare()   Legacy callback
clk_ops.prepare_hw()Callback which writes to the hardware
clk_ops.prepare_done()  Callback which checks whether the prepare is completed

So now the core can do:

clk_prepare()
{
/* Legacy code should go away */
if (ops->prepare) {
  ops->prepare();
  return;
}

if (ops->prepare_hw)
  ops->prepare_hw();

if (!ops->prepare_done())
return;

if (early_boot_or_suspend_resume()) {
/*
 * Busy loop when we can't schedule in early boot,
 * suspend and resume.
 */
while (!ops->prepare_done())
  ;
} else {
while (!ops->prepare_done())
  usleep(clk->prepare_delay);
}
}

As a side effect that allows us to remove the gazillion of

while (!hw_ready)
  usleep();

copies all over the place and we have a single point of control where we allow
the clocks to busy loop.

Toughts?

Thanks,

tglx

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

[Dong Aisheng]

Hi Thomas,

On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:
> On Wed, 27 Apr 2016, Dong Aisheng wrote:
> > On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> > > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> > >> Shawn,
> > >> What's your suggestion?
> > >
> > > I think this needs more discussion, and I just dropped Stefan's patch
> > > from my tree.
> > >
> > > We need to firstly understand why this is happening.  The .prepare hook
> > > is defined to be non-atomic context, and so that we call sleep function
> > > in there.  We did everything right.  Why are we getting the warning?  If
> > > I'm correct, this warning only happens on i.MX7D.  Why is that?
> > >
> > 
> > Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> > time init, the irq is still not enabled. It fixes the issue indirectly.
> > See:
> > asmlinkage __visible void __init start_kernel(void)
> > {
> > /*
> >  * Set up the scheduler prior starting any interrupts (such as the
> >  * timer interrupt). Full topology setup happens at smp_init()
> >  * time - but meanwhile we still have a functioning scheduler.
> >  */
> > sched_init();
> > .
> > time_init();
> > ..
> > WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> > early_boot_irqs_disabled = false;
> > local_irq_enable();
> > }
> > 
> > The issue can only happen when PLL enable causes a schedule during
> > imx_clock_init().
> 
> Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
> try to work around such an issue by doing magic irq_disabled() checks and busy
> loops. Fix the call site and be done with it.
> 

IRQ chip and clocksource are also initialised before kernel_init()
which may call clk APIs like clk_prepare_enable().
We may not be able to guarantee those clocks are unsleepable.

e.g.
For IRQ chip, the arm,gic documents the optional clocks property in
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt.
Although there seems no user currently, not sure there might be
a exception in the future.

For clocksource driver, it's quite common to call clk APIs to
enable and get clock rate which may also cause sleep.
e.g. ARM twd clock in arch/arm/kernel/smp_twd.c.

If we have to avoid the possible sleep caused by these clocks,
we may need to manually check it and change the related clocks
(e.g PLLs) from sleepable to busy loops.
But that is not a good solution and may also not stable when
clock varies.

It looks like not easy to find a generic solution for them.
What's your suggestion?

And Stephen,

Besides above possible sleep, the clk framework itself may also
cause unexpected sleep before kernel_init().
1) clk framework now supports CLK_IS_CRITICAL flag which allows to
enable the clock during register in __clk_core_init(), it usually
can happen in time_init()->of_clk_init();

2) clk framework also support set default configuration like set_parent
and set_rate during register clk providers which also happens in
time_init()->of_clk_init().

Any ideas?
Do you think if we should move them onto a bit later stage after
kernel_init()?

Regards
Dong Aisheng

> Thanks,
> 
>   tglx
> 
> 

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

[Dong Aisheng]

Hi Thomas,

On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:
> On Wed, 27 Apr 2016, Dong Aisheng wrote:
> > On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> > > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> > >> Shawn,
> > >> What's your suggestion?
> > >
> > > I think this needs more discussion, and I just dropped Stefan's patch
> > > from my tree.
> > >
> > > We need to firstly understand why this is happening.  The .prepare hook
> > > is defined to be non-atomic context, and so that we call sleep function
> > > in there.  We did everything right.  Why are we getting the warning?  If
> > > I'm correct, this warning only happens on i.MX7D.  Why is that?
> > >
> > 
> > Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> > time init, the irq is still not enabled. It fixes the issue indirectly.
> > See:
> > asmlinkage __visible void __init start_kernel(void)
> > {
> > /*
> >  * Set up the scheduler prior starting any interrupts (such as the
> >  * timer interrupt). Full topology setup happens at smp_init()
> >  * time - but meanwhile we still have a functioning scheduler.
> >  */
> > sched_init();
> > .
> > time_init();
> > ..
> > WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> > early_boot_irqs_disabled = false;
> > local_irq_enable();
> > }
> > 
> > The issue can only happen when PLL enable causes a schedule during
> > imx_clock_init().
> 
> Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
> try to work around such an issue by doing magic irq_disabled() checks and busy
> loops. Fix the call site and be done with it.
> 

IRQ chip and clocksource are also initialised before kernel_init()
which may call clk APIs like clk_prepare_enable().
We may not be able to guarantee those clocks are unsleepable.

e.g.
For IRQ chip, the arm,gic documents the optional clocks property in
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt.
Although there seems no user currently, not sure there might be
a exception in the future.

For clocksource driver, it's quite common to call clk APIs to
enable and get clock rate which may also cause sleep.
e.g. ARM twd clock in arch/arm/kernel/smp_twd.c.

If we have to avoid the possible sleep caused by these clocks,
we may need to manually check it and change the related clocks
(e.g PLLs) from sleepable to busy loops.
But that is not a good solution and may also not stable when
clock varies.

It looks like not easy to find a generic solution for them.
What's your suggestion?

And Stephen,

Besides above possible sleep, the clk framework itself may also
cause unexpected sleep before kernel_init().
1) clk framework now supports CLK_IS_CRITICAL flag which allows to
enable the clock during register in __clk_core_init(), it usually
can happen in time_init()->of_clk_init();

2) clk framework also support set default configuration like set_parent
and set_rate during register clk providers which also happens in
time_init()->of_clk_init().

Any ideas?
Do you think if we should move them onto a bit later stage after
kernel_init()?

Regards
Dong Aisheng

> Thanks,
> 
>   tglx
> 
> 

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

[Stefan Agner]

On 2016-04-27 03:15, Thomas Gleixner wrote:
> On Wed, 27 Apr 2016, Dong Aisheng wrote:
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
> 
> Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
> try to work around such an issue by doing magic irq_disabled() checks and busy
> loops. Fix the call site and be done with it.

What do you mean exactly by fix the call site? The patch I proposed
(https://lkml.org/lkml/2016/1/29/695) fixes the call site...

But I see Dong's argument here, irqs_disabled is the wrong way to figure
out whether we are in kernel_init. What is the right approach to
distinguish whether we are allowed to sleep?

--
Stefan

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

[Stefan Agner]

On 2016-04-27 03:15, Thomas Gleixner wrote:
> On Wed, 27 Apr 2016, Dong Aisheng wrote:
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
> 
> Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
> try to work around such an issue by doing magic irq_disabled() checks and busy
> loops. Fix the call site and be done with it.

What do you mean exactly by fix the call site? The patch I proposed
(https://lkml.org/lkml/2016/1/29/695) fixes the call site...

But I see Dong's argument here, irqs_disabled is the wrong way to figure
out whether we are in kernel_init. What is the right approach to
distinguish whether we are allowed to sleep?

--
Stefan

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

[Thomas Gleixner]

On Wed, 27 Apr 2016, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> >> Shawn,
> >> What's your suggestion?
> >
> > I think this needs more discussion, and I just dropped Stefan's patch
> > from my tree.
> >
> > We need to firstly understand why this is happening.  The .prepare hook
> > is defined to be non-atomic context, and so that we call sleep function
> > in there.  We did everything right.  Why are we getting the warning?  If
> > I'm correct, this warning only happens on i.MX7D.  Why is that?
> >
> 
> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> time init, the irq is still not enabled. It fixes the issue indirectly.
> See:
> asmlinkage __visible void __init start_kernel(void)
> {
> /*
>  * Set up the scheduler prior starting any interrupts (such as the
>  * timer interrupt). Full topology setup happens at smp_init()
>  * time - but meanwhile we still have a functioning scheduler.
>  */
> sched_init();
> .
> time_init();
> ..
> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> early_boot_irqs_disabled = false;
> local_irq_enable();
> }
> 
> The issue can only happen when PLL enable causes a schedule during
> imx_clock_init().

Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
try to work around such an issue by doing magic irq_disabled() checks and busy
loops. Fix the call site and be done with it.

Thanks,

tglx

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

[Thomas Gleixner]

On Wed, 27 Apr 2016, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> >> Shawn,
> >> What's your suggestion?
> >
> > I think this needs more discussion, and I just dropped Stefan's patch
> > from my tree.
> >
> > We need to firstly understand why this is happening.  The .prepare hook
> > is defined to be non-atomic context, and so that we call sleep function
> > in there.  We did everything right.  Why are we getting the warning?  If
> > I'm correct, this warning only happens on i.MX7D.  Why is that?
> >
> 
> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> time init, the irq is still not enabled. It fixes the issue indirectly.
> See:
> asmlinkage __visible void __init start_kernel(void)
> {
> /*
>  * Set up the scheduler prior starting any interrupts (such as the
>  * timer interrupt). Full topology setup happens at smp_init()
>  * time - but meanwhile we still have a functioning scheduler.
>  */
> sched_init();
> .
> time_init();
> ..
> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> early_boot_irqs_disabled = false;
> local_irq_enable();
> }
> 
> The issue can only happen when PLL enable causes a schedule during
> imx_clock_init().

Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
try to work around such an issue by doing magic irq_disabled() checks and busy
loops. Fix the call site and be done with it.

Thanks,

tglx

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 3:34 PM, Stefan Agner  wrote:
> On 2016-04-26 19:57, Dong Aisheng wrote:
>> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>>> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
 Shawn,
 What's your suggestion?
>>>
>>> I think this needs more discussion, and I just dropped Stefan's patch
>>> from my tree.
>>>
>>> We need to firstly understand why this is happening.  The .prepare hook
>>> is defined to be non-atomic context, and so that we call sleep function
>>> in there.  We did everything right.  Why are we getting the warning?  If
>>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>>
>>
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
>> Not all PLL has this issue.
>> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
>> which requires more delay time and cause usleep.
>> Because clk framework does not support MX7D clock types (operation requires
>> parents on), we simply enable all clocks in imx7d_clocks_init().
>>
>> If apply my this patch series:
>> https://lkml.org/lkml/2016/4/20/199
>> The issue can also be gone.
>
> Oh ok, it does make sense to enable as few clocks as possible.
>
> However, even if we do not enable lots of clocks at that time, and this
> helps to avoid the problem for now, it could still be that
> someone/something requests a clock early during boot, leading to a PLL
> enable... Shouldn't we make sure that those base clocks can be enabled
> even during early boot..?
>

The point as Shawn pointed is that we even shouldn't call clk_prepare_enable
at that time.
So we may first try to eliminate those callings in imx7d_clocks_init.

> --
> Stefan

Regards
Dong Aisheng

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 3:34 PM, Stefan Agner  wrote:
> On 2016-04-26 19:57, Dong Aisheng wrote:
>> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>>> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
 Shawn,
 What's your suggestion?
>>>
>>> I think this needs more discussion, and I just dropped Stefan's patch
>>> from my tree.
>>>
>>> We need to firstly understand why this is happening.  The .prepare hook
>>> is defined to be non-atomic context, and so that we call sleep function
>>> in there.  We did everything right.  Why are we getting the warning?  If
>>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>>
>>
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
>> Not all PLL has this issue.
>> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
>> which requires more delay time and cause usleep.
>> Because clk framework does not support MX7D clock types (operation requires
>> parents on), we simply enable all clocks in imx7d_clocks_init().
>>
>> If apply my this patch series:
>> https://lkml.org/lkml/2016/4/20/199
>> The issue can also be gone.
>
> Oh ok, it does make sense to enable as few clocks as possible.
>
> However, even if we do not enable lots of clocks at that time, and this
> helps to avoid the problem for now, it could still be that
> someone/something requests a clock early during boot, leading to a PLL
> enable... Shouldn't we make sure that those base clocks can be enabled
> even during early boot..?
>

The point as Shawn pointed is that we even shouldn't call clk_prepare_enable
at that time.
So we may first try to eliminate those callings in imx7d_clocks_init.

> --
> Stefan

Regards
Dong Aisheng

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 3:28 PM, Stefan Agner  wrote:
> On 2016-04-26 19:56, Fabio Estevam wrote:
>> On Tue, Apr 26, 2016 at 11:45 PM, Dong Aisheng  wrote:
>>
 We need to firstly understand why this is happening.  The .prepare hook
 is defined to be non-atomic context, and so that we call sleep function
 in there.  We did everything right.  Why are we getting the warning?  If
 I'm correct, this warning only happens on i.MX7D.  Why is that?

>>>
>>> This is mainly caused by during kernel early booting, there's only one init 
>>> idle
>>> task running.
>>> See:
>>> void __init sched_init(void)
>>> {
>>> .
>>> /*
>>>  * Make us the idle thread. Technically, schedule() should not be
>>>  * called from this thread, however somewhere below it might be,
>>>  * but because we are the idle thread, we just pick up running again
>>>  * when this runqueue becomes "idle".
>>>  */
>>> init_idle(current, smp_processor_id());
>>> ...
>>> }
>>>
>>> And the idle sched class indicates it's not valid to schedule for idle task.
>>> const struct sched_class idle_sched_class = {
>>> /* .next is NULL */
>>> /* no enqueue/yield_task for idle tasks */
>>>
>>> /* dequeue is not valid, we print a debug message there: */
>>> .dequeue_task   = dequeue_task_idle,
>>> ...
>>>
>>> }
>>>
>>> /*
>>>  * It is not legal to sleep in the idle task - print a warning
>>>  * message if some code attempts to do it:
>>>  */
>>> static void
>>> dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
>>> {
>>> raw_spin_unlock_irq(>lock);
>>> printk(KERN_ERR "bad: scheduling from the idle thread!\n");
>>> dump_stack();
>>> raw_spin_lock_irq(>lock);
>>> }
>>>
>>>
>>> Below is the full log of imx7d booting FYI.
>>
>> This does not answer Shawn's question: why do we see this only on mx7d?
>
> I was wondering that too My theory is that either on i.MX 6 the
> clocks enable almost instantly leading to no sleep, or they are just
> bootloader/firmware on...?
>

Yes, for core plls, they're already enabled in bootloader.
We observed this issue on MX7D is because we rudely enable all clocks
for it and MX7D AV PLLs which will lead to sleep reveals this issue.

Regards
Dong Aisheng

> --
> Stefan

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 3:28 PM, Stefan Agner  wrote:
> On 2016-04-26 19:56, Fabio Estevam wrote:
>> On Tue, Apr 26, 2016 at 11:45 PM, Dong Aisheng  wrote:
>>
 We need to firstly understand why this is happening.  The .prepare hook
 is defined to be non-atomic context, and so that we call sleep function
 in there.  We did everything right.  Why are we getting the warning?  If
 I'm correct, this warning only happens on i.MX7D.  Why is that?

>>>
>>> This is mainly caused by during kernel early booting, there's only one init 
>>> idle
>>> task running.
>>> See:
>>> void __init sched_init(void)
>>> {
>>> .
>>> /*
>>>  * Make us the idle thread. Technically, schedule() should not be
>>>  * called from this thread, however somewhere below it might be,
>>>  * but because we are the idle thread, we just pick up running again
>>>  * when this runqueue becomes "idle".
>>>  */
>>> init_idle(current, smp_processor_id());
>>> ...
>>> }
>>>
>>> And the idle sched class indicates it's not valid to schedule for idle task.
>>> const struct sched_class idle_sched_class = {
>>> /* .next is NULL */
>>> /* no enqueue/yield_task for idle tasks */
>>>
>>> /* dequeue is not valid, we print a debug message there: */
>>> .dequeue_task   = dequeue_task_idle,
>>> ...
>>>
>>> }
>>>
>>> /*
>>>  * It is not legal to sleep in the idle task - print a warning
>>>  * message if some code attempts to do it:
>>>  */
>>> static void
>>> dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
>>> {
>>> raw_spin_unlock_irq(>lock);
>>> printk(KERN_ERR "bad: scheduling from the idle thread!\n");
>>> dump_stack();
>>> raw_spin_lock_irq(>lock);
>>> }
>>>
>>>
>>> Below is the full log of imx7d booting FYI.
>>
>> This does not answer Shawn's question: why do we see this only on mx7d?
>
> I was wondering that too My theory is that either on i.MX 6 the
> clocks enable almost instantly leading to no sleep, or they are just
> bootloader/firmware on...?
>

Yes, for core plls, they're already enabled in bootloader.
We observed this issue on MX7D is because we rudely enable all clocks
for it and MX7D AV PLLs which will lead to sleep reveals this issue.

Regards
Dong Aisheng

> --
> Stefan

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 3:24 PM, Shawn Guo  wrote:
> On Wed, Apr 27, 2016 at 10:57:21AM +0800, Dong Aisheng wrote:
>> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> >> Shawn,
>> >> What's your suggestion?
>> >
>> > I think this needs more discussion, and I just dropped Stefan's patch
>> > from my tree.
>> >
>> > We need to firstly understand why this is happening.  The .prepare hook
>> > is defined to be non-atomic context, and so that we call sleep function
>> > in there.  We did everything right.  Why are we getting the warning?  If
>> > I'm correct, this warning only happens on i.MX7D.  Why is that?
>> >
>>
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
>> Not all PLL has this issue.
>> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
>> which requires more delay time and cause usleep.
>> Because clk framework does not support MX7D clock types (operation requires
>> parents on), we simply enable all clocks in imx7d_clocks_init().
>>
>> If apply my this patch series:
>> https://lkml.org/lkml/2016/4/20/199
>> The issue can also be gone.
>
> Thanks for the info.  It sounds like that we are fixing the problem in
> the wrong place, i.e. clk_pllv3_prepare().  The function does nothing
> wrong, since the .prepare hook is defined to be one that can sleep.  If
> we see sleep warning in a context calling clk_prepare(), that probably
> means we shouldn't make the function call from that context.
>

Yes, i agree.
I'm trying to get rid of these calls.

Simply remove them or delay to arch_init will cause kernel fail to boot.
Still checking the root cause.

> Shawn

Regards
Dong Aisheng

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 3:24 PM, Shawn Guo  wrote:
> On Wed, Apr 27, 2016 at 10:57:21AM +0800, Dong Aisheng wrote:
>> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> >> Shawn,
>> >> What's your suggestion?
>> >
>> > I think this needs more discussion, and I just dropped Stefan's patch
>> > from my tree.
>> >
>> > We need to firstly understand why this is happening.  The .prepare hook
>> > is defined to be non-atomic context, and so that we call sleep function
>> > in there.  We did everything right.  Why are we getting the warning?  If
>> > I'm correct, this warning only happens on i.MX7D.  Why is that?
>> >
>>
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
>> Not all PLL has this issue.
>> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
>> which requires more delay time and cause usleep.
>> Because clk framework does not support MX7D clock types (operation requires
>> parents on), we simply enable all clocks in imx7d_clocks_init().
>>
>> If apply my this patch series:
>> https://lkml.org/lkml/2016/4/20/199
>> The issue can also be gone.
>
> Thanks for the info.  It sounds like that we are fixing the problem in
> the wrong place, i.e. clk_pllv3_prepare().  The function does nothing
> wrong, since the .prepare hook is defined to be one that can sleep.  If
> we see sleep warning in a context calling clk_prepare(), that probably
> means we shouldn't make the function call from that context.
>

Yes, i agree.
I'm trying to get rid of these calls.

Simply remove them or delay to arch_init will cause kernel fail to boot.
Still checking the root cause.

> Shawn

Regards
Dong Aisheng

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

[Stefan Agner]

On 2016-04-26 19:57, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>>> Shawn,
>>> What's your suggestion?
>>
>> I think this needs more discussion, and I just dropped Stefan's patch
>> from my tree.
>>
>> We need to firstly understand why this is happening.  The .prepare hook
>> is defined to be non-atomic context, and so that we call sleep function
>> in there.  We did everything right.  Why are we getting the warning?  If
>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>
> 
> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> time init, the irq is still not enabled. It fixes the issue indirectly.
> See:
> asmlinkage __visible void __init start_kernel(void)
> {
> /*
>  * Set up the scheduler prior starting any interrupts (such as the
>  * timer interrupt). Full topology setup happens at smp_init()
>  * time - but meanwhile we still have a functioning scheduler.
>  */
> sched_init();
> .
> time_init();
> ..
> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> early_boot_irqs_disabled = false;
> local_irq_enable();
> }
> 
> The issue can only happen when PLL enable causes a schedule during
> imx_clock_init().
> Not all PLL has this issue.
> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
> which requires more delay time and cause usleep.
> Because clk framework does not support MX7D clock types (operation requires
> parents on), we simply enable all clocks in imx7d_clocks_init().
> 
> If apply my this patch series:
> https://lkml.org/lkml/2016/4/20/199
> The issue can also be gone.

Oh ok, it does make sense to enable as few clocks as possible.

However, even if we do not enable lots of clocks at that time, and this
helps to avoid the problem for now, it could still be that
someone/something requests a clock early during boot, leading to a PLL
enable... Shouldn't we make sure that those base clocks can be enabled
even during early boot..?

--
Stefan

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

[Stefan Agner]

On 2016-04-26 19:57, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>>> Shawn,
>>> What's your suggestion?
>>
>> I think this needs more discussion, and I just dropped Stefan's patch
>> from my tree.
>>
>> We need to firstly understand why this is happening.  The .prepare hook
>> is defined to be non-atomic context, and so that we call sleep function
>> in there.  We did everything right.  Why are we getting the warning?  If
>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>
> 
> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> time init, the irq is still not enabled. It fixes the issue indirectly.
> See:
> asmlinkage __visible void __init start_kernel(void)
> {
> /*
>  * Set up the scheduler prior starting any interrupts (such as the
>  * timer interrupt). Full topology setup happens at smp_init()
>  * time - but meanwhile we still have a functioning scheduler.
>  */
> sched_init();
> .
> time_init();
> ..
> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> early_boot_irqs_disabled = false;
> local_irq_enable();
> }
> 
> The issue can only happen when PLL enable causes a schedule during
> imx_clock_init().
> Not all PLL has this issue.
> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
> which requires more delay time and cause usleep.
> Because clk framework does not support MX7D clock types (operation requires
> parents on), we simply enable all clocks in imx7d_clocks_init().
> 
> If apply my this patch series:
> https://lkml.org/lkml/2016/4/20/199
> The issue can also be gone.

Oh ok, it does make sense to enable as few clocks as possible.

However, even if we do not enable lots of clocks at that time, and this
helps to avoid the problem for now, it could still be that
someone/something requests a clock early during boot, leading to a PLL
enable... Shouldn't we make sure that those base clocks can be enabled
even during early boot..?

--
Stefan

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

[Stefan Agner]

On 2016-04-26 19:56, Fabio Estevam wrote:
> On Tue, Apr 26, 2016 at 11:45 PM, Dong Aisheng  wrote:
> 
>>> We need to firstly understand why this is happening.  The .prepare hook
>>> is defined to be non-atomic context, and so that we call sleep function
>>> in there.  We did everything right.  Why are we getting the warning?  If
>>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>>
>>
>> This is mainly caused by during kernel early booting, there's only one init 
>> idle
>> task running.
>> See:
>> void __init sched_init(void)
>> {
>> .
>> /*
>>  * Make us the idle thread. Technically, schedule() should not be
>>  * called from this thread, however somewhere below it might be,
>>  * but because we are the idle thread, we just pick up running again
>>  * when this runqueue becomes "idle".
>>  */
>> init_idle(current, smp_processor_id());
>> ...
>> }
>>
>> And the idle sched class indicates it's not valid to schedule for idle task.
>> const struct sched_class idle_sched_class = {
>> /* .next is NULL */
>> /* no enqueue/yield_task for idle tasks */
>>
>> /* dequeue is not valid, we print a debug message there: */
>> .dequeue_task   = dequeue_task_idle,
>> ...
>>
>> }
>>
>> /*
>>  * It is not legal to sleep in the idle task - print a warning
>>  * message if some code attempts to do it:
>>  */
>> static void
>> dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
>> {
>> raw_spin_unlock_irq(>lock);
>> printk(KERN_ERR "bad: scheduling from the idle thread!\n");
>> dump_stack();
>> raw_spin_lock_irq(>lock);
>> }
>>
>>
>> Below is the full log of imx7d booting FYI.
> 
> This does not answer Shawn's question: why do we see this only on mx7d?

I was wondering that too My theory is that either on i.MX 6 the
clocks enable almost instantly leading to no sleep, or they are just
bootloader/firmware on...?

--
Stefan

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

[Stefan Agner]

On 2016-04-26 19:56, Fabio Estevam wrote:
> On Tue, Apr 26, 2016 at 11:45 PM, Dong Aisheng  wrote:
> 
>>> We need to firstly understand why this is happening.  The .prepare hook
>>> is defined to be non-atomic context, and so that we call sleep function
>>> in there.  We did everything right.  Why are we getting the warning?  If
>>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>>
>>
>> This is mainly caused by during kernel early booting, there's only one init 
>> idle
>> task running.
>> See:
>> void __init sched_init(void)
>> {
>> .
>> /*
>>  * Make us the idle thread. Technically, schedule() should not be
>>  * called from this thread, however somewhere below it might be,
>>  * but because we are the idle thread, we just pick up running again
>>  * when this runqueue becomes "idle".
>>  */
>> init_idle(current, smp_processor_id());
>> ...
>> }
>>
>> And the idle sched class indicates it's not valid to schedule for idle task.
>> const struct sched_class idle_sched_class = {
>> /* .next is NULL */
>> /* no enqueue/yield_task for idle tasks */
>>
>> /* dequeue is not valid, we print a debug message there: */
>> .dequeue_task   = dequeue_task_idle,
>> ...
>>
>> }
>>
>> /*
>>  * It is not legal to sleep in the idle task - print a warning
>>  * message if some code attempts to do it:
>>  */
>> static void
>> dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
>> {
>> raw_spin_unlock_irq(>lock);
>> printk(KERN_ERR "bad: scheduling from the idle thread!\n");
>> dump_stack();
>> raw_spin_lock_irq(>lock);
>> }
>>
>>
>> Below is the full log of imx7d booting FYI.
> 
> This does not answer Shawn's question: why do we see this only on mx7d?

I was wondering that too My theory is that either on i.MX 6 the
clocks enable almost instantly leading to no sleep, or they are just
bootloader/firmware on...?

--
Stefan

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

[Stefan Agner]

On 2016-04-27 00:24, Shawn Guo wrote:
> On Wed, Apr 27, 2016 at 10:57:21AM +0800, Dong Aisheng wrote:
>> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> >> Shawn,
>> >> What's your suggestion?
>> >
>> > I think this needs more discussion, and I just dropped Stefan's patch
>> > from my tree.
>> >
>> > We need to firstly understand why this is happening.  The .prepare hook
>> > is defined to be non-atomic context, and so that we call sleep function
>> > in there.  We did everything right.  Why are we getting the warning?  If
>> > I'm correct, this warning only happens on i.MX7D.  Why is that?
>> >
>>
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
>> Not all PLL has this issue.
>> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
>> which requires more delay time and cause usleep.
>> Because clk framework does not support MX7D clock types (operation requires
>> parents on), we simply enable all clocks in imx7d_clocks_init().
>>
>> If apply my this patch series:
>> https://lkml.org/lkml/2016/4/20/199
>> The issue can also be gone.
> 
> Thanks for the info.  It sounds like that we are fixing the problem in
> the wrong place, i.e. clk_pllv3_prepare().  The function does nothing
> wrong, since the .prepare hook is defined to be one that can sleep.  If
> we see sleep warning in a context calling clk_prepare(), that probably
> means we shouldn't make the function call from that context.
> 

According to the stack trace in the answer to Stephens question the call
comes from imx7d_clocks_init. I doubt we can avoid that those clocks get
enabled there...

--
Stefan

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

[Stefan Agner]

On 2016-04-27 00:24, Shawn Guo wrote:
> On Wed, Apr 27, 2016 at 10:57:21AM +0800, Dong Aisheng wrote:
>> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
>> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> >> Shawn,
>> >> What's your suggestion?
>> >
>> > I think this needs more discussion, and I just dropped Stefan's patch
>> > from my tree.
>> >
>> > We need to firstly understand why this is happening.  The .prepare hook
>> > is defined to be non-atomic context, and so that we call sleep function
>> > in there.  We did everything right.  Why are we getting the warning?  If
>> > I'm correct, this warning only happens on i.MX7D.  Why is that?
>> >
>>
>> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
>> time init, the irq is still not enabled. It fixes the issue indirectly.
>> See:
>> asmlinkage __visible void __init start_kernel(void)
>> {
>> /*
>>  * Set up the scheduler prior starting any interrupts (such as the
>>  * timer interrupt). Full topology setup happens at smp_init()
>>  * time - but meanwhile we still have a functioning scheduler.
>>  */
>> sched_init();
>> .
>> time_init();
>> ..
>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable();
>> }
>>
>> The issue can only happen when PLL enable causes a schedule during
>> imx_clock_init().
>> Not all PLL has this issue.
>> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
>> which requires more delay time and cause usleep.
>> Because clk framework does not support MX7D clock types (operation requires
>> parents on), we simply enable all clocks in imx7d_clocks_init().
>>
>> If apply my this patch series:
>> https://lkml.org/lkml/2016/4/20/199
>> The issue can also be gone.
> 
> Thanks for the info.  It sounds like that we are fixing the problem in
> the wrong place, i.e. clk_pllv3_prepare().  The function does nothing
> wrong, since the .prepare hook is defined to be one that can sleep.  If
> we see sleep warning in a context calling clk_prepare(), that probably
> means we shouldn't make the function call from that context.
> 

According to the stack trace in the answer to Stephens question the call
comes from imx7d_clocks_init. I doubt we can avoid that those clocks get
enabled there...

--
Stefan

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

[Shawn Guo]

On Wed, Apr 27, 2016 at 10:57:21AM +0800, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> >> Shawn,
> >> What's your suggestion?
> >
> > I think this needs more discussion, and I just dropped Stefan's patch
> > from my tree.
> >
> > We need to firstly understand why this is happening.  The .prepare hook
> > is defined to be non-atomic context, and so that we call sleep function
> > in there.  We did everything right.  Why are we getting the warning?  If
> > I'm correct, this warning only happens on i.MX7D.  Why is that?
> >
> 
> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> time init, the irq is still not enabled. It fixes the issue indirectly.
> See:
> asmlinkage __visible void __init start_kernel(void)
> {
> /*
>  * Set up the scheduler prior starting any interrupts (such as the
>  * timer interrupt). Full topology setup happens at smp_init()
>  * time - but meanwhile we still have a functioning scheduler.
>  */
> sched_init();
> .
> time_init();
> ..
> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> early_boot_irqs_disabled = false;
> local_irq_enable();
> }
> 
> The issue can only happen when PLL enable causes a schedule during
> imx_clock_init().
> Not all PLL has this issue.
> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
> which requires more delay time and cause usleep.
> Because clk framework does not support MX7D clock types (operation requires
> parents on), we simply enable all clocks in imx7d_clocks_init().
> 
> If apply my this patch series:
> https://lkml.org/lkml/2016/4/20/199
> The issue can also be gone.

Thanks for the info.  It sounds like that we are fixing the problem in
the wrong place, i.e. clk_pllv3_prepare().  The function does nothing
wrong, since the .prepare hook is defined to be one that can sleep.  If
we see sleep warning in a context calling clk_prepare(), that probably
means we shouldn't make the function call from that context.

Shawn

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

[Shawn Guo]

On Wed, Apr 27, 2016 at 10:57:21AM +0800, Dong Aisheng wrote:
> On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> > On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> >> Shawn,
> >> What's your suggestion?
> >
> > I think this needs more discussion, and I just dropped Stefan's patch
> > from my tree.
> >
> > We need to firstly understand why this is happening.  The .prepare hook
> > is defined to be non-atomic context, and so that we call sleep function
> > in there.  We did everything right.  Why are we getting the warning?  If
> > I'm correct, this warning only happens on i.MX7D.  Why is that?
> >
> 
> Why Stefan's patch works (checking irqs_disabled()) is because during kernel
> time init, the irq is still not enabled. It fixes the issue indirectly.
> See:
> asmlinkage __visible void __init start_kernel(void)
> {
> /*
>  * Set up the scheduler prior starting any interrupts (such as the
>  * timer interrupt). Full topology setup happens at smp_init()
>  * time - but meanwhile we still have a functioning scheduler.
>  */
> sched_init();
> .
> time_init();
> ..
> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
> early_boot_irqs_disabled = false;
> local_irq_enable();
> }
> 
> The issue can only happen when PLL enable causes a schedule during
> imx_clock_init().
> Not all PLL has this issue.
> The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
> which requires more delay time and cause usleep.
> Because clk framework does not support MX7D clock types (operation requires
> parents on), we simply enable all clocks in imx7d_clocks_init().
> 
> If apply my this patch series:
> https://lkml.org/lkml/2016/4/20/199
> The issue can also be gone.

Thanks for the info.  It sounds like that we are fixing the problem in
the wrong place, i.e. clk_pllv3_prepare().  The function does nothing
wrong, since the .prepare hook is defined to be one that can sleep.  If
we see sleep warning in a context calling clk_prepare(), that probably
means we shouldn't make the function call from that context.

Shawn

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> Shawn,
>> What's your suggestion?
>
> I think this needs more discussion, and I just dropped Stefan's patch
> from my tree.
>
> We need to firstly understand why this is happening.  The .prepare hook
> is defined to be non-atomic context, and so that we call sleep function
> in there.  We did everything right.  Why are we getting the warning?  If
> I'm correct, this warning only happens on i.MX7D.  Why is that?
>

Why Stefan's patch works (checking irqs_disabled()) is because during kernel
time init, the irq is still not enabled. It fixes the issue indirectly.
See:
asmlinkage __visible void __init start_kernel(void)
{
/*
 * Set up the scheduler prior starting any interrupts (such as the
 * timer interrupt). Full topology setup happens at smp_init()
 * time - but meanwhile we still have a functioning scheduler.
 */
sched_init();
.
time_init();
..
WARN(!irqs_disabled(), "Interrupts were enabled early\n");
early_boot_irqs_disabled = false;
local_irq_enable();
}

The issue can only happen when PLL enable causes a schedule during
imx_clock_init().
Not all PLL has this issue.
The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
which requires more delay time and cause usleep.
Because clk framework does not support MX7D clock types (operation requires
parents on), we simply enable all clocks in imx7d_clocks_init().

If apply my this patch series:
https://lkml.org/lkml/2016/4/20/199
The issue can also be gone.

Regards
Dong Aisheng

> Shawn

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> Shawn,
>> What's your suggestion?
>
> I think this needs more discussion, and I just dropped Stefan's patch
> from my tree.
>
> We need to firstly understand why this is happening.  The .prepare hook
> is defined to be non-atomic context, and so that we call sleep function
> in there.  We did everything right.  Why are we getting the warning?  If
> I'm correct, this warning only happens on i.MX7D.  Why is that?
>

Why Stefan's patch works (checking irqs_disabled()) is because during kernel
time init, the irq is still not enabled. It fixes the issue indirectly.
See:
asmlinkage __visible void __init start_kernel(void)
{
/*
 * Set up the scheduler prior starting any interrupts (such as the
 * timer interrupt). Full topology setup happens at smp_init()
 * time - but meanwhile we still have a functioning scheduler.
 */
sched_init();
.
time_init();
..
WARN(!irqs_disabled(), "Interrupts were enabled early\n");
early_boot_irqs_disabled = false;
local_irq_enable();
}

The issue can only happen when PLL enable causes a schedule during
imx_clock_init().
Not all PLL has this issue.
The issue happens on MX7D pll_audio_main_clk/pll_video_main_clk
which requires more delay time and cause usleep.
Because clk framework does not support MX7D clock types (operation requires
parents on), we simply enable all clocks in imx7d_clocks_init().

If apply my this patch series:
https://lkml.org/lkml/2016/4/20/199
The issue can also be gone.

Regards
Dong Aisheng

> Shawn

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

[Fabio Estevam]

On Tue, Apr 26, 2016 at 11:45 PM, Dong Aisheng  wrote:

>> We need to firstly understand why this is happening.  The .prepare hook
>> is defined to be non-atomic context, and so that we call sleep function
>> in there.  We did everything right.  Why are we getting the warning?  If
>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>
>
> This is mainly caused by during kernel early booting, there's only one init 
> idle
> task running.
> See:
> void __init sched_init(void)
> {
> .
> /*
>  * Make us the idle thread. Technically, schedule() should not be
>  * called from this thread, however somewhere below it might be,
>  * but because we are the idle thread, we just pick up running again
>  * when this runqueue becomes "idle".
>  */
> init_idle(current, smp_processor_id());
> ...
> }
>
> And the idle sched class indicates it's not valid to schedule for idle task.
> const struct sched_class idle_sched_class = {
> /* .next is NULL */
> /* no enqueue/yield_task for idle tasks */
>
> /* dequeue is not valid, we print a debug message there: */
> .dequeue_task   = dequeue_task_idle,
> ...
>
> }
>
> /*
>  * It is not legal to sleep in the idle task - print a warning
>  * message if some code attempts to do it:
>  */
> static void
> dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
> {
> raw_spin_unlock_irq(>lock);
> printk(KERN_ERR "bad: scheduling from the idle thread!\n");
> dump_stack();
> raw_spin_lock_irq(>lock);
> }
>
>
> Below is the full log of imx7d booting FYI.

This does not answer Shawn's question: why do we see this only on mx7d?

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

[Fabio Estevam]

On Tue, Apr 26, 2016 at 11:45 PM, Dong Aisheng  wrote:

>> We need to firstly understand why this is happening.  The .prepare hook
>> is defined to be non-atomic context, and so that we call sleep function
>> in there.  We did everything right.  Why are we getting the warning?  If
>> I'm correct, this warning only happens on i.MX7D.  Why is that?
>>
>
> This is mainly caused by during kernel early booting, there's only one init 
> idle
> task running.
> See:
> void __init sched_init(void)
> {
> .
> /*
>  * Make us the idle thread. Technically, schedule() should not be
>  * called from this thread, however somewhere below it might be,
>  * but because we are the idle thread, we just pick up running again
>  * when this runqueue becomes "idle".
>  */
> init_idle(current, smp_processor_id());
> ...
> }
>
> And the idle sched class indicates it's not valid to schedule for idle task.
> const struct sched_class idle_sched_class = {
> /* .next is NULL */
> /* no enqueue/yield_task for idle tasks */
>
> /* dequeue is not valid, we print a debug message there: */
> .dequeue_task   = dequeue_task_idle,
> ...
>
> }
>
> /*
>  * It is not legal to sleep in the idle task - print a warning
>  * message if some code attempts to do it:
>  */
> static void
> dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
> {
> raw_spin_unlock_irq(>lock);
> printk(KERN_ERR "bad: scheduling from the idle thread!\n");
> dump_stack();
> raw_spin_lock_irq(>lock);
> }
>
>
> Below is the full log of imx7d booting FYI.

This does not answer Shawn's question: why do we see this only on mx7d?

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> Shawn,
>> What's your suggestion?
>
> I think this needs more discussion, and I just dropped Stefan's patch
> from my tree.
>
> We need to firstly understand why this is happening.  The .prepare hook
> is defined to be non-atomic context, and so that we call sleep function
> in there.  We did everything right.  Why are we getting the warning?  If
> I'm correct, this warning only happens on i.MX7D.  Why is that?
>

This is mainly caused by during kernel early booting, there's only one init idle
task running.
See:
void __init sched_init(void)
{
.
/*
 * Make us the idle thread. Technically, schedule() should not be
 * called from this thread, however somewhere below it might be,
 * but because we are the idle thread, we just pick up running again
 * when this runqueue becomes "idle".
 */
init_idle(current, smp_processor_id());
...
}

And the idle sched class indicates it's not valid to schedule for idle task.
const struct sched_class idle_sched_class = {
/* .next is NULL */
/* no enqueue/yield_task for idle tasks */

/* dequeue is not valid, we print a debug message there: */
.dequeue_task   = dequeue_task_idle,
...

}

/*
 * It is not legal to sleep in the idle task - print a warning
 * message if some code attempts to do it:
 */
static void
dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
{
raw_spin_unlock_irq(>lock);
printk(KERN_ERR "bad: scheduling from the idle thread!\n");
dump_stack();
raw_spin_lock_irq(>lock);
}


Below is the full log of imx7d booting FYI.

You can ignore the first two warning (releasing a pinned lock) which
is a side affection
of idle task schedule during booting.

[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.6.0-rc1-00011-gee55b3d17805-dirty
(b29396@shlinux2) (gcc version 4.9.1 (GCC) ) #779 SMP Tue Apr 26
18:17:26 CST 2016
[0.00] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=10c5387d
[0.00] CPU: div instructions available: patching division code
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[0.00] Machine model: Freescale i.MX7 SabreSD Board
[0.00] cma: Reserved 16 MiB at 0xbf00
[0.00] Memory policy: Data cache writealloc
[0.00] On node 0 totalpages: 262144
[0.00] free_area_init_node: node 0, pgdat c0d72900,
node_mem_map ef7f9000
[0.00]   Normal zone: 1536 pages used for memmap
[0.00]   Normal zone: 0 pages reserved
[0.00]   Normal zone: 196608 pages, LIFO batch:31
[0.00]   HighMem zone: 65536 pages, LIFO batch:15
[0.00] percpu: Embedded 13 pages/cpu @ef7bd000 s20544 r8192
d24512 u53248
[0.00] pcpu-alloc: s20544 r8192 d24512 u53248 alloc=13*4096
[0.00] pcpu-alloc: [0] 0 [0] 1
[0.00] Built 1 zonelists in Zone order, mobility grouping on.
Total pages: 260608
[0.00] Kernel command line: console=ttymxc0,115200
root=/dev/nfs ip=dhcp
nfsroot=10.192.224.44:/data/rootfs_home/b29396/rootfs-yocto-L4.1.X-RC5,v3,tcp
log_buf_len=2M
[0.00] log_buf_len: 2097152 bytes
[0.00] early log buf free: 260724(99%)
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 999884K/1048576K available (8196K kernel code,
464K rwdata, 2828K rodata, 1024K init, 8223K bss, 32308K reserved,
16384K cma-reserved, 245760K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector  : 0x - 0x1000   (   4 kB)
[0.00] fixmap  : 0xffc0 - 0xfff0   (3072 kB)
[0.00] vmalloc : 0xf080 - 0xff80   ( 240 MB)
[0.00] lowmem  : 0xc000 - 0xf000   ( 768 MB)
[0.00] pkmap   : 0xbfe0 - 0xc000   (   2 MB)
[0.00] modules : 0xbf00 - 0xbfe0   (  14 MB)
[0.00]   .text : 0xc0008000 - 0xc0bc406c   (12017 kB)
[0.00]   .init : 0xc0c0 - 0xc0d0   (1024 kB)
[0.00]   .data : 0xc0d0 - 0xc0d74100   ( 465 kB)
[0.00].bss : 0xc0d76000 - 0xc157dfa0   (8224 kB)
[0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[0.00] Running RCU self tests
[0.00] Hierarchical RCU implementation.
[0.00]  RCU lockdep checking is enabled.
[0.00]  Build-time adjustment of leaf fanout to 32.
[0.00]  RCU restricting CPUs from NR_CPUS=4 to nr_cpu_ids=2.
[0.00] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] [ cut here 

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

[Dong Aisheng]

On Wed, Apr 27, 2016 at 9:58 AM, Shawn Guo  wrote:
> On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
>> Shawn,
>> What's your suggestion?
>
> I think this needs more discussion, and I just dropped Stefan's patch
> from my tree.
>
> We need to firstly understand why this is happening.  The .prepare hook
> is defined to be non-atomic context, and so that we call sleep function
> in there.  We did everything right.  Why are we getting the warning?  If
> I'm correct, this warning only happens on i.MX7D.  Why is that?
>

This is mainly caused by during kernel early booting, there's only one init idle
task running.
See:
void __init sched_init(void)
{
.
/*
 * Make us the idle thread. Technically, schedule() should not be
 * called from this thread, however somewhere below it might be,
 * but because we are the idle thread, we just pick up running again
 * when this runqueue becomes "idle".
 */
init_idle(current, smp_processor_id());
...
}

And the idle sched class indicates it's not valid to schedule for idle task.
const struct sched_class idle_sched_class = {
/* .next is NULL */
/* no enqueue/yield_task for idle tasks */

/* dequeue is not valid, we print a debug message there: */
.dequeue_task   = dequeue_task_idle,
...

}

/*
 * It is not legal to sleep in the idle task - print a warning
 * message if some code attempts to do it:
 */
static void
dequeue_task_idle(struct rq *rq, struct task_struct *p, int flags)
{
raw_spin_unlock_irq(>lock);
printk(KERN_ERR "bad: scheduling from the idle thread!\n");
dump_stack();
raw_spin_lock_irq(>lock);
}


Below is the full log of imx7d booting FYI.

You can ignore the first two warning (releasing a pinned lock) which
is a side affection
of idle task schedule during booting.

[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.6.0-rc1-00011-gee55b3d17805-dirty
(b29396@shlinux2) (gcc version 4.9.1 (GCC) ) #779 SMP Tue Apr 26
18:17:26 CST 2016
[0.00] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=10c5387d
[0.00] CPU: div instructions available: patching division code
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[0.00] Machine model: Freescale i.MX7 SabreSD Board
[0.00] cma: Reserved 16 MiB at 0xbf00
[0.00] Memory policy: Data cache writealloc
[0.00] On node 0 totalpages: 262144
[0.00] free_area_init_node: node 0, pgdat c0d72900,
node_mem_map ef7f9000
[0.00]   Normal zone: 1536 pages used for memmap
[0.00]   Normal zone: 0 pages reserved
[0.00]   Normal zone: 196608 pages, LIFO batch:31
[0.00]   HighMem zone: 65536 pages, LIFO batch:15
[0.00] percpu: Embedded 13 pages/cpu @ef7bd000 s20544 r8192
d24512 u53248
[0.00] pcpu-alloc: s20544 r8192 d24512 u53248 alloc=13*4096
[0.00] pcpu-alloc: [0] 0 [0] 1
[0.00] Built 1 zonelists in Zone order, mobility grouping on.
Total pages: 260608
[0.00] Kernel command line: console=ttymxc0,115200
root=/dev/nfs ip=dhcp
nfsroot=10.192.224.44:/data/rootfs_home/b29396/rootfs-yocto-L4.1.X-RC5,v3,tcp
log_buf_len=2M
[0.00] log_buf_len: 2097152 bytes
[0.00] early log buf free: 260724(99%)
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 999884K/1048576K available (8196K kernel code,
464K rwdata, 2828K rodata, 1024K init, 8223K bss, 32308K reserved,
16384K cma-reserved, 245760K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector  : 0x - 0x1000   (   4 kB)
[0.00] fixmap  : 0xffc0 - 0xfff0   (3072 kB)
[0.00] vmalloc : 0xf080 - 0xff80   ( 240 MB)
[0.00] lowmem  : 0xc000 - 0xf000   ( 768 MB)
[0.00] pkmap   : 0xbfe0 - 0xc000   (   2 MB)
[0.00] modules : 0xbf00 - 0xbfe0   (  14 MB)
[0.00]   .text : 0xc0008000 - 0xc0bc406c   (12017 kB)
[0.00]   .init : 0xc0c0 - 0xc0d0   (1024 kB)
[0.00]   .data : 0xc0d0 - 0xc0d74100   ( 465 kB)
[0.00].bss : 0xc0d76000 - 0xc157dfa0   (8224 kB)
[0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[0.00] Running RCU self tests
[0.00] Hierarchical RCU implementation.
[0.00]  RCU lockdep checking is enabled.
[0.00]  Build-time adjustment of leaf fanout to 32.
[0.00]  RCU restricting CPUs from NR_CPUS=4 to nr_cpu_ids=2.
[0.00] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] [ cut here ]
[0.00] 

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

[Shawn Guo]

On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> Shawn,
> What's your suggestion?

I think this needs more discussion, and I just dropped Stefan's patch
from my tree.

We need to firstly understand why this is happening.  The .prepare hook
is defined to be non-atomic context, and so that we call sleep function
in there.  We did everything right.  Why are we getting the warning?  If
I'm correct, this warning only happens on i.MX7D.  Why is that?

Shawn

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

[Shawn Guo]

On Tue, Apr 26, 2016 at 07:27:03PM +0800, Dong Aisheng wrote:
> Shawn,
> What's your suggestion?

I think this needs more discussion, and I just dropped Stefan's patch
from my tree.

We need to firstly understand why this is happening.  The .prepare hook
is defined to be non-atomic context, and so that we call sleep function
in there.  We did everything right.  Why are we getting the warning?  If
I'm correct, this warning only happens on i.MX7D.  Why is that?

Shawn

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

[Stefan Agner]

On 2016-01-29 17:16, Stephen Boyd wrote:
> On 01/29, Stefan Agner wrote:
>> If a clock gets enabled early during boot time, it can lead to a PLL
>> startup. The wait_lock function makes sure that the PLL is really
>> stareted up before it gets used. However, the function sleeps which
>> leads to scheduling and an error:
>> bad: scheduling from the idle thread!
>> ...
> 
> Can you please share the full splat? I have no clue what's going
> on.

Finally, full splat:

...
[0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2,
Nodes=1
[0.00] Running RCU self tests
[0.00] Hierarchical RCU implementation.
[0.00]  RCU lockdep checking is enabled.
[0.00]  Build-time adjustment of leaf fanout to 32.
[0.00]  RCU restricting CPUs from NR_CPUS=4 to nr_cpu_ids=2.
[0.00] RCU: Adjusting geometry for rcu_fanout_leaf=32,
nr_cpu_ids=2
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] [ cut here ]
[0.00] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:3407
lock_release+0x398/0x3a0()
[0.00] releasing a pinned lock
[0.00] Modules linked in:
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.5.0-rc1-00013-gdb45d67 #40
[0.00] Hardware name: Freescale i.MX7 Dual (Device Tree)
[0.00] Backtrace:
[0.00] [] (dump_backtrace) from []
(show_stack+0x18/0x1c)
[0.00]  r7:c006f8f8 r6:0d4f r5: r4:c0ad541c
[0.00] [] (show_stack) from []
(dump_stack+0x80/0x90)
[0.00] [] (dump_stack) from []
(warn_slowpath_common+0x88/0xb8)
[0.00]  r5:0009 r4:c0abdc58
[0.00] [] (warn_slowpath_common) from []
(warn_slowpath_fmt+0x38/0x40)
[0.00]  r8:0002 r7:0001 r6:cedc3fd0 r5:c0ac24e0
r4:c0984048
[0.00] [] (warn_slowpath_fmt) from []
(lock_release+0x398/0x3a0)
[0.00]  r3:fff0 r2:c0984048
[0.00]  r4:c0ac2500
[0.00] [] (lock_release) from []
(_raw_spin_unlock_irq+0x20/0x34)
[0.00]  r10: r9:c0ac2384 r8:cedc3fd0 r7:c0abe6d0
r6:0001 r5:cedc3fc0
[0.00]  r4:cedc3fc0
[0.00] [] (_raw_spin_unlock_irq) from []
(dequeue_task_idle+0x14/0x30)
[0.00]  r5:cedc3fc0 r4:cedc3fc0
[0.00] [] (dequeue_task_idle) from []
(deactivate_task+0x64/0x68)
[0.00]  r5:cedc3fc0 r4:c0ac2040
[0.00] [] (deactivate_task) from []
(__schedule+0x29c/0x67c)
[0.00]  r7:c0abe6d0 r6:c0abafc0 r5:c0ac2040 r4:cedc3fc0
[0.00] [] (__schedule) from []
(schedule+0x48/0xa0)
[0.00]  r10:c0b0d91c r9:0036 r8:c1331c3c r7:
r6:0006ddd0 r5:c0abde08
[0.00]  r4:c0abc000
[0.00] [] (schedule) from []
(schedule_hrtimeout_range_clock+0xbc/0x130)
[0.00]  r5:c0abde08 r4:0001
[0.00] [] (schedule_hrtimeout_range_clock) from
[] (schedule_hrtimeout_range+0x14/0x18)
[0.00]  r7:0003 r6:8ad1 r5:ce804fc0 r4:c0abe100
[0.00] [] (schedule_hrtimeout_range) from []
(usleep_range+0x64/0x6c)
[0.00] [] (usleep_range) from []
(clk_pllv3_wait_lock+0x80/0xbc)
[0.00] [] (clk_pllv3_wait_lock) from []
(clk_pllv3_prepare+0x2c/0x30)
[0.00]  r7:0003 r6:d0864490 r5:c1331c3c r4:ce807680
[0.00] [] (clk_pllv3_prepare) from []
(clk_core_prepare+0xa0/0xc4)
[0.00] [] (clk_core_prepare) from []
(clk_prepare+0x20/0x38)
[0.00]  r5:c1331c3c r4:ce80c040
[0.00] [] (clk_prepare) from []
(imx7d_clocks_init+0x5ee0/0x5f6c)
[0.00]  r5:c1331c3c r4:ce80c040
[0.00] [] (imx7d_clocks_init) from []
(of_clk_init+0x148/0x1d8)
[0.00]  r10:cede50b4 r9:0003 r8:0001 r7:c0abdf60
r6:c0abdf68 r5:
[0.00]  r4:ce8043c0
[0.00] [] (of_clk_init) from []
(time_init+0x30/0x38)
[0.00]  r10:cefffb80 r9:c0aa6a48 r8:c0b24000 r7:
r6:c0abe4c0 r5:c0b24000
[0.00]  r4:
[0.00] [] (time_init) from []
(start_kernel+0x2b4/0x3ec)
[0.00] [] (start_kernel) from [<8000807c>]
(0x8000807c)
[0.00]  r10: r9:410fc075 r8:8000406a r7:c0ac39b4
r6:c0aa6a44 r5:c0abe540
[0.00]  r4:c0b24294
[0.00] ---[ end trace cb88537fdc8fa200 ]---
[0.00] [ cut here ]
[0.00] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2601
trace_hardirqs_on_caller+0x1e8/0x1fc()
[0.00] DEBUG_LOCKS_WARN_ON(unlikely(early_boot_irqs_disabled))
[0.00] Modules linked in:
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Tainted: GW  
4.5.0-rc1-00013-gdb45d67 #40
[0.00] Hardware name: Freescale i.MX7 Dual (Device Tree)   
[0.00] Backtrace:
[0.00] [] (dump_backtrace) from []
(show_stack+0x18/0x1c)
[0.00]  r7:c006cb24 r6:0a29 r5: r4:c0ad541c
[0.00] [] (show_stack) from []
(dump_stack+0x80/0x90)
[0.00] [] (dump_stack) from []
(warn_slowpath_common+0x88/0xb8)
[0.00]  r5:0009 r4:c0abdc88
[0.00] [] 

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

[Stefan Agner]

On 2016-01-29 17:16, Stephen Boyd wrote:
> On 01/29, Stefan Agner wrote:
>> If a clock gets enabled early during boot time, it can lead to a PLL
>> startup. The wait_lock function makes sure that the PLL is really
>> stareted up before it gets used. However, the function sleeps which
>> leads to scheduling and an error:
>> bad: scheduling from the idle thread!
>> ...
> 
> Can you please share the full splat? I have no clue what's going
> on.

Finally, full splat:

...
[0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2,
Nodes=1
[0.00] Running RCU self tests
[0.00] Hierarchical RCU implementation.
[0.00]  RCU lockdep checking is enabled.
[0.00]  Build-time adjustment of leaf fanout to 32.
[0.00]  RCU restricting CPUs from NR_CPUS=4 to nr_cpu_ids=2.
[0.00] RCU: Adjusting geometry for rcu_fanout_leaf=32,
nr_cpu_ids=2
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] [ cut here ]
[0.00] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:3407
lock_release+0x398/0x3a0()
[0.00] releasing a pinned lock
[0.00] Modules linked in:
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.5.0-rc1-00013-gdb45d67 #40
[0.00] Hardware name: Freescale i.MX7 Dual (Device Tree)
[0.00] Backtrace:
[0.00] [] (dump_backtrace) from []
(show_stack+0x18/0x1c)
[0.00]  r7:c006f8f8 r6:0d4f r5: r4:c0ad541c
[0.00] [] (show_stack) from []
(dump_stack+0x80/0x90)
[0.00] [] (dump_stack) from []
(warn_slowpath_common+0x88/0xb8)
[0.00]  r5:0009 r4:c0abdc58
[0.00] [] (warn_slowpath_common) from []
(warn_slowpath_fmt+0x38/0x40)
[0.00]  r8:0002 r7:0001 r6:cedc3fd0 r5:c0ac24e0
r4:c0984048
[0.00] [] (warn_slowpath_fmt) from []
(lock_release+0x398/0x3a0)
[0.00]  r3:fff0 r2:c0984048
[0.00]  r4:c0ac2500
[0.00] [] (lock_release) from []
(_raw_spin_unlock_irq+0x20/0x34)
[0.00]  r10: r9:c0ac2384 r8:cedc3fd0 r7:c0abe6d0
r6:0001 r5:cedc3fc0
[0.00]  r4:cedc3fc0
[0.00] [] (_raw_spin_unlock_irq) from []
(dequeue_task_idle+0x14/0x30)
[0.00]  r5:cedc3fc0 r4:cedc3fc0
[0.00] [] (dequeue_task_idle) from []
(deactivate_task+0x64/0x68)
[0.00]  r5:cedc3fc0 r4:c0ac2040
[0.00] [] (deactivate_task) from []
(__schedule+0x29c/0x67c)
[0.00]  r7:c0abe6d0 r6:c0abafc0 r5:c0ac2040 r4:cedc3fc0
[0.00] [] (__schedule) from []
(schedule+0x48/0xa0)
[0.00]  r10:c0b0d91c r9:0036 r8:c1331c3c r7:
r6:0006ddd0 r5:c0abde08
[0.00]  r4:c0abc000
[0.00] [] (schedule) from []
(schedule_hrtimeout_range_clock+0xbc/0x130)
[0.00]  r5:c0abde08 r4:0001
[0.00] [] (schedule_hrtimeout_range_clock) from
[] (schedule_hrtimeout_range+0x14/0x18)
[0.00]  r7:0003 r6:8ad1 r5:ce804fc0 r4:c0abe100
[0.00] [] (schedule_hrtimeout_range) from []
(usleep_range+0x64/0x6c)
[0.00] [] (usleep_range) from []
(clk_pllv3_wait_lock+0x80/0xbc)
[0.00] [] (clk_pllv3_wait_lock) from []
(clk_pllv3_prepare+0x2c/0x30)
[0.00]  r7:0003 r6:d0864490 r5:c1331c3c r4:ce807680
[0.00] [] (clk_pllv3_prepare) from []
(clk_core_prepare+0xa0/0xc4)
[0.00] [] (clk_core_prepare) from []
(clk_prepare+0x20/0x38)
[0.00]  r5:c1331c3c r4:ce80c040
[0.00] [] (clk_prepare) from []
(imx7d_clocks_init+0x5ee0/0x5f6c)
[0.00]  r5:c1331c3c r4:ce80c040
[0.00] [] (imx7d_clocks_init) from []
(of_clk_init+0x148/0x1d8)
[0.00]  r10:cede50b4 r9:0003 r8:0001 r7:c0abdf60
r6:c0abdf68 r5:
[0.00]  r4:ce8043c0
[0.00] [] (of_clk_init) from []
(time_init+0x30/0x38)
[0.00]  r10:cefffb80 r9:c0aa6a48 r8:c0b24000 r7:
r6:c0abe4c0 r5:c0b24000
[0.00]  r4:
[0.00] [] (time_init) from []
(start_kernel+0x2b4/0x3ec)
[0.00] [] (start_kernel) from [<8000807c>]
(0x8000807c)
[0.00]  r10: r9:410fc075 r8:8000406a r7:c0ac39b4
r6:c0aa6a44 r5:c0abe540
[0.00]  r4:c0b24294
[0.00] ---[ end trace cb88537fdc8fa200 ]---
[0.00] [ cut here ]
[0.00] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2601
trace_hardirqs_on_caller+0x1e8/0x1fc()
[0.00] DEBUG_LOCKS_WARN_ON(unlikely(early_boot_irqs_disabled))
[0.00] Modules linked in:
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Tainted: GW  
4.5.0-rc1-00013-gdb45d67 #40
[0.00] Hardware name: Freescale i.MX7 Dual (Device Tree)   
[0.00] Backtrace:
[0.00] [] (dump_backtrace) from []
(show_stack+0x18/0x1c)
[0.00]  r7:c006cb24 r6:0a29 r5: r4:c0ad541c
[0.00] [] (show_stack) from []
(dump_stack+0x80/0x90)
[0.00] [] (dump_stack) from []
(warn_slowpath_common+0x88/0xb8)
[0.00]  r5:0009 r4:c0abdc88
[0.00] [] 

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

[Dong Aisheng]

On Tue, Apr 26, 2016 at 7:16 PM, Dong Aisheng  wrote:
> On Tue, Apr 26, 2016 at 5:31 PM, Lucas Stach  wrote:
>> Am Dienstag, den 26.04.2016, 13:51 +0800 schrieb Dong Aisheng:
>>> Hi Shawn,
>>>
>>> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
>>> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
>>> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
>>> >> > If a clock gets enabled early during boot time, it can lead to a PLL
>>> >> > startup. The wait_lock function makes sure that the PLL is really
>>> >> > stareted up before it gets used. However, the function sleeps which
>>> >> > leads to scheduling and an error:
>>> >> > bad: scheduling from the idle thread!
>>> >> > ...
>>> >> >
>>> >> > Use udelay in case IRQ's are still disabled.
>>> >> >
>>> >> > Signed-off-by: Stefan Agner 
>>> >> > ---
>>> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
>>> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>>> >> >
>>> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
>>> >> > index c05c43d..b5ff561 100644
>>> >> > --- a/drivers/clk/imx/clk-pllv3.c
>>> >> > +++ b/drivers/clk/imx/clk-pllv3.c
>>> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 
>>> >> > *pll)
>>> >> > break;
>>> >> > if (time_after(jiffies, timeout))
>>> >> > break;
>>> >> > -   usleep_range(50, 500);
>>> >> > +   if (unlikely(irqs_disabled()))
>>> >>
>>> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
>>> >> But this line indicates it's possible to be called in irq context.
>>> >> Although it's only happened during kernel boot phase where irq is
>>> >> still not enabled.
>>> >> It seems schedule_debug() somehow did not catch it during early boot
>>> >> phase. Maybe schedule guys can help explain.
>>> >>
>>> >> My question is if it's really worthy to introduce this confusion
>>> >> to fix the issue since the delay is minor?
>>> >
>>> > I do not understand why it's confusing.  The code already obviously
>>> > indicates this is a special handling for cases where irq is still not
>>> > enabled, rather than for irq context.
>>> >
>>>
>>> The code itself has nothing telling it's a special handling for the
>>> case where irq is
>>> still not enabled.
>>> Even it tells, it may still cause confusing by adding complexity in
>>> clk_pllv3_prepare()
>>> which actually should be called in non-atomic context as it could sleep.
>>>
>>> > The patch is to fix the "bad: scheduling from the idle thread!" warning
>>> > rather than minimize the delay.  Do you have an opinion on how to fix
>>> > the warning?
>>> >
>>>
>>> I just wonder maybe we could simply just using udelay(50) instead of
>>> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
>>> What do you think of it?
>>
>> Why should we needless burn CPU time in the likely case of
>> clk_pllv3_prepare() being called in sleepable context?
>>
>
> I think because the delay time is not big.
> And pll clks are system basic clocks and less likely to be frequently
> enabled/disabled.
>
>> Using udelay() in a sleepable context is even more confusing than having
>> the special case for clk_prepare() being called in atomic context IMHO.
>>
>
> I can't agree having special case by checking
> unlikely(irqs_disabled()) is better
> which is obviously more confusing IMHO.
> I'd more like to hide it from users.
>
> I see other platforms like samsung also implements pll using udelay.
> But difference is that they implement it in .enable(I) clalback not prepare.
>
> How about simply remove usleep_range to poll instead of using udelay?
> Cause most PLL enable may be more faster than 50ns.
>
> And according to kernel doc, for delay time less than 10ns,
> udelay or polling is recommended.
>

Shawn,
What's your suggestion?

Regards
Dong Aisheng

>> Regards,
>> Lucas
>>
>
> Regards
> Dong Aisheng

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

[Dong Aisheng]

On Tue, Apr 26, 2016 at 7:16 PM, Dong Aisheng  wrote:
> On Tue, Apr 26, 2016 at 5:31 PM, Lucas Stach  wrote:
>> Am Dienstag, den 26.04.2016, 13:51 +0800 schrieb Dong Aisheng:
>>> Hi Shawn,
>>>
>>> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
>>> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
>>> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
>>> >> > If a clock gets enabled early during boot time, it can lead to a PLL
>>> >> > startup. The wait_lock function makes sure that the PLL is really
>>> >> > stareted up before it gets used. However, the function sleeps which
>>> >> > leads to scheduling and an error:
>>> >> > bad: scheduling from the idle thread!
>>> >> > ...
>>> >> >
>>> >> > Use udelay in case IRQ's are still disabled.
>>> >> >
>>> >> > Signed-off-by: Stefan Agner 
>>> >> > ---
>>> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
>>> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>>> >> >
>>> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
>>> >> > index c05c43d..b5ff561 100644
>>> >> > --- a/drivers/clk/imx/clk-pllv3.c
>>> >> > +++ b/drivers/clk/imx/clk-pllv3.c
>>> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 
>>> >> > *pll)
>>> >> > break;
>>> >> > if (time_after(jiffies, timeout))
>>> >> > break;
>>> >> > -   usleep_range(50, 500);
>>> >> > +   if (unlikely(irqs_disabled()))
>>> >>
>>> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
>>> >> But this line indicates it's possible to be called in irq context.
>>> >> Although it's only happened during kernel boot phase where irq is
>>> >> still not enabled.
>>> >> It seems schedule_debug() somehow did not catch it during early boot
>>> >> phase. Maybe schedule guys can help explain.
>>> >>
>>> >> My question is if it's really worthy to introduce this confusion
>>> >> to fix the issue since the delay is minor?
>>> >
>>> > I do not understand why it's confusing.  The code already obviously
>>> > indicates this is a special handling for cases where irq is still not
>>> > enabled, rather than for irq context.
>>> >
>>>
>>> The code itself has nothing telling it's a special handling for the
>>> case where irq is
>>> still not enabled.
>>> Even it tells, it may still cause confusing by adding complexity in
>>> clk_pllv3_prepare()
>>> which actually should be called in non-atomic context as it could sleep.
>>>
>>> > The patch is to fix the "bad: scheduling from the idle thread!" warning
>>> > rather than minimize the delay.  Do you have an opinion on how to fix
>>> > the warning?
>>> >
>>>
>>> I just wonder maybe we could simply just using udelay(50) instead of
>>> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
>>> What do you think of it?
>>
>> Why should we needless burn CPU time in the likely case of
>> clk_pllv3_prepare() being called in sleepable context?
>>
>
> I think because the delay time is not big.
> And pll clks are system basic clocks and less likely to be frequently
> enabled/disabled.
>
>> Using udelay() in a sleepable context is even more confusing than having
>> the special case for clk_prepare() being called in atomic context IMHO.
>>
>
> I can't agree having special case by checking
> unlikely(irqs_disabled()) is better
> which is obviously more confusing IMHO.
> I'd more like to hide it from users.
>
> I see other platforms like samsung also implements pll using udelay.
> But difference is that they implement it in .enable(I) clalback not prepare.
>
> How about simply remove usleep_range to poll instead of using udelay?
> Cause most PLL enable may be more faster than 50ns.
>
> And according to kernel doc, for delay time less than 10ns,
> udelay or polling is recommended.
>

Shawn,
What's your suggestion?

Regards
Dong Aisheng

>> Regards,
>> Lucas
>>
>
> Regards
> Dong Aisheng

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

[Dong Aisheng]

On Tue, Apr 26, 2016 at 5:31 PM, Lucas Stach  wrote:
> Am Dienstag, den 26.04.2016, 13:51 +0800 schrieb Dong Aisheng:
>> Hi Shawn,
>>
>> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
>> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
>> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
>> >> > If a clock gets enabled early during boot time, it can lead to a PLL
>> >> > startup. The wait_lock function makes sure that the PLL is really
>> >> > stareted up before it gets used. However, the function sleeps which
>> >> > leads to scheduling and an error:
>> >> > bad: scheduling from the idle thread!
>> >> > ...
>> >> >
>> >> > Use udelay in case IRQ's are still disabled.
>> >> >
>> >> > Signed-off-by: Stefan Agner 
>> >> > ---
>> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
>> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
>> >> > index c05c43d..b5ff561 100644
>> >> > --- a/drivers/clk/imx/clk-pllv3.c
>> >> > +++ b/drivers/clk/imx/clk-pllv3.c
>> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
>> >> > break;
>> >> > if (time_after(jiffies, timeout))
>> >> > break;
>> >> > -   usleep_range(50, 500);
>> >> > +   if (unlikely(irqs_disabled()))
>> >>
>> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
>> >> But this line indicates it's possible to be called in irq context.
>> >> Although it's only happened during kernel boot phase where irq is
>> >> still not enabled.
>> >> It seems schedule_debug() somehow did not catch it during early boot
>> >> phase. Maybe schedule guys can help explain.
>> >>
>> >> My question is if it's really worthy to introduce this confusion
>> >> to fix the issue since the delay is minor?
>> >
>> > I do not understand why it's confusing.  The code already obviously
>> > indicates this is a special handling for cases where irq is still not
>> > enabled, rather than for irq context.
>> >
>>
>> The code itself has nothing telling it's a special handling for the
>> case where irq is
>> still not enabled.
>> Even it tells, it may still cause confusing by adding complexity in
>> clk_pllv3_prepare()
>> which actually should be called in non-atomic context as it could sleep.
>>
>> > The patch is to fix the "bad: scheduling from the idle thread!" warning
>> > rather than minimize the delay.  Do you have an opinion on how to fix
>> > the warning?
>> >
>>
>> I just wonder maybe we could simply just using udelay(50) instead of
>> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
>> What do you think of it?
>
> Why should we needless burn CPU time in the likely case of
> clk_pllv3_prepare() being called in sleepable context?
>

I think because the delay time is not big.
And pll clks are system basic clocks and less likely to be frequently
enabled/disabled.

> Using udelay() in a sleepable context is even more confusing than having
> the special case for clk_prepare() being called in atomic context IMHO.
>

I can't agree having special case by checking
unlikely(irqs_disabled()) is better
which is obviously more confusing IMHO.
I'd more like to hide it from users.

I see other platforms like samsung also implements pll using udelay.
But difference is that they implement it in .enable(I) clalback not prepare.

How about simply remove usleep_range to poll instead of using udelay?
Cause most PLL enable may be more faster than 50ns.

And according to kernel doc, for delay time less than 10ns,
udelay or polling is recommended.

> Regards,
> Lucas
>

Regards
Dong Aisheng

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

[Dong Aisheng]

On Tue, Apr 26, 2016 at 5:31 PM, Lucas Stach  wrote:
> Am Dienstag, den 26.04.2016, 13:51 +0800 schrieb Dong Aisheng:
>> Hi Shawn,
>>
>> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
>> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
>> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
>> >> > If a clock gets enabled early during boot time, it can lead to a PLL
>> >> > startup. The wait_lock function makes sure that the PLL is really
>> >> > stareted up before it gets used. However, the function sleeps which
>> >> > leads to scheduling and an error:
>> >> > bad: scheduling from the idle thread!
>> >> > ...
>> >> >
>> >> > Use udelay in case IRQ's are still disabled.
>> >> >
>> >> > Signed-off-by: Stefan Agner 
>> >> > ---
>> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
>> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
>> >> > index c05c43d..b5ff561 100644
>> >> > --- a/drivers/clk/imx/clk-pllv3.c
>> >> > +++ b/drivers/clk/imx/clk-pllv3.c
>> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
>> >> > break;
>> >> > if (time_after(jiffies, timeout))
>> >> > break;
>> >> > -   usleep_range(50, 500);
>> >> > +   if (unlikely(irqs_disabled()))
>> >>
>> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
>> >> But this line indicates it's possible to be called in irq context.
>> >> Although it's only happened during kernel boot phase where irq is
>> >> still not enabled.
>> >> It seems schedule_debug() somehow did not catch it during early boot
>> >> phase. Maybe schedule guys can help explain.
>> >>
>> >> My question is if it's really worthy to introduce this confusion
>> >> to fix the issue since the delay is minor?
>> >
>> > I do not understand why it's confusing.  The code already obviously
>> > indicates this is a special handling for cases where irq is still not
>> > enabled, rather than for irq context.
>> >
>>
>> The code itself has nothing telling it's a special handling for the
>> case where irq is
>> still not enabled.
>> Even it tells, it may still cause confusing by adding complexity in
>> clk_pllv3_prepare()
>> which actually should be called in non-atomic context as it could sleep.
>>
>> > The patch is to fix the "bad: scheduling from the idle thread!" warning
>> > rather than minimize the delay.  Do you have an opinion on how to fix
>> > the warning?
>> >
>>
>> I just wonder maybe we could simply just using udelay(50) instead of
>> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
>> What do you think of it?
>
> Why should we needless burn CPU time in the likely case of
> clk_pllv3_prepare() being called in sleepable context?
>

I think because the delay time is not big.
And pll clks are system basic clocks and less likely to be frequently
enabled/disabled.

> Using udelay() in a sleepable context is even more confusing than having
> the special case for clk_prepare() being called in atomic context IMHO.
>

I can't agree having special case by checking
unlikely(irqs_disabled()) is better
which is obviously more confusing IMHO.
I'd more like to hide it from users.

I see other platforms like samsung also implements pll using udelay.
But difference is that they implement it in .enable(I) clalback not prepare.

How about simply remove usleep_range to poll instead of using udelay?
Cause most PLL enable may be more faster than 50ns.

And according to kernel doc, for delay time less than 10ns,
udelay or polling is recommended.

> Regards,
> Lucas
>

Regards
Dong Aisheng

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

[Lucas Stach]

Am Dienstag, den 26.04.2016, 13:51 +0800 schrieb Dong Aisheng:
> Hi Shawn,
> 
> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> >> > If a clock gets enabled early during boot time, it can lead to a PLL
> >> > startup. The wait_lock function makes sure that the PLL is really
> >> > stareted up before it gets used. However, the function sleeps which
> >> > leads to scheduling and an error:
> >> > bad: scheduling from the idle thread!
> >> > ...
> >> >
> >> > Use udelay in case IRQ's are still disabled.
> >> >
> >> > Signed-off-by: Stefan Agner 
> >> > ---
> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> >> > index c05c43d..b5ff561 100644
> >> > --- a/drivers/clk/imx/clk-pllv3.c
> >> > +++ b/drivers/clk/imx/clk-pllv3.c
> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
> >> > break;
> >> > if (time_after(jiffies, timeout))
> >> > break;
> >> > -   usleep_range(50, 500);
> >> > +   if (unlikely(irqs_disabled()))
> >>
> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
> >> But this line indicates it's possible to be called in irq context.
> >> Although it's only happened during kernel boot phase where irq is
> >> still not enabled.
> >> It seems schedule_debug() somehow did not catch it during early boot
> >> phase. Maybe schedule guys can help explain.
> >>
> >> My question is if it's really worthy to introduce this confusion
> >> to fix the issue since the delay is minor?
> >
> > I do not understand why it's confusing.  The code already obviously
> > indicates this is a special handling for cases where irq is still not
> > enabled, rather than for irq context.
> >
> 
> The code itself has nothing telling it's a special handling for the
> case where irq is
> still not enabled.
> Even it tells, it may still cause confusing by adding complexity in
> clk_pllv3_prepare()
> which actually should be called in non-atomic context as it could sleep.
> 
> > The patch is to fix the "bad: scheduling from the idle thread!" warning
> > rather than minimize the delay.  Do you have an opinion on how to fix
> > the warning?
> >
> 
> I just wonder maybe we could simply just using udelay(50) instead of
> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
> What do you think of it?

Why should we needless burn CPU time in the likely case of
clk_pllv3_prepare() being called in sleepable context?

Using udelay() in a sleepable context is even more confusing than having
the special case for clk_prepare() being called in atomic context IMHO.

Regards,
Lucas

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

[Lucas Stach]

Am Dienstag, den 26.04.2016, 13:51 +0800 schrieb Dong Aisheng:
> Hi Shawn,
> 
> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> >> > If a clock gets enabled early during boot time, it can lead to a PLL
> >> > startup. The wait_lock function makes sure that the PLL is really
> >> > stareted up before it gets used. However, the function sleeps which
> >> > leads to scheduling and an error:
> >> > bad: scheduling from the idle thread!
> >> > ...
> >> >
> >> > Use udelay in case IRQ's are still disabled.
> >> >
> >> > Signed-off-by: Stefan Agner 
> >> > ---
> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> >> > index c05c43d..b5ff561 100644
> >> > --- a/drivers/clk/imx/clk-pllv3.c
> >> > +++ b/drivers/clk/imx/clk-pllv3.c
> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
> >> > break;
> >> > if (time_after(jiffies, timeout))
> >> > break;
> >> > -   usleep_range(50, 500);
> >> > +   if (unlikely(irqs_disabled()))
> >>
> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
> >> But this line indicates it's possible to be called in irq context.
> >> Although it's only happened during kernel boot phase where irq is
> >> still not enabled.
> >> It seems schedule_debug() somehow did not catch it during early boot
> >> phase. Maybe schedule guys can help explain.
> >>
> >> My question is if it's really worthy to introduce this confusion
> >> to fix the issue since the delay is minor?
> >
> > I do not understand why it's confusing.  The code already obviously
> > indicates this is a special handling for cases where irq is still not
> > enabled, rather than for irq context.
> >
> 
> The code itself has nothing telling it's a special handling for the
> case where irq is
> still not enabled.
> Even it tells, it may still cause confusing by adding complexity in
> clk_pllv3_prepare()
> which actually should be called in non-atomic context as it could sleep.
> 
> > The patch is to fix the "bad: scheduling from the idle thread!" warning
> > rather than minimize the delay.  Do you have an opinion on how to fix
> > the warning?
> >
> 
> I just wonder maybe we could simply just using udelay(50) instead of
> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
> What do you think of it?

Why should we needless burn CPU time in the likely case of
clk_pllv3_prepare() being called in sleepable context?

Using udelay() in a sleepable context is even more confusing than having
the special case for clk_prepare() being called in atomic context IMHO.

Regards,
Lucas

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

[Shawn Guo]

On Tue, Apr 26, 2016 at 01:51:13PM +0800, Dong Aisheng wrote:
> Hi Shawn,
> 
> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> >> > If a clock gets enabled early during boot time, it can lead to a PLL
> >> > startup. The wait_lock function makes sure that the PLL is really
> >> > stareted up before it gets used. However, the function sleeps which
> >> > leads to scheduling and an error:
> >> > bad: scheduling from the idle thread!
> >> > ...
> >> >
> >> > Use udelay in case IRQ's are still disabled.
> >> >
> >> > Signed-off-by: Stefan Agner 
> >> > ---
> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> >> > index c05c43d..b5ff561 100644
> >> > --- a/drivers/clk/imx/clk-pllv3.c
> >> > +++ b/drivers/clk/imx/clk-pllv3.c
> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
> >> > break;
> >> > if (time_after(jiffies, timeout))
> >> > break;
> >> > -   usleep_range(50, 500);
> >> > +   if (unlikely(irqs_disabled()))
> >>
> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
> >> But this line indicates it's possible to be called in irq context.
> >> Although it's only happened during kernel boot phase where irq is
> >> still not enabled.
> >> It seems schedule_debug() somehow did not catch it during early boot
> >> phase. Maybe schedule guys can help explain.
> >>
> >> My question is if it's really worthy to introduce this confusion
> >> to fix the issue since the delay is minor?
> >
> > I do not understand why it's confusing.  The code already obviously
> > indicates this is a special handling for cases where irq is still not
> > enabled, rather than for irq context.
> >
> 
> The code itself has nothing telling it's a special handling for the
> case where irq is
> still not enabled.

I think the following if-clause is telling that.

if (unlikely(irqs_disabled()))

> Even it tells, it may still cause confusing by adding complexity in
> clk_pllv3_prepare()
> which actually should be called in non-atomic context as it could sleep.

I agree with you on that.

> > The patch is to fix the "bad: scheduling from the idle thread!" warning
> > rather than minimize the delay.  Do you have an opinion on how to fix
> > the warning?
> >
> 
> I just wonder maybe we could simply just using udelay(50) instead of
> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
> What do you think of it?

I'm fine with it.  Since I haven't sent the patch to clk maintainers, I
could replace Stefan's patch with yours, if you can send me a patch
quickly.

Shawn

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

[Shawn Guo]

On Tue, Apr 26, 2016 at 01:51:13PM +0800, Dong Aisheng wrote:
> Hi Shawn,
> 
> On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
> > On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
> >> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> >> > If a clock gets enabled early during boot time, it can lead to a PLL
> >> > startup. The wait_lock function makes sure that the PLL is really
> >> > stareted up before it gets used. However, the function sleeps which
> >> > leads to scheduling and an error:
> >> > bad: scheduling from the idle thread!
> >> > ...
> >> >
> >> > Use udelay in case IRQ's are still disabled.
> >> >
> >> > Signed-off-by: Stefan Agner 
> >> > ---
> >> >  drivers/clk/imx/clk-pllv3.c | 5 -
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> >> > index c05c43d..b5ff561 100644
> >> > --- a/drivers/clk/imx/clk-pllv3.c
> >> > +++ b/drivers/clk/imx/clk-pllv3.c
> >> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
> >> > break;
> >> > if (time_after(jiffies, timeout))
> >> > break;
> >> > -   usleep_range(50, 500);
> >> > +   if (unlikely(irqs_disabled()))
> >>
> >> This causes a bit confusion that clk_pllv3_prepare is sleepable.
> >> But this line indicates it's possible to be called in irq context.
> >> Although it's only happened during kernel boot phase where irq is
> >> still not enabled.
> >> It seems schedule_debug() somehow did not catch it during early boot
> >> phase. Maybe schedule guys can help explain.
> >>
> >> My question is if it's really worthy to introduce this confusion
> >> to fix the issue since the delay is minor?
> >
> > I do not understand why it's confusing.  The code already obviously
> > indicates this is a special handling for cases where irq is still not
> > enabled, rather than for irq context.
> >
> 
> The code itself has nothing telling it's a special handling for the
> case where irq is
> still not enabled.

I think the following if-clause is telling that.

if (unlikely(irqs_disabled()))

> Even it tells, it may still cause confusing by adding complexity in
> clk_pllv3_prepare()
> which actually should be called in non-atomic context as it could sleep.

I agree with you on that.

> > The patch is to fix the "bad: scheduling from the idle thread!" warning
> > rather than minimize the delay.  Do you have an opinion on how to fix
> > the warning?
> >
> 
> I just wonder maybe we could simply just using udelay(50) instead of
> usleep_range(50, 500) to eliminate the confusing since it's minor cast.
> What do you think of it?

I'm fine with it.  Since I haven't sent the patch to clk maintainers, I
could replace Stefan's patch with yours, if you can send me a patch
quickly.

Shawn

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

[Dong Aisheng]

Hi Shawn,

On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
> On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
>> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
>> > If a clock gets enabled early during boot time, it can lead to a PLL
>> > startup. The wait_lock function makes sure that the PLL is really
>> > stareted up before it gets used. However, the function sleeps which
>> > leads to scheduling and an error:
>> > bad: scheduling from the idle thread!
>> > ...
>> >
>> > Use udelay in case IRQ's are still disabled.
>> >
>> > Signed-off-by: Stefan Agner 
>> > ---
>> >  drivers/clk/imx/clk-pllv3.c | 5 -
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
>> > index c05c43d..b5ff561 100644
>> > --- a/drivers/clk/imx/clk-pllv3.c
>> > +++ b/drivers/clk/imx/clk-pllv3.c
>> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
>> > break;
>> > if (time_after(jiffies, timeout))
>> > break;
>> > -   usleep_range(50, 500);
>> > +   if (unlikely(irqs_disabled()))
>>
>> This causes a bit confusion that clk_pllv3_prepare is sleepable.
>> But this line indicates it's possible to be called in irq context.
>> Although it's only happened during kernel boot phase where irq is
>> still not enabled.
>> It seems schedule_debug() somehow did not catch it during early boot
>> phase. Maybe schedule guys can help explain.
>>
>> My question is if it's really worthy to introduce this confusion
>> to fix the issue since the delay is minor?
>
> I do not understand why it's confusing.  The code already obviously
> indicates this is a special handling for cases where irq is still not
> enabled, rather than for irq context.
>

The code itself has nothing telling it's a special handling for the
case where irq is
still not enabled.
Even it tells, it may still cause confusing by adding complexity in
clk_pllv3_prepare()
which actually should be called in non-atomic context as it could sleep.

> The patch is to fix the "bad: scheduling from the idle thread!" warning
> rather than minimize the delay.  Do you have an opinion on how to fix
> the warning?
>

I just wonder maybe we could simply just using udelay(50) instead of
usleep_range(50, 500) to eliminate the confusing since it's minor cast.
What do you think of it?

>> Furthermore, shouldn't it be udelay(500)?
>
> 500 is for the worst case of sleep, and 50 is good enough for delay.
>

Yes, you''re right.
We have a loop, so 50ns one time should be good.

> Shawn

Regards
Dong Aisheng

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

[Dong Aisheng]

Hi Shawn,

On Tue, Apr 26, 2016 at 9:23 AM, Shawn Guo  wrote:
> On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
>> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
>> > If a clock gets enabled early during boot time, it can lead to a PLL
>> > startup. The wait_lock function makes sure that the PLL is really
>> > stareted up before it gets used. However, the function sleeps which
>> > leads to scheduling and an error:
>> > bad: scheduling from the idle thread!
>> > ...
>> >
>> > Use udelay in case IRQ's are still disabled.
>> >
>> > Signed-off-by: Stefan Agner 
>> > ---
>> >  drivers/clk/imx/clk-pllv3.c | 5 -
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
>> > index c05c43d..b5ff561 100644
>> > --- a/drivers/clk/imx/clk-pllv3.c
>> > +++ b/drivers/clk/imx/clk-pllv3.c
>> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
>> > break;
>> > if (time_after(jiffies, timeout))
>> > break;
>> > -   usleep_range(50, 500);
>> > +   if (unlikely(irqs_disabled()))
>>
>> This causes a bit confusion that clk_pllv3_prepare is sleepable.
>> But this line indicates it's possible to be called in irq context.
>> Although it's only happened during kernel boot phase where irq is
>> still not enabled.
>> It seems schedule_debug() somehow did not catch it during early boot
>> phase. Maybe schedule guys can help explain.
>>
>> My question is if it's really worthy to introduce this confusion
>> to fix the issue since the delay is minor?
>
> I do not understand why it's confusing.  The code already obviously
> indicates this is a special handling for cases where irq is still not
> enabled, rather than for irq context.
>

The code itself has nothing telling it's a special handling for the
case where irq is
still not enabled.
Even it tells, it may still cause confusing by adding complexity in
clk_pllv3_prepare()
which actually should be called in non-atomic context as it could sleep.

> The patch is to fix the "bad: scheduling from the idle thread!" warning
> rather than minimize the delay.  Do you have an opinion on how to fix
> the warning?
>

I just wonder maybe we could simply just using udelay(50) instead of
usleep_range(50, 500) to eliminate the confusing since it's minor cast.
What do you think of it?

>> Furthermore, shouldn't it be udelay(500)?
>
> 500 is for the worst case of sleep, and 50 is good enough for delay.
>

Yes, you''re right.
We have a loop, so 50ns one time should be good.

> Shawn

Regards
Dong Aisheng

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

[Shawn Guo]

On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> > If a clock gets enabled early during boot time, it can lead to a PLL
> > startup. The wait_lock function makes sure that the PLL is really
> > stareted up before it gets used. However, the function sleeps which
> > leads to scheduling and an error:
> > bad: scheduling from the idle thread!
> > ...
> > 
> > Use udelay in case IRQ's are still disabled.
> > 
> > Signed-off-by: Stefan Agner 
> > ---
> >  drivers/clk/imx/clk-pllv3.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> > index c05c43d..b5ff561 100644
> > --- a/drivers/clk/imx/clk-pllv3.c
> > +++ b/drivers/clk/imx/clk-pllv3.c
> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
> > break;
> > if (time_after(jiffies, timeout))
> > break;
> > -   usleep_range(50, 500);
> > +   if (unlikely(irqs_disabled()))
> 
> This causes a bit confusion that clk_pllv3_prepare is sleepable.
> But this line indicates it's possible to be called in irq context.
> Although it's only happened during kernel boot phase where irq is
> still not enabled.
> It seems schedule_debug() somehow did not catch it during early boot
> phase. Maybe schedule guys can help explain.
> 
> My question is if it's really worthy to introduce this confusion
> to fix the issue since the delay is minor?

I do not understand why it's confusing.  The code already obviously
indicates this is a special handling for cases where irq is still not
enabled, rather than for irq context.

The patch is to fix the "bad: scheduling from the idle thread!" warning
rather than minimize the delay.  Do you have an opinion on how to fix
the warning?

> Furthermore, shouldn't it be udelay(500)?

500 is for the worst case of sleep, and 50 is good enough for delay.

Shawn

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

[Shawn Guo]

On Thu, Apr 21, 2016 at 11:45:20AM +0800, Dong Aisheng wrote:
> On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> > If a clock gets enabled early during boot time, it can lead to a PLL
> > startup. The wait_lock function makes sure that the PLL is really
> > stareted up before it gets used. However, the function sleeps which
> > leads to scheduling and an error:
> > bad: scheduling from the idle thread!
> > ...
> > 
> > Use udelay in case IRQ's are still disabled.
> > 
> > Signed-off-by: Stefan Agner 
> > ---
> >  drivers/clk/imx/clk-pllv3.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> > index c05c43d..b5ff561 100644
> > --- a/drivers/clk/imx/clk-pllv3.c
> > +++ b/drivers/clk/imx/clk-pllv3.c
> > @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
> > break;
> > if (time_after(jiffies, timeout))
> > break;
> > -   usleep_range(50, 500);
> > +   if (unlikely(irqs_disabled()))
> 
> This causes a bit confusion that clk_pllv3_prepare is sleepable.
> But this line indicates it's possible to be called in irq context.
> Although it's only happened during kernel boot phase where irq is
> still not enabled.
> It seems schedule_debug() somehow did not catch it during early boot
> phase. Maybe schedule guys can help explain.
> 
> My question is if it's really worthy to introduce this confusion
> to fix the issue since the delay is minor?

I do not understand why it's confusing.  The code already obviously
indicates this is a special handling for cases where irq is still not
enabled, rather than for irq context.

The patch is to fix the "bad: scheduling from the idle thread!" warning
rather than minimize the delay.  Do you have an opinion on how to fix
the warning?

> Furthermore, shouldn't it be udelay(500)?

500 is for the worst case of sleep, and 50 is good enough for delay.

Shawn

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

[Dong Aisheng]

On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
> leads to scheduling and an error:
> bad: scheduling from the idle thread!
> ...
> 
> Use udelay in case IRQ's are still disabled.
> 
> Signed-off-by: Stefan Agner 
> ---
>  drivers/clk/imx/clk-pllv3.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> index c05c43d..b5ff561 100644
> --- a/drivers/clk/imx/clk-pllv3.c
> +++ b/drivers/clk/imx/clk-pllv3.c
> @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
>   break;
>   if (time_after(jiffies, timeout))
>   break;
> - usleep_range(50, 500);
> + if (unlikely(irqs_disabled()))

This causes a bit confusion that clk_pllv3_prepare is sleepable.
But this line indicates it's possible to be called in irq context.
Although it's only happened during kernel boot phase where irq is
still not enabled.
It seems schedule_debug() somehow did not catch it during early boot
phase. Maybe schedule guys can help explain.

My question is if it's really worthy to introduce this confusion
to fix the issue since the delay is minor?

Furthermore, shouldn't it be udelay(500)?

Shawn,
What's your idea?

Regards
Dong Aisheng

> + udelay(50);
> + else
> + usleep_range(50, 500);
>   } while (1);
>  
>   return readl_relaxed(pll->base) & BM_PLL_LOCK ? 0 : -ETIMEDOUT;
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dong Aisheng]

On Fri, Jan 29, 2016 at 02:49:23PM -0800, Stefan Agner wrote:
> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
> leads to scheduling and an error:
> bad: scheduling from the idle thread!
> ...
> 
> Use udelay in case IRQ's are still disabled.
> 
> Signed-off-by: Stefan Agner 
> ---
>  drivers/clk/imx/clk-pllv3.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> index c05c43d..b5ff561 100644
> --- a/drivers/clk/imx/clk-pllv3.c
> +++ b/drivers/clk/imx/clk-pllv3.c
> @@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
>   break;
>   if (time_after(jiffies, timeout))
>   break;
> - usleep_range(50, 500);
> + if (unlikely(irqs_disabled()))

This causes a bit confusion that clk_pllv3_prepare is sleepable.
But this line indicates it's possible to be called in irq context.
Although it's only happened during kernel boot phase where irq is
still not enabled.
It seems schedule_debug() somehow did not catch it during early boot
phase. Maybe schedule guys can help explain.

My question is if it's really worthy to introduce this confusion
to fix the issue since the delay is minor?

Furthermore, shouldn't it be udelay(500)?

Shawn,
What's your idea?

Regards
Dong Aisheng

> + udelay(50);
> + else
> + usleep_range(50, 500);
>   } while (1);
>  
>   return readl_relaxed(pll->base) & BM_PLL_LOCK ? 0 : -ETIMEDOUT;
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shawn Guo]

On Fri, Apr 15, 2016 at 06:00:53PM -0700, Stephen Boyd wrote:
> On 01/29, Stefan Agner wrote:
> > If a clock gets enabled early during boot time, it can lead to a PLL
> > startup. The wait_lock function makes sure that the PLL is really
> > stareted up before it gets used. However, the function sleeps which
> > leads to scheduling and an error:
> > bad: scheduling from the idle thread!
> > ...
> > 
> > Use udelay in case IRQ's are still disabled.
> > 
> > Signed-off-by: Stefan Agner 
> 
> This is really old. Shawn, are you picking these up? I'm removing
> these from my queue for now.

Yes, I'm picking them up.

Shawn

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

[Shawn Guo]

On Fri, Apr 15, 2016 at 06:00:53PM -0700, Stephen Boyd wrote:
> On 01/29, Stefan Agner wrote:
> > If a clock gets enabled early during boot time, it can lead to a PLL
> > startup. The wait_lock function makes sure that the PLL is really
> > stareted up before it gets used. However, the function sleeps which
> > leads to scheduling and an error:
> > bad: scheduling from the idle thread!
> > ...
> > 
> > Use udelay in case IRQ's are still disabled.
> > 
> > Signed-off-by: Stefan Agner 
> 
> This is really old. Shawn, are you picking these up? I'm removing
> these from my queue for now.

Yes, I'm picking them up.

Shawn

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

[Stephen Boyd]

On 01/29, Stefan Agner wrote:
> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
> leads to scheduling and an error:
> bad: scheduling from the idle thread!
> ...
> 
> Use udelay in case IRQ's are still disabled.
> 
> Signed-off-by: Stefan Agner 

This is really old. Shawn, are you picking these up? I'm removing
these from my queue for now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

On 01/29, Stefan Agner wrote:
> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
> leads to scheduling and an error:
> bad: scheduling from the idle thread!
> ...
> 
> Use udelay in case IRQ's are still disabled.
> 
> Signed-off-by: Stefan Agner 

This is really old. Shawn, are you picking these up? I'm removing
these from my queue for now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

On 01/29, Stefan Agner wrote:
> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
> leads to scheduling and an error:
> bad: scheduling from the idle thread!
> ...

Can you please share the full splat? I have no clue what's going
on.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Joshua Clayton]

On Fri, 29 Jan 2016 14:49:23 -0800
Stefan Agner  wrote:

> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
s/stareted/started/

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

[Stefan Agner]

If a clock gets enabled early during boot time, it can lead to a PLL
startup. The wait_lock function makes sure that the PLL is really
stareted up before it gets used. However, the function sleeps which
leads to scheduling and an error:
bad: scheduling from the idle thread!
...

Use udelay in case IRQ's are still disabled.

Signed-off-by: Stefan Agner 
---
 drivers/clk/imx/clk-pllv3.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
index c05c43d..b5ff561 100644
--- a/drivers/clk/imx/clk-pllv3.c
+++ b/drivers/clk/imx/clk-pllv3.c
@@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
break;
if (time_after(jiffies, timeout))
break;
-   usleep_range(50, 500);
+   if (unlikely(irqs_disabled()))
+   udelay(50);
+   else
+   usleep_range(50, 500);
} while (1);
 
return readl_relaxed(pll->base) & BM_PLL_LOCK ? 0 : -ETIMEDOUT;
-- 
2.7.0

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

[Joshua Clayton]

On Fri, 29 Jan 2016 14:49:23 -0800
Stefan Agner  wrote:

> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
s/stareted/started/

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

[Stephen Boyd]

On 01/29, Stefan Agner wrote:
> If a clock gets enabled early during boot time, it can lead to a PLL
> startup. The wait_lock function makes sure that the PLL is really
> stareted up before it gets used. However, the function sleeps which
> leads to scheduling and an error:
> bad: scheduling from the idle thread!
> ...

Can you please share the full splat? I have no clue what's going
on.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stefan Agner]

If a clock gets enabled early during boot time, it can lead to a PLL
startup. The wait_lock function makes sure that the PLL is really
stareted up before it gets used. However, the function sleeps which
leads to scheduling and an error:
bad: scheduling from the idle thread!
...

Use udelay in case IRQ's are still disabled.

Signed-off-by: Stefan Agner 
---
 drivers/clk/imx/clk-pllv3.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
index c05c43d..b5ff561 100644
--- a/drivers/clk/imx/clk-pllv3.c
+++ b/drivers/clk/imx/clk-pllv3.c
@@ -63,7 +63,10 @@ static int clk_pllv3_wait_lock(struct clk_pllv3 *pll)
break;
if (time_after(jiffies, timeout))
break;
-   usleep_range(50, 500);
+   if (unlikely(irqs_disabled()))
+   udelay(50);
+   else
+   usleep_range(50, 500);
} while (1);
 
return readl_relaxed(pll->base) & BM_PLL_LOCK ? 0 : -ETIMEDOUT;
-- 
2.7.0