[patch] properly stop devices before poweroff

[patch] properly stop devices before poweroff

[Pavel Machek]

Without this patch, Linux provokes emergency disk shutdowns and
similar nastiness. It was in SuSE kernels for some time, IIRC.

Signed-off-by: Pavel Machek <pavel@suse.cz>

--- clean/kernel/sys.c	2005-03-19 00:32:32.000000000 +0100
+++ linux/kernel/sys.c	2005-03-22 12:20:53.000000000 +0100
@@ -404,6 +404,7 @@
 	case LINUX_REBOOT_CMD_HALT:
 		notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
 		system_state = SYSTEM_HALT;
+		device_suspend(PMSG_SUSPEND);
 		device_shutdown();
 		printk(KERN_EMERG "System halted.\n");
 		machine_halt();
@@ -414,6 +415,7 @@
 	case LINUX_REBOOT_CMD_POWER_OFF:
 		notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
 		system_state = SYSTEM_POWER_OFF;
+		device_suspend(PMSG_SUSPEND);
 		device_shutdown();
 		printk(KERN_EMERG "Power down.\n");
 		machine_power_off();
@@ -430,6 +432,7 @@
 
 		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, buffer);
 		system_state = SYSTEM_RESTART;
+		device_suspend(PMSG_FREEZE);
 		device_shutdown();
 		printk(KERN_EMERG "Restarting system with command '%s'.\n", buffer);
 		machine_restart(buffer);

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [patch] properly stop devices before poweroff

[tvrtko.ursulin]

On 21/04/2005 12:13:46 linux-kernel-owner wrote:

>Without this patch, Linux provokes emergency disk shutdowns and
>similar nastiness. It was in SuSE kernels for some time, IIRC.

OMG! And I did try to raise that issue 10 months ago, see below:

http://www.ussg.iu.edu/hypermail/linux/kernel/0406.0/0242.html

Re: [patch] properly stop devices before poweroff

[Pavel Machek]

Hi!

> >Without this patch, Linux provokes emergency disk shutdowns and
> >similar nastiness. It was in SuSE kernels for some time, IIRC.
> 
> OMG! And I did try to raise that issue 10 months ago, see below:
> 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0406.0/0242.html

Heh, verify that this patch works for you and it might finally get
fixed...

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [patch] properly stop devices before poweroff

[tvrtko.ursulin]

On 21/04/2005 12:38:29 linux-kernel-owner wrote:

>> >Without this patch, Linux provokes emergency disk shutdowns and
>> >similar nastiness. It was in SuSE kernels for some time, IIRC.
>> OMG! And I did try to raise that issue 10 months ago, see below:
>> http://www.ussg.iu.edu/hypermail/linux/kernel/0406.0/0242.html
>
>Heh, verify that this patch works for you and it might finally get
>fixed...

I can't "hear" the difference between normal and emergency shutdown, and 
information I found does not suggest they are recorded amongst s.m.a.r.t 
attributes. Power_Cycle_Count, Load_Cycle_Count and 
Power-Off_Retract_Count seem to be in sync, although common sense would 
suggests that the last one should possibly not increment on a clean 
shutdown. I will experiment a bit when I find some spare time.

Re: [patch] properly stop devices before poweroff

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> 
> Without this patch, Linux provokes emergency disk shutdowns and
> similar nastiness. It was in SuSE kernels for some time, IIRC.
> 

With this patch when running `halt -p' my ia64 Tiger (using
tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
and then hangs up.

Unfortunately it all seems to happen after the serial port has been
disabled because nothing comes out.  I set the console to a squitty font
and took a piccy.  See
http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg

I guess it's an ia64 problem.  I'll leave the patch in -mm for now.

> 
> --- clean/kernel/sys.c	2005-03-19 00:32:32.000000000 +0100
> +++ linux/kernel/sys.c	2005-03-22 12:20:53.000000000 +0100
> @@ -404,6 +404,7 @@
>  	case LINUX_REBOOT_CMD_HALT:
>  		notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
>  		system_state = SYSTEM_HALT;
> +		device_suspend(PMSG_SUSPEND);
>  		device_shutdown();
>  		printk(KERN_EMERG "System halted.\n");
>  		machine_halt();
> @@ -414,6 +415,7 @@
>  	case LINUX_REBOOT_CMD_POWER_OFF:
>  		notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
>  		system_state = SYSTEM_POWER_OFF;
> +		device_suspend(PMSG_SUSPEND);
>  		device_shutdown();
>  		printk(KERN_EMERG "Power down.\n");
>  		machine_power_off();
> @@ -430,6 +432,7 @@
>  
>  		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, buffer);
>  		system_state = SYSTEM_RESTART;
> +		device_suspend(PMSG_FREEZE);
>  		device_shutdown();
>  		printk(KERN_EMERG "Restarting system with command '%s'.\n", buffer);
>  		machine_restart(buffer);
> 
> -- 
> Boycott Kodak -- for their patent abuse against Java.

Re: [patch] properly stop devices before poweroff

[Zwane Mwaikambo]

On Fri, 29 Apr 2005, Andrew Morton wrote:

> Pavel Machek <pavel@ucw.cz> wrote:
> >
> > 
> > Without this patch, Linux provokes emergency disk shutdowns and
> > similar nastiness. It was in SuSE kernels for some time, IIRC.
> > 
> 
> With this patch when running `halt -p' my ia64 Tiger (using
> tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
> and then hangs up.
> 
> Unfortunately it all seems to happen after the serial port has been
> disabled because nothing comes out.  I set the console to a squitty font
> and took a piccy.  See
> http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg
> 
> I guess it's an ia64 problem.  I'll leave the patch in -mm for now.

