RE: [patch] properly stop devices before poweroff

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

[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

[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( ttlQ timeD8769.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

[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]

>> 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