* [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() @ 2013-04-07 2:14 Huacai Chen 2013-04-07 8:46 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Huacai Chen @ 2013-04-07 2:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, linux-kernel, Fuxin Zhang, Huacai Chen, stable 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() 2013-04-07 2:14 [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() Huacai Chen @ 2013-04-07 8:46 ` Rafael J. Wysocki 2013-04-07 15:29 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2013-04-07 8:46 UTC (permalink / raw) To: Huacai Chen, Andrew Morton; +Cc: linux-pm, linux-kernel, Fuxin Zhang, stable 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() 2013-04-07 8:46 ` Rafael J. Wysocki @ 2013-04-07 15:29 ` Greg KH 2013-04-07 21:10 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2013-04-07 15:29 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Huacai Chen, Andrew Morton, linux-pm, linux-kernel, Fuxin Zhang, stable 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() 2013-04-07 15:29 ` Greg KH @ 2013-04-07 21:10 ` Rafael J. Wysocki 2013-04-08 2:51 ` chenhc 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2013-04-07 21:10 UTC (permalink / raw) To: Greg KH, Huacai Chen Cc: Andrew Morton, linux-pm, linux-kernel, Fuxin Zhang, stable 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() 2013-04-07 21:10 ` Rafael J. Wysocki @ 2013-04-08 2:51 ` chenhc 2013-04-08 10:55 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: chenhc @ 2013-04-08 2:51 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg KH, Andrew Morton, linux-pm, linux-kernel, Fuxin Zhang, stable > 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. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() 2013-04-08 2:51 ` chenhc @ 2013-04-08 10:55 ` Rafael J. Wysocki 2013-04-08 14:29 ` chenhc 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2013-04-08 10:55 UTC (permalink / raw) To: chenhc; +Cc: Greg KH, Andrew Morton, linux-pm, linux-kernel, Fuxin Zhang, stable 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() 2013-04-08 10:55 ` Rafael J. Wysocki @ 2013-04-08 14:29 ` chenhc 0 siblings, 0 replies; 7+ messages in thread From: chenhc @ 2013-04-08 14:29 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg KH, Andrew Morton, linux-pm, linux-kernel, Fuxin Zhang, stable > 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. > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-04-08 14:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-07 2:14 [PATCH] PM/reboot: call syscore_shutdown() after disable_nonboot_cpus() Huacai Chen 2013-04-07 8:46 ` Rafael J. Wysocki 2013-04-07 15:29 ` Greg KH 2013-04-07 21:10 ` Rafael J. Wysocki 2013-04-08 2:51 ` chenhc 2013-04-08 10:55 ` Rafael J. Wysocki 2013-04-08 14:29 ` chenhc
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).