Could you cat /proc/interrupts during runtime? It looks like the device 
being suspended never went through pci_device_enable() (e.g. ethernet 
interface wasn't up). It's harmless right now.

Thanks,
	Zwane

Re: [patch] properly stop devices before poweroff

[Andrew Morton]

Zwane Mwaikambo <zwane@arm.linux.org.uk> wrote:
>
> On Fri, 29 Apr 2005, Andrew Morton wrote:
> 
> > Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > 
> > > Without this patch, Linux provokes emergency disk shutdowns and
> > > similar nastiness. It was in SuSE kernels for some time, IIRC.
> > > 
> > 
> > With this patch when running `halt -p' my ia64 Tiger (using
> > tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
> > and then hangs up.
> > 
> > Unfortunately it all seems to happen after the serial port has been
> > disabled because nothing comes out.  I set the console to a squitty font
> > and took a piccy.  See
> > http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg
> > 
> > I guess it's an ia64 problem.  I'll leave the patch in -mm for now.
> 
> Could you cat /proc/interrupts during runtime?

           CPU0       CPU1       CPU2       CPU3       
 28:          0          0          0          0          LSAPIC  cpe_poll
 29:          0          0          0          0          LSAPIC  cmc_poll
 30:          0          0          0          0  IO-SAPIC-level  cpe_hndlr
 31:          0          0          0          0          LSAPIC  cmc_hndlr
 34:         19          0          0          0   IO-SAPIC-edge  ide0
 39:          0          0          0          0  IO-SAPIC-level  acpi
 48:        382          0          0          0  IO-SAPIC-level  eth0
 49:          0       2835          0          0  IO-SAPIC-level  ioc0
 50:          0          0         29          0  IO-SAPIC-level  ioc1
 51:          0          0          0          0  IO-SAPIC-level  uhci_hcd:usb1
 52:         51          0          0          0  IO-SAPIC-level  uhci_hcd:usb2
232:          0          0          0          0          LSAPIC  mca_rdzv
238:          0          0          0          0          LSAPIC  perfmon
239:     122785     128748     129310     130523          LSAPIC  timer
240:          0          0          0          0          LSAPIC  mca_wkup
254:         29         77        101        102          LSAPIC  IPI
ERR:          0

> It looks like the device 
> being suspended never went through pci_device_enable() (e.g. ethernet 
> interface wasn't up). It's harmless right now.

Everything's up.  Perhaps we're trying to disable devices more than once?

Re: [patch] properly stop devices before poweroff

[Pavel Machek]

Hi!

> > It looks like the device 
> > being suspended never went through pci_device_enable() (e.g. ethernet 
> > interface wasn't up). It's harmless right now.
> 
> Everything's up.  Perhaps we're trying to disable devices more than once?

Maybe userspace ifconfigs eth0 down, then we device_suspend it?
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [patch] properly stop devices before poweroff

[Kenji Kaneshige]

Hi,

Andrew Morton wrote:
> Pavel Machek <pavel@ucw.cz> wrote:
> 
>>
>>Without this patch, Linux provokes emergency disk shutdowns and
>>similar nastiness. It was in SuSE kernels for some time, IIRC.
>>
> 
> 
> With this patch when running `halt -p' my ia64 Tiger (using
> tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
> and then hangs up.
> 
> Unfortunately it all seems to happen after the serial port has been
> disabled because nothing comes out.  I set the console to a squitty font
> and took a piccy.  See
> http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg
> 
> I guess it's an ia64 problem.  I'll leave the patch in -mm for now.
> 

I guess the stream of badness was occured as follows:

    pcibios_disable_device() for ia64 assumes that pci_enable_device()
    and pci_disable_device() are balanced. But with 'properly stop
    devices before power off' patch, pci_disable_device() becomes to be
    called twice for e1000 device at halt time, through reboot_notifier_list
    callback and through device_suspend(). As a result, iosapic_unregister_intr()
    was called for already unregistered gsi and then stream of badness
    was displayed.

I think the following patch will remove this stream of badness. I'm
sorry but I have not checked if the stream of badness is actually
removed because I'm on vacation and I can't look at my display
(I'm working via remote console). Could you try this patch?

By the way, I don't think this stream of badness is related to hang up,
because the problem (hang up) was reproduced even on my test kernel that
doesn't call pcibios_disable_device().

Thanks,
Kenji Kaneshige
---


There might be some cases that pci_disable_device() is called even if
the device is already disabled. In this case, pcibios_disable_device()
should not call acpi_pci_irq_disable() for the device.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>


---

 linux-2.6.12-rc3-mm2-kanesige/arch/ia64/pci/pci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

diff -puN arch/ia64/pci/pci.c~fix_pcibios_disable_device_ia64 arch/ia64/pci/pci.c
--- linux-2.6.12-rc3-mm2/arch/ia64/pci/pci.c~fix_pcibios_disable_device_ia64	2005-05-02 03:12:23.000000000 +0900
+++ linux-2.6.12-rc3-mm2-kanesige/arch/ia64/pci/pci.c	2005-05-02 03:12:23.000000000 +0900
@@ -512,7 +512,8 @@ pcibios_enable_device (struct pci_dev *d
 void
 pcibios_disable_device (struct pci_dev *dev)
 {
-	acpi_pci_irq_disable(dev);
+	if (dev->is_enabled)
+		acpi_pci_irq_disable(dev);
 }
 #endif /* CONFIG_ACPI_DEALLOCATE_IRQ */
 

_

Re: [patch] properly stop devices before poweroff

[Pavel Machek]

Hi!

> >>Without this patch, Linux provokes emergency disk shutdowns and
> >>similar nastiness. It was in SuSE kernels for some time, IIRC.
> >>
> >
> >
> >With this patch when running `halt -p' my ia64 Tiger (using
> >tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
> >and then hangs up.
> >
> >Unfortunately it all seems to happen after the serial port has been
> >disabled because nothing comes out.  I set the console to a squitty font
> >and took a piccy.  See
> >http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg
> >
> >I guess it's an ia64 problem.  I'll leave the patch in -mm for now.
> >
> 
> I guess the stream of badness was occured as follows:
> 
>    pcibios_disable_device() for ia64 assumes that pci_enable_device()
>    and pci_disable_device() are balanced. But with 'properly stop
>    devices before power off' patch, pci_disable_device() becomes to be
>    called twice for e1000 device at halt time, through reboot_notifier_list
>    callback and through device_suspend(). As a result, 
>    iosapic_unregister_intr()
>    was called for already unregistered gsi and then stream of badness
>    was displayed.
> 
> I think the following patch will remove this stream of badness. I'm
> sorry but I have not checked if the stream of badness is actually
> removed because I'm on vacation and I can't look at my display
> (I'm working via remote console). Could you try this patch?
> 
> By the way, I don't think this stream of badness is related to hang up,
> because the problem (hang up) was reproduced even on my test kernel that
> doesn't call pcibios_disable_device().

Looks good to me. Plus I guess we should remove reboot notifier from
e1000... It should be obsoleted by driver model.
								Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [patch] properly stop devices before poweroff

[Adam Belay]

On Mon, May 02, 2005 at 04:09:08AM +0900, Kenji Kaneshige wrote:
> Hi,
> 
> Andrew Morton wrote:
> >Pavel Machek <pavel@ucw.cz> wrote:
> >
> >>
> >>Without this patch, Linux provokes emergency disk shutdowns and
> >>similar nastiness. It was in SuSE kernels for some time, IIRC.
> >>
> >
> >
> >With this patch when running `halt -p' my ia64 Tiger (using
> >tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
> >and then hangs up.
> >
> >Unfortunately it all seems to happen after the serial port has been
> >disabled because nothing comes out.  I set the console to a squitty font
> >and took a piccy.  See
> >http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg
> >
> >I guess it's an ia64 problem.  I'll leave the patch in -mm for now.
> >
> 
> I guess the stream of badness was occured as follows:
> 
>    pcibios_disable_device() for ia64 assumes that pci_enable_device()
>    and pci_disable_device() are balanced. But with 'properly stop
>    devices before power off' patch, pci_disable_device() becomes to be
>    called twice for e1000 device at halt time, through reboot_notifier_list
>    callback and through device_suspend(). As a result, 
>    iosapic_unregister_intr()
>    was called for already unregistered gsi and then stream of badness
>    was displayed.
> 
> I think the following patch will remove this stream of badness. I'm
> sorry but I have not checked if the stream of badness is actually
> removed because I'm on vacation and I can't look at my display
> (I'm working via remote console). Could you try this patch?
> 
> By the way, I don't think this stream of badness is related to hang up,
> because the problem (hang up) was reproduced even on my test kernel that
> doesn't call pcibios_disable_device().
> 
> Thanks,
> Kenji Kaneshige
> ---
> 
> 
> There might be some cases that pci_disable_device() is called even if
> the device is already disabled. In this case, pcibios_disable_device()
> should not call acpi_pci_irq_disable() for the device.
> 
> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> 

Although this would solve the problem, it may or may not be the right thing to
do.  The bug is not in pci_disable_device(), it's in the fact that we're
calling pci_disable_device() twice.  Whether pci_disable_device() should ignore
this or create an error is an implementation decision.  It might make sense to
have it print a warning. Greg, what are your thoughts?

What's important is that we don't want to suspend the device twice (in this
case suspend and reboot_notifier).

Thanks,
Adam

Re: [patch] properly stop devices before poweroff

[Adam Belay]

On Thu, Apr 21, 2005 at 01:13:46PM +0200, Pavel Machek wrote:
> 
> Without this patch, Linux provokes emergency disk shutdowns and
> similar nastiness. It was in SuSE kernels for some time, IIRC.
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>

Hi Pavel,

Although I like using pm_message_t, I'm not sure if we want to commit to only
PMSG_SUSPEND and PMSG_FREEZE for shutdown and reboot.  Would it be possible
to create a PMSG_HALT and PMSG_REBOOT?  I think this would give drivers more
control and flexability to make the right decision.  What is your opinion?

Of course, I'm still considering the posibility that we really want to do
PMSG_SUSPEND on a shutdown.  This may work ok on X86, I'm not sure about other
architectures.

I know you mentioned previously adding more flags and data to pm_message_t,
what exactly are your plans?

Thanks,
Adam

Re: [patch] properly stop devices before poweroff

[Pavel Machek]

Hi!

> > Without this patch, Linux provokes emergency disk shutdowns and
> > similar nastiness. It was in SuSE kernels for some time, IIRC.
> > 
> > Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> Hi Pavel,
> 
> Although I like using pm_message_t, I'm not sure if we want to commit to only
> PMSG_SUSPEND and PMSG_FREEZE for shutdown and reboot.  Would it be possible
> to create a PMSG_HALT and PMSG_REBOOT?  I think this would give drivers more
> control and flexability to make the right decision.  What is your opinion?
> 
> Of course, I'm still considering the posibility that we really want to do
> PMSG_SUSPEND on a shutdown.  This may work ok on X86, I'm not sure about other
> architectures.

Thats okay, nobody really knows yet. I believe that SUSPEND and HALT
are very similar, and flags best way to separate them. I believe that
FREEZE and REBOOT are very similar, too, and again would use flags to
tell between them.

> I know you mentioned previously adding more flags and data to pm_message_t,
> what exactly are your plans?

First I want type checking for pm_message_t. That's 2.6.12-early
material. Then, when it is *really clear* that flags are needed, I'll
add them. "really needed" as in "we have a driver where it matters".
									Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [patch] properly stop devices before poweroff

[Adam Belay]

On Mon, May 02, 2005 at 01:16:35AM +0200, Pavel Machek wrote:
> Hi!
> 
> > > Without this patch, Linux provokes emergency disk shutdowns and
> > > similar nastiness. It was in SuSE kernels for some time, IIRC.
> > > 
> > > Signed-off-by: Pavel Machek <pavel@suse.cz>
> > 
> > Hi Pavel,
> > 
> > Although I like using pm_message_t, I'm not sure if we want to commit to only
> > PMSG_SUSPEND and PMSG_FREEZE for shutdown and reboot.  Would it be possible
> > to create a PMSG_HALT and PMSG_REBOOT?  I think this would give drivers more
> > control and flexability to make the right decision.  What is your opinion?
> > 
> > Of course, I'm still considering the posibility that we really want to do
> > PMSG_SUSPEND on a shutdown.  This may work ok on X86, I'm not sure about other
> > architectures.
> 
> Thats okay, nobody really knows yet. I believe that SUSPEND and HALT
> are very similar, and flags best way to separate them. I believe that
> FREEZE and REBOOT are very similar, too, and again would use flags to
> tell between them.

I'm not so sure about SUSPEND and HALT being similar.  It might be much faster
to have a routine that ignores slow state changes and goes directly for
stopping device activity.  Still, I'm not entirely decided.  On ACPI systems
SUSPEND should generally work, as it's the intended behavior for S4 which is
basically like S5 in many cases.  However, having a specific HALT message
might allow driver developers to clarify this ambiguity on a per-device basis.

As for FREEZE, it does seem to match with REBOOT well.  But it really depends
on what other things we intend to use FREEZE for (ex. CPUFREQ might require
something slightly different).

> 
> > I know you mentioned previously adding more flags and data to pm_message_t,
> > what exactly are your plans?
> 
> First I want type checking for pm_message_t. That's 2.6.12-early
> material. Then, when it is *really clear* that flags are needed, I'll
> add them. "really needed" as in "we have a driver where it matters".

I think it's already clear that we need to pass the specific device state.
Also the intended global system state transition might be helpful.

However, at the moment I'm considering using a slightly different API for
these sort of things.  It would include "->prepare_state_change()" and
"->complete_state_change()"  I'll have further justification for these
changes soon, however, I would like to leave the current stuff around also
as it would still be useful for shutdown and reboot, non-PM suspend issues,
and backward compatibilty.

Thanks,
Adam

Re: [patch] properly stop devices before poweroff

[Pavel Machek]

Hi!

> > Thats okay, nobody really knows yet. I believe that SUSPEND and HALT
> > are very similar, and flags best way to separate them. I believe that
> > FREEZE and REBOOT are very similar, too, and again would use flags to
> > tell between them.
> 
> I'm not so sure about SUSPEND and HALT being similar.  It might be much faster
> to have a routine that ignores slow state changes and goes directly for
> stopping device activity.  Still, I'm not entirely decided.  On ACPI systems
> SUSPEND should generally work, as it's the intended behavior for S4 which is
> basically like S5 in many cases.  However, having a specific HALT message
> might allow driver developers to clarify this ambiguity on a per-device basis.

Umm, you are right, HALT is somewhere between FREEZE and SUSPEND.

> > First I want type checking for pm_message_t. That's 2.6.12-early
> > material. Then, when it is *really clear* that flags are needed, I'll
> > add them. "really needed" as in "we have a driver where it matters".
> 
> I think it's already clear that we need to pass the specific device state.
> Also the intended global system state transition might be helpful.

Global system state transition should be clear from flags. Specific
device state is needed for selective suspend, elsewhere I'd go for
helper function.

> However, at the moment I'm considering using a slightly different API for
> these sort of things.  It would include "->prepare_state_change()" and
> "->complete_state_change()"  I'll have further justification for these
> changes soon, however, I would like to leave the current stuff around also
> as it would still be useful for shutdown and reboot, non-PM suspend issues,
> and backward compatibilty.

I'd prefer not to have more callbacks (unless absolutely
neccessary). We used to have them, and people got it wrong....

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [patch] properly stop devices before poweroff

[Andrew Morton]

Adam Belay <ambx1@neo.rr.com> wrote:
>
> On Mon, May 02, 2005 at 04:09:08AM +0900, Kenji Kaneshige wrote:
> > Hi,
> > 
> > Andrew Morton wrote:
> > >Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > >>
> > >>Without this patch, Linux provokes emergency disk shutdowns and
> > >>similar nastiness. It was in SuSE kernels for some time, IIRC.
> > >>
> > >
> > >
> > >With this patch when running `halt -p' my ia64 Tiger (using
> > >tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
> > >and then hangs up.

A little reminder that this bug remains unfixed...

> > >Unfortunately it all seems to happen after the serial port has been
> > >disabled because nothing comes out.  I set the console to a squitty font
> > >and took a piccy.  See
> > >http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg
> > >
> > >I guess it's an ia64 problem.  I'll leave the patch in -mm for now.
> > >
> > 
> > I guess the stream of badness was occured as follows:
> > 
> >    pcibios_disable_device() for ia64 assumes that pci_enable_device()
> >    and pci_disable_device() are balanced. But with 'properly stop
> >    devices before power off' patch, pci_disable_device() becomes to be
> >    called twice for e1000 device at halt time, through reboot_notifier_list
> >    callback and through device_suspend(). As a result, 
> >    iosapic_unregister_intr()
> >    was called for already unregistered gsi and then stream of badness
> >    was displayed.
> > 
> > I think the following patch will remove this stream of badness. I'm
> > sorry but I have not checked if the stream of badness is actually
> > removed because I'm on vacation and I can't look at my display
> > (I'm working via remote console). Could you try this patch?
> > 
> > By the way, I don't think this stream of badness is related to hang up,
> > because the problem (hang up) was reproduced even on my test kernel that
> > doesn't call pcibios_disable_device().
> > 
> > Thanks,
> > Kenji Kaneshige
> > ---
> > 
> > 
> > There might be some cases that pci_disable_device() is called even if
> > the device is already disabled. In this case, pcibios_disable_device()
> > should not call acpi_pci_irq_disable() for the device.
> > 
> > Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> > 
> 
> Although this would solve the problem, it may or may not be the right thing to
> do.  The bug is not in pci_disable_device(), it's in the fact that we're
> calling pci_disable_device() twice.  Whether pci_disable_device() should ignore
> this or create an error is an implementation decision.  It might make sense to
> have it print a warning. Greg, what are your thoughts?
> 
> What's important is that we don't want to suspend the device twice (in this
> case suspend and reboot_notifier).
> 
> Thanks,
> Adam
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] properly stop devices before poweroff

[Kenji Kaneshige]

Hi,

I don't think the problem (hang up) is ia64 specific because
it is reproduced on my i386 box. The problem (hang up) seems
to occur on the machine that has Fusion MPT SCSI host adapter.
'SYNCHRONIZE_CACHE' command to MPT SCSI seems to never return
at device_shutdown() time.

Thanks,
Kenji Kaneshige


Andrew Morton wrote:
> Adam Belay <ambx1@neo.rr.com> wrote:
> 
>>On Mon, May 02, 2005 at 04:09:08AM +0900, Kenji Kaneshige wrote:
>>
>>>Hi,
>>>
>>>Andrew Morton wrote:
>>>
>>>>Pavel Machek <pavel@ucw.cz> wrote:
>>>>
>>>>
>>>>>Without this patch, Linux provokes emergency disk shutdowns and
>>>>>similar nastiness. It was in SuSE kernels for some time, IIRC.
>>>>>
>>>>
>>>>
>>>>With this patch when running `halt -p' my ia64 Tiger (using
>>>>tiger_defconfig) gets a stream of badnesses in iosapic_unregister_intr()
>>>>and then hangs up.
> 
> 
> A little reminder that this bug remains unfixed...
> 
> 
>>>>Unfortunately it all seems to happen after the serial port has been
>>>>disabled because nothing comes out.  I set the console to a squitty font
>>>>and took a piccy.  See
>>>>http://www.zip.com.au/~akpm/linux/patches/stuff/dsc02505.jpg
>>>>
>>>>I guess it's an ia64 problem.  I'll leave the patch in -mm for now.
>>>>
>>>
>>>I guess the stream of badness was occured as follows:
>>>
>>>   pcibios_disable_device() for ia64 assumes that pci_enable_device()
>>>   and pci_disable_device() are balanced. But with 'properly stop
>>>   devices before power off' patch, pci_disable_device() becomes to be
>>>   called twice for e1000 device at halt time, through reboot_notifier_list
>>>   callback and through device_suspend(). As a result, 
>>>   iosapic_unregister_intr()
>>>   was called for already unregistered gsi and then stream of badness
>>>   was displayed.
>>>
>>>I think the following patch will remove this stream of badness. I'm
>>>sorry but I have not checked if the stream of badness is actually
>>>removed because I'm on vacation and I can't look at my display
>>>(I'm working via remote console). Could you try this patch?
>>>
>>>By the way, I don't think this stream of badness is related to hang up,
>>>because the problem (hang up) was reproduced even on my test kernel that
>>>doesn't call pcibios_disable_device().
>>>
>>>Thanks,
>>>Kenji Kaneshige
>>>---
>>>
>>>
>>>There might be some cases that pci_disable_device() is called even if
>>>the device is already disabled. In this case, pcibios_disable_device()
>>>should not call acpi_pci_irq_disable() for the device.
>>>
>>>Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
>>>
>>Although this would solve the problem, it may or may not be the right thing to
>>do.  The bug is not in pci_disable_device(), it's in the fact that we're
>>calling pci_disable_device() twice.  Whether pci_disable_device() should ignore
>>this or create an error is an implementation decision.  It might make sense to
>>have it print a warning. Greg, what are your thoughts?
>>
>>What's important is that we don't want to suspend the device twice (in this
>>case suspend and reboot_notifier).
>>
>>Thanks,
>>Adam
>>-
>>To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [patch] properly stop devices before poweroff

[Luck, Tony]

I started on my OLS homework from Andrew ... and began looking
into what is going on here.

The story so far: Pavel added calls to device_suspend() to three of
the cases in the sys_reboot() path.  This stopped ia64 from being
able to shutdown.  There is a oops with a stacktrace pointing back
at the sys_reboot call.

Initial analysis from Kenji Kaneshige said that we might fix this
by adding a patch to the ia64 version of pcibios_disable_device()
to make it check whether the device was enabled before calling
acpi_pci_irq_disable().  This might fix things if the issue was
simply problems with this being called twice.  Pavel sent a
"Looks good", Adam said "Is it the right fix?"

I just tried it ... it doesn't work, we still see an oops from
the sys_reboot() ... so shutdown hangs, and we can sidestep the
issue of whether this is ideologically correct.

Then I wondered whether it was just an e1000 problem (since Pavel
had also commented that we should perhaps removed the reboot notifier
from the e1000 driver).  So I rebuilt my kernel with e1000 as a module
and ran "rmmod e1000" before running shutdown.  Which promptly failed
with a stack trace that ran through mptscsih driver instead of e1000.
[N.B. Kaneshige-san has also noted that his i386 system which has the
mptfusion also hangs executing a SYNCHRONIZE_CACHE command]. 

Code examination in the e1000 case shows that we are a bit confused
and call first:

notifier_call_chain(reboot_notifier, ...)
  e1000_notify_reboot(...)
    e1000_suspend(...)
      pci_disable_device(...)
      pci_set_power_state(...)

and then:

device_suspend(...)
   suspend_device() == dev->bus->suspend == pci_device_suspend
     e1000_suspend()
       pci_disable_device()
       pciset_power_state()

So it looks like Pavel is right ... registering the reboot notifier is
bogus in e1000.  Commenting out the register/unregister also allows
me to get past the e1000 so that I can fail in mptscsih_suspend() path.
But the fusion driver doesn't register a reboot notifier, so I can't
try the same trick there :-)

Andrew: How did you get the squitty font on ia64?  The stack trace
from the oops flies off the top of the screen.  I've tried a few
"vga=0x0f07" type options, but I keep getting a big font.

-Tony

Re: [patch] properly stop devices before poweroff

[Andrew Morton]

"Luck, Tony" <tony.luck@intel.com> wrote:
>
> I started on my OLS homework from Andrew ... and began looking
> into what is going on here.
> 

Thanks ;) I guess we'll end up with a better kernel, even though you appear
to be an innocent victim here.

> 
> Andrew: How did you get the squitty font on ia64?  The stack trace
> from the oops flies off the top of the screen.  I've tried a few
> "vga=0x0f07" type options, but I keep getting a big font.

I put

SYSFONT="iso08.08"

in /etc/sysconfig/i18n

Re: [patch] properly stop devices before poweroff

[tony.luck]

Andrew Morton wrote:
> "Luck, Tony" <tony.luck@intel.com> wrote:
> >
> > I started on my OLS homework from Andrew ... and began looking
> > into what is going on here.
> > 
> 
> Thanks ;) I guess we'll end up with a better kernel, even though you appear
> to be an innocent victim here.

