[PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Lorenzo Pieralisi]

Rafael, Daniel,

while reviewing CPUidle code I bumped into a couple of leftovers
from the merge window and put together these two patches to try
to "fix" them (I could not trigger any issue, just code inspection),
please let me know what you think.

Thanks,
Lorenzo

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>

Lorenzo Pieralisi (2):
  drivers: cpuidle: remove stale irq disabling call in
    cpuidle_enter_freeze()
  drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

 drivers/cpuidle/cpuidle.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

-- 
2.2.1

[PATCH 1/2] drivers: cpuidle: remove stale irq disabling call in cpuidle_enter_freeze()

[Lorenzo Pieralisi]

On return from cpuidle_enter_freeze() irqs are re-enabled by the function
caller (ie cpuidle_idle_call) in the idle loop. This patch removes a stale
local_irq_disable() call and its stale comment in cpuidle_enter_freeze(),
since they disagree and do not serve a useful purpose.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/cpuidle/cpuidle.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 4d53458..f47edc6c 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -144,9 +144,6 @@ void cpuidle_enter_freeze(void)
 		cpuidle_enter(drv, dev, index);
 	else
 		arch_cpu_idle();
-
-	/* Interrupts are enabled again here. */
-	local_irq_disable();
 }
 
 /**
-- 
2.2.1

Re: [PATCH 1/2] drivers: cpuidle: remove stale irq disabling call in cpuidle_enter_freeze()

[Daniel Lezcano]

On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> On return from cpuidle_enter_freeze() irqs are re-enabled by the function
> caller (ie cpuidle_idle_call) in the idle loop. This patch removes a stale
> local_irq_disable() call and its stale comment in cpuidle_enter_freeze(),
> since they disagree and do not serve a useful purpose.
>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>   drivers/cpuidle/cpuidle.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 4d53458..f47edc6c 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -144,9 +144,6 @@ void cpuidle_enter_freeze(void)
>   		cpuidle_enter(drv, dev, index);
>   	else
>   		arch_cpu_idle();
> -
> -	/* Interrupts are enabled again here. */
> -	local_irq_disable();
>   }

Hmm, I think Rafael added this prevent lockdep to raise a warning.

Otherwise, cpuidle_enter or arch_cpu_idle enables the irq again and then 
when exiting the cpu_idle_call, we enable them again, so leading to a 
lockdep WARN in trace_hardirqs_on_caller.

That said, if we have to do this, it may reveal something is wrong in 
the code.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] drivers: cpuidle: remove stale irq disabling call in cpuidle_enter_freeze()

[Lorenzo Pieralisi]

On Wed, Feb 25, 2015 at 02:13:23PM +0000, Daniel Lezcano wrote:
> On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > On return from cpuidle_enter_freeze() irqs are re-enabled by the function
> > caller (ie cpuidle_idle_call) in the idle loop. This patch removes a stale
> > local_irq_disable() call and its stale comment in cpuidle_enter_freeze(),
> > since they disagree and do not serve a useful purpose.
> >
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >   drivers/cpuidle/cpuidle.c | 3 ---
> >   1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > index 4d53458..f47edc6c 100644
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -144,9 +144,6 @@ void cpuidle_enter_freeze(void)
> >   		cpuidle_enter(drv, dev, index);
> >   	else
> >   		arch_cpu_idle();
> > -
> > -	/* Interrupts are enabled again here. */
> > -	local_irq_disable();
> >   }
> 
> Hmm, I think Rafael added this prevent lockdep to raise a warning.

Ok, so the comment is there to say "at this point of execution IRQs
are enabled", it does not refer to local_irq_disable() call effects,
that's misleading and not necessarily nice, at least it should
be explained.

> Otherwise, cpuidle_enter or arch_cpu_idle enables the irq again and then 
> when exiting the cpu_idle_call, we enable them again, so leading to a 
> lockdep WARN in trace_hardirqs_on_caller.

Would not it be better to enable irqs in cpuidle_enter_freeze() on
returning from enter_freeze_proper() and remove the local_irq_enable()
call in the cpuidle_idle_call() before jumping to exit_idle ?

> That said, if we have to do this, it may reveal something is wrong in 
> the code.

I just spotted code through inspection, I have to say at the moment it
is not very clear what it is meant to achieve, so I put together this
patch.

Lorenzo

Re: [PATCH 1/2] drivers: cpuidle: remove stale irq disabling call in cpuidle_enter_freeze()

[Rafael J. Wysocki]

On Wednesday, February 25, 2015 02:39:17 PM Lorenzo Pieralisi wrote:
> On Wed, Feb 25, 2015 at 02:13:23PM +0000, Daniel Lezcano wrote:
> > On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > > On return from cpuidle_enter_freeze() irqs are re-enabled by the function
> > > caller (ie cpuidle_idle_call) in the idle loop. This patch removes a stale
> > > local_irq_disable() call and its stale comment in cpuidle_enter_freeze(),
> > > since they disagree and do not serve a useful purpose.
> > >
> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > ---
> > >   drivers/cpuidle/cpuidle.c | 3 ---
> > >   1 file changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > index 4d53458..f47edc6c 100644
> > > --- a/drivers/cpuidle/cpuidle.c
> > > +++ b/drivers/cpuidle/cpuidle.c
> > > @@ -144,9 +144,6 @@ void cpuidle_enter_freeze(void)
> > >   		cpuidle_enter(drv, dev, index);
> > >   	else
> > >   		arch_cpu_idle();
> > > -
> > > -	/* Interrupts are enabled again here. */
> > > -	local_irq_disable();
> > >   }
> > 
> > Hmm, I think Rafael added this prevent lockdep to raise a warning.
> 
> Ok, so the comment is there to say "at this point of execution IRQs
> are enabled", it does not refer to local_irq_disable() call effects,
> that's misleading and not necessarily nice, at least it should
> be explained.
> 
> > Otherwise, cpuidle_enter or arch_cpu_idle enables the irq again and then 
> > when exiting the cpu_idle_call, we enable them again, so leading to a 
> > lockdep WARN in trace_hardirqs_on_caller.
> 
> Would not it be better to enable irqs in cpuidle_enter_freeze() on
> returning from enter_freeze_proper() and remove the local_irq_enable()
> call in the cpuidle_idle_call() before jumping to exit_idle ?
> 
> > That said, if we have to do this, it may reveal something is wrong in 
> > the code.
> 
> I just spotted code through inspection, I have to say at the moment it
> is not very clear what it is meant to achieve, so I put together this
> patch.

So there are two code paths in cpuidle_idle_call(), the enter_freeze_proper()
one which does *not* re-enable interrupts and the one you modified which does
that.  The local_irq_disable() is to keep things consistent.

I'm not entirely against of re-arranging things here, but a patch like the
(untested) one below might be more appropriate.

Rafael (who would appreciate it if people asked questions instead of sending
patches on a hunch).


---
 drivers/cpuidle/cpuidle.c |    2 +-
 kernel/sched/idle.c       |    1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -132,6 +132,7 @@ void cpuidle_enter_freeze(void)
 	index = cpuidle_find_deepest_state(drv, dev, true);
 	if (index >= 0) {
 		enter_freeze_proper(drv, dev, index);
+		local_irq_enable();
 		return;
 	}
 
@@ -146,7 +147,6 @@ void cpuidle_enter_freeze(void)
 		arch_cpu_idle();
 
 	/* Interrupts are enabled again here. */
-	local_irq_disable();
 }
 
 /**
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -116,7 +116,6 @@ static void cpuidle_idle_call(void)
 	 */
 	if (idle_should_freeze()) {
 		cpuidle_enter_freeze();
-		local_irq_enable();
 		goto exit_idle;
 	}
 

Re: [PATCH 1/2] drivers: cpuidle: remove stale irq disabling call in cpuidle_enter_freeze()

[Lorenzo Pieralisi]

On Wed, Feb 25, 2015 at 11:36:10PM +0000, Rafael J. Wysocki wrote:
> On Wednesday, February 25, 2015 02:39:17 PM Lorenzo Pieralisi wrote:
> > On Wed, Feb 25, 2015 at 02:13:23PM +0000, Daniel Lezcano wrote:
> > > On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > > > On return from cpuidle_enter_freeze() irqs are re-enabled by the function
> > > > caller (ie cpuidle_idle_call) in the idle loop. This patch removes a stale
> > > > local_irq_disable() call and its stale comment in cpuidle_enter_freeze(),
> > > > since they disagree and do not serve a useful purpose.
> > > >
> > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > ---
> > > >   drivers/cpuidle/cpuidle.c | 3 ---
> > > >   1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > > index 4d53458..f47edc6c 100644
> > > > --- a/drivers/cpuidle/cpuidle.c
> > > > +++ b/drivers/cpuidle/cpuidle.c
> > > > @@ -144,9 +144,6 @@ void cpuidle_enter_freeze(void)
> > > >   		cpuidle_enter(drv, dev, index);
> > > >   	else
> > > >   		arch_cpu_idle();
> > > > -
> > > > -	/* Interrupts are enabled again here. */
> > > > -	local_irq_disable();
> > > >   }
> > > 
> > > Hmm, I think Rafael added this prevent lockdep to raise a warning.
> > 
> > Ok, so the comment is there to say "at this point of execution IRQs
> > are enabled", it does not refer to local_irq_disable() call effects,
> > that's misleading and not necessarily nice, at least it should
> > be explained.
> > 
> > > Otherwise, cpuidle_enter or arch_cpu_idle enables the irq again and then 
> > > when exiting the cpu_idle_call, we enable them again, so leading to a 
> > > lockdep WARN in trace_hardirqs_on_caller.
> > 
> > Would not it be better to enable irqs in cpuidle_enter_freeze() on
> > returning from enter_freeze_proper() and remove the local_irq_enable()
> > call in the cpuidle_idle_call() before jumping to exit_idle ?
> > 
> > > That said, if we have to do this, it may reveal something is wrong in 
> > > the code.
> > 
> > I just spotted code through inspection, I have to say at the moment it
> > is not very clear what it is meant to achieve, so I put together this
> > patch.
> 
> So there are two code paths in cpuidle_idle_call(), the enter_freeze_proper()
> one which does *not* re-enable interrupts and the one you modified which does
> that.  The local_irq_disable() is to keep things consistent.
> 
> I'm not entirely against of re-arranging things here, but a patch like the
> (untested) one below might be more appropriate.
> 
> Rafael (who would appreciate it if people asked questions instead of sending
> patches on a hunch).

I understand that, I wanted to just send [patch 2], this patch was more
a way to get a clarification than anything else, asking would have been more
appropriate, sorry.

Anyway, I did not like disabling IRQs to just re-enable them on function
return, in particular the comment below seemed to apply to the following
line, which is a bit misleading.

/* Interrupts are enabled again here. */
local_irq_disable();

