[PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[Huacai Chen]

As commit 40dc166c (PM / Core: Introduce struct syscore_ops for core
subsystems PM) say, syscore_ops operations should be carried with one
CPU on-line and interrupts disabled. However, after commit f96972f2d
(kernel/sys.c: call disable_nonboot_cpus() in kernel_restart()),
syscore_shutdown() is called before disable_nonboot_cpus(), so break
the rules. We have a MIPS machine with a 8259A PIC, and there is an
external timer (HPET) linked at 8259A. Since 8259A has been shutdown
too early (by syscore_shutdown()), disable_nonboot_cpus() runs without
timer interrupt, so it hangs and reboot fails. This patch call
syscore_shutdown() a little later (after disable_nonboot_cpus()) to
avoid reboot failure, this is the same way as poweroff does.

BTW, add disable_nonboot_cpus() in kernel_halt() for consistency.

Signed-off-by: Huacai Chen <chenhc@lemote.com>
Cc: <stable@vger.kernel.org>
---
 kernel/sys.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 39c9c4a..0da73cf 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -324,7 +324,6 @@ void kernel_restart_prepare(char *cmd)
 	system_state = SYSTEM_RESTART;
 	usermodehelper_disable();
 	device_shutdown();
-	syscore_shutdown();
 }
 
 /**
@@ -370,6 +369,7 @@ void kernel_restart(char *cmd)
 {
 	kernel_restart_prepare(cmd);
 	disable_nonboot_cpus();
+	syscore_shutdown();
 	if (!cmd)
 		printk(KERN_EMERG "Restarting system.\n");
 	else
@@ -395,6 +395,7 @@ static void kernel_shutdown_prepare(enum system_states state)
 void kernel_halt(void)
 {
 	kernel_shutdown_prepare(SYSTEM_HALT);
+	disable_nonboot_cpus();
 	syscore_shutdown();
 	printk(KERN_EMERG "System halted.\n");
 	kmsg_dump(KMSG_DUMP_HALT);
-- 
1.7.7.3

Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[Rafael J. Wysocki]

On Sunday, April 07, 2013 10:14:14 AM Huacai Chen wrote:
> As commit 40dc166c (PM / Core: Introduce struct syscore_ops for core
> subsystems PM) say, syscore_ops operations should be carried with one
> CPU on-line and interrupts disabled. However, after commit f96972f2d
> (kernel/sys.c: call disable_nonboot_cpus() in kernel_restart()),
> syscore_shutdown() is called before disable_nonboot_cpus(), so break
> the rules. We have a MIPS machine with a 8259A PIC, and there is an
> external timer (HPET) linked at 8259A. Since 8259A has been shutdown
> too early (by syscore_shutdown()), disable_nonboot_cpus() runs without
> timer interrupt, so it hangs and reboot fails. This patch call
> syscore_shutdown() a little later (after disable_nonboot_cpus()) to
> avoid reboot failure, this is the same way as poweroff does.
> 
> BTW, add disable_nonboot_cpus() in kernel_halt() for consistency.
> 
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> Cc: <stable@vger.kernel.org>

While I agree with the changes, I'm not sure if I'm the right maintainer,
as this isn't really PM code.

Andrew, should I take this?

Rafael


> ---
>  kernel/sys.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 39c9c4a..0da73cf 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -324,7 +324,6 @@ void kernel_restart_prepare(char *cmd)
>  	system_state = SYSTEM_RESTART;
>  	usermodehelper_disable();
>  	device_shutdown();
> -	syscore_shutdown();
>  }
>  
>  /**
> @@ -370,6 +369,7 @@ void kernel_restart(char *cmd)
>  {
>  	kernel_restart_prepare(cmd);
>  	disable_nonboot_cpus();
> +	syscore_shutdown();
>  	if (!cmd)
>  		printk(KERN_EMERG "Restarting system.\n");
>  	else
> @@ -395,6 +395,7 @@ static void kernel_shutdown_prepare(enum system_states state)
>  void kernel_halt(void)
>  {
>  	kernel_shutdown_prepare(SYSTEM_HALT);
> +	disable_nonboot_cpus();
>  	syscore_shutdown();
>  	printk(KERN_EMERG "System halted.\n");
>  	kmsg_dump(KMSG_DUMP_HALT);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[Greg KH]

On Sun, Apr 07, 2013 at 10:46:00AM +0200, Rafael J. Wysocki wrote:
> On Sunday, April 07, 2013 10:14:14 AM Huacai Chen wrote:
> > As commit 40dc166c (PM / Core: Introduce struct syscore_ops for core
> > subsystems PM) say, syscore_ops operations should be carried with one
> > CPU on-line and interrupts disabled. However, after commit f96972f2d
> > (kernel/sys.c: call disable_nonboot_cpus() in kernel_restart()),
> > syscore_shutdown() is called before disable_nonboot_cpus(), so break
> > the rules. We have a MIPS machine with a 8259A PIC, and there is an
> > external timer (HPET) linked at 8259A. Since 8259A has been shutdown
> > too early (by syscore_shutdown()), disable_nonboot_cpus() runs without
> > timer interrupt, so it hangs and reboot fails. This patch call
> > syscore_shutdown() a little later (after disable_nonboot_cpus()) to
> > avoid reboot failure, this is the same way as poweroff does.
> > 
> > BTW, add disable_nonboot_cpus() in kernel_halt() for consistency.
> > 
> > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > Cc: <stable@vger.kernel.org>
> 
> While I agree with the changes, I'm not sure if I'm the right maintainer,
> as this isn't really PM code.
> 
> Andrew, should I take this?

Andrew is on vacation for a few weeks, so you might need to take this
through your tree.

greg k-h

Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[Rafael J. Wysocki]

On Sunday, April 07, 2013 08:29:32 AM Greg KH wrote:
> On Sun, Apr 07, 2013 at 10:46:00AM +0200, Rafael J. Wysocki wrote:
> > On Sunday, April 07, 2013 10:14:14 AM Huacai Chen wrote:
> > > As commit 40dc166c (PM / Core: Introduce struct syscore_ops for core
> > > subsystems PM) say, syscore_ops operations should be carried with one
> > > CPU on-line and interrupts disabled. However, after commit f96972f2d
> > > (kernel/sys.c: call disable_nonboot_cpus() in kernel_restart()),
> > > syscore_shutdown() is called before disable_nonboot_cpus(), so break
> > > the rules. We have a MIPS machine with a 8259A PIC, and there is an
> > > external timer (HPET) linked at 8259A. Since 8259A has been shutdown
> > > too early (by syscore_shutdown()), disable_nonboot_cpus() runs without
> > > timer interrupt, so it hangs and reboot fails. This patch call
> > > syscore_shutdown() a little later (after disable_nonboot_cpus()) to
> > > avoid reboot failure, this is the same way as poweroff does.
> > > 
> > > BTW, add disable_nonboot_cpus() in kernel_halt() for consistency.
> > > 
> > > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > > Cc: <stable@vger.kernel.org>
> > 
> > While I agree with the changes, I'm not sure if I'm the right maintainer,
> > as this isn't really PM code.
> > 
> > Andrew, should I take this?
> 
> Andrew is on vacation for a few weeks, so you might need to take this
> through your tree.

OK

But, it looks like we should actually disable interrupts on the remaining
CPU after we've called disable_nonboot_cpus() so that the syscore_shutdown()
assumptions are satisfied which the patch doesn't do.

Chen (I apologize if that's not the right part of your full name to use here),
do you think that's not necessary and if so, then for what reason?

Rafael


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

Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[chenhc]

> On Sunday, April 07, 2013 08:29:32 AM Greg KH wrote:
>> On Sun, Apr 07, 2013 at 10:46:00AM +0200, Rafael J. Wysocki wrote:
>> > On Sunday, April 07, 2013 10:14:14 AM Huacai Chen wrote:
>> > > As commit 40dc166c (PM / Core: Introduce struct syscore_ops for core
>> > > subsystems PM) say, syscore_ops operations should be carried with
>> one
>> > > CPU on-line and interrupts disabled. However, after commit f96972f2d
>> > > (kernel/sys.c: call disable_nonboot_cpus() in kernel_restart()),
>> > > syscore_shutdown() is called before disable_nonboot_cpus(), so break
>> > > the rules. We have a MIPS machine with a 8259A PIC, and there is an
>> > > external timer (HPET) linked at 8259A. Since 8259A has been shutdown
>> > > too early (by syscore_shutdown()), disable_nonboot_cpus() runs
>> without
>> > > timer interrupt, so it hangs and reboot fails. This patch call
>> > > syscore_shutdown() a little later (after disable_nonboot_cpus()) to
>> > > avoid reboot failure, this is the same way as poweroff does.
>> > >
>> > > BTW, add disable_nonboot_cpus() in kernel_halt() for consistency.
>> > >
>> > > Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> > > Cc: <stable@vger.kernel.org>
>> >
>> > While I agree with the changes, I'm not sure if I'm the right
>> maintainer,
>> > as this isn't really PM code.
>> >
>> > Andrew, should I take this?
>>
>> Andrew is on vacation for a few weeks, so you might need to take this
>> through your tree.
>
> OK
>
> But, it looks like we should actually disable interrupts on the remaining
> CPU after we've called disable_nonboot_cpus() so that the
> syscore_shutdown()
> assumptions are satisfied which the patch doesn't do.
>
> Chen (I apologize if that's not the right part of your full name to use
> here),
> do you think that's not necessary and if so, then for what reason?

Reboot and poweroff are both OK after I move syscore_shutdown(), I also don't
know whether we should disable interrupts. Since you are the author of
commit 40dc166c (PM / Core: Introduce struct syscore_ops for core subsystems
 PM), please tell me why syscore_shutdown() need interrupt disabled? (I just
know that syscore_shutdown() will cause disable_nonboot_cpus() hang)

BTW, Chen is my last name and you needn't worry about this.

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

Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[Rafael J. Wysocki]

On Monday, April 08, 2013 10:51:45 AM chenhc@lemote.com wrote:
> > On Sunday, April 07, 2013 08:29:32 AM Greg KH wrote:
> >> On Sun, Apr 07, 2013 at 10:46:00AM +0200, Rafael J. Wysocki wrote:
> >> > On Sunday, April 07, 2013 10:14:14 AM Huacai Chen wrote:
> >> > > As commit 40dc166c (PM / Core: Introduce struct syscore_ops for core
> >> > > subsystems PM) say, syscore_ops operations should be carried with
> >> one
> >> > > CPU on-line and interrupts disabled. However, after commit f96972f2d
> >> > > (kernel/sys.c: call disable_nonboot_cpus() in kernel_restart()),
> >> > > syscore_shutdown() is called before disable_nonboot_cpus(), so break
> >> > > the rules. We have a MIPS machine with a 8259A PIC, and there is an
> >> > > external timer (HPET) linked at 8259A. Since 8259A has been shutdown
> >> > > too early (by syscore_shutdown()), disable_nonboot_cpus() runs
> >> without
> >> > > timer interrupt, so it hangs and reboot fails. This patch call
> >> > > syscore_shutdown() a little later (after disable_nonboot_cpus()) to
> >> > > avoid reboot failure, this is the same way as poweroff does.
> >> > >
> >> > > BTW, add disable_nonboot_cpus() in kernel_halt() for consistency.
> >> > >
> >> > > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> >> > > Cc: <stable@vger.kernel.org>
> >> >
> >> > While I agree with the changes, I'm not sure if I'm the right
> >> maintainer,
> >> > as this isn't really PM code.
> >> >
> >> > Andrew, should I take this?
> >>
> >> Andrew is on vacation for a few weeks, so you might need to take this
> >> through your tree.
> >
> > OK
> >
> > But, it looks like we should actually disable interrupts on the remaining
> > CPU after we've called disable_nonboot_cpus() so that the
> > syscore_shutdown()
> > assumptions are satisfied which the patch doesn't do.
> >
> > Chen (I apologize if that's not the right part of your full name to use
> > here),
> > do you think that's not necessary and if so, then for what reason?
> 
> Reboot and poweroff are both OK after I move syscore_shutdown(), I also don't
> know whether we should disable interrupts. Since you are the author of
> commit 40dc166c (PM / Core: Introduce struct syscore_ops for core subsystems
>  PM), please tell me why syscore_shutdown() need interrupt disabled?

Well, the syscore_ callbacks generally do things that may go wrong when an
interrupt comes in while they are being done.  At least that's what happens
for the suspend/resume syscore_ callbacks, but I'm not really sure about the
shutdown ones.

I suppose we can leave it as is for the time being.  At least I'm not aware of
any problems related to that.

Thanks,
Rafael


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

Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus()

[chenhc]

> On Monday, April 08, 2013 10:51:45 AM chenhc@lemote.com wrote:
>> > On Sunday, April 07, 2013 08:29:32 AM Greg KH wrote:
>> >> On Sun, Apr 07, 2013 at 10:46:00AM +0200, Rafael J. Wysocki wrote:
>> >> > On Sunday, April 07, 2013 10:14:14 AM Huacai Chen wrote:
>> >> > > As commit 40dc166c (PM / Core: Introduce struct syscore_ops for
>> core
>> >> > > subsystems PM) say, syscore_ops operations should be carried with
>> >> one
>> >> > > CPU on-line and interrupts disabled. However, after commit
>> f96972f2d
>> >> > > (kernel/sys.c: call disable_nonboot_cpus() in kernel_restart()),
>> >> > > syscore_shutdown() is called before disable_nonboot_cpus(), so
>> break
>> >> > > the rules. We have a MIPS machine with a 8259A PIC, and there is
>> an
>> >> > > external timer (HPET) linked at 8259A. Since 8259A has been
>> shutdown
>> >> > > too early (by syscore_shutdown()), disable_nonboot_cpus() runs
>> >> without
>> >> > > timer interrupt, so it hangs and reboot fails. This patch call
>> >> > > syscore_shutdown() a little later (after disable_nonboot_cpus())
>> to
>> >> > > avoid reboot failure, this is the same way as poweroff does.
>> >> > >
>> >> > > BTW, add disable_nonboot_cpus() in kernel_halt() for consistency.
>> >> > >
>> >> > > Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> >> > > Cc: <stable@vger.kernel.org>
>> >> >
>> >> > While I agree with the changes, I'm not sure if I'm the right
>> >> maintainer,
>> >> > as this isn't really PM code.
>> >> >
>> >> > Andrew, should I take this?
>> >>
>> >> Andrew is on vacation for a few weeks, so you might need to take this
>> >> through your tree.
>> >
>> > OK
>> >
>> > But, it looks like we should actually disable interrupts on the
>> remaining
>> > CPU after we've called disable_nonboot_cpus() so that the
>> > syscore_shutdown()
>> > assumptions are satisfied which the patch doesn't do.
>> >
>> > Chen (I apologize if that's not the right part of your full name to
>> use
>> > here),
>> > do you think that's not necessary and if so, then for what reason?
>>
>> Reboot and poweroff are both OK after I move syscore_shutdown(), I also
>> don't
>> know whether we should disable interrupts. Since you are the author of
>> commit 40dc166c (PM / Core: Introduce struct syscore_ops for core
>> subsystems
>>  PM), please tell me why syscore_shutdown() need interrupt disabled?
>
> Well, the syscore_ callbacks generally do things that may go wrong when an
> interrupt comes in while they are being done.  At least that's what
> happens
> for the suspend/resume syscore_ callbacks, but I'm not really sure about
> the
> shutdown ones.
>
> I suppose we can leave it as is for the time being.  At least I'm not
> aware of
> any problems related to that.
OK, linux-3.0, 3.2 and 3.4 -stable branch also need this patch because they
also have merged the commit f96972f2d (kernel/sys.c: call
disable_nonboot_cpus() in kernel_restart())

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