[PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
from triggering by disabling nonboot CPUs before we finally enter the platform
suspend.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/disk.c |    1 +
 kernel/power/user.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
===================================================================
--- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
+++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
@@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
 	switch(mode) {
 	case PM_DISK_PLATFORM:
 		if (pm_ops && pm_ops->enter) {
+			disable_nonboot_cpus();
 			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
 			pm_ops->enter(PM_SUSPEND_DISK);
 			break;
Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
===================================================================
--- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
+++ linux-2.6.21-rc2-mm2/kernel/power/user.c
@@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
 
 		case PMOPS_ENTER:
 			if (data->platform_suspend) {
+				disable_nonboot_cpus();
 				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
 				error = pm_ops->enter(PM_SUSPEND_DISK);
-				error = 0;
 			}
 			break;
 

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Pavel Machek]

Hi!

> Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> from triggering by disabling nonboot CPUs before we finally enter the platform
> suspend.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/disk.c |    1 +
>  kernel/power/user.c |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> ===================================================================
> --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
>  	switch(mode) {
>  	case PM_DISK_PLATFORM:
>  		if (pm_ops && pm_ops->enter) {
> +			disable_nonboot_cpus();
>  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
>  			pm_ops->enter(PM_SUSPEND_DISK);
>  			break;

...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
otherwise we proceed with cpus enabled?

That looks ugly.

Is the warning bogus? Or maybe we should *always* disable nonboot cpus
in powerdown path?

> Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> ===================================================================
> --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
>  
>  		case PMOPS_ENTER:
>  			if (data->platform_suspend) {
> +				disable_nonboot_cpus();
>  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
>  				error = pm_ops->enter(PM_SUSPEND_DISK);
> -				error = 0;
>  			}
>  			break;

Foe an userland application, disabling cpus during pmops_enter is at
least surprising.......

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

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Andrew Morton]

On Wed, 7 Mar 2007 20:44:11 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> from triggering by disabling nonboot CPUs before we finally enter the platform
> suspend.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/disk.c |    1 +
>  kernel/power/user.c |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> ===================================================================
> --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
>  	switch(mode) {
>  	case PM_DISK_PLATFORM:
>  		if (pm_ops && pm_ops->enter) {
> +			disable_nonboot_cpus();
>  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
>  			pm_ops->enter(PM_SUSPEND_DISK);
>  			break;
> Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> ===================================================================
> --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
>  
>  		case PMOPS_ENTER:
>  			if (data->platform_suspend) {
> +				disable_nonboot_cpus();
>  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
>  				error = pm_ops->enter(PM_SUSPEND_DISK);
> -				error = 0;
>  			}
>  			break;

Is this considered 2.6.21 material?  If so why?

Thanks.

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Wednesday, 7 March 2007 22:16, Andrew Morton wrote:
> On Wed, 7 Mar 2007 20:44:11 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > from triggering by disabling nonboot CPUs before we finally enter the platform
> > suspend.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/power/disk.c |    1 +
> >  kernel/power/user.c |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > ===================================================================
> > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> >  	switch(mode) {
> >  	case PM_DISK_PLATFORM:
> >  		if (pm_ops && pm_ops->enter) {
> > +			disable_nonboot_cpus();
> >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> >  			pm_ops->enter(PM_SUSPEND_DISK);
> >  			break;
> > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > ===================================================================
> > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> >  
> >  		case PMOPS_ENTER:
> >  			if (data->platform_suspend) {
> > +				disable_nonboot_cpus();
> >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > -				error = 0;
> >  			}
> >  			break;
> 
> Is this considered 2.6.21 material?  If so why?

Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
triggers every time an SMP x86_64 box is suspended to disk using the platform
mode (default), which is quite annoying IMHO and users think something wrong is
going on.  This will probably cause them to report the problem and I'd rather
like to avoid handling these reports. ;-)

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Pavel Machek]

Hi!

> > > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > from triggering by disabling nonboot CPUs before we finally enter the platform
> > > suspend.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  kernel/power/disk.c |    1 +
> > >  kernel/power/user.c |    2 +-
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > ===================================================================
> > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > >  	switch(mode) {
> > >  	case PM_DISK_PLATFORM:
> > >  		if (pm_ops && pm_ops->enter) {
> > > +			disable_nonboot_cpus();
> > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > >  			break;
> > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > ===================================================================
> > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > >  
> > >  		case PMOPS_ENTER:
> > >  			if (data->platform_suspend) {
> > > +				disable_nonboot_cpus();
> > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > -				error = 0;
> > >  			}
> > >  			break;
> > 
> > Is this considered 2.6.21 material?  If so why?
> 
> Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> triggers every time an SMP x86_64 box is suspended to disk using the platform
> mode (default), which is quite annoying IMHO and users think something wrong is
> going on.  This will probably cause them to report the problem and I'd rather
> like to avoid handling these reports. ;-)

I do not quite like the patch (explanation in separate mail). Can we
remove the WARN_ON() ? ;-).
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Andrew Morton]

On Wed, 7 Mar 2007 23:14:29 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Wednesday, 7 March 2007 22:16, Andrew Morton wrote:
> > On Wed, 7 Mar 2007 20:44:11 +0100
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > from triggering by disabling nonboot CPUs before we finally enter the platform
> > > suspend.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  kernel/power/disk.c |    1 +
> > >  kernel/power/user.c |    2 +-
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > ===================================================================
> > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > >  	switch(mode) {
> > >  	case PM_DISK_PLATFORM:
> > >  		if (pm_ops && pm_ops->enter) {
> > > +			disable_nonboot_cpus();
> > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > >  			break;
> > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > ===================================================================
> > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > >  
> > >  		case PMOPS_ENTER:
> > >  			if (data->platform_suspend) {
> > > +				disable_nonboot_cpus();
> > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > -				error = 0;
> > >  			}
> > >  			break;
> > 
> > Is this considered 2.6.21 material?  If so why?
> 
> Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> triggers every time an SMP x86_64 box is suspended to disk using the platform
> mode (default), which is quite annoying IMHO and users think something wrong is
> going on.  This will probably cause them to report the problem and I'd rather
> like to avoid handling these reports. ;-)

Well sure - if patches were always error-free, we'd always apply them
immediately.

The question is: is the risk of this patch breaking things exceeded by the
benefit which you describe?

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Wednesday, 7 March 2007 23:49, Andrew Morton wrote:
> On Wed, 7 Mar 2007 23:14:29 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Wednesday, 7 March 2007 22:16, Andrew Morton wrote:
> > > On Wed, 7 Mar 2007 20:44:11 +0100
> > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > 
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > > from triggering by disabling nonboot CPUs before we finally enter the platform
> > > > suspend.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > >  kernel/power/disk.c |    1 +
> > > >  kernel/power/user.c |    2 +-
> > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > > >  	switch(mode) {
> > > >  	case PM_DISK_PLATFORM:
> > > >  		if (pm_ops && pm_ops->enter) {
> > > > +			disable_nonboot_cpus();
> > > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > > >  			break;
> > > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > > >  
> > > >  		case PMOPS_ENTER:
> > > >  			if (data->platform_suspend) {
> > > > +				disable_nonboot_cpus();
> > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > -				error = 0;
> > > >  			}
> > > >  			break;
> > > 
> > > Is this considered 2.6.21 material?  If so why?
> > 
> > Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > triggers every time an SMP x86_64 box is suspended to disk using the platform
> > mode (default), which is quite annoying IMHO and users think something wrong is
> > going on.  This will probably cause them to report the problem and I'd rather
> > like to avoid handling these reports. ;-)
> 
> Well sure - if patches were always error-free, we'd always apply them
> immediately.
> 
> The question is: is the risk of this patch breaking things exceeded by the
> benefit which you describe?

Well, it has survived some testing (http://lkml.org/lkml/2007/3/7/16).  Also,
before the code ordering in 2.6.21-rc* we had been running on one CPU
here, so I think the risk is small.

We could remove the WARN_ON() as Pavel has just suggested, but first I'd like
to know who put it there and why.

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Wednesday, 7 March 2007 23:19, Pavel Machek wrote:
> Hi!
> 
> > > > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > > from triggering by disabling nonboot CPUs before we finally enter the platform
> > > > suspend.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > >  kernel/power/disk.c |    1 +
> > > >  kernel/power/user.c |    2 +-
> > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > > >  	switch(mode) {
> > > >  	case PM_DISK_PLATFORM:
> > > >  		if (pm_ops && pm_ops->enter) {
> > > > +			disable_nonboot_cpus();
> > > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > > >  			break;
> > > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > > >  
> > > >  		case PMOPS_ENTER:
> > > >  			if (data->platform_suspend) {
> > > > +				disable_nonboot_cpus();
> > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > -				error = 0;
> > > >  			}
> > > >  			break;
> > > 
> > > Is this considered 2.6.21 material?  If so why?
> > 
> > Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > triggers every time an SMP x86_64 box is suspended to disk using the platform
> > mode (default), which is quite annoying IMHO and users think something wrong is
> > going on.  This will probably cause them to report the problem and I'd rather
> > like to avoid handling these reports. ;-)
> 
> I do not quite like the patch (explanation in separate mail). Can we
> remove the WARN_ON() ? ;-).

Yes, if you can tell me why it's there. :-)

Rafael

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Dave Jones]

On Thu, Mar 08, 2007 at 12:13:05AM +0100, Rafael J. Wysocki wrote:

 > > > Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
 > > > triggers every time an SMP x86_64 box is suspended to disk using the platform
 > > > mode (default), which is quite annoying IMHO and users think something wrong is
 > > > going on.  This will probably cause them to report the problem and I'd rather
 > > > like to avoid handling these reports. ;-)
 > > 
 > > Well sure - if patches were always error-free, we'd always apply them
 > > immediately.
 > > 
 > > The question is: is the risk of this patch breaking things exceeded by the
 > > benefit which you describe?
 > 
 > Well, it has survived some testing (http://lkml.org/lkml/2007/3/7/16).  Also,
 > before the code ordering in 2.6.21-rc* we had been running on one CPU
 > here, so I think the risk is small.
 > 
 > We could remove the WARN_ON() as Pavel has just suggested, but first I'd like
 > to know who put it there and why.

It was introduced as part of ..

commit 55b2355eefc2f160246226d4d69fed431173a4d5
Author: Shaohua Li <shaohua.li@intel.com>
Date:   Fri Jun 23 02:04:49 2006 -0700

    [PATCH] don't use flush_tlb_all in suspend time
    
    flush_tlb_all uses on_each_cpu, which will disable/enable interrupt.
    In suspend/resume time, this will make interrupt wrongly enabled.
    
    Signed-off-by: Shaohua Li <shaohua.li@intel.com>
    Cc: Pavel Machek <pavel@ucw.cz>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>




-- 
http://www.codemonkey.org.uk

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Thursday, 8 March 2007 01:20, Dave Jones wrote:
> On Thu, Mar 08, 2007 at 12:13:05AM +0100, Rafael J. Wysocki wrote:
> 
>  > > > Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
>  > > > triggers every time an SMP x86_64 box is suspended to disk using the platform
>  > > > mode (default), which is quite annoying IMHO and users think something wrong is
>  > > > going on.  This will probably cause them to report the problem and I'd rather
>  > > > like to avoid handling these reports. ;-)
>  > > 
>  > > Well sure - if patches were always error-free, we'd always apply them
>  > > immediately.
>  > > 
>  > > The question is: is the risk of this patch breaking things exceeded by the
>  > > benefit which you describe?
>  > 
>  > Well, it has survived some testing (http://lkml.org/lkml/2007/3/7/16).  Also,
>  > before the code ordering in 2.6.21-rc* we had been running on one CPU
>  > here, so I think the risk is small.
>  > 
>  > We could remove the WARN_ON() as Pavel has just suggested, but first I'd like
>  > to know who put it there and why.
> 
> It was introduced as part of ..
> 
> commit 55b2355eefc2f160246226d4d69fed431173a4d5
> Author: Shaohua Li <shaohua.li@intel.com>
> Date:   Fri Jun 23 02:04:49 2006 -0700
> 
>     [PATCH] don't use flush_tlb_all in suspend time
>     
>     flush_tlb_all uses on_each_cpu, which will disable/enable interrupt.
>     In suspend/resume time, this will make interrupt wrongly enabled.

Ah, thanks.

So the question is what can go wrong if we ignore the TLBs of the other
CPUs that may be on-line when init_low_mapping() is executed.

Frankly, I don't know.

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Len Brown]

On Wednesday 07 March 2007 18:13, Rafael J. Wysocki wrote:
> On Wednesday, 7 March 2007 23:49, Andrew Morton wrote:
> > On Wed, 7 Mar 2007 23:14:29 +0100
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > On Wednesday, 7 March 2007 22:16, Andrew Morton wrote:
> > > > On Wed, 7 Mar 2007 20:44:11 +0100
> > > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > > 
> > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > 
> > > > > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > > > from triggering by disabling nonboot CPUs before we finally enter the platform
> > > > > suspend.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > ---
> > > > >  kernel/power/disk.c |    1 +
> > > > >  kernel/power/user.c |    2 +-
> > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > ===================================================================
> > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > > > >  	switch(mode) {
> > > > >  	case PM_DISK_PLATFORM:
> > > > >  		if (pm_ops && pm_ops->enter) {
> > > > > +			disable_nonboot_cpus();
> > > > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > > > >  			break;
> > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > ===================================================================
> > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > > > >  
> > > > >  		case PMOPS_ENTER:
> > > > >  			if (data->platform_suspend) {
> > > > > +				disable_nonboot_cpus();
> > > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > > -				error = 0;
> > > > >  			}
> > > > >  			break;
> > > > 
> > > > Is this considered 2.6.21 material?  If so why?
> > > 
> > > Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > triggers every time an SMP x86_64 box is suspended to disk using the platform
> > > mode (default), which is quite annoying IMHO and users think something wrong is
> > > going on.  This will probably cause them to report the problem and I'd rather
> > > like to avoid handling these reports. ;-)
> > 
> > Well sure - if patches were always error-free, we'd always apply them
> > immediately.
> > 
> > The question is: is the risk of this patch breaking things exceeded by the
> > benefit which you describe?
> 
> Well, it has survived some testing (http://lkml.org/lkml/2007/3/7/16).  Also,
> before the code ordering in 2.6.21-rc* we had been running on one CPU
> here, so I think the risk is small.
> 
> We could remove the WARN_ON() as Pavel has just suggested, but first I'd like
> to know who put it there and why.
> 

Shaohua added it between 2.6.17 and 2.6.18
55b2355eefc2f160246226d4d69fed431173a4d5

-Len

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Friday, 9 March 2007 02:11, Len Brown wrote:
> On Wednesday 07 March 2007 18:13, Rafael J. Wysocki wrote:
> > On Wednesday, 7 March 2007 23:49, Andrew Morton wrote:
> > > On Wed, 7 Mar 2007 23:14:29 +0100
> > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > 
> > > > On Wednesday, 7 March 2007 22:16, Andrew Morton wrote:
> > > > > On Wed, 7 Mar 2007 20:44:11 +0100
> > > > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > > > 
> > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > 
> > > > > > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > > > > from triggering by disabling nonboot CPUs before we finally enter the platform
> > > > > > suspend.
> > > > > > 
> > > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > ---
> > > > > >  kernel/power/disk.c |    1 +
> > > > > >  kernel/power/user.c |    2 +-
> > > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > > > > >  	switch(mode) {
> > > > > >  	case PM_DISK_PLATFORM:
> > > > > >  		if (pm_ops && pm_ops->enter) {
> > > > > > +			disable_nonboot_cpus();
> > > > > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > > > > >  			break;
> > > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > > > > >  
> > > > > >  		case PMOPS_ENTER:
> > > > > >  			if (data->platform_suspend) {
> > > > > > +				disable_nonboot_cpus();
> > > > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > > > -				error = 0;
> > > > > >  			}
> > > > > >  			break;
> > > > > 
> > > > > Is this considered 2.6.21 material?  If so why?
> > > > 
> > > > Well, the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > > triggers every time an SMP x86_64 box is suspended to disk using the platform
> > > > mode (default), which is quite annoying IMHO and users think something wrong is
> > > > going on.  This will probably cause them to report the problem and I'd rather
> > > > like to avoid handling these reports. ;-)
> > > 
> > > Well sure - if patches were always error-free, we'd always apply them
> > > immediately.
> > > 
> > > The question is: is the risk of this patch breaking things exceeded by the
> > > benefit which you describe?
> > 
> > Well, it has survived some testing (http://lkml.org/lkml/2007/3/7/16).  Also,
> > before the code ordering in 2.6.21-rc* we had been running on one CPU
> > here, so I think the risk is small.
> > 
> > We could remove the WARN_ON() as Pavel has just suggested, but first I'd like
> > to know who put it there and why.
> > 
> 
> Shaohua added it between 2.6.17 and 2.6.18
> 55b2355eefc2f160246226d4d69fed431173a4d5

Yes, DaveJ has already noticed that, but what it's there for?

Rafael

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Wednesday, 7 March 2007 22:07, Pavel Machek wrote:
> Hi!
> 
> > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > from triggering by disabling nonboot CPUs before we finally enter the platform
> > suspend.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/power/disk.c |    1 +
> >  kernel/power/user.c |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > ===================================================================
> > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> >  	switch(mode) {
> >  	case PM_DISK_PLATFORM:
> >  		if (pm_ops && pm_ops->enter) {
> > +			disable_nonboot_cpus();
> >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> >  			pm_ops->enter(PM_SUSPEND_DISK);
> >  			break;
> 
> ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> otherwise we proceed with cpus enabled?
> 
> That looks ugly.
> 
> Is the warning bogus?

Well, maybe.  I'm not sure.

> Or maybe we should *always* disable nonboot cpus in powerdown path?

I think we should do that.

> > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > ===================================================================
> > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> >  
> >  		case PMOPS_ENTER:
> >  			if (data->platform_suspend) {
> > +				disable_nonboot_cpus();
> >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > -				error = 0;
> >  			}
> >  			break;
> 
> Foe an userland application, disabling cpus during pmops_enter is at
> least surprising.......

Yes, but this is not a usual ioctl().  OTOH, we can call enable_nonboot_cpus()
if pm_ops->enter(PM_SUSPEND_DISK) returns an error (otherwise it souldn't
return at all, no?).

Rafael

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Pavel Machek]

Hi!

> > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > ===================================================================
> > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > >  	switch(mode) {
> > >  	case PM_DISK_PLATFORM:
> > >  		if (pm_ops && pm_ops->enter) {
> > > +			disable_nonboot_cpus();
> > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > >  			break;
> > 
> > ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> > otherwise we proceed with cpus enabled?
> > 
> > That looks ugly.
> > 
> > Is the warning bogus?
> 
> Well, maybe.  I'm not sure.
> 
> > Or maybe we should *always* disable nonboot cpus in powerdown path?
> 
> I think we should do that.

That would be acceptable.

> > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > ===================================================================
> > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > >  
> > >  		case PMOPS_ENTER:
> > >  			if (data->platform_suspend) {
> > > +				disable_nonboot_cpus();
> > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > -				error = 0;
> > >  			}
> > >  			break;
> > 
> > Foe an userland application, disabling cpus during pmops_enter is at
> > least surprising.......
> 
> Yes, but this is not a usual ioctl().  OTOH, we can call enable_nonboot_cpus()
> if pm_ops->enter(PM_SUSPEND_DISK) returns an error (otherwise it souldn't
> return at all, no?).

Ok.
									Pavel

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

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Heiko Carstens]

On Wed, Mar 07, 2007 at 09:07:17PM +0000, Pavel Machek wrote:
> Hi!
> 
> > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > from triggering by disabling nonboot CPUs before we finally enter the platform
> > suspend.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/power/disk.c |    1 +
> >  kernel/power/user.c |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > ===================================================================
> > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> >  	switch(mode) {
> >  	case PM_DISK_PLATFORM:
> >  		if (pm_ops && pm_ops->enter) {
> > +			disable_nonboot_cpus();
> >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> >  			pm_ops->enter(PM_SUSPEND_DISK);
> >  			break;
> 
> ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> otherwise we proceed with cpus enabled?
> 
> That looks ugly.
> 
> Is the warning bogus? Or maybe we should *always* disable nonboot cpus
> in powerdown path?

Is disable_nonboot_cpus() assuming that first_cpu(cpu_present_map) is
the boot cpu? Just wondering why disable_nonboot_cpus() isn't using just
any_online_cpu(cpu_online_map)...

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Friday, 9 March 2007 13:29, Heiko Carstens wrote:
> On Wed, Mar 07, 2007 at 09:07:17PM +0000, Pavel Machek wrote:
> > Hi!
> > 
> > > Prevent the WARN_ON() in arch/x86_64/kernel/acpi/sleep.c:init_low_mapping()
> > > from triggering by disabling nonboot CPUs before we finally enter the platform
> > > suspend.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  kernel/power/disk.c |    1 +
> > >  kernel/power/user.c |    2 +-
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > ===================================================================
> > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > >  	switch(mode) {
> > >  	case PM_DISK_PLATFORM:
> > >  		if (pm_ops && pm_ops->enter) {
> > > +			disable_nonboot_cpus();
> > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > >  			break;
> > 
> > ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> > otherwise we proceed with cpus enabled?
> > 
> > That looks ugly.
> > 
> > Is the warning bogus? Or maybe we should *always* disable nonboot cpus
> > in powerdown path?
> 
> Is disable_nonboot_cpus() assuming that first_cpu(cpu_present_map) is
> the boot cpu? Just wondering why disable_nonboot_cpus() isn't using just
> any_online_cpu(cpu_online_map)...

Is your question related to the code in kernel/cpu.c?

Rafael

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

Hi,

On Friday, 9 March 2007 09:54, Pavel Machek wrote:
> Hi!
> 
> > > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > > >  	switch(mode) {
> > > >  	case PM_DISK_PLATFORM:
> > > >  		if (pm_ops && pm_ops->enter) {
> > > > +			disable_nonboot_cpus();
> > > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > > >  			break;
> > > 
> > > ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> > > otherwise we proceed with cpus enabled?
> > > 
> > > That looks ugly.
> > > 
> > > Is the warning bogus?
> > 
> > Well, maybe.  I'm not sure.
> > 
> > > Or maybe we should *always* disable nonboot cpus in powerdown path?
> > 
> > I think we should do that.
> 
> That would be acceptable.
> 
> > > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > > >  
> > > >  		case PMOPS_ENTER:
> > > >  			if (data->platform_suspend) {
> > > > +				disable_nonboot_cpus();
> > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > -				error = 0;
> > > >  			}
> > > >  			break;
> > > 
> > > Foe an userland application, disabling cpus during pmops_enter is at
> > > least surprising.......
> > 
> > Yes, but this is not a usual ioctl().  OTOH, we can call enable_nonboot_cpus()
> > if pm_ops->enter(PM_SUSPEND_DISK) returns an error (otherwise it souldn't
> > return at all, no?).
> 
> Ok.

Well, does the appended patch look better?

Rafael


---
 kernel/power/disk.c |    1 +
 kernel/power/user.c |    3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.21-rc3/kernel/power/disk.c
===================================================================
--- linux-2.6.21-rc3.orig/kernel/power/disk.c
+++ linux-2.6.21-rc3/kernel/power/disk.c
@@ -58,6 +58,7 @@ static inline int platform_prepare(void)
 
 static void power_down(suspend_disk_method_t mode)
 {
+	disable_nonboot_cpus();
 	switch(mode) {
 	case PM_DISK_PLATFORM:
 		if (pm_ops && pm_ops->enter) {
Index: linux-2.6.21-rc3/kernel/power/user.c
===================================================================
--- linux-2.6.21-rc3.orig/kernel/power/user.c
+++ linux-2.6.21-rc3/kernel/power/user.c
@@ -402,9 +402,10 @@ static int snapshot_ioctl(struct inode *
 
 		case PMOPS_ENTER:
 			if (data->platform_suspend) {
+				disable_nonboot_cpus();
 				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
 				error = pm_ops->enter(PM_SUSPEND_DISK);
-				error = 0;
+				enable_nonboot_cpus();
 			}
 			break;
 

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Pavel Machek]

Hi!

> > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > ===================================================================
> > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > > > >  	switch(mode) {
> > > > >  	case PM_DISK_PLATFORM:
> > > > >  		if (pm_ops && pm_ops->enter) {
> > > > > +			disable_nonboot_cpus();
> > > > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > > > >  			break;
> > > > 
> > > > ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> > > > otherwise we proceed with cpus enabled?
> > > > 
> > > > That looks ugly.
> > > > 
> > > > Is the warning bogus?
> > > 
> > > Well, maybe.  I'm not sure.
> > > 
> > > > Or maybe we should *always* disable nonboot cpus in powerdown path?
> > > 
> > > I think we should do that.
> > 
> > That would be acceptable.
> > 
> > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > ===================================================================
> > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > > > >  
> > > > >  		case PMOPS_ENTER:
> > > > >  			if (data->platform_suspend) {
> > > > > +				disable_nonboot_cpus();
> > > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > > -				error = 0;
> > > > >  			}
> > > > >  			break;
> > > > 
> > > > Foe an userland application, disabling cpus during pmops_enter is at
> > > > least surprising.......
> > > 
> > > Yes, but this is not a usual ioctl().  OTOH, we can call enable_nonboot_cpus()
> > > if pm_ops->enter(PM_SUSPEND_DISK) returns an error (otherwise it souldn't
> > > return at all, no?).
> > 
> > Ok.
> 
> Well, does the appended patch look better?

Yes.

> ---
>  kernel/power/disk.c |    1 +
>  kernel/power/user.c |    3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.21-rc3/kernel/power/disk.c
> ===================================================================
> --- linux-2.6.21-rc3.orig/kernel/power/disk.c
> +++ linux-2.6.21-rc3/kernel/power/disk.c
> @@ -58,6 +58,7 @@ static inline int platform_prepare(void)
>  
>  static void power_down(suspend_disk_method_t mode)
>  {
> +	disable_nonboot_cpus();
>  	switch(mode) {
>  	case PM_DISK_PLATFORM:
>  		if (pm_ops && pm_ops->enter) {
> Index: linux-2.6.21-rc3/kernel/power/user.c
> ===================================================================
> --- linux-2.6.21-rc3.orig/kernel/power/user.c
> +++ linux-2.6.21-rc3/kernel/power/user.c
> @@ -402,9 +402,10 @@ static int snapshot_ioctl(struct inode *
>  
>  		case PMOPS_ENTER:
>  			if (data->platform_suspend) {
> +				disable_nonboot_cpus();
>  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
>  				error = pm_ops->enter(PM_SUSPEND_DISK);
> -				error = 0;
> +				enable_nonboot_cpus();

Why did we discard return code in previous versions? Do we still want
to do that?

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

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Friday, 9 March 2007 22:07, Pavel Machek wrote:
> Hi!
> 
> > > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/disk.c
> > > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/disk.c
> > > > > > @@ -61,6 +61,7 @@ static void power_down(suspend_disk_meth
> > > > > >  	switch(mode) {
> > > > > >  	case PM_DISK_PLATFORM:
> > > > > >  		if (pm_ops && pm_ops->enter) {
> > > > > > +			disable_nonboot_cpus();
> > > > > >  			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > > >  			pm_ops->enter(PM_SUSPEND_DISK);
> > > > > >  			break;
> > > > > 
> > > > > ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> > > > > otherwise we proceed with cpus enabled?
> > > > > 
> > > > > That looks ugly.
> > > > > 
> > > > > Is the warning bogus?
> > > > 
> > > > Well, maybe.  I'm not sure.
> > > > 
> > > > > Or maybe we should *always* disable nonboot cpus in powerdown path?
> > > > 
> > > > I think we should do that.
> > > 
> > > That would be acceptable.
> > > 
> > > > > > Index: linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.21-rc2-mm2.orig/kernel/power/user.c
> > > > > > +++ linux-2.6.21-rc2-mm2/kernel/power/user.c
> > > > > > @@ -398,9 +398,9 @@ static int snapshot_ioctl(struct inode *
> > > > > >  
> > > > > >  		case PMOPS_ENTER:
> > > > > >  			if (data->platform_suspend) {
> > > > > > +				disable_nonboot_cpus();
> > > > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > > > -				error = 0;
> > > > > >  			}
> > > > > >  			break;
> > > > > 
> > > > > Foe an userland application, disabling cpus during pmops_enter is at
> > > > > least surprising.......
> > > > 
> > > > Yes, but this is not a usual ioctl().  OTOH, we can call enable_nonboot_cpus()
> > > > if pm_ops->enter(PM_SUSPEND_DISK) returns an error (otherwise it souldn't
> > > > return at all, no?).
> > > 
> > > Ok.
> > 
> > Well, does the appended patch look better?
> 
> Yes.
> 
> > ---
> >  kernel/power/disk.c |    1 +
> >  kernel/power/user.c |    3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.21-rc3/kernel/power/disk.c
> > ===================================================================
> > --- linux-2.6.21-rc3.orig/kernel/power/disk.c
> > +++ linux-2.6.21-rc3/kernel/power/disk.c
> > @@ -58,6 +58,7 @@ static inline int platform_prepare(void)
> >  
> >  static void power_down(suspend_disk_method_t mode)
> >  {
> > +	disable_nonboot_cpus();
> >  	switch(mode) {
> >  	case PM_DISK_PLATFORM:
> >  		if (pm_ops && pm_ops->enter) {
> > Index: linux-2.6.21-rc3/kernel/power/user.c
> > ===================================================================
> > --- linux-2.6.21-rc3.orig/kernel/power/user.c
> > +++ linux-2.6.21-rc3/kernel/power/user.c
> > @@ -402,9 +402,10 @@ static int snapshot_ioctl(struct inode *
> >  
> >  		case PMOPS_ENTER:
> >  			if (data->platform_suspend) {
> > +				disable_nonboot_cpus();
> >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > -				error = 0;
> > +				enable_nonboot_cpus();
> 
> Why did we discard return code in previous versions? Do we still want
> to do that?

I think it was a mistake.

Rafael

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Pavel Machek]

Hi!

> > ...so, if pm_ops is non-null, power_down does nonboot cpu disabling,
> > otherwise we proceed with cpus enabled?
> > 
> > That looks ugly.
> > 
> > Is the warning bogus? Or maybe we should *always* disable nonboot cpus
> > in powerdown path?
> 
> Is disable_nonboot_cpus() assuming that first_cpu(cpu_present_map) is
> the boot cpu? Just wondering why disable_nonboot_cpus() isn't using just

I'd  say so. It is nicer (and required on some APM systems?) to do
shutdown from the boot cpu.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Pavel Machek]

Hi!

> > > Index: linux-2.6.21-rc3/kernel/power/user.c
> > > ===================================================================
> > > --- linux-2.6.21-rc3.orig/kernel/power/user.c
> > > +++ linux-2.6.21-rc3/kernel/power/user.c
> > > @@ -402,9 +402,10 @@ static int snapshot_ioctl(struct inode *
> > >  
> > >  		case PMOPS_ENTER:
> > >  			if (data->platform_suspend) {
> > > +				disable_nonboot_cpus();
> > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > -				error = 0;
> > > +				enable_nonboot_cpus();
> > 
> > Why did we discard return code in previous versions? Do we still want
> > to do that?
> 
> I think it was a mistake.

I took a look at git-annotate, and it is yours code, so I assume you
are right. ACK, then.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Rafael J. Wysocki]

On Friday, 9 March 2007 23:13, Pavel Machek wrote:
> Hi!
> 
> > > > Index: linux-2.6.21-rc3/kernel/power/user.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rc3.orig/kernel/power/user.c
> > > > +++ linux-2.6.21-rc3/kernel/power/user.c
> > > > @@ -402,9 +402,10 @@ static int snapshot_ioctl(struct inode *
> > > >  
> > > >  		case PMOPS_ENTER:
> > > >  			if (data->platform_suspend) {
> > > > +				disable_nonboot_cpus();
> > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > -				error = 0;
> > > > +				enable_nonboot_cpus();
> > > 
> > > Why did we discard return code in previous versions? Do we still want
> > > to do that?
> > 
> > I think it was a mistake.
> 
> I took a look at git-annotate, and it is yours code, so I assume you
> are right. ACK, then.

Thanks!

BTW, what about the patch at http://lkml.org/lkml/2007/3/8/363?

Rafael

Re: [PATCH] swsusp: Disable nonboot CPUs before entering platform suspend

[Pavel Machek]

On Fri 2007-03-09 23:34:00, Rafael J. Wysocki wrote:
> On Friday, 9 March 2007 23:13, Pavel Machek wrote:
> > Hi!
> > 
> > > > > Index: linux-2.6.21-rc3/kernel/power/user.c
> > > > > ===================================================================
> > > > > --- linux-2.6.21-rc3.orig/kernel/power/user.c
> > > > > +++ linux-2.6.21-rc3/kernel/power/user.c
> > > > > @@ -402,9 +402,10 @@ static int snapshot_ioctl(struct inode *
> > > > >  
> > > > >  		case PMOPS_ENTER:
> > > > >  			if (data->platform_suspend) {
> > > > > +				disable_nonboot_cpus();
> > > > >  				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> > > > >  				error = pm_ops->enter(PM_SUSPEND_DISK);
> > > > > -				error = 0;
> > > > > +				enable_nonboot_cpus();
> > > > 
> > > > Why did we discard return code in previous versions? Do we still want
> > > > to do that?
> > > 
> > > I think it was a mistake.
> > 
> > I took a look at git-annotate, and it is yours code, so I assume you
> > are right. ACK, then.
> 
> Thanks!
> 
> BTW, what about the patch at http://lkml.org/lkml/2007/3/8/363?

Seems obviously correct to me (ACK), but I did not have time to test it.

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