> 
> 
> ---
>  drivers/cpuidle/cpuidle.c |    2 +-
>  kernel/sched/idle.c       |    1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -132,6 +132,7 @@ void cpuidle_enter_freeze(void)
>  	index = cpuidle_find_deepest_state(drv, dev, true);
>  	if (index >= 0) {
>  		enter_freeze_proper(drv, dev, index);
> +		local_irq_enable();
>  		return;
>  	}
>  
> @@ -146,7 +147,6 @@ void cpuidle_enter_freeze(void)
>  		arch_cpu_idle();
>  
>  	/* Interrupts are enabled again here. */
> -	local_irq_disable();
>  }
>  
>  /**
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -116,7 +116,6 @@ static void cpuidle_idle_call(void)
>  	 */
>  	if (idle_should_freeze()) {
>  		cpuidle_enter_freeze();
> -		local_irq_enable();
>  		goto exit_idle;
>  	}
>  

It looks fine, I will test it. I would add a comment to
cpuidle_enter_freeze() to document it must return with IRQs
enabled.

Thanks,
Lorenzo

Re: [PATCH 1/2] drivers: cpuidle: remove stale irq disabling call in cpuidle_enter_freeze()

[Rafael J. Wysocki]

On Thu, Feb 26, 2015 at 10:48 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Feb 25, 2015 at 11:36:10PM +0000, Rafael J. Wysocki wrote:
>> On Wednesday, February 25, 2015 02:39:17 PM Lorenzo Pieralisi wrote:
>> > On Wed, Feb 25, 2015 at 02:13:23PM +0000, Daniel Lezcano wrote:

[cut]

>> I'm not entirely against of re-arranging things here, but a patch like the
>> (untested) one below might be more appropriate.
>>
>> Rafael (who would appreciate it if people asked questions instead of sending
>> patches on a hunch).
>
> I understand that, I wanted to just send [patch 2], this patch was more
> a way to get a clarification than anything else, asking would have been more
> appropriate, sorry.
>
> Anyway, I did not like disabling IRQs to just re-enable them on function
> return, in particular the comment below seemed to apply to the following
> line, which is a bit misleading.

I see.

>
> /* Interrupts are enabled again here. */
> local_irq_disable();
>
>>
>>
>> ---
>>  drivers/cpuidle/cpuidle.c |    2 +-
>>  kernel/sched/idle.c       |    1 -
>>  2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/cpuidle/cpuidle.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
>> +++ linux-pm/drivers/cpuidle/cpuidle.c
>> @@ -132,6 +132,7 @@ void cpuidle_enter_freeze(void)
>>       index = cpuidle_find_deepest_state(drv, dev, true);
>>       if (index >= 0) {
>>               enter_freeze_proper(drv, dev, index);
>> +             local_irq_enable();
>>               return;
>>       }
>>
>> @@ -146,7 +147,6 @@ void cpuidle_enter_freeze(void)
>>               arch_cpu_idle();
>>
>>       /* Interrupts are enabled again here. */
>> -     local_irq_disable();
>>  }
>>
>>  /**
>> Index: linux-pm/kernel/sched/idle.c
>> ===================================================================
>> --- linux-pm.orig/kernel/sched/idle.c
>> +++ linux-pm/kernel/sched/idle.c
>> @@ -116,7 +116,6 @@ static void cpuidle_idle_call(void)
>>        */
>>       if (idle_should_freeze()) {
>>               cpuidle_enter_freeze();
>> -             local_irq_enable();
>>               goto exit_idle;
>>       }
>>
>
> It looks fine, I will test it. I would add a comment to
> cpuidle_enter_freeze() to document it must return with IRQs
> enabled.

OK

Rafael

[PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

[Lorenzo Pieralisi]

The changes in commit:

381063133246 ("PM / sleep: Re-implement suspend-to-idle handling")

let suspend-to-idle code bypass the cpuidle_select() function to
enter the deepest idle state. The sanity checks carried out in
cpuidle_select() are bypassed too and this can cause breakage
on systems that try to suspend-to-idle with no registered cpuidle
driver.

This patch factors out a function cpuidle_device_disabled() that
is used to carry out sanity checks (ie CPUidle is disabled on the
cpu executing the code) in both cpuidle_select() and cpuidle_enter_freeze()
so that the checks are unified and carried out in both control paths.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/cpuidle/cpuidle.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index f47edc6c..344fe6c 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -44,6 +44,12 @@ void disable_cpuidle(void)
 	off = 1;
 }
 
+static bool cpuidle_device_disabled(struct cpuidle_driver *drv,
+				    struct cpuidle_device *dev)
+{
+	return (off || !initialized || !drv || !dev || !dev->enabled);
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int index;
 
+	if (cpuidle_device_disabled(drv, dev)) {
+		arch_cpu_idle();
+		return;
+	}
+
 	/*
 	 * Find the deepest state with ->enter_freeze present, which guarantees
 	 * that interrupts won't be enabled when it exits and allows the tick to
@@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-	if (off || !initialized)
-		return -ENODEV;
-
-	if (!drv || !dev || !dev->enabled)
-		return -EBUSY;
+	if (cpuidle_device_disabled(drv, dev))
+		return -1;
 
 	return cpuidle_curr_governor->select(drv, dev);
 }
-- 
2.2.1

Re: [PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

[Daniel Lezcano]

On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> The changes in commit:
>
> 381063133246 ("PM / sleep: Re-implement suspend-to-idle handling")
>
> let suspend-to-idle code bypass the cpuidle_select() function to
> enter the deepest idle state. The sanity checks carried out in
> cpuidle_select() are bypassed too and this can cause breakage
> on systems that try to suspend-to-idle with no registered cpuidle
> driver.
>
> This patch factors out a function cpuidle_device_disabled() that
> is used to carry out sanity checks (ie CPUidle is disabled on the
> cpu executing the code) in both cpuidle_select() and cpuidle_enter_freeze()
> so that the checks are unified and carried out in both control paths.
>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>   drivers/cpuidle/cpuidle.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index f47edc6c..344fe6c 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -44,6 +44,12 @@ void disable_cpuidle(void)
>   	off = 1;
>   }
>
> +static bool cpuidle_device_disabled(struct cpuidle_driver *drv,
> +				    struct cpuidle_device *dev)
> +{
> +	return (off || !initialized || !drv || !dev || !dev->enabled);
> +}

This is getting a bit fuzzy IMO. What means disabled ? :)

>   /**
>    * cpuidle_play_dead - cpu off-lining
>    *
> @@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
>   	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>   	int index;

I think this is exploding before because of dev == NULL in the line above.

> +	if (cpuidle_device_disabled(drv, dev)) {
> +		arch_cpu_idle();
> +		return;
> +	}
> +
>   	/*
>   	 * Find the deepest state with ->enter_freeze present, which guarantees
>   	 * that interrupts won't be enabled when it exits and allows the tick to
> @@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>    */
>   int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>   {
> -	if (off || !initialized)
> -		return -ENODEV;
> -
> -	if (!drv || !dev || !dev->enabled)
> -		return -EBUSY;
> +	if (cpuidle_device_disabled(drv, dev))
> +		return -1;
>
>   	return cpuidle_curr_governor->select(drv, dev);
>   }
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

[Lorenzo Pieralisi]

On Wed, Feb 25, 2015 at 02:30:49PM +0000, Daniel Lezcano wrote:
> On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > The changes in commit:
> >
> > 381063133246 ("PM / sleep: Re-implement suspend-to-idle handling")
> >
> > let suspend-to-idle code bypass the cpuidle_select() function to
> > enter the deepest idle state. The sanity checks carried out in
> > cpuidle_select() are bypassed too and this can cause breakage
> > on systems that try to suspend-to-idle with no registered cpuidle
> > driver.
> >
> > This patch factors out a function cpuidle_device_disabled() that
> > is used to carry out sanity checks (ie CPUidle is disabled on the
> > cpu executing the code) in both cpuidle_select() and cpuidle_enter_freeze()
> > so that the checks are unified and carried out in both control paths.
> >
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >   drivers/cpuidle/cpuidle.c | 18 +++++++++++++-----
> >   1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > index f47edc6c..344fe6c 100644
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -44,6 +44,12 @@ void disable_cpuidle(void)
> >   	off = 1;
> >   }
> >
> > +static bool cpuidle_device_disabled(struct cpuidle_driver *drv,
> > +				    struct cpuidle_device *dev)
> > +{
> > +	return (off || !initialized || !drv || !dev || !dev->enabled);
> > +}
> 
> This is getting a bit fuzzy IMO. What means disabled ? :)

Well, that's just the current checks in cpuidle_select() (that by
the way is supposed to return an index) merged together with a function
name, to reuse the same checks in cpuidle_enter_freeze().
I have no problem leaving the checks as they are at the moment and
replicate them in cpuidle_enter_freeze() but given your remark below,
we should do something different in there.

> 
> >   /**
> >    * cpuidle_play_dead - cpu off-lining
> >    *
> > @@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
> >   	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> >   	int index;
> 
> I think this is exploding before because of dev == NULL in the line above.

Yes, good point so my attempt at consolidating the sanity checks above
is not valid, but something has to be done regardless.

Lorenzo

> > +	if (cpuidle_device_disabled(drv, dev)) {
> > +		arch_cpu_idle();
> > +		return;
> > +	}
> > +
> >   	/*
> >   	 * Find the deepest state with ->enter_freeze present, which guarantees
> >   	 * that interrupts won't be enabled when it exits and allows the tick to
> > @@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> >    */
> >   int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> >   {
> > -	if (off || !initialized)
> > -		return -ENODEV;
> > -
> > -	if (!drv || !dev || !dev->enabled)
> > -		return -EBUSY;
> > +	if (cpuidle_device_disabled(drv, dev))
> > +		return -1;
> >
> >   	return cpuidle_curr_governor->select(drv, dev);
> >   }
> >
> 
> 
> -- 
>   <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 
> 

Re: [PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

[Rafael J. Wysocki]

On Wednesday, February 25, 2015 02:47:37 PM Lorenzo Pieralisi wrote:
> On Wed, Feb 25, 2015 at 02:30:49PM +0000, Daniel Lezcano wrote:
> > On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > > The changes in commit:
> > >
> > > 381063133246 ("PM / sleep: Re-implement suspend-to-idle handling")
> > >
> > > let suspend-to-idle code bypass the cpuidle_select() function to
> > > enter the deepest idle state. The sanity checks carried out in
> > > cpuidle_select() are bypassed too and this can cause breakage
> > > on systems that try to suspend-to-idle with no registered cpuidle
> > > driver.
> > >
> > > This patch factors out a function cpuidle_device_disabled() that
> > > is used to carry out sanity checks (ie CPUidle is disabled on the
> > > cpu executing the code) in both cpuidle_select() and cpuidle_enter_freeze()
> > > so that the checks are unified and carried out in both control paths.
> > >
> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > ---
> > >   drivers/cpuidle/cpuidle.c | 18 +++++++++++++-----
> > >   1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > index f47edc6c..344fe6c 100644
> > > --- a/drivers/cpuidle/cpuidle.c
> > > +++ b/drivers/cpuidle/cpuidle.c
> > > @@ -44,6 +44,12 @@ void disable_cpuidle(void)
> > >   	off = 1;
> > >   }
> > >
> > > +static bool cpuidle_device_disabled(struct cpuidle_driver *drv,
> > > +				    struct cpuidle_device *dev)
> > > +{
> > > +	return (off || !initialized || !drv || !dev || !dev->enabled);
> > > +}
> > 
> > This is getting a bit fuzzy IMO. What means disabled ? :)
> 
> Well, that's just the current checks in cpuidle_select() (that by
> the way is supposed to return an index) merged together with a function
> name, to reuse the same checks in cpuidle_enter_freeze().
> I have no problem leaving the checks as they are at the moment and
> replicate them in cpuidle_enter_freeze() but given your remark below,
> we should do something different in there.

