From: Liu ShuoX <[email protected]>
In shutdown progress, system is possible to do power transition
(such as suspend-to-ram) in parallel. It is unreasonable. So,
fixes it by adding a system_state checking and queue try_to_suspend
again when system status is not running.
Signed-off-by: Liu ShuoX <[email protected]>
---
kernel/power/autosleep.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
index c6422ff..9012ecf 100644
--- a/kernel/power/autosleep.c
+++ b/kernel/power/autosleep.c
@@ -32,7 +32,8 @@ static void try_to_suspend(struct work_struct *work)
mutex_lock(&autosleep_lock);
- if (!pm_save_wakeup_count(initial_count)) {
+ if (!pm_save_wakeup_count(initial_count) ||
+ system_state != SYSTEM_RUNNING) {
mutex_unlock(&autosleep_lock);
goto out;
}
--
1.7.1
On Thu, 2013-07-11 at 16:03 +0800, [email protected] wrote:
> From: Liu ShuoX <[email protected]>
>
> In shutdown progress, system is possible to do power transition
> (such as suspend-to-ram) in parallel. It is unreasonable. So,
> fixes it by adding a system_state checking and queue try_to_suspend
> again when system status is not running.
>
> Signed-off-by: Liu ShuoX <[email protected]>
> ---
> kernel/power/autosleep.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
Without this patch, we hit an hang issue on Android.
Scenario:
Kernel starts shutdown and calls all device driver's shutdown callback.
When a driver's shutdown is called, the last wakelock is released and
suspend-to-ram starts. However, as some driver's shut down callbacks
already shut down devices and disable runtime pm, the suspend-to-ram
calls driver's suspend callback without noticing that device is already
off and causes crash.
We know the drivers should be fixed, but can we also change generic
codes a little to make it stronger?
> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> index c6422ff..9012ecf 100644
> --- a/kernel/power/autosleep.c
> +++ b/kernel/power/autosleep.c
> @@ -32,7 +32,8 @@ static void try_to_suspend(struct work_struct *work)
>
> mutex_lock(&autosleep_lock);
>
> - if (!pm_save_wakeup_count(initial_count)) {
> + if (!pm_save_wakeup_count(initial_count) ||
> + system_state != SYSTEM_RUNNING) {
> mutex_unlock(&autosleep_lock);
> goto out;
> }
On Fri, 12 Jul 2013, Yanmin Zhang wrote:
> On Thu, 2013-07-11 at 16:03 +0800, [email protected] wrote:
> > From: Liu ShuoX <[email protected]>
> >
> > In shutdown progress, system is possible to do power transition
> > (such as suspend-to-ram) in parallel. It is unreasonable. So,
> > fixes it by adding a system_state checking and queue try_to_suspend
> > again when system status is not running.
> >
> > Signed-off-by: Liu ShuoX <[email protected]>
> > ---
> > kernel/power/autosleep.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> Without this patch, we hit an hang issue on Android.
>
> Scenario:
> Kernel starts shutdown and calls all device driver's shutdown callback.
> When a driver's shutdown is called, the last wakelock is released and
> suspend-to-ram starts. However, as some driver's shut down callbacks
> already shut down devices and disable runtime pm, the suspend-to-ram
> calls driver's suspend callback without noticing that device is already
> off and causes crash.
> We know the drivers should be fixed, but can we also change generic
> codes a little to make it stronger?
Indeed, this does seem like the sort of thing we want to have.
Allowing an "automatic" or "opportunistic" sleep in the middle of a
shutdown makes no sense at all. In fact, it might be a good idea to
disable system sleep completely at this time -- not even a write to
/sys/power/state should be allowed to interrupt a shutdown.
Alan Stern
On Fri 2013-07-12 10:37:33, Alan Stern wrote:
> On Fri, 12 Jul 2013, Yanmin Zhang wrote:
>
> > On Thu, 2013-07-11 at 16:03 +0800, [email protected] wrote:
> > > From: Liu ShuoX <[email protected]>
> > >
> > > In shutdown progress, system is possible to do power transition
> > > (such as suspend-to-ram) in parallel. It is unreasonable. So,
> > > fixes it by adding a system_state checking and queue try_to_suspend
> > > again when system status is not running.
> > >
> > > Signed-off-by: Liu ShuoX <[email protected]>
> > > ---
> > > kernel/power/autosleep.c | 3 ++-
> > > 1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > Without this patch, we hit an hang issue on Android.
> >
> > Scenario:
> > Kernel starts shutdown and calls all device driver's shutdown callback.
> > When a driver's shutdown is called, the last wakelock is released and
> > suspend-to-ram starts. However, as some driver's shut down callbacks
> > already shut down devices and disable runtime pm, the suspend-to-ram
> > calls driver's suspend callback without noticing that device is already
> > off and causes crash.
> > We know the drivers should be fixed, but can we also change generic
> > codes a little to make it stronger?
>
> Indeed, this does seem like the sort of thing we want to have.
> Allowing an "automatic" or "opportunistic" sleep in the middle of a
> shutdown makes no sense at all. In fact, it might be a good idea to
> disable system sleep completely at this time -- not even a write to
> /sys/power/state should be allowed to interrupt a shutdown.
I'm not completely sure, but as long as userland is running, we should
have system_state == RUNNING, no?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Friday, July 12, 2013 02:14:11 PM Yanmin Zhang wrote:
> On Thu, 2013-07-11 at 16:03 +0800, [email protected] wrote:
> > From: Liu ShuoX <[email protected]>
> >
> > In shutdown progress, system is possible to do power transition
> > (such as suspend-to-ram) in parallel. It is unreasonable. So,
> > fixes it by adding a system_state checking and queue try_to_suspend
> > again when system status is not running.
> >
> > Signed-off-by: Liu ShuoX <[email protected]>
> > ---
> > kernel/power/autosleep.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> Without this patch, we hit an hang issue on Android.
>
> Scenario:
> Kernel starts shutdown and calls all device driver's shutdown callback.
> When a driver's shutdown is called, the last wakelock is released and
> suspend-to-ram starts. However, as some driver's shut down callbacks
> already shut down devices and disable runtime pm, the suspend-to-ram
> calls driver's suspend callback without noticing that device is already
> off and causes crash.
OK, queued up as a fix for 3.11, with a modified changelog (I used your
scenario above in it).
Thanks,
Rafael
> We know the drivers should be fixed, but can we also change generic
> codes a little to make it stronger?
>
> > diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> > index c6422ff..9012ecf 100644
> > --- a/kernel/power/autosleep.c
> > +++ b/kernel/power/autosleep.c
> > @@ -32,7 +32,8 @@ static void try_to_suspend(struct work_struct *work)
> >
> > mutex_lock(&autosleep_lock);
> >
> > - if (!pm_save_wakeup_count(initial_count)) {
> > + if (!pm_save_wakeup_count(initial_count) ||
> > + system_state != SYSTEM_RUNNING) {
> > mutex_unlock(&autosleep_lock);
> > goto out;
> > }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Sat, 2013-07-13 at 13:56 +0200, Pavel Machek wrote:
> On Fri 2013-07-12 10:37:33, Alan Stern wrote:
> > On Fri, 12 Jul 2013, Yanmin Zhang wrote:
> >
> > > On Thu, 2013-07-11 at 16:03 +0800, [email protected] wrote:
> > > > From: Liu ShuoX <[email protected]>
> > > >
> > > > In shutdown progress, system is possible to do power transition
> > > > (such as suspend-to-ram) in parallel. It is unreasonable. So,
> > > > fixes it by adding a system_state checking and queue try_to_suspend
> > > > again when system status is not running.
> > > >
> > > > Signed-off-by: Liu ShuoX <[email protected]>
> > > > ---
> > > > kernel/power/autosleep.c | 3 ++-
> > > > 1 files changed, 2 insertions(+), 1 deletions(-)
> > > >
> > > Without this patch, we hit an hang issue on Android.
> > >
> > > Scenario:
> > > Kernel starts shutdown and calls all device driver's shutdown callback.
> > > When a driver's shutdown is called, the last wakelock is released and
> > > suspend-to-ram starts. However, as some driver's shut down callbacks
> > > already shut down devices and disable runtime pm, the suspend-to-ram
> > > calls driver's suspend callback without noticing that device is already
> > > off and causes crash.
> > > We know the drivers should be fixed, but can we also change generic
> > > codes a little to make it stronger?
> >
> > Indeed, this does seem like the sort of thing we want to have.
> > Allowing an "automatic" or "opportunistic" sleep in the middle of a
> > shutdown makes no sense at all. In fact, it might be a good idea to
> > disable system sleep completely at this time -- not even a write to
> > /sys/power/state should be allowed to interrupt a shutdown.
>
> I'm not completely sure, but as long as userland is running, we should
> have system_state == RUNNING, no?
No. The reboot/shutdown is started by application processes, then kernel
changes system_state quickly. See kernel_restart_prepare, kernel_shutdown_prepare.
But indeed, it's a good question. We hit many other issues around rebooting.
When system is rebooting, user space processes are still running. There is no
freeze step. Some processes might jump in to access devices. To avoid it, we
have to add some tricks in some device drivers. It's a little crazy.