The "Badness in iosapic_unregister_intr at arch/ia64/kernel/iosapic.c:851"
messages are caused by a missing call to free_irq() in the mpt/fusion driver.
I think that it should go here ... but someone with a clue should verify:

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1384,6 +1384,8 @@ mpt_suspend(struct pci_dev *pdev, pm_mes
 	/* Clear any lingering interrupt */
 	CHIPREG_WRITE32(&ioc->chip->IntStatus, 0);
 
+	free_irq(ioc->pci_irq, ioc);
+
 	pci_disable_device(pdev);
 	pci_set_power_state(pdev, device_state);
 

But even this doesn't fix the hang during shutdown :-(

The remaining problem is cause by the order of the calls in sys_reboot:

                device_suspend(PMSG_SUSPEND);
                device_shutdown();

The call to device_suspend() shuts down the mpt/fusion driver.  But then
device_shutdown() calls sd_shutdown() which prints:

  Synchronizing SCSI cache for disk sdb

and then calls sd_sync_cache().  Now since we suspended mpt/fusion, this is
going to go nowhere.

I don't know how to fix this.  Re-ordering the suspend & shutdown just looks
wrong.

-Tony

Re: [patch] properly stop devices before poweroff

[Andrew Morton]

tony.luck@intel.com wrote:
>
> Andrew Morton wrote:
> > "Luck, Tony" <tony.luck@intel.com> wrote:
> > >
> > > I started on my OLS homework from Andrew ... and began looking
> > > into what is going on here.
> > > 
> > 
> > Thanks ;) I guess we'll end up with a better kernel, even though you appear
> > to be an innocent victim here.
> 
> The "Badness in iosapic_unregister_intr at arch/ia64/kernel/iosapic.c:851"
> messages are caused by a missing call to free_irq() in the mpt/fusion driver.
> I think that it should go here ... but someone with a clue should verify:
> 
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -1384,6 +1384,8 @@ mpt_suspend(struct pci_dev *pdev, pm_mes
>  	/* Clear any lingering interrupt */
>  	CHIPREG_WRITE32(&ioc->chip->IntStatus, 0);
>  
> +	free_irq(ioc->pci_irq, ioc);
> +
>  	pci_disable_device(pdev);
>  	pci_set_power_state(pdev, device_state);
>  

OK, great.  Pavel, can you check this over please?

> But even this doesn't fix the hang during shutdown :-(
> 
> The remaining problem is cause by the order of the calls in sys_reboot:
> 
>                 device_suspend(PMSG_SUSPEND);
>                 device_shutdown();
> 
> The call to device_suspend() shuts down the mpt/fusion driver.  But then
> device_shutdown() calls sd_shutdown() which prints:
> 
>   Synchronizing SCSI cache for disk sdb
> 
> and then calls sd_sync_cache().  Now since we suspended mpt/fusion, this is
> going to go nowhere.
> 
> I don't know how to fix this.  Re-ordering the suspend & shutdown just looks
> wrong.

Again, Pavel has been working on this code and might be able to suggest
something which is appropriate for 2.6.13...

Re: [patch] properly stop devices before poweroff

[Pavel Machek]

Hi!

> > > I started on my OLS homework from Andrew ... and began looking
> > > into what is going on here.
> > > 
> > 
> > Thanks ;) I guess we'll end up with a better kernel, even though you appear
> > to be an innocent victim here.
> 
> The "Badness in iosapic_unregister_intr at arch/ia64/kernel/iosapic.c:851"
> messages are caused by a missing call to free_irq() in the mpt/fusion driver.
> I think that it should go here ... but someone with a clue should verify:
> 
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -1384,6 +1384,8 @@ mpt_suspend(struct pci_dev *pdev, pm_mes
>  	/* Clear any lingering interrupt */
>  	CHIPREG_WRITE32(&ioc->chip->IntStatus, 0);
>  
> +	free_irq(ioc->pci_irq, ioc);
> +
>  	pci_disable_device(pdev);
>  	pci_set_power_state(pdev, device_state);

Looks good, but check with Len Brown. He did relevant ACPI changes.

> But even this doesn't fix the hang during shutdown :-(
> 
> The remaining problem is cause by the order of the calls in sys_reboot:
> 
>                 device_suspend(PMSG_SUSPEND);
>                 device_shutdown();
> 
> The call to device_suspend() shuts down the mpt/fusion driver.  But then
> device_shutdown() calls sd_shutdown() which prints:
> 
>   Synchronizing SCSI cache for disk sdb

Okay, looks to me like sd_shutdown should be done from
device_suspend(), not device_shutdown [because it needs scsi subsystem
to be functional]. Also for reboot we probably want PMSG_FREEZE (not
PMSG_SUSPEND), to keep it slightly faster.

								Pavel
-- 
teflon -- maybe it is a trademark, but it should not be.

RE: [patch] properly stop devices before poweroff

[Luck, Tony]

>> The remaining problem is cause by the order of the calls in sys_reboot:
>> 
>>                 device_suspend(PMSG_SUSPEND);
>>                 device_shutdown();
>> 
>> The call to device_suspend() shuts down the mpt/fusion 
>driver.  But then
>> device_shutdown() calls sd_shutdown() which prints:
>> 
>>   Synchronizing SCSI cache for disk sdb
>
>Okay, looks to me like sd_shutdown should be done from
>device_suspend(), not device_shutdown [because it needs scsi subsystem
>to be functional]. Also for reboot we probably want PMSG_FREEZE (not
>PMSG_SUSPEND), to keep it slightly faster.

Are you going to work on making those changes?

-Tony

Re: [patch] properly stop devices before poweroff

[Kenji Kaneshige]

Hi,

I think the patch Tony mentioned (ia64-halt-hangup-fix.patch)
was already merged into -mm tree. But as Tony said, it doesn't
work and removing reboot notifier of e1000 looks better than
changing ia64 version of pcibios_disable_device().

After all, I think ia64-halt-hangup-fix.patch should be removed
from -mm tree.

Thanks,
Kenji Kaneshige


Luck, Tony wrote:
> I started on my OLS homework from Andrew ... and began looking
> into what is going on here.
> 
> The story so far: Pavel added calls to device_suspend() to three of
> the cases in the sys_reboot() path.  This stopped ia64 from being
> able to shutdown.  There is a oops with a stacktrace pointing back
> at the sys_reboot call.
> 
> Initial analysis from Kenji Kaneshige said that we might fix this
> by adding a patch to the ia64 version of pcibios_disable_device()
> to make it check whether the device was enabled before calling
> acpi_pci_irq_disable().  This might fix things if the issue was
> simply problems with this being called twice.  Pavel sent a
> "Looks good", Adam said "Is it the right fix?"
> 
> I just tried it ... it doesn't work, we still see an oops from
> the sys_reboot() ... so shutdown hangs, and we can sidestep the
> issue of whether this is ideologically correct.
> 
> Then I wondered whether it was just an e1000 problem (since Pavel
> had also commented that we should perhaps removed the reboot notifier
> from the e1000 driver).  So I rebuilt my kernel with e1000 as a module
> and ran "rmmod e1000" before running shutdown.  Which promptly failed
> with a stack trace that ran through mptscsih driver instead of e1000.
> [N.B. Kaneshige-san has also noted that his i386 system which has the
> mptfusion also hangs executing a SYNCHRONIZE_CACHE command]. 
> 
> Code examination in the e1000 case shows that we are a bit confused
> and call first:
> 
> notifier_call_chain(reboot_notifier, ...)
>   e1000_notify_reboot(...)
>     e1000_suspend(...)
>       pci_disable_device(...)
>       pci_set_power_state(...)
> 
> and then:
> 
> device_suspend(...)
>    suspend_device() == dev->bus->suspend == pci_device_suspend
>      e1000_suspend()
>        pci_disable_device()
>        pciset_power_state()
> 
> So it looks like Pavel is right ... registering the reboot notifier is
> bogus in e1000.  Commenting out the register/unregister also allows
> me to get past the e1000 so that I can fail in mptscsih_suspend() path.
> But the fusion driver doesn't register a reboot notifier, so I can't
> try the same trick there :-)
> 
> Andrew: How did you get the squitty font on ia64?  The stack trace
> from the oops flies off the top of the screen.  I've tried a few
> "vga=0x0f07" type options, but I keep getting a big font.
> 
> -Tony
> 