Maybe something like the patch below (untested)?

Rafael


---
 drivers/cpuidle/cpuidle.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,6 +44,18 @@ void disable_cpuidle(void)
 	off = 1;
 }
 
+int cpuidle_check_availability(struct cpuidle_driver *drv,
+			       struct cpuidle_device *dev)
+{
+	if (off || !initialized)
+		return -ENODEV;
+
+	if (!drv || !dev || !dev->enabled)
+		return -EBUSY;
+
+	return 0;
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -76,8 +88,13 @@ static int cpuidle_find_deepest_state(st
 				      struct cpuidle_device *dev, bool freeze)
 {
 	unsigned int latency_req = 0;
-	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
+	int i, ret;
+
+	ret = cpuidle_check_availability(drv, dev);
+	if (ret)
+		return ret;
 
+	ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
 	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
@@ -205,13 +222,8 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-	if (off || !initialized)
-		return -ENODEV;
-
-	if (!drv || !dev || !dev->enabled)
-		return -EBUSY;
-
-	return cpuidle_curr_governor->select(drv, dev);
+	int ret = cpuidle_check_availability(drv, dev);
+	return ret ? ret : cpuidle_curr_governor->select(drv, dev);
 }
 
 /**

Re: [PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

[Rafael J. Wysocki]

On Thursday, February 26, 2015 12:50:58 AM Rafael J. Wysocki wrote:
> On Wednesday, February 25, 2015 02:47:37 PM Lorenzo Pieralisi wrote:
> > On Wed, Feb 25, 2015 at 02:30:49PM +0000, Daniel Lezcano wrote:
> > > On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > > > The changes in commit:
> > > >
> > > > 381063133246 ("PM / sleep: Re-implement suspend-to-idle handling")
> > > >
> > > > let suspend-to-idle code bypass the cpuidle_select() function to
> > > > enter the deepest idle state. The sanity checks carried out in
> > > > cpuidle_select() are bypassed too and this can cause breakage
> > > > on systems that try to suspend-to-idle with no registered cpuidle
> > > > driver.
> > > >
> > > > This patch factors out a function cpuidle_device_disabled() that
> > > > is used to carry out sanity checks (ie CPUidle is disabled on the
> > > > cpu executing the code) in both cpuidle_select() and cpuidle_enter_freeze()
> > > > so that the checks are unified and carried out in both control paths.
> > > >
> > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > ---
> > > >   drivers/cpuidle/cpuidle.c | 18 +++++++++++++-----
> > > >   1 file changed, 13 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > > index f47edc6c..344fe6c 100644
> > > > --- a/drivers/cpuidle/cpuidle.c
> > > > +++ b/drivers/cpuidle/cpuidle.c
> > > > @@ -44,6 +44,12 @@ void disable_cpuidle(void)
> > > >   	off = 1;
> > > >   }
> > > >
> > > > +static bool cpuidle_device_disabled(struct cpuidle_driver *drv,
> > > > +				    struct cpuidle_device *dev)
> > > > +{
> > > > +	return (off || !initialized || !drv || !dev || !dev->enabled);
> > > > +}
> > > 
> > > This is getting a bit fuzzy IMO. What means disabled ? :)
> > 
> > Well, that's just the current checks in cpuidle_select() (that by
> > the way is supposed to return an index) merged together with a function
> > name, to reuse the same checks in cpuidle_enter_freeze().
> > I have no problem leaving the checks as they are at the moment and
> > replicate them in cpuidle_enter_freeze() but given your remark below,
> > we should do something different in there.
> 
> Maybe something like the patch below (untested)?

That's slightly inefficient for things that don't support ->enter_freeze
and, moreover, all negative return values of cpuidle_select() are equivalent,
so I ended up with something similar to the original Lorenzo's patch (on
top of https://patchwork.kernel.org/patch/5885171/ (still untested).

Rafael


---
 drivers/cpuidle/cpuidle.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,6 +44,12 @@ void disable_cpuidle(void)
 	off = 1;
 }
 
+static bool cpuidle_not_available(struct cpuidle_driver *drv,
+				  struct cpuidle_device *dev)
+{
+	return off || !initialized || !drv || !dev || !dev->enabled;
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -124,6 +130,9 @@ void cpuidle_enter_freeze(void)
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int index;
 
+	if (cpuidle_not_available(drv, dev))
+		goto fallback;
+
 	/*
 	 * Find the deepest state with ->enter_freeze present, which guarantees
 	 * that interrupts won't be enabled when it exits and allows the tick to
@@ -141,11 +150,14 @@ void cpuidle_enter_freeze(void)
 	 * at all and try to enter it normally.
 	 */
 	index = cpuidle_find_deepest_state(drv, dev, false);
-	if (index >= 0)
+	if (index >= 0) {
 		cpuidle_enter(drv, dev, index);
-	else
-		arch_cpu_idle();
+		/* Interrupts are enabled again here. */
+		return;
+	}
 
+ fallback:
+	arch_cpu_idle();
 	/* Interrupts are enabled again here. */
 }
 
@@ -205,12 +217,9 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-	if (off || !initialized)
+	if (cpuidle_not_available(drv, dev))
 		return -ENODEV;
 
-	if (!drv || !dev || !dev->enabled)
-		return -EBUSY;
-
 	return cpuidle_curr_governor->select(drv, dev);
 }
 

Re: [PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

[Lorenzo Pieralisi]

On Wed, Feb 25, 2015 at 02:30:49PM +0000, Daniel Lezcano wrote:

[...]

> > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > index f47edc6c..344fe6c 100644
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -44,6 +44,12 @@ void disable_cpuidle(void)
> >   	off = 1;
> >   }
> >
> > +static bool cpuidle_device_disabled(struct cpuidle_driver *drv,
> > +				    struct cpuidle_device *dev)
> > +{
> > +	return (off || !initialized || !drv || !dev || !dev->enabled);
> > +}
> 
> This is getting a bit fuzzy IMO. What means disabled ? :)
> 
> >   /**
> >    * cpuidle_play_dead - cpu off-lining
> >    *
> > @@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
> >   	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> >   	int index;
> 
> I think this is exploding before because of dev == NULL in the line above.

Actually not, cpuidle_get_cpu_driver() checks the dev pointer, so
we might end up with drv == NULL and dev == NULL, and the check I added
still applies and it is effective I think.

Lorenzo

[PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Rafael J. Wysocki]

Me versions of the two $subject patches follow.

Thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/2] idle / sleep: Avoid excessive interrupts disabling and enabling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Disabling interrupts at the end of cpuidle_enter_freeze() is not
useful, because its caller, cpuidle_idle_call(), re-enables them
right away after invoking it.

To avoid that unnecessary back and forth dance with interrupts,
make cpuidle_enter_freeze() enable interrupts after calling
enter_freeze_proper() and drop the local_irq_disable() at its
end, so that all of the code paths in it end up with interrupts
enabled.  Then, cpuidle_idle_call() will not need to re-enable
interrupts after calling cpuidle_enter_freeze() any more, because
the latter will return with interrupts enabled, in analogy with
cpuidle_enter().

Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |    6 +++---
 kernel/sched/idle.c       |    1 -
 2 files changed, 3 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -117,6 +117,8 @@ static void enter_freeze_proper(struct c
  * If there are states with the ->enter_freeze callback, find the deepest of
  * them and enter it with frozen tick.  Otherwise, find the deepest state
  * available and enter it normally.
+ *
+ * Returns with enabled interrupts.
  */
 void cpuidle_enter_freeze(void)
 {
@@ -132,6 +134,7 @@ void cpuidle_enter_freeze(void)
 	index = cpuidle_find_deepest_state(drv, dev, true);
 	if (index >= 0) {
 		enter_freeze_proper(drv, dev, index);
+		local_irq_enable();
 		return;
 	}
 
@@ -144,9 +147,6 @@ void cpuidle_enter_freeze(void)
 		cpuidle_enter(drv, dev, index);
 	else
 		arch_cpu_idle();
-
-	/* Interrupts are enabled again here. */
-	local_irq_disable();
 }
 
 /**
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -116,7 +116,6 @@ static void cpuidle_idle_call(void)
 	 */
 	if (idle_should_freeze()) {
 		cpuidle_enter_freeze();
-		local_irq_enable();
 		goto exit_idle;
 	}
 

[PATCH 2/2] cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze() too

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify cpuidle_enter_freeze() to do the sanity checks done by
cpuidle_select() to avoid crashing the suspend-to-idle code
path in case something is missing.

Original-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,6 +44,12 @@ void disable_cpuidle(void)
 	off = 1;
 }
 
