[PATCH] PM: invoke suspend notifications after console switch

[PATCH] PM: invoke suspend notifications after console switch

[Johannes Berg]

A subsequent patch will enable apm-emulation notification for suspends
triggered in any way by using the suspend notifications. This causes
the system to lock up between X being needed to switch away from the
VT and X already waiting for resume in the apm ioctl.

This patch moves the console switch (if enabled) before the suspend
notification (and after the resume notification) to avoid this issue.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 kernel/power/main.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- everything.orig/kernel/power/main.c	2007-12-17 01:52:29.617085178 +0100
+++ everything/kernel/power/main.c	2007-12-17 01:53:23.547064887 +0100
@@ -73,12 +73,12 @@ static int suspend_prepare(void)
 	if (!suspend_ops || !suspend_ops->enter)
 		return -EPERM;
 
+	pm_prepare_console();
+
 	error = pm_notifier_call_chain(PM_SUSPEND_PREPARE);
 	if (error)
 		goto Finish;
 
-	pm_prepare_console();
-
 	if (suspend_freeze_processes()) {
 		error = -EAGAIN;
 		goto Thaw;
@@ -98,9 +98,9 @@ static int suspend_prepare(void)
 
  Thaw:
 	suspend_thaw_processes();
-	pm_restore_console();
  Finish:
 	pm_notifier_call_chain(PM_POST_SUSPEND);
+	pm_restore_console();
 	return error;
 }
 
@@ -192,8 +192,8 @@ int suspend_devices_and_enter(suspend_st
 static void suspend_finish(void)
 {
 	suspend_thaw_processes();
-	pm_restore_console();
 	pm_notifier_call_chain(PM_POST_SUSPEND);
+	pm_restore_console();
 }
 
 

Re: [PATCH] PM: invoke suspend notifications after console switch

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 1929 bytes --]


On Mon, 2007-12-17 at 02:09 +0100, Johannes Berg wrote:
> A subsequent patch will enable apm-emulation notification for suspends
> triggered in any way by using the suspend notifications. This causes
> the system to lock up between X being needed to switch away from the
> VT and X already waiting for resume in the apm ioctl.
> 
> This patch moves the console switch (if enabled) before the suspend
> notification (and after the resume notification) to avoid this issue.

I don't see this in the suspend git tree yet, anything wrong with it?

> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
>  kernel/power/main.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- everything.orig/kernel/power/main.c	2007-12-17 01:52:29.617085178 +0100
> +++ everything/kernel/power/main.c	2007-12-17 01:53:23.547064887 +0100
> @@ -73,12 +73,12 @@ static int suspend_prepare(void)
>  	if (!suspend_ops || !suspend_ops->enter)
>  		return -EPERM;
>  
> +	pm_prepare_console();
> +
>  	error = pm_notifier_call_chain(PM_SUSPEND_PREPARE);
>  	if (error)
>  		goto Finish;
>  
> -	pm_prepare_console();
> -
>  	if (suspend_freeze_processes()) {
>  		error = -EAGAIN;
>  		goto Thaw;
> @@ -98,9 +98,9 @@ static int suspend_prepare(void)
>  
>   Thaw:
>  	suspend_thaw_processes();
> -	pm_restore_console();
>   Finish:
>  	pm_notifier_call_chain(PM_POST_SUSPEND);
> +	pm_restore_console();
>  	return error;
>  }
>  
> @@ -192,8 +192,8 @@ int suspend_devices_and_enter(suspend_st
>  static void suspend_finish(void)
>  {
>  	suspend_thaw_processes();
> -	pm_restore_console();
>  	pm_notifier_call_chain(PM_POST_SUSPEND);
> +	pm_restore_console();
>  }
>  
> 
> 
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
> 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PM: invoke suspend notifications after console switch

[Rafael J. Wysocki]

On Thursday, 10 of January 2008, Johannes Berg wrote:
> 
> On Mon, 2007-12-17 at 02:09 +0100, Johannes Berg wrote:
> > A subsequent patch will enable apm-emulation notification for suspends
> > triggered in any way by using the suspend notifications. This causes
> > the system to lock up between X being needed to switch away from the
> > VT and X already waiting for resume in the apm ioctl.
> > 
> > This patch moves the console switch (if enabled) before the suspend
> > notification (and after the resume notification) to avoid this issue.
> 
> I don't see this in the suspend git tree yet, anything wrong with it?

No, it's fine.

I was distracted by the recent ACPI ordering issue, will push it to Len
shortly.

Thanks,
Rafael

Re: [PATCH] PM: invoke suspend notifications after console switch

[Pavel Machek]

On Thu 2008-01-10 14:14:36, Johannes Berg wrote:
> 
> On Mon, 2007-12-17 at 02:09 +0100, Johannes Berg wrote:
> > A subsequent patch will enable apm-emulation notification for suspends
> > triggered in any way by using the suspend notifications. This causes
> > the system to lock up between X being needed to switch away from the
> > VT and X already waiting for resume in the apm ioctl.
> > 
> > This patch moves the console switch (if enabled) before the suspend
> > notification (and after the resume notification) to avoid this issue.
> 
> I don't see this in the suspend git tree yet, anything wrong with it?

Its pretty intrusive I'd say. And it is wrong; we'd prefer userspace
to know what we are doing; if they are told we are suspending,
userspace may be able to do something more clever than long console
switch.

I'd prefer this not to go into mainline.
							Pavel

> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > ---
> >  kernel/power/main.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > --- everything.orig/kernel/power/main.c	2007-12-17 01:52:29.617085178 +0100
> > +++ everything/kernel/power/main.c	2007-12-17 01:53:23.547064887 +0100
> > @@ -73,12 +73,12 @@ static int suspend_prepare(void)
> >  	if (!suspend_ops || !suspend_ops->enter)
> >  		return -EPERM;
> >  
> > +	pm_prepare_console();
> > +
> >  	error = pm_notifier_call_chain(PM_SUSPEND_PREPARE);
> >  	if (error)
> >  		goto Finish;
> >  
> > -	pm_prepare_console();
> > -
> >  	if (suspend_freeze_processes()) {
> >  		error = -EAGAIN;
> >  		goto Thaw;
> > @@ -98,9 +98,9 @@ static int suspend_prepare(void)
> >  
> >   Thaw:
> >  	suspend_thaw_processes();
> > -	pm_restore_console();
> >   Finish:
> >  	pm_notifier_call_chain(PM_POST_SUSPEND);
> > +	pm_restore_console();
> >  	return error;
> >  }
> >  
> > @@ -192,8 +192,8 @@ int suspend_devices_and_enter(suspend_st
> >  static void suspend_finish(void)
> >  {
> >  	suspend_thaw_processes();
> > -	pm_restore_console();
> >  	pm_notifier_call_chain(PM_POST_SUSPEND);
> > +	pm_restore_console();
> >  }
> >  
> > 
> > 
> > 
> > _______________________________________________
> > linux-pm mailing list
> > linux-pm@lists.linux-foundation.org
> > https://lists.linux-foundation.org/mailman/listinfo/linux-pm
> > 



> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM: invoke suspend notifications after console switch

[Rafael J. Wysocki]

On Friday, 11 of January 2008, Pavel Machek wrote:
> On Thu 2008-01-10 14:14:36, Johannes Berg wrote:
> > 
> > On Mon, 2007-12-17 at 02:09 +0100, Johannes Berg wrote:
> > > A subsequent patch will enable apm-emulation notification for suspends
> > > triggered in any way by using the suspend notifications. This causes
> > > the system to lock up between X being needed to switch away from the
> > > VT and X already waiting for resume in the apm ioctl.
> > > 
> > > This patch moves the console switch (if enabled) before the suspend
> > > notification (and after the resume notification) to avoid this issue.
> > 
> > I don't see this in the suspend git tree yet, anything wrong with it?
> 
> Its pretty intrusive I'd say. And it is wrong; we'd prefer userspace
> to know what we are doing; if they are told we are suspending,
> userspace may be able to do something more clever than long console
> switch.
> 
> I'd prefer this not to go into mainline.

Well, in uswsusp we do the console switch before the suspend notifiers
(although from the user land).

Rafael

Re: [PATCH] PM: invoke suspend notifications after console switch

[Rafael J. Wysocki]

On Wednesday, 23 of January 2008, Rafael J. Wysocki wrote:
> On Friday, 11 of January 2008, Pavel Machek wrote:
> > On Thu 2008-01-10 14:14:36, Johannes Berg wrote:
> > > 
> > > On Mon, 2007-12-17 at 02:09 +0100, Johannes Berg wrote:
> > > > A subsequent patch will enable apm-emulation notification for suspends
> > > > triggered in any way by using the suspend notifications. This causes
> > > > the system to lock up between X being needed to switch away from the
> > > > VT and X already waiting for resume in the apm ioctl.
> > > > 
> > > > This patch moves the console switch (if enabled) before the suspend
> > > > notification (and after the resume notification) to avoid this issue.
> > > 
> > > I don't see this in the suspend git tree yet, anything wrong with it?
> > 
> > Its pretty intrusive I'd say.

How so?  There's only one user of the suspend notifiers right now, AFAICS.

> > And it is wrong; we'd prefer userspace 
> > to know what we are doing; if they are told we are suspending,
> > userspace may be able to do something more clever than long console
> > switch.

Well, we're not telling the userspace that we're suspending right now.

Also, the patch only reorders the console switch vs the suspend notifiers.
The console switch still happens before the freezer is run.

Thanks,
Rafael

Re: [PATCH] PM: invoke suspend notifications after console switch

[Pavel Machek]

> > > > > triggered in any way by using the suspend notifications. This causes
> > > > > the system to lock up between X being needed to switch away from the
> > > > > VT and X already waiting for resume in the apm ioctl.
> > > > > 
> > > > > This patch moves the console switch (if enabled) before the suspend
> > > > > notification (and after the resume notification) to avoid this issue.
> > > > 
> > > > I don't see this in the suspend git tree yet, anything wrong with it?
> > > 
> > > Its pretty intrusive I'd say.
> 
> How so?  There's only one user of the suspend notifiers right now, AFAICS.

Ok... that and consistence with uswsusp makes it acceptable I guess.
Feel free to add my acked-by and sorry for noise.

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM: invoke suspend notifications after console switch

[Benjamin Herrenschmidt]

> Its pretty intrusive I'd say. And it is wrong; we'd prefer userspace
> to know what we are doing; if they are told we are suspending,
> userspace may be able to do something more clever than long console
> switch.
> 
> I'd prefer this not to go into mainline.

So you'd prefer mainline to be broken and X to lock up ? nice !

The console switch happens -anyway- with the current code right ? So we
aren't changing that.

However, (even today I believe), users of /dev/apm_bios, such as X, will
deadlock the VT subsystem if they get notified of the suspend before the
kernel initiated console switch happen (which can happen today if the
suspend is triggered by an APM application I -think- (to be verified)
and will happen more once Johannes next patch gets in which fixes a
regression of current APM emulation which doesn't notify user space when
the suspend isn't APM originated.

So this patch will fix it by console switching before notifying them,
which is by far the right way to go for now. In the future, we may have
smarter ways to handle that graphic subsystem (and smarter gfx drivers)
and may want to provide ways to totally disable the console switching,
but that's a separate issue. In fact, some distro already disable the
console switch in their kernels for various reasons.

But if we keep the console switch we have today, we need this patch to
make it work properly and not potentially deadlock with X.

Ben.

Re: [PATCH] PM: invoke suspend notifications after console switch

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 1164 bytes --]


On Thu, 2008-01-24 at 11:46 +1100, Benjamin Herrenschmidt wrote:
> > Its pretty intrusive I'd say. And it is wrong; we'd prefer userspace
> > to know what we are doing; if they are told we are suspending,
> > userspace may be able to do something more clever than long console
> > switch.
> > 
> > I'd prefer this not to go into mainline.
> 
> So you'd prefer mainline to be broken and X to lock up ? nice !
> 
> The console switch happens -anyway- with the current code right ? So we
> aren't changing that.

Mind you, that's the (only!) other alternative: removing the in-kernel
console switch completely which is the only way to allow userspace to do
something "more clever than long console switch".

> However, (even today I believe), users of /dev/apm_bios, such as X, will
> deadlock the VT subsystem if they get notified of the suspend before the
> kernel initiated console switch happen (which can happen today if the
> suspend is triggered by an APM application I -think- (to be verified)

Yeah I'm pretty sure that can happen, but in fact, that will happen
regardless of this patch until my other patch is applied.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PM: invoke suspend notifications after console switch

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 1139 bytes --]


> Its pretty intrusive I'd say.

I can sort of subscribe to that, but the whole suspend stuff is pretty
intrusive overall.

>  And it is wrong;

Obviously, I disagree.

>  we'd prefer userspace
> to know what we are doing; if they are told we are suspending,
> userspace may be able to do something more clever than long console
> switch.

Are you even talking about this patch? We don't currently give anyone a
choice: We *always* do a "long console switch" anyway! If userspace were
to decide that it can do much better and X can support suspend, then now
we take the decision off it and switch to a suspend console while
userspace is thinking there's no need.

What this patch enables is basically allowing us to remove the suspend
console switch. How? Currently, suspend notifications with a whole bunch
of ramifications all over the place are invoked before the console
switch. By moving the console switch up, we essentially make the
sequence identical to (a) removing the in-kernel console switch and (b)
replacing it with userspace doing a console switch right before asking
us to suspend.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]