Re: [patch] properly stop devices before poweroff

[Andrew Morton]

Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote:
>
> After all, I think ia64-halt-hangup-fix.patch should be removed
>  from -mm tree.

yup, I already tested-and-dropped it, thanks.

Re: [PATCH] properly stop devices before poweroff

[Olaf Hering]

 On Sun, Jun 26, Linux Kernel Mailing List wrote:

> tree e2de713c76ddb42b091305b88aa7ca4938081789
> parent 5ce47e59c9688d8480ae41100117d8188c191401
> author Pavel Machek <pavel@ucw.cz> Sun, 26 Jun 2005 04:55:11 -0700
> committer Linus Torvalds <torvalds@ppc970.osdl.org> Sun, 26 Jun 2005 06:24:33 -0700
> 
> [PATCH] properly stop devices before poweroff
> 
> Without this patch, Linux provokes emergency disk shutdowns and
> similar nastiness. It was in SuSE kernels for some time, IIRC.
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
>  include/linux/pm.h |   33 +++++++++++++++++++++------------
>  kernel/sys.c       |    3 +++
>  2 files changed, 24 insertions(+), 12 deletions(-)

> +++ b/kernel/sys.c
> @@ -405,6 +405,7 @@ asmlinkage long sys_reboot(int magic1, i
>  	case LINUX_REBOOT_CMD_HALT:
>  		notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
>  		system_state = SYSTEM_HALT;
> +		device_suspend(PMSG_SUSPEND);
>  		device_shutdown();
>  		printk(KERN_EMERG "System halted.\n");
>  		machine_halt();
> @@ -415,6 +416,7 @@ asmlinkage long sys_reboot(int magic1, i
>  	case LINUX_REBOOT_CMD_POWER_OFF:
>  		notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
>  		system_state = SYSTEM_POWER_OFF;
> +		device_suspend(PMSG_SUSPEND);
>  		device_shutdown();
>  		printk(KERN_EMERG "Power down.\n");
>  		machine_power_off();

This change for 'case LINUX_REBOOT_CMD_POWER_OFF' causes an endless hang
after 'halt -p' on my Macs with USB keyboard.
It went into rc1, but the hang in an usb device (1-1.3) shows up only
with rc3. Why is device_suspend() called anyway if the
system will go down anyway in a few milliseconds?