+static bool cpuidle_not_available(struct cpuidle_driver *drv,
+				  struct cpuidle_device *dev)
+{
+	return off || !initialized || !drv || !dev || !dev->enabled;
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -126,6 +132,9 @@ void cpuidle_enter_freeze(void)
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int index;
 
+	if (cpuidle_not_available(drv, dev))
+		goto fallback;
+
 	/*
 	 * Find the deepest state with ->enter_freeze present, which guarantees
 	 * that interrupts won't be enabled when it exits and allows the tick to
@@ -143,10 +152,13 @@ void cpuidle_enter_freeze(void)
 	 * at all and try to enter it normally.
 	 */
 	index = cpuidle_find_deepest_state(drv, dev, false);
-	if (index >= 0)
+	if (index >= 0) {
 		cpuidle_enter(drv, dev, index);
-	else
-		arch_cpu_idle();
+		return;
+	}
+
+ fallback:
+	arch_cpu_idle();
 }
 
 /**
@@ -205,12 +217,9 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-	if (off || !initialized)
+	if (cpuidle_not_available(drv, dev))
 		return -ENODEV;
 
-	if (!drv || !dev || !dev->enabled)
-		return -EBUSY;
-
 	return cpuidle_curr_governor->select(drv, dev);
 }
 

Re: [PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Peter Zijlstra]

On Fri, Feb 27, 2015 at 12:37:54AM +0100, Rafael J. Wysocki wrote:
> Me versions of the two $subject patches follow.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Lorenzo Pieralisi]

[CC'ed Preeti]

On Thu, Feb 26, 2015 at 11:37:54PM +0000, Rafael J. Wysocki wrote:
> Me versions of the two $subject patches follow.

Thank you. I am testing them and I have run into the following issue.

Starting with:

3810631 ("PM / sleep: Re-implement suspend-to-idle handling")

the suspend-to-idle code path in the cpuidle_idle_call() bypasses
the CPUIDLE_FLAG_TIMER_STOP code path entirely. Now, on most of
the current ARM platforms, the deepest idle state loses the tick device
context, therefore this means that going to idle through
suspend-to-idle becomes a brute force way of nuking the tick,
unless I am missing something here.

I am experiencing hangs on resume from suspend-to-idle when the broadcast
timer is the broadast-hrtimer (ie there is no HW broadcast timer in the
platform) and the deepest idle states lose the tick device context (ie
they are CPUIDLE_FLAG_TIMER_STOP), I hope Preeti can help me test this on
Power, still chasing the issue.

I could not reproduce the issue with a HW broadcast timer device.

Platform has deepest idle states that allow CPUs shutdown where the
local tick device is gone on entry, I am trying to provide you with a
backtrace, I need time to debug.

The question I have: is it safe to bypass the CPUIDLE_FLAG_TIMER_STOP
and related broadcast mode entry/exit in the suspend-to-idle path ?

I do not think it is, but I am asking.

I can "force" tick freeze by initializing the enter_freeze pointer
in the idle states (that's the next thing I will test), but still, for
platforms where that's not possible my question above is still valid.

Thanks,
Lorenzo

Re: [PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Rafael J. Wysocki]

On Friday, February 27, 2015 10:00:00 AM Lorenzo Pieralisi wrote:
> [CC'ed Preeti]
> 
> On Thu, Feb 26, 2015 at 11:37:54PM +0000, Rafael J. Wysocki wrote:
> > Me versions of the two $subject patches follow.
> 
> Thank you. I am testing them and I have run into the following issue.
> 
> Starting with:
> 
> 3810631 ("PM / sleep: Re-implement suspend-to-idle handling")
> 
> the suspend-to-idle code path in the cpuidle_idle_call() bypasses
> the CPUIDLE_FLAG_TIMER_STOP code path entirely.

Hmm, this looks like a mistake.  Sorry about that.

> Now, on most of
> the current ARM platforms, the deepest idle state loses the tick device
> context, therefore this means that going to idle through
> suspend-to-idle becomes a brute force way of nuking the tick,
> unless I am missing something here.
> 
> I am experiencing hangs on resume from suspend-to-idle when the broadcast
> timer is the broadast-hrtimer (ie there is no HW broadcast timer in the
> platform) and the deepest idle states lose the tick device context (ie
> they are CPUIDLE_FLAG_TIMER_STOP), I hope Preeti can help me test this on
> Power, still chasing the issue.
> 
> I could not reproduce the issue with a HW broadcast timer device.
> 
> Platform has deepest idle states that allow CPUs shutdown where the
> local tick device is gone on entry, I am trying to provide you with a
> backtrace, I need time to debug.
> 
> The question I have: is it safe to bypass the CPUIDLE_FLAG_TIMER_STOP
> and related broadcast mode entry/exit in the suspend-to-idle path ?
> 
> I do not think it is, but I am asking.

It isn't in general, but it would be OK in the enter_freeze_proper() path
where the tick is suspended anyway.

> I can "force" tick freeze by initializing the enter_freeze pointer
> in the idle states (that's the next thing I will test), but still, for
> platforms where that's not possible my question above is still valid.

Right.

Does the patch below help (on top of the previous ones)?

Rafael


---
 drivers/cpuidle/cpuidle.c |   31 ++++++++++++++++++++++++++-----
 kernel/sched/idle.c       |   17 ++---------------
 2 files changed, 28 insertions(+), 20 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -230,15 +230,36 @@ int cpuidle_select(struct cpuidle_driver
  * @dev:   the cpuidle device
  * @index: the index in the idle state table
  *
- * Returns the index in the idle state, < 0 in case of error.
- * The error code depends on the backend driver
+ * Returns the index in the idle state, < 0 in case of error.  -EBUSY is
+ * returned to indicate that the target state was temporarily unavailable.
+ * The other error codes depend on the backend driver.
  */
 int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		  int index)
 {
-	if (cpuidle_state_is_coupled(dev, drv, index))
-		return cpuidle_enter_state_coupled(dev, drv, index);
-	return cpuidle_enter_state(dev, drv, index);
+	unsigned int broadcast;
+	int ret;
+
+	broadcast = drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP;
+
+	/*
+	 * Tell the time framework to switch to a broadcast timer
+	 * because our local timer will be shutdown. If a local timer
+	 * is used from another cpu as a broadcast timer, this call may
+	 * fail if it is not available
+	 */
+	if (broadcast &&
+	    clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu))
+		return -EBUSY;
+
+	ret = cpuidle_state_is_coupled(dev, drv, index) ?
+		cpuidle_enter_state_coupled(dev, drv, index) :
+		cpuidle_enter_state(dev, drv, index);
+
+	if (broadcast)
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+
+	return ret;
 }
 
 /**
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
-	unsigned int broadcast;
 
 	/*
 	 * Check if the idle task must be rescheduled. If it is the
@@ -151,18 +150,6 @@ use_default:
 		goto exit_idle;
 	}
 
-	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
-
-	/*
-	 * Tell the time framework to switch to a broadcast timer
-	 * because our local timer will be shutdown. If a local timer
-	 * is used from another cpu as a broadcast timer, this call may
-	 * fail if it is not available
-	 */
-	if (broadcast &&
-	    clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu))
-		goto use_default;
-
 	/* Take note of the planned idle state. */
 	idle_set_state(this_rq(), &drv->states[next_state]);
 
@@ -176,8 +163,8 @@ use_default:
 	/* The cpu is no longer idle or about to enter idle. */
 	idle_set_state(this_rq(), NULL);
 
-	if (broadcast)
-		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+	if (entered_state == -EBUSY)
+		goto use_default;
 
 	/*
 	 * Give the governor an opportunity to reflect on the outcome

Re: [PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Lorenzo Pieralisi]

On Fri, Feb 27, 2015 at 10:11:54PM +0000, Rafael J. Wysocki wrote:
> On Friday, February 27, 2015 10:00:00 AM Lorenzo Pieralisi wrote:
> > [CC'ed Preeti]
> > 
> > On Thu, Feb 26, 2015 at 11:37:54PM +0000, Rafael J. Wysocki wrote:
> > > Me versions of the two $subject patches follow.
> > 
> > Thank you. I am testing them and I have run into the following issue.
> > 
> > Starting with:
> > 
> > 3810631 ("PM / sleep: Re-implement suspend-to-idle handling")
> > 
> > the suspend-to-idle code path in the cpuidle_idle_call() bypasses
> > the CPUIDLE_FLAG_TIMER_STOP code path entirely.
> 
> Hmm, this looks like a mistake.  Sorry about that.

Thank you for looking into this !

> > Now, on most of
> > the current ARM platforms, the deepest idle state loses the tick device
> > context, therefore this means that going to idle through
> > suspend-to-idle becomes a brute force way of nuking the tick,
> > unless I am missing something here.
> > 
> > I am experiencing hangs on resume from suspend-to-idle when the broadcast
> > timer is the broadast-hrtimer (ie there is no HW broadcast timer in the
> > platform) and the deepest idle states lose the tick device context (ie
> > they are CPUIDLE_FLAG_TIMER_STOP), I hope Preeti can help me test this on
> > Power, still chasing the issue.
> > 
> > I could not reproduce the issue with a HW broadcast timer device.
> > 
> > Platform has deepest idle states that allow CPUs shutdown where the
> > local tick device is gone on entry, I am trying to provide you with a
> > backtrace, I need time to debug.
> > 
> > The question I have: is it safe to bypass the CPUIDLE_FLAG_TIMER_STOP
> > and related broadcast mode entry/exit in the suspend-to-idle path ?
> > 
> > I do not think it is, but I am asking.
> 
> It isn't in general, but it would be OK in the enter_freeze_proper() path
> where the tick is suspended anyway.

Entering state through enter_freeze() (ie adding the function pointer
in the idle states, on platforms where it is appropriate), therefore
freezing the tick solves the issue, so I think we should patch all ARM
drivers that can benefit from this interesting new feature.

> > I can "force" tick freeze by initializing the enter_freeze pointer
> > in the idle states (that's the next thing I will test), but still, for
> > platforms where that's not possible my question above is still valid.
> 
> Right.
> 
> Does the patch below help (on top of the previous ones)?

I need HW to test, I will do that first thing on Monday, and send
you the appropriate tags. I already put together a similar patch
and tested it yesterday, so I can say already that it solves the
problem, I will test your patch on Monday and get back to you
(patch 1 and 2 in this series already tested on a platform with no
CPUidle driver registered, and they work and fix the NULL drv
dereference issue).

Thank you !
Lorenzo

> 
> Rafael
> 
> 
> ---
>  drivers/cpuidle/cpuidle.c |   31 ++++++++++++++++++++++++++-----
>  kernel/sched/idle.c       |   17 ++---------------
>  2 files changed, 28 insertions(+), 20 deletions(-)
> 
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -230,15 +230,36 @@ int cpuidle_select(struct cpuidle_driver
>   * @dev:   the cpuidle device
>   * @index: the index in the idle state table
>   *
> - * Returns the index in the idle state, < 0 in case of error.
> - * The error code depends on the backend driver
> + * Returns the index in the idle state, < 0 in case of error.  -EBUSY is
> + * returned to indicate that the target state was temporarily unavailable.
> + * The other error codes depend on the backend driver.
>   */
>  int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  		  int index)
>  {
> -	if (cpuidle_state_is_coupled(dev, drv, index))
> -		return cpuidle_enter_state_coupled(dev, drv, index);
> -	return cpuidle_enter_state(dev, drv, index);
> +	unsigned int broadcast;
> +	int ret;
> +
> +	broadcast = drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP;
> +
> +	/*
> +	 * Tell the time framework to switch to a broadcast timer
> +	 * because our local timer will be shutdown. If a local timer
> +	 * is used from another cpu as a broadcast timer, this call may
> +	 * fail if it is not available
> +	 */
> +	if (broadcast &&
> +	    clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu))
> +		return -EBUSY;
> +
> +	ret = cpuidle_state_is_coupled(dev, drv, index) ?
> +		cpuidle_enter_state_coupled(dev, drv, index) :
> +		cpuidle_enter_state(dev, drv, index);
> +
> +	if (broadcast)
> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +
> +	return ret;
>  }
>  
>  /**
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
>  	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>  	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>  	int next_state, entered_state;
> -	unsigned int broadcast;
>  
>  	/*
>  	 * Check if the idle task must be rescheduled. If it is the
> @@ -151,18 +150,6 @@ use_default:
>  		goto exit_idle;
>  	}
>  
> -	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
> -
> -	/*
> -	 * Tell the time framework to switch to a broadcast timer
> -	 * because our local timer will be shutdown. If a local timer
> -	 * is used from another cpu as a broadcast timer, this call may
> -	 * fail if it is not available
> -	 */
> -	if (broadcast &&
> -	    clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu))
> -		goto use_default;
> -
>  	/* Take note of the planned idle state. */
>  	idle_set_state(this_rq(), &drv->states[next_state]);
>  
> @@ -176,8 +163,8 @@ use_default:
>  	/* The cpu is no longer idle or about to enter idle. */
>  	idle_set_state(this_rq(), NULL);
>  
> -	if (broadcast)
> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +	if (entered_state == -EBUSY)
> +		goto use_default;
>  
>  	/*
>  	 * Give the governor an opportunity to reflect on the outcome
> 
> 
> 

Re: [PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Rafael J. Wysocki]

On Saturday, February 28, 2015 11:54:23 AM Lorenzo Pieralisi wrote:
> On Fri, Feb 27, 2015 at 10:11:54PM +0000, Rafael J. Wysocki wrote:
> > On Friday, February 27, 2015 10:00:00 AM Lorenzo Pieralisi wrote:
> > > [CC'ed Preeti]
> > > 
> > > On Thu, Feb 26, 2015 at 11:37:54PM +0000, Rafael J. Wysocki wrote:
> > > > Me versions of the two $subject patches follow.
> > > 
> > > Thank you. I am testing them and I have run into the following issue.
> > > 
> > > Starting with:
> > > 
> > > 3810631 ("PM / sleep: Re-implement suspend-to-idle handling")
> > > 
> > > the suspend-to-idle code path in the cpuidle_idle_call() bypasses
> > > the CPUIDLE_FLAG_TIMER_STOP code path entirely.
> > 
> > Hmm, this looks like a mistake.  Sorry about that.
> 
> Thank you for looking into this !
> 
> > > Now, on most of
> > > the current ARM platforms, the deepest idle state loses the tick device
> > > context, therefore this means that going to idle through
> > > suspend-to-idle becomes a brute force way of nuking the tick,
> > > unless I am missing something here.
> > > 
> > > I am experiencing hangs on resume from suspend-to-idle when the broadcast
> > > timer is the broadast-hrtimer (ie there is no HW broadcast timer in the
> > > platform) and the deepest idle states lose the tick device context (ie
> > > they are CPUIDLE_FLAG_TIMER_STOP), I hope Preeti can help me test this on
> > > Power, still chasing the issue.
> > > 
> > > I could not reproduce the issue with a HW broadcast timer device.
> > > 
> > > Platform has deepest idle states that allow CPUs shutdown where the
> > > local tick device is gone on entry, I am trying to provide you with a
> > > backtrace, I need time to debug.
> > > 
> > > The question I have: is it safe to bypass the CPUIDLE_FLAG_TIMER_STOP
> > > and related broadcast mode entry/exit in the suspend-to-idle path ?
> > > 
> > > I do not think it is, but I am asking.
> > 
> > It isn't in general, but it would be OK in the enter_freeze_proper() path
> > where the tick is suspended anyway.
> 
> Entering state through enter_freeze() (ie adding the function pointer
> in the idle states, on platforms where it is appropriate), therefore
> freezing the tick solves the issue, so I think we should patch all ARM
> drivers that can benefit from this interesting new feature.
> 
> > > I can "force" tick freeze by initializing the enter_freeze pointer
> > > in the idle states (that's the next thing I will test), but still, for
> > > platforms where that's not possible my question above is still valid.
> > 
> > Right.
> > 
> > Does the patch below help (on top of the previous ones)?
> 
> I need HW to test, I will do that first thing on Monday, and send
> you the appropriate tags. I already put together a similar patch
> and tested it yesterday, so I can say already that it solves the
> problem, I will test your patch on Monday and get back to you
> (patch 1 and 2 in this series already tested on a platform with no
> CPUidle driver registered, and they work and fix the NULL drv
> dereference issue).

Cool, thanks!

Below is a slightly cleaner version of the patch with a changelog.  Can you
please test this one?

Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: cpuidle / sleep: Use broadcast timer for states that stop local timer

Commit 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
overlooked the fact that entering some sufficiently deep idle states
by CPUs may cause their local timers to stop and in those cases it
is necessary to switch over to a broadcase timer prior to entering
the idle state.  If the cpuidle driver in use does not provide
the new ->enter_freeze callback for any of the idle states, that
problem affects suspend-to-idle too, but it is not taken into account
after the changes made by commit 381063133246.

Fix that by moving the CPUIDLE_FLAG_TIMER_STOP flag check and the
broadcast timer manipulations following it from cpuidle_idle_call()
to cpuidle_enter() which will cause them to be done, if necessary,
in the suspend-to-idle case as well as for runtime idle.

Fixes: 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |   34 +++++++++++++++++++++++++++++-----
 kernel/sched/idle.c       |   17 ++---------------
 2 files changed, 31 insertions(+), 20 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -230,15 +230,39 @@ int cpuidle_select(struct cpuidle_driver
  * @dev:   the cpuidle device
  * @index: the index in the idle state table
  *
- * Returns the index in the idle state, < 0 in case of error.
- * The error code depends on the backend driver
+ * Returns the index in the idle state, < 0 in case of error.  -EBUSY is
+ * returned to indicate that the target state was temporarily inaccessible.
+ * The other error codes depend on the backend driver.
  */
 int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		  int index)
 {
-	if (cpuidle_state_is_coupled(dev, drv, index))
-		return cpuidle_enter_state_coupled(dev, drv, index);
-	return cpuidle_enter_state(dev, drv, index);
+	unsigned int broadcast;
+	int ret;
+
+	broadcast = drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP;
+
+	/*
+	 * Tell the time framework to switch to a broadcast timer
+	 * because our local timer will be shutdown. If a local timer
+	 * is used from another cpu as a broadcast timer, this call may
+	 * fail if it is not available
+	 */
+	if (broadcast) {
+		ret = clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
+					 &dev->cpu);
+		if (ret)
+			return ret;
+	}
+
+	ret = cpuidle_state_is_coupled(dev, drv, index) ?
+		cpuidle_enter_state_coupled(dev, drv, index) :
+		cpuidle_enter_state(dev, drv, index);
+
+	if (broadcast)
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+
+	return ret;
 }
 
 /**
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
-	unsigned int broadcast;
 
 	/*
 	 * Check if the idle task must be rescheduled. If it is the
@@ -151,18 +150,6 @@ use_default:
 		goto exit_idle;
 	}
 
-	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
-
-	/*
-	 * Tell the time framework to switch to a broadcast timer
-	 * because our local timer will be shutdown. If a local timer
-	 * is used from another cpu as a broadcast timer, this call may
-	 * fail if it is not available
-	 */
-	if (broadcast &&
-	    clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu))
-		goto use_default;
-
 	/* Take note of the planned idle state. */
 	idle_set_state(this_rq(), &drv->states[next_state]);
 
@@ -176,8 +163,8 @@ use_default:
 	/* The cpu is no longer idle or about to enter idle. */
 	idle_set_state(this_rq(), NULL);
 
-	if (broadcast)
-		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+	if (entered_state == -EBUSY)
+		goto use_default;
 
 	/*
 	 * Give the governor an opportunity to reflect on the outcome

Re: [PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Lorenzo Pieralisi]

On Sat, Feb 28, 2015 at 11:58:21PM +0000, Rafael J. Wysocki wrote:
> On Saturday, February 28, 2015 11:54:23 AM Lorenzo Pieralisi wrote:
> > On Fri, Feb 27, 2015 at 10:11:54PM +0000, Rafael J. Wysocki wrote:
> > > On Friday, February 27, 2015 10:00:00 AM Lorenzo Pieralisi wrote:
> > > > [CC'ed Preeti]
> > > > 
> > > > On Thu, Feb 26, 2015 at 11:37:54PM +0000, Rafael J. Wysocki wrote:
> > > > > Me versions of the two $subject patches follow.
> > > > 
> > > > Thank you. I am testing them and I have run into the following issue.
> > > > 
> > > > Starting with:
> > > > 
> > > > 3810631 ("PM / sleep: Re-implement suspend-to-idle handling")
> > > > 
> > > > the suspend-to-idle code path in the cpuidle_idle_call() bypasses
> > > > the CPUIDLE_FLAG_TIMER_STOP code path entirely.
> > > 
> > > Hmm, this looks like a mistake.  Sorry about that.
> > 
> > Thank you for looking into this !
> > 
> > > > Now, on most of
> > > > the current ARM platforms, the deepest idle state loses the tick device
> > > > context, therefore this means that going to idle through
> > > > suspend-to-idle becomes a brute force way of nuking the tick,
> > > > unless I am missing something here.
> > > > 
> > > > I am experiencing hangs on resume from suspend-to-idle when the broadcast
> > > > timer is the broadast-hrtimer (ie there is no HW broadcast timer in the
> > > > platform) and the deepest idle states lose the tick device context (ie
> > > > they are CPUIDLE_FLAG_TIMER_STOP), I hope Preeti can help me test this on
> > > > Power, still chasing the issue.
> > > > 
> > > > I could not reproduce the issue with a HW broadcast timer device.
> > > > 
> > > > Platform has deepest idle states that allow CPUs shutdown where the
> > > > local tick device is gone on entry, I am trying to provide you with a
> > > > backtrace, I need time to debug.
> > > > 
> > > > The question I have: is it safe to bypass the CPUIDLE_FLAG_TIMER_STOP
> > > > and related broadcast mode entry/exit in the suspend-to-idle path ?
> > > > 
> > > > I do not think it is, but I am asking.
> > > 
> > > It isn't in general, but it would be OK in the enter_freeze_proper() path
> > > where the tick is suspended anyway.
> > 
> > Entering state through enter_freeze() (ie adding the function pointer
> > in the idle states, on platforms where it is appropriate), therefore
> > freezing the tick solves the issue, so I think we should patch all ARM
> > drivers that can benefit from this interesting new feature.
> > 
> > > > I can "force" tick freeze by initializing the enter_freeze pointer
> > > > in the idle states (that's the next thing I will test), but still, for
> > > > platforms where that's not possible my question above is still valid.
> > > 
> > > Right.
> > > 
> > > Does the patch below help (on top of the previous ones)?
> > 
> > I need HW to test, I will do that first thing on Monday, and send
> > you the appropriate tags. I already put together a similar patch
> > and tested it yesterday, so I can say already that it solves the
> > problem, I will test your patch on Monday and get back to you
> > (patch 1 and 2 in this series already tested on a platform with no
> > CPUidle driver registered, and they work and fix the NULL drv
> > dereference issue).
> 
> Cool, thanks!
> 
> Below is a slightly cleaner version of the patch with a changelog.  Can you
> please test this one?
> 
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: cpuidle / sleep: Use broadcast timer for states that stop local timer
> 
> Commit 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
> overlooked the fact that entering some sufficiently deep idle states
> by CPUs may cause their local timers to stop and in those cases it
> is necessary to switch over to a broadcase timer prior to entering
> the idle state.  If the cpuidle driver in use does not provide
> the new ->enter_freeze callback for any of the idle states, that
> problem affects suspend-to-idle too, but it is not taken into account
> after the changes made by commit 381063133246.
> 
> Fix that by moving the CPUIDLE_FLAG_TIMER_STOP flag check and the
> broadcast timer manipulations following it from cpuidle_idle_call()
> to cpuidle_enter() which will cause them to be done, if necessary,
> in the suspend-to-idle case as well as for runtime idle.
> 
> Fixes: 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/cpuidle.c |   34 +++++++++++++++++++++++++++++-----
>  kernel/sched/idle.c       |   17 ++---------------
>  2 files changed, 31 insertions(+), 20 deletions(-)
> 
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -230,15 +230,39 @@ int cpuidle_select(struct cpuidle_driver
>   * @dev:   the cpuidle device
>   * @index: the index in the idle state table
>   *
> - * Returns the index in the idle state, < 0 in case of error.
> - * The error code depends on the backend driver
> + * Returns the index in the idle state, < 0 in case of error.  -EBUSY is
> + * returned to indicate that the target state was temporarily inaccessible.
> + * The other error codes depend on the backend driver.
>   */
>  int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  		  int index)
>  {
> -	if (cpuidle_state_is_coupled(dev, drv, index))
> -		return cpuidle_enter_state_coupled(dev, drv, index);
> -	return cpuidle_enter_state(dev, drv, index);
> +	unsigned int broadcast;
> +	int ret;
> +
> +	broadcast = drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP;
> +
> +	/*
> +	 * Tell the time framework to switch to a broadcast timer
> +	 * because our local timer will be shutdown. If a local timer
> +	 * is used from another cpu as a broadcast timer, this call may
> +	 * fail if it is not available
> +	 */
> +	if (broadcast) {
> +		ret = clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> +					 &dev->cpu);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = cpuidle_state_is_coupled(dev, drv, index) ?
> +		cpuidle_enter_state_coupled(dev, drv, index) :
> +		cpuidle_enter_state(dev, drv, index);
> +
> +	if (broadcast)
> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +
> +	return ret;

You have to check this return value in cpuidle_enter_freeze() too
otherwise we return to the idle thread on -EBUSY without
even executing arch_cpu_idle() and with IRQ disabled, code hits
the WARN_ON_ONCE line 180.

There are multiple ways of fixing the issue, either you check the
cpuidle_enter_freeze() return value (you add one) to cpuidle_idle_call()
to make code consistent with the cpuidle_idle_call "normal" idle
behaviour or you add the return value check in cpuidle_enter_freeze(),
I am fine both ways.

Thanks,
Lorenzo

>  }
>  
>  /**
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
>  	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>  	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>  	int next_state, entered_state;
> -	unsigned int broadcast;
>  
>  	/*
>  	 * Check if the idle task must be rescheduled. If it is the
> @@ -151,18 +150,6 @@ use_default:
>  		goto exit_idle;
>  	}
>  
> -	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
> -
> -	/*
> -	 * Tell the time framework to switch to a broadcast timer
> -	 * because our local timer will be shutdown. If a local timer
> -	 * is used from another cpu as a broadcast timer, this call may
> -	 * fail if it is not available
> -	 */
> -	if (broadcast &&
> -	    clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu))
> -		goto use_default;
> -
>  	/* Take note of the planned idle state. */
>  	idle_set_state(this_rq(), &drv->states[next_state]);
>  
> @@ -176,8 +163,8 @@ use_default:
>  	/* The cpu is no longer idle or about to enter idle. */
>  	idle_set_state(this_rq(), NULL);
>  
> -	if (broadcast)
> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +	if (entered_state == -EBUSY)
> +		goto use_default;
>  
>  	/*
>  	 * Give the governor an opportunity to reflect on the outcome
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH 0/2] drivers: cpuidle: minor suspend-to-idle fixes

[Rafael J. Wysocki]

On Monday, March 02, 2015 10:08:23 AM Lorenzo Pieralisi wrote:
> On Sat, Feb 28, 2015 at 11:58:21PM +0000, Rafael J. Wysocki wrote:
> > On Saturday, February 28, 2015 11:54:23 AM Lorenzo Pieralisi wrote:

[cut]

> > Index: linux-pm/drivers/cpuidle/cpuidle.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> > +++ linux-pm/drivers/cpuidle/cpuidle.c
> > @@ -230,15 +230,39 @@ int cpuidle_select(struct cpuidle_driver
> >   * @dev:   the cpuidle device
> >   * @index: the index in the idle state table
> >   *
> > - * Returns the index in the idle state, < 0 in case of error.
> > - * The error code depends on the backend driver
> > + * Returns the index in the idle state, < 0 in case of error.  -EBUSY is
> > + * returned to indicate that the target state was temporarily inaccessible.
> > + * The other error codes depend on the backend driver.
> >   */
> >  int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> >  		  int index)
> >  {
> > -	if (cpuidle_state_is_coupled(dev, drv, index))
> > -		return cpuidle_enter_state_coupled(dev, drv, index);
> > -	return cpuidle_enter_state(dev, drv, index);
> > +	unsigned int broadcast;
> > +	int ret;
> > +
> > +	broadcast = drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP;
> > +
> > +	/*
> > +	 * Tell the time framework to switch to a broadcast timer
> > +	 * because our local timer will be shutdown. If a local timer
> > +	 * is used from another cpu as a broadcast timer, this call may
> > +	 * fail if it is not available
> > +	 */
> > +	if (broadcast) {
> > +		ret = clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> > +					 &dev->cpu);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = cpuidle_state_is_coupled(dev, drv, index) ?
> > +		cpuidle_enter_state_coupled(dev, drv, index) :
> > +		cpuidle_enter_state(dev, drv, index);
> > +
> > +	if (broadcast)
> > +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> > +
> > +	return ret;
> 
> You have to check this return value in cpuidle_enter_freeze() too
> otherwise we return to the idle thread on -EBUSY without
> even executing arch_cpu_idle() and with IRQ disabled, code hits
> the WARN_ON_ONCE line 180.

Right.

> There are multiple ways of fixing the issue, either you check the
> cpuidle_enter_freeze() return value (you add one) to cpuidle_idle_call()
> to make code consistent with the cpuidle_idle_call "normal" idle
> behaviour or you add the return value check in cpuidle_enter_freeze(),
> I am fine both ways.

Well, in both cases we'd end up with a function enabling interrupts on exit
in some cases and not doing that in some other ones.  Not nice.

Below is an alternative to that (on top of the previous patches).  Can you
test it please?

Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: cpuidle / sleep: Use broadcast timer for states that stop local timer

Commit 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
overlooked the fact that entering some sufficiently deep idle states
by CPUs may cause their local timers to stop and in those cases it
is necessary to switch over to a broadcase timer prior to entering
the idle state.  If the cpuidle driver in use does not provide
the new ->enter_freeze callback for any of the idle states, that
problem affects suspend-to-idle too, but it is not taken into account
after the changes made by commit 381063133246.

Fix that by changing the definition of cpuidle_enter_freeze() and
re-arranging of the code in cpuidle_idle_call(), so the former does
not call cpuidle_enter() any more and the fallback case is handled
by cpuidle_idle_call() directly.

Fixes: 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |   53 +++++++++++++++++++---------------------------
 include/linux/cpuidle.h   |    8 +++++-
 kernel/sched/idle.c       |   18 +++++++++++++--
 3 files changed, 43 insertions(+), 36 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,9 +44,11 @@ void disable_cpuidle(void)
 	off = 1;
 }
 
-static bool cpuidle_not_available(struct cpuidle_driver *drv,
-				  struct cpuidle_device *dev)
+bool cpuidle_not_available(void)
 {
+	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
 	return off || !initialized || !drv || !dev || !dev->enabled;
 }
 
@@ -73,13 +75,13 @@ int cpuidle_play_dead(void)
 }
 
 /**
- * cpuidle_find_deepest_state - Find deepest state meeting specific conditions.
+ * find_deepest_state - Find deepest state meeting specific conditions.
  * @drv: cpuidle driver for the given CPU.
  * @dev: cpuidle device for the given CPU.
  * @freeze: Whether or not the state should be suitable for suspend-to-idle.
  */
-static int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
-				      struct cpuidle_device *dev, bool freeze)
+static int find_deepest_state(struct cpuidle_driver *drv,
+			      struct cpuidle_device *dev, bool freeze)
 {
 	unsigned int latency_req = 0;
 	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
@@ -98,6 +100,17 @@ static int cpuidle_find_deepest_state(st
 	return ret;
 }
 
+/**
+ * cpuidle_find_deepest_state - Find the deepest available idle state.
+ */
+int cpuidle_find_deepest_state(void)
+{
+	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
+	return find_deepest_state(drv, dev, false);
+}
+
 static void enter_freeze_proper(struct cpuidle_driver *drv,
 				struct cpuidle_device *dev, int index)
 {
@@ -123,42 +136,23 @@ static void enter_freeze_proper(struct c
  * If there are states with the ->enter_freeze callback, find the deepest of
  * them and enter it with frozen tick.  Otherwise, find the deepest state
  * available and enter it normally.
- *
- * Returns with enabled interrupts.
  */
-void cpuidle_enter_freeze(void)
+int cpuidle_enter_freeze(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int index;
 
-	if (cpuidle_not_available(drv, dev))
-		goto fallback;
-
 	/*
 	 * Find the deepest state with ->enter_freeze present, which guarantees
 	 * that interrupts won't be enabled when it exits and allows the tick to
 	 * be frozen safely.
 	 */
-	index = cpuidle_find_deepest_state(drv, dev, true);
-	if (index >= 0) {
+	index = find_deepest_state(drv, dev, true);
+	if (index >= 0)
 		enter_freeze_proper(drv, dev, index);
-		local_irq_enable();
-		return;
-	}
-
-	/*
-	 * It is not safe to freeze the tick, find the deepest state available
-	 * at all and try to enter it normally.
-	 */
-	index = cpuidle_find_deepest_state(drv, dev, false);
-	if (index >= 0) {
-		cpuidle_enter(drv, dev, index);
-		return;
-	}
 
- fallback:
-	arch_cpu_idle();
+	return index;
 }
 
 /**
@@ -217,9 +211,6 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-	if (cpuidle_not_available(drv, dev))
-		return -ENODEV;
-
 	return cpuidle_curr_governor->select(drv, dev);
 }
 
Index: linux-pm/include/linux/cpuidle.h
===================================================================
--- linux-pm.orig/include/linux/cpuidle.h
+++ linux-pm/include/linux/cpuidle.h
@@ -126,6 +126,7 @@ struct cpuidle_driver {
 
 #ifdef CONFIG_CPU_IDLE
 extern void disable_cpuidle(void);
+extern bool cpuidle_not_available(void);
 
 extern int cpuidle_select(struct cpuidle_driver *drv,
 			  struct cpuidle_device *dev);
@@ -150,11 +151,13 @@ extern void cpuidle_resume(void);
 extern int cpuidle_enable_device(struct cpuidle_device *dev);
 extern void cpuidle_disable_device(struct cpuidle_device *dev);
 extern int cpuidle_play_dead(void);
-extern void cpuidle_enter_freeze(void);
+extern int cpuidle_find_deepest_state(void);
+extern int cpuidle_enter_freeze(void);
 
 extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
 #else
 static inline void disable_cpuidle(void) { }
+static inline bool cpuidle_not_available(void) {return true; }
 static inline int cpuidle_select(struct cpuidle_driver *drv,
 				 struct cpuidle_device *dev)
 {return -ENODEV; }
@@ -183,7 +186,8 @@ static inline int cpuidle_enable_device(
 {return -ENODEV; }
 static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
 static inline int cpuidle_play_dead(void) {return -ENODEV; }
-static inline void cpuidle_enter_freeze(void) { }
+static inline int cpuidle_find_deepest_state(void) {return -ENODEV; }
+static inline int cpuidle_enter_freeze(void) {return -ENODEV; }
 static inline struct cpuidle_driver *cpuidle_get_cpu_driver(
 	struct cpuidle_device *dev) {return NULL; }
 #endif
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -105,6 +105,9 @@ static void cpuidle_idle_call(void)
 	 */
 	rcu_idle_enter();
 
+	if (cpuidle_not_available())
+		goto use_default;
+
 	/*
 	 * Suspend-to-idle ("freeze") is a system state in which all user space
 	 * has been frozen, all I/O devices have been suspended and the only
@@ -115,8 +118,17 @@ static void cpuidle_idle_call(void)
 	 * until a proper wakeup interrupt happens.
 	 */
 	if (idle_should_freeze()) {
-		cpuidle_enter_freeze();
-		goto exit_idle;
+		entered_state = cpuidle_enter_freeze();
+		if (entered_state >= 0) {
+			local_irq_enable();
+			goto exit_idle;
+		}
+
+		next_state = cpuidle_find_deepest_state();
+		if (next_state >= 0)
+			goto enter_idle;
+		else
+			goto use_default;
 	}
 
 	/*
@@ -138,7 +150,7 @@ use_default:
 		goto exit_idle;
 	}
 
-
+enter_idle:
 	/*
 	 * The idle task must be scheduled, it is pointless to
 	 * go to idle, just update no idle residency and get

[PATCH 0/2] cpuidle / sleep: fix timer stopping regression (was: drivers: cpuidle: minor suspend-to-idle fixes)

[Rafael J. Wysocki]

On Monday, March 02, 2015 02:13:05 PM Rafael J. Wysocki wrote:
> On Monday, March 02, 2015 10:08:23 AM Lorenzo Pieralisi wrote:
> > On Sat, Feb 28, 2015 at 11:58:21PM +0000, Rafael J. Wysocki wrote:
> > > On Saturday, February 28, 2015 11:54:23 AM Lorenzo Pieralisi wrote:
> 
> [cut]
> 
> > > Index: linux-pm/drivers/cpuidle/cpuidle.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> > > +++ linux-pm/drivers/cpuidle/cpuidle.c
> > > @@ -230,15 +230,39 @@ int cpuidle_select(struct cpuidle_driver
> > >   * @dev:   the cpuidle device
> > >   * @index: the index in the idle state table
> > >   *
> > > - * Returns the index in the idle state, < 0 in case of error.
> > > - * The error code depends on the backend driver
> > > + * Returns the index in the idle state, < 0 in case of error.  -EBUSY is
> > > + * returned to indicate that the target state was temporarily inaccessible.
> > > + * The other error codes depend on the backend driver.
> > >   */
> > >  int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > >  		  int index)
> > >  {
> > > -	if (cpuidle_state_is_coupled(dev, drv, index))
> > > -		return cpuidle_enter_state_coupled(dev, drv, index);
> > > -	return cpuidle_enter_state(dev, drv, index);
> > > +	unsigned int broadcast;
> > > +	int ret;
> > > +
> > > +	broadcast = drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP;
> > > +
> > > +	/*
> > > +	 * Tell the time framework to switch to a broadcast timer
> > > +	 * because our local timer will be shutdown. If a local timer
> > > +	 * is used from another cpu as a broadcast timer, this call may
> > > +	 * fail if it is not available
> > > +	 */
> > > +	if (broadcast) {
> > > +		ret = clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> > > +					 &dev->cpu);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	ret = cpuidle_state_is_coupled(dev, drv, index) ?
> > > +		cpuidle_enter_state_coupled(dev, drv, index) :
> > > +		cpuidle_enter_state(dev, drv, index);
> > > +
> > > +	if (broadcast)
> > > +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> > > +
> > > +	return ret;
> > 
> > You have to check this return value in cpuidle_enter_freeze() too
> > otherwise we return to the idle thread on -EBUSY without
> > even executing arch_cpu_idle() and with IRQ disabled, code hits
> > the WARN_ON_ONCE line 180.
> 
> Right.
> 
> > There are multiple ways of fixing the issue, either you check the
> > cpuidle_enter_freeze() return value (you add one) to cpuidle_idle_call()
> > to make code consistent with the cpuidle_idle_call "normal" idle
> > behaviour or you add the return value check in cpuidle_enter_freeze(),
> > I am fine both ways.
> 
> Well, in both cases we'd end up with a function enabling interrupts on exit
> in some cases and not doing that in some other ones.  Not nice.
> 
> Below is an alternative to that (on top of the previous patches).  Can you
> test it please?

Actually, this one is still slightly incorrect, because we only should call
cpuidle_reflect() if we've called cpuidle_select() before.  Also it's better
to pass cpuidle_driver and cpuidle_device to all functions called by
cpuidle_idle_call().

Two patches will follow.  [1/2] is a cleanup re-arranging the code in
cpuidle_idle_call() to move the fallback path to the end of the function.
[2/2] is a replacement for the patch sent previously.

Please test.

Rafael

[PATCH 1/2] cpuidle: Clean up fallback handling in cpuidle_idle_call()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move the fallback code path in cpuidle_idle_call() to the end of the
function to avoid jumping to a label in a an if () branch.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 kernel/sched/idle.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -124,20 +124,8 @@ static void cpuidle_idle_call(void)
 	 * Fall back to the default arch idle method on errors.
 	 */
 	next_state = cpuidle_select(drv, dev);
-	if (next_state < 0) {
-use_default:
-		/*
-		 * We can't use the cpuidle framework, let's use the default
-		 * idle routine.
-		 */
-		if (current_clr_polling_and_test())
-			local_irq_enable();
-		else
-			arch_cpu_idle();
-
-		goto exit_idle;
-	}
-
+	if (next_state < 0)
+		goto use_default;
 
 	/*
 	 * The idle task must be scheduled, it is pointless to
@@ -195,6 +183,19 @@ exit_idle:
 
 	rcu_idle_exit();
 	start_critical_timings();
+	return;
+
+use_default:
+	/*
+	 * We can't use the cpuidle framework, let's use the default
+	 * idle routine.
+	 */
+	if (current_clr_polling_and_test())
+		local_irq_enable();
+	else
+		arch_cpu_idle();
+
+	goto exit_idle;
 }
 
 /*

Re: [PATCH 1/2] cpuidle: Clean up fallback handling in cpuidle_idle_call()

[Lorenzo Pieralisi]

On Mon, Mar 02, 2015 at 02:51:35PM +0000, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move the fallback code path in cpuidle_idle_call() to the end of the
> function to avoid jumping to a label in a an if () branch.

Nit: "in an if () branch"

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  kernel/sched/idle.c |   29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -124,20 +124,8 @@ static void cpuidle_idle_call(void)
>  	 * Fall back to the default arch idle method on errors.
>  	 */
>  	next_state = cpuidle_select(drv, dev);
> -	if (next_state < 0) {
> -use_default:
> -		/*
> -		 * We can't use the cpuidle framework, let's use the default
> -		 * idle routine.
> -		 */
> -		if (current_clr_polling_and_test())
> -			local_irq_enable();
> -		else
> -			arch_cpu_idle();
> -
> -		goto exit_idle;
> -	}
> -
> +	if (next_state < 0)
> +		goto use_default;
>  
>  	/*
>  	 * The idle task must be scheduled, it is pointless to
> @@ -195,6 +183,19 @@ exit_idle:
>  
>  	rcu_idle_exit();
>  	start_critical_timings();
> +	return;
> +
> +use_default:
> +	/*
> +	 * We can't use the cpuidle framework, let's use the default
> +	 * idle routine.
> +	 */
> +	if (current_clr_polling_and_test())
> +		local_irq_enable();
> +	else
> +		arch_cpu_idle();
> +
> +	goto exit_idle;

I wonder whether making the code at label use_default a function saves us
some jumping around, not sure it is worth the churn, your call, I am ok as
it is.

Lorenzo

Re: [PATCH 1/2] cpuidle: Clean up fallback handling in cpuidle_idle_call()

[Rafael J. Wysocki]

On Monday, March 02, 2015 04:05:36 PM Lorenzo Pieralisi wrote:
> On Mon, Mar 02, 2015 at 02:51:35PM +0000, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Move the fallback code path in cpuidle_idle_call() to the end of the
> > function to avoid jumping to a label in a an if () branch.
> 
> Nit: "in an if () branch"
> 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  kernel/sched/idle.c |   29 +++++++++++++++--------------
> >  1 file changed, 15 insertions(+), 14 deletions(-)
> > 
> > Index: linux-pm/kernel/sched/idle.c
> > ===================================================================
> > --- linux-pm.orig/kernel/sched/idle.c
> > +++ linux-pm/kernel/sched/idle.c
> > @@ -124,20 +124,8 @@ static void cpuidle_idle_call(void)
> >  	 * Fall back to the default arch idle method on errors.
> >  	 */
> >  	next_state = cpuidle_select(drv, dev);
> > -	if (next_state < 0) {
> > -use_default:
> > -		/*
> > -		 * We can't use the cpuidle framework, let's use the default
> > -		 * idle routine.
> > -		 */
> > -		if (current_clr_polling_and_test())
> > -			local_irq_enable();
> > -		else
> > -			arch_cpu_idle();
> > -
> > -		goto exit_idle;
> > -	}
> > -
> > +	if (next_state < 0)
> > +		goto use_default;
> >  
> >  	/*
> >  	 * The idle task must be scheduled, it is pointless to
> > @@ -195,6 +183,19 @@ exit_idle:
> >  
> >  	rcu_idle_exit();
> >  	start_critical_timings();
> > +	return;
> > +
> > +use_default:
> > +	/*
> > +	 * We can't use the cpuidle framework, let's use the default
> > +	 * idle routine.
> > +	 */
> > +	if (current_clr_polling_and_test())
> > +		local_irq_enable();
> > +	else
> > +		arch_cpu_idle();
> > +
> > +	goto exit_idle;
> 
> I wonder whether making the code at label use_default a function saves us
> some jumping around, not sure it is worth the churn, your call, I am ok as
> it is.

We'd need to jump anyway, we might only need to jump one time less and we'd
need to call the function for multiple times instead if I'm not mistaken.

I'm going to apply the current version.

Rafael

[PATCH 2/2] cpuidle / sleep: Use broadcast timer for states that stop local timer

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
overlooked the fact that entering some sufficiently deep idle states
by CPUs may cause their local timers to stop and in those cases it
is necessary to switch over to a broadcase timer prior to entering
the idle state.  If the cpuidle driver in use does not provide
the new ->enter_freeze callback for any of the idle states, that
problem affects suspend-to-idle too, but it is not taken into account
after the changes made by commit 381063133246.

Fix that by changing the definition of cpuidle_enter_freeze() and
re-arranging of the code in cpuidle_idle_call(), so the former does
not call cpuidle_enter() any more and the fallback case is handled
by cpuidle_idle_call() directly.

Fixes: 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |   59 ++++++++++++++++------------------------------
 include/linux/cpuidle.h   |   17 +++++++++++--
 kernel/sched/idle.c       |   30 ++++++++++++++++-------
 3 files changed, 57 insertions(+), 49 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,8 +44,8 @@ void disable_cpuidle(void)
 	off = 1;
 }
 
-static bool cpuidle_not_available(struct cpuidle_driver *drv,
-				  struct cpuidle_device *dev)
+bool cpuidle_not_available(struct cpuidle_driver *drv,
+			   struct cpuidle_device *dev)
 {
 	return off || !initialized || !drv || !dev || !dev->enabled;
 }
@@ -72,14 +72,8 @@ int cpuidle_play_dead(void)
 	return -ENODEV;
 }
 
-/**
- * cpuidle_find_deepest_state - Find deepest state meeting specific conditions.
- * @drv: cpuidle driver for the given CPU.
- * @dev: cpuidle device for the given CPU.
- * @freeze: Whether or not the state should be suitable for suspend-to-idle.
- */
-static int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
-				      struct cpuidle_device *dev, bool freeze)
+static int find_deepest_state(struct cpuidle_driver *drv,
+			      struct cpuidle_device *dev, bool freeze)
 {
 	unsigned int latency_req = 0;
 	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
@@ -98,6 +92,17 @@ static int cpuidle_find_deepest_state(st
 	return ret;
 }
 
+/**
+ * cpuidle_find_deepest_state - Find the deepest available idle state.
+ * @drv: cpuidle driver for the given CPU.
+ * @dev: cpuidle device for the given CPU.
+ */
+int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
+			       struct cpuidle_device *dev)
+{
+	return find_deepest_state(drv, dev, false);
+}
+
 static void enter_freeze_proper(struct cpuidle_driver *drv,
 				struct cpuidle_device *dev, int index)
 {
@@ -119,46 +124,27 @@ static void enter_freeze_proper(struct c
 
 /**
  * cpuidle_enter_freeze - Enter an idle state suitable for suspend-to-idle.
+ * @drv: cpuidle driver for the given CPU.
+ * @dev: cpuidle device for the given CPU.
  *
  * If there are states with the ->enter_freeze callback, find the deepest of
  * them and enter it with frozen tick.  Otherwise, find the deepest state
  * available and enter it normally.
- *
- * Returns with enabled interrupts.
  */
-void cpuidle_enter_freeze(void)
+int cpuidle_enter_freeze(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int index;
 
-	if (cpuidle_not_available(drv, dev))
-		goto fallback;
-
 	/*
 	 * Find the deepest state with ->enter_freeze present, which guarantees
 	 * that interrupts won't be enabled when it exits and allows the tick to
 	 * be frozen safely.
 	 */
-	index = cpuidle_find_deepest_state(drv, dev, true);
-	if (index >= 0) {
+	index = find_deepest_state(drv, dev, true);
+	if (index >= 0)
 		enter_freeze_proper(drv, dev, index);
-		local_irq_enable();
-		return;
-	}
 
-	/*
-	 * It is not safe to freeze the tick, find the deepest state available
-	 * at all and try to enter it normally.
-	 */
-	index = cpuidle_find_deepest_state(drv, dev, false);
-	if (index >= 0) {
-		cpuidle_enter(drv, dev, index);
-		return;
-	}
-
- fallback:
-	arch_cpu_idle();
+	return index;
 }
 
 /**
@@ -217,9 +203,6 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-	if (cpuidle_not_available(drv, dev))
-		return -ENODEV;
-
 	return cpuidle_curr_governor->select(drv, dev);
 }
 
Index: linux-pm/include/linux/cpuidle.h
===================================================================
--- linux-pm.orig/include/linux/cpuidle.h
+++ linux-pm/include/linux/cpuidle.h
@@ -126,6 +126,8 @@ struct cpuidle_driver {
 
 #ifdef CONFIG_CPU_IDLE
 extern void disable_cpuidle(void);
+extern bool cpuidle_not_available(struct cpuidle_driver *drv,
+				  struct cpuidle_device *dev);
 
 extern int cpuidle_select(struct cpuidle_driver *drv,
 			  struct cpuidle_device *dev);
@@ -150,11 +152,17 @@ extern void cpuidle_resume(void);
 extern int cpuidle_enable_device(struct cpuidle_device *dev);
 extern void cpuidle_disable_device(struct cpuidle_device *dev);
 extern int cpuidle_play_dead(void);
-extern void cpuidle_enter_freeze(void);
+extern int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
+				      struct cpuidle_device *dev);
+extern int cpuidle_enter_freeze(struct cpuidle_driver *drv,
+				struct cpuidle_device *dev);
 
 extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
 #else
 static inline void disable_cpuidle(void) { }
+static inline bool cpuidle_not_available(struct cpuidle_driver *drv,
+					 struct cpuidle_device *dev)
+{return true; }
 static inline int cpuidle_select(struct cpuidle_driver *drv,
 				 struct cpuidle_device *dev)
 {return -ENODEV; }
@@ -183,7 +191,12 @@ static inline int cpuidle_enable_device(
 {return -ENODEV; }
 static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
 static inline int cpuidle_play_dead(void) {return -ENODEV; }
-static inline void cpuidle_enter_freeze(void) { }
+static inline int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
+					     struct cpuidle_device *dev)
+{return -ENODEV; }
+static inline int cpuidle_enter_freeze(struct cpuidle_driver *drv,
+				       struct cpuidle_device *dev)
+{return -ENODEV; }
 static inline struct cpuidle_driver *cpuidle_get_cpu_driver(
 	struct cpuidle_device *dev) {return NULL; }
 #endif
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -82,6 +82,7 @@ static void cpuidle_idle_call(void)
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
 	unsigned int broadcast;
+	bool reflect;
 
 	/*
 	 * Check if the idle task must be rescheduled. If it is the
@@ -105,6 +106,9 @@ static void cpuidle_idle_call(void)
 	 */
 	rcu_idle_enter();
 
+	if (cpuidle_not_available(drv, dev))
+		goto use_default;
+
 	/*
 	 * Suspend-to-idle ("freeze") is a system state in which all user space
 	 * has been frozen, all I/O devices have been suspended and the only
@@ -115,15 +119,22 @@ static void cpuidle_idle_call(void)
 	 * until a proper wakeup interrupt happens.
 	 */
 	if (idle_should_freeze()) {
-		cpuidle_enter_freeze();
-		goto exit_idle;
+		entered_state = cpuidle_enter_freeze(drv, dev);
+		if (entered_state >= 0) {
+			local_irq_enable();
+			goto exit_idle;
+		}
+
+		reflect = false;
+		next_state = cpuidle_find_deepest_state(drv, dev);
+	} else {
+		reflect = true;
+		/*
+		 * Ask the cpuidle framework to choose a convenient idle state.
+		 */
+		next_state = cpuidle_select(drv, dev);
 	}
-
-	/*
-	 * Ask the cpuidle framework to choose a convenient idle state.
-	 * Fall back to the default arch idle method on errors.
-	 */
-	next_state = cpuidle_select(drv, dev);
+	/* Fall back to the default arch idle method on errors. */
 	if (next_state < 0)
 		goto use_default;
 
@@ -170,7 +181,8 @@ static void cpuidle_idle_call(void)
 	/*
 	 * Give the governor an opportunity to reflect on the outcome
 	 */
-	cpuidle_reflect(dev, entered_state);
+	if (reflect)
+		cpuidle_reflect(dev, entered_state);
 
 exit_idle:
 	__current_set_polling();

Re: [PATCH 2/2] cpuidle / sleep: Use broadcast timer for states that stop local timer

[Lorenzo Pieralisi]

On Mon, Mar 02, 2015 at 02:53:28PM +0000, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
> overlooked the fact that entering some sufficiently deep idle states
> by CPUs may cause their local timers to stop and in those cases it
> is necessary to switch over to a broadcase timer prior to entering

s/broadcase/broadcast

> the idle state.  If the cpuidle driver in use does not provide
> the new ->enter_freeze callback for any of the idle states, that
> problem affects suspend-to-idle too, but it is not taken into account
> after the changes made by commit 381063133246.
> 
> Fix that by changing the definition of cpuidle_enter_freeze() and
> re-arranging of the code in cpuidle_idle_call(), so the former does
> not call cpuidle_enter() any more and the fallback case is handled
> by cpuidle_idle_call() directly.
> 
> Fixes: 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Minor comment below, otherwise on the 4-patch series:

Tested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

[...]

>  /**
>   * cpuidle_enter_freeze - Enter an idle state suitable for suspend-to-idle.
> + * @drv: cpuidle driver for the given CPU.
> + * @dev: cpuidle device for the given CPU.
>   *
>   * If there are states with the ->enter_freeze callback, find the deepest of
>   * them and enter it with frozen tick.  Otherwise, find the deepest state
>   * available and enter it normally.

Comment above becomes stale so you should update it, other than that it
seems fine.

Thanks,
Lorenzo

Re: [PATCH 2/2] cpuidle / sleep: Use broadcast timer for states that stop local timer

[Rafael J. Wysocki]

On Monday, March 02, 2015 04:27:06 PM Lorenzo Pieralisi wrote:
> On Mon, Mar 02, 2015 at 02:53:28PM +0000, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Commit 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
> > overlooked the fact that entering some sufficiently deep idle states
> > by CPUs may cause their local timers to stop and in those cases it
> > is necessary to switch over to a broadcase timer prior to entering
> 
> s/broadcase/broadcast

Yup, thanks!

> > the idle state.  If the cpuidle driver in use does not provide
> > the new ->enter_freeze callback for any of the idle states, that
> > problem affects suspend-to-idle too, but it is not taken into account
> > after the changes made by commit 381063133246.
> > 
> > Fix that by changing the definition of cpuidle_enter_freeze() and
> > re-arranging of the code in cpuidle_idle_call(), so the former does
> > not call cpuidle_enter() any more and the fallback case is handled
> > by cpuidle_idle_call() directly.
> > 
> > Fixes: 381063133246 (PM / sleep: Re-implement suspend-to-idle handling)
> > Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Minor comment below, otherwise on the 4-patch series:
> 
> Tested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> [...]
> 
> >  /**
> >   * cpuidle_enter_freeze - Enter an idle state suitable for suspend-to-idle.
> > + * @drv: cpuidle driver for the given CPU.
> > + * @dev: cpuidle device for the given CPU.
> >   *
> >   * If there are states with the ->enter_freeze callback, find the deepest of
> >   * them and enter it with frozen tick.  Otherwise, find the deepest state
> >   * available and enter it normally.
> 
> Comment above becomes stale so you should update it, other than that it
> seems fine.

Sure, I forgot to update it, thanks for pointing this out.

I'll queue up the two (with the above fixed) in my tree then.

Peter, any objections to this one and the [1/2]?

Rafael