[PATCH] 2.6.13-rc3-git5: fix Bug #4416 (0/2)

[PATCH] 2.6.13-rc3-git5: fix Bug #4416 (0/2)

[Rafael J. Wysocki]

Hi,

The following two patches are necessary to prevent my box (Asus L5D) from
hanging solid during resume from disk.

The problem is that, apparently, some out-of-order interrupts may be
generated during resume and if some drivers do not call free_irq() on
suspend, these interrupts are mishandled.  For reference please see
http://bugzilla.kernel.org/show_bug.cgi?id=4416
and
http://sourceforge.net/mailarchive/message.php?msg_id=12448907

The first patch adds free_irq() and request_irq() to the suspend and resume
routines, respectively, in the snd_intel8x0 driver.

The second one adds basic suspend/resume support to the sk98lin driver.

Please consider for applying,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

[PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Rafael J. Wysocki]

The following patch adds free_irq() and request_irq() to the suspend and
resume, respectively, routines in the snd_intel8x0 driver.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

--- linux-2.6.13-rc3-git5/sound/pci/intel8x0.c	2005-07-23 19:26:43.000000000 +0200
+++ patched/sound/pci/intel8x0.c	2005-07-25 18:21:36.000000000 +0200
@@ -2373,6 +2373,8 @@ static int intel8x0_suspend(snd_card_t *
 	for (i = 0; i < 3; i++)
 		if (chip->ac97[i])
 			snd_ac97_suspend(chip->ac97[i]);
+	if (chip->irq >= 0)
+		free_irq(chip->irq, (void *)chip);
 	pci_disable_device(chip->pci);
 	return 0;
 }
@@ -2384,7 +2386,14 @@ static int intel8x0_resume(snd_card_t *c
 
 	pci_enable_device(chip->pci);
 	pci_set_master(chip->pci);
-	snd_intel8x0_chip_init(chip, 0);
+	if (request_irq(chip->irq, snd_intel8x0_interrupt, SA_INTERRUPT|SA_SHIRQ, card->shortname, (void *)chip)) {
+		snd_printk("unable to grab IRQ %d\n", chip->irq);
+		chip->irq = -1;
+		pci_disable_device(chip->pci);
+		return -EBUSY;
+	}
+	synchronize_irq(chip->irq);
+	snd_intel8x0_chip_init(chip, 1);
 
 	/* refill nocache */
 	if (chip->fix_nocache)

Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Takashi Iwai]

At Tue, 26 Jul 2005 12:51:47 +0200,
Rafael J. Wysocki wrote:
> 
> The following patch adds free_irq() and request_irq() to the suspend and
> resume, respectively, routines in the snd_intel8x0 driver.

The patch looks OK to me although I have some concerns.

- The error in resume can't be handled properly.

  What should we do for the error of request_irq()?

- Adding this to all drivers seem too much.

  We just need to stop the irq processing until resume, so something
  like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
  more uesful?
  

Takashi

Re: [ACPI] Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Pavel Machek]

Hi!

> > The following patch adds free_irq() and request_irq() to the suspend and
> > resume, respectively, routines in the snd_intel8x0 driver.
> 
> The patch looks OK to me although I have some concerns.
> 
> - The error in resume can't be handled properly.
> 
>   What should we do for the error of request_irq()?
> 
> - Adding this to all drivers seem too much.

There's probably no other way. Talk to Len Brown.

>   We just need to stop the irq processing until resume, so something
>   like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
>   more uesful?

Its more complex than that. Irq numbers may change during resume.
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [ACPI] Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Takashi Iwai]

At Wed, 27 Jul 2005 22:52:49 +0200,
Pavel Machek wrote:
> 
> Hi!
> 
> > > The following patch adds free_irq() and request_irq() to the suspend and
> > > resume, respectively, routines in the snd_intel8x0 driver.
> > 
> > The patch looks OK to me although I have some concerns.
> > 
> > - The error in resume can't be handled properly.
> > 
> >   What should we do for the error of request_irq()?
> > 
> > - Adding this to all drivers seem too much.
> 
> There's probably no other way. Talk to Len Brown.
> 
> >   We just need to stop the irq processing until resume, so something
> >   like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
> >   more uesful?
> 
> Its more complex than that. Irq numbers may change during resume.

Hmm, then the patch looks wrong.  It assumes that the irq number is
as same as before suspend.

Sorry for ignorance, but, is only irq affected?  Are the other
resources like ioport consistent after resume?


Takashi

Re: [ACPI] Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Pavel Machek]

Hi!

> > > > The following patch adds free_irq() and request_irq() to the suspend and
> > > > resume, respectively, routines in the snd_intel8x0 driver.
> > > 
> > > The patch looks OK to me although I have some concerns.
> > > 
> > > - The error in resume can't be handled properly.
> > > 
> > >   What should we do for the error of request_irq()?
> > > 
> > > - Adding this to all drivers seem too much.
> > 
> > There's probably no other way. Talk to Len Brown.
> > 
> > >   We just need to stop the irq processing until resume, so something
> > >   like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
> > >   more uesful?
> > 
> > Its more complex than that. Irq numbers may change during resume.
> 
> Hmm, then the patch looks wrong.  It assumes that the irq number is
> as same as before suspend.

> Sorry for ignorance, but, is only irq affected?  Are the other
> resources like ioport consistent after resume?

I think it is only irq, but Len, please take correct me if I'm
wrong. Irq number can change during suspend/resume cycle, right?

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

Re: [ACPI] Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Shaohua Li]

On Thu, 2005-07-28 at 10:06 +0200, Takashi Iwai wrote:
> At Wed, 27 Jul 2005 22:52:49 +0200,
> Pavel Machek wrote:
> > 
> > Hi!
> > 
> > > > The following patch adds free_irq() and request_irq() to the suspend and
> > > > resume, respectively, routines in the snd_intel8x0 driver.
> > > 
> > > The patch looks OK to me although I have some concerns.
> > > 
> > > - The error in resume can't be handled properly.
> > > 
> > >   What should we do for the error of request_irq()?
> > > 
> > > - Adding this to all drivers seem too much.
> > 
> > There's probably no other way. Talk to Len Brown.
> > 
> > >   We just need to stop the irq processing until resume, so something
> > >   like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
> > >   more uesful?
> > 
> > Its more complex than that. Irq numbers may change during resume.
> 
> Hmm, then the patch looks wrong.  It assumes that the irq number is
> as same as before suspend.
> 
> Sorry for ignorance, but, is only irq affected?  Are the other
> resources like ioport consistent after resume?
Resource changes might be helpful for resource balance (device hotplug).
But suspend/resume doesn't use it. So resources are consistent after
resume except irq to me.

Thanks,
Shaohua

Re: [ACPI] Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Rafael J. Wysocki]

Hi,

On Thursday, 28 of July 2005 10:06, Takashi Iwai wrote:
> At Wed, 27 Jul 2005 22:52:49 +0200,
> Pavel Machek wrote:
> > 
> > Hi!
> > 
> > > > The following patch adds free_irq() and request_irq() to the suspend and
> > > > resume, respectively, routines in the snd_intel8x0 driver.
> > > 
> > > The patch looks OK to me although I have some concerns.
> > > 
> > > - The error in resume can't be handled properly.
> > > 
> > >   What should we do for the error of request_irq()?
> > > 
> > > - Adding this to all drivers seem too much.
> > 
> > There's probably no other way. Talk to Len Brown.
> > 
> > >   We just need to stop the irq processing until resume, so something
> > >   like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
> > >   more uesful?
> > 
> > Its more complex than that. Irq numbers may change during resume.
> 
> Hmm, then the patch looks wrong.  It assumes that the irq number is
> as same as before suspend.

Well, that''s the theory, but frankly I don't see a practical reason.  I have never
seen this happening.  Practically, for this to happen, you'll have to reconfigure
the BIOS accross suspend/resume which is dangerous anyway.

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [ACPI] Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Shaohua Li]

On Thu, 2005-07-28 at 10:43 +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> On Thursday, 28 of July 2005 10:06, Takashi Iwai wrote:
> > At Wed, 27 Jul 2005 22:52:49 +0200,
> > Pavel Machek wrote:
> > > 
> > > Hi!
> > > 
> > > > > The following patch adds free_irq() and request_irq() to the suspend and
> > > > > resume, respectively, routines in the snd_intel8x0 driver.
> > > > 
> > > > The patch looks OK to me although I have some concerns.
> > > > 
> > > > - The error in resume can't be handled properly.
> > > > 
> > > >   What should we do for the error of request_irq()?
> > > > 
> > > > - Adding this to all drivers seem too much.
> > > 
> > > There's probably no other way. Talk to Len Brown.
> > > 
> > > >   We just need to stop the irq processing until resume, so something
> > > >   like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
> > > >   more uesful?
> > > 
> > > Its more complex than that. Irq numbers may change during resume.
> > 
> > Hmm, then the patch looks wrong.  It assumes that the irq number is
> > as same as before suspend.
> 
> Well, that''s the theory, but frankly I don't see a practical reason.  I have never
> seen this happening.  Practically, for this to happen, you'll have to reconfigure
> the BIOS accross suspend/resume which is dangerous anyway.
>From my understanding, we should not have such assumption. Say all
device drivers with the same ioapic pin have freed irq. We might mask
the ioapic pin and even free the vector. Then after you request_irq
again, the interrupt might get different vector. If you enable MSI, then
it's broken.
In IA64, we already have such staff (mask ioapic pin and free vector).

Thanks,
Shaohua

Re: [ACPI] Re: [Alsa-devel] [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (1/2)

[Rafael J. Wysocki]

On Thursday, 28 of July 2005 10:48, Shaohua Li wrote:
> On Thu, 2005-07-28 at 10:43 +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Thursday, 28 of July 2005 10:06, Takashi Iwai wrote:
> > > At Wed, 27 Jul 2005 22:52:49 +0200,
> > > Pavel Machek wrote:
> > > > 
> > > > Hi!
> > > > 
> > > > > > The following patch adds free_irq() and request_irq() to the suspend and
> > > > > > resume, respectively, routines in the snd_intel8x0 driver.
> > > > > 
> > > > > The patch looks OK to me although I have some concerns.
> > > > > 
> > > > > - The error in resume can't be handled properly.
> > > > > 
> > > > >   What should we do for the error of request_irq()?
> > > > > 
> > > > > - Adding this to all drivers seem too much.
> > > > 
> > > > There's probably no other way. Talk to Len Brown.
> > > > 
> > > > >   We just need to stop the irq processing until resume, so something
> > > > >   like suspend_irq(irq, dev_id) and resume_irq(irq, dev_id) would be
> > > > >   more uesful?
> > > > 
> > > > Its more complex than that. Irq numbers may change during resume.
> > > 
> > > Hmm, then the patch looks wrong.  It assumes that the irq number is
> > > as same as before suspend.
> > 
> > Well, that''s the theory, but frankly I don't see a practical reason.  I have never
> > seen this happening.  Practically, for this to happen, you'll have to reconfigure
> > the BIOS accross suspend/resume which is dangerous anyway.
> From my understanding, we should not have such assumption. Say all
> device drivers with the same ioapic pin have freed irq. We might mask
> the ioapic pin and even free the vector. Then after you request_irq
> again, the interrupt might get different vector. If you enable MSI, then
> it's broken.
> In IA64, we already have such staff (mask ioapic pin and free vector).

All right then.  Still the patch is urgently needed for some systems that do
not have MSI and IMO the detection of the IRQ change can be implemented
on top of it.

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (2/2)

[Rafael J. Wysocki]

The following patch adds basic suspend/resume support to the sk98lin
network driver.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

--- linux-2.6.13-rc3-git5/drivers/net/sk98lin/skge.c	2005-07-23 19:26:29.000000000 +0200
+++ patched/drivers/net/sk98lin/skge.c	2005-07-25 18:17:57.000000000 +0200
@@ -5133,6 +5133,75 @@ static void __devexit skge_remove_one(st
 	kfree(pAC);
 }
 
+#ifdef CONFIG_PM
+static int skge_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+	DEV_NET *pNet = netdev_priv(dev);
+	SK_AC *pAC = pNet->pAC;
+	struct net_device *otherdev = pAC->dev[1];
+
+	if (pNet->Up) {
+		pAC->WasIfUp[0] = SK_TRUE;
+		DoPrintInterfaceChange = SK_FALSE;
+		SkDrvDeInitAdapter(pAC, 0);  /* performs SkGeClose */
+	}
+	if (otherdev != dev) {
+		pNet = netdev_priv(otherdev);
+		if (pNet->Up) {
+			pAC->WasIfUp[1] = SK_TRUE;
+			DoPrintInterfaceChange = SK_FALSE;
+			SkDrvDeInitAdapter(pAC, 1);  /* performs SkGeClose */
+		}
+	}
+
+	pci_save_state(pdev);
+	pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
+	if (pAC->AllocFlag & SK_ALLOC_IRQ) {
+		free_irq(dev->irq, dev);
+	}
+	pci_disable_device(pdev);
+	pci_set_power_state(pdev, pci_choose_state(pdev, state));
+
+	return 0;
+}
+
+static int skge_resume(struct pci_dev *pdev)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+	DEV_NET *pNet = netdev_priv(dev);
+	SK_AC *pAC = pNet->pAC;
+	int ret;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	pci_enable_device(pdev);
+	pci_set_master(pdev);
+	if (pAC->GIni.GIMacsFound == 2)
+		ret = request_irq(dev->irq, SkGeIsr, SA_SHIRQ, pAC->Name, dev);
+	else
+		ret = request_irq(dev->irq, SkGeIsrOnePort, SA_SHIRQ, pAC->Name, dev);
+	if (ret) {
+		printk(KERN_WARNING "sk98lin: unable to acquire IRQ %d\n", dev->irq);
+		pAC->AllocFlag &= ~SK_ALLOC_IRQ;
+		dev->irq = 0;
+		pci_disable_device(pdev);
+		return -EBUSY;
+	}
+
+	if (pAC->WasIfUp[0] == SK_TRUE) {
+		DoPrintInterfaceChange = SK_FALSE;
+		SkDrvInitAdapter(pAC, 0);    /* first device  */
+	}
+	if (pAC->dev[1] != dev && pAC->WasIfUp[1] == SK_TRUE) {
+		DoPrintInterfaceChange = SK_FALSE;
+		SkDrvInitAdapter(pAC, 1);    /* second device  */
+	}
+
+	return 0;
+}
+#endif
+
 static struct pci_device_id skge_pci_tbl[] = {
 	{ PCI_VENDOR_ID_3COM, 0x1700, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
 	{ PCI_VENDOR_ID_3COM, 0x80eb, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
@@ -5158,6 +5227,8 @@ static struct pci_driver skge_driver = {
 	.id_table	= skge_pci_tbl,
 	.probe		= skge_probe_one,
 	.remove		= __devexit_p(skge_remove_one),
+	.suspend	= skge_suspend,
+	.resume		= skge_resume,
 };
 
 static int __init skge_init(void)

Re: [ACPI] Re: [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (2/2)

[Carl-Daniel Hailfinger]

Rafael J. Wysocki schrieb:
> The following patch adds basic suspend/resume support to the sk98lin
> network driver.
[snipped]

The current in-kernel sk98lin driver is years behind the version
downloadable from Syskonnect. Maybe it would make sense to update
it first before applying any new patches.
http://www.syskonnect.com/support/driver/d0102_driver.html

Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/

Re: [ACPI] Re: [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (2/2)

[Rafael J. Wysocki]

On Tuesday, 26 of July 2005 14:25, Carl-Daniel Hailfinger wrote:
> Rafael J. Wysocki schrieb:
> > The following patch adds basic suspend/resume support to the sk98lin
> > network driver.
> [snipped]
> 
> The current in-kernel sk98lin driver is years behind the version
> downloadable from Syskonnect. Maybe it would make sense to update
> it first before applying any new patches.
> http://www.syskonnect.com/support/driver/d0102_driver.html

You are right, but I don't know who should do this.  I have only submitted
the patch to eliminate a problem with the current kernel.

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [ACPI] Re: [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (2/2)

[Peter Buckingham]

Rafael J. Wysocki wrote:
> On Tuesday, 26 of July 2005 14:25, Carl-Daniel Hailfinger wrote:
>>The current in-kernel sk98lin driver is years behind the version
>>downloadable from Syskonnect. Maybe it would make sense to update
>>it first before applying any new patches.
>>http://www.syskonnect.com/support/driver/d0102_driver.html
> 
> 
> You are right, but I don't know who should do this.  I have only submitted
> the patch to eliminate a problem with the current kernel.

have a look at the skge driver, this is a cleaned up version of the 
sk98lin. Although it doesn't support all of the devices, ie ones based 
on the Yukon 2.

peter

Re: [ACPI] Re: [PATCH] 2.6.13-rc3-git5: fix Bug #4416 (2/2)

[Rafael J. Wysocki]

On Tuesday, 26 of July 2005 23:11, Peter Buckingham wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday, 26 of July 2005 14:25, Carl-Daniel Hailfinger wrote:
> >>The current in-kernel sk98lin driver is years behind the version
> >>downloadable from Syskonnect. Maybe it would make sense to update
> >>it first before applying any new patches.
> >>http://www.syskonnect.com/support/driver/d0102_driver.html
> > 
> > 
> > You are right, but I don't know who should do this.  I have only submitted
> > the patch to eliminate a problem with the current kernel.
> 
> have a look at the skge driver, this is a cleaned up version of the 
> sk98lin. Although it doesn't support all of the devices, ie ones based 
> on the Yukon 2.

Thanks for the hint, I will.  From the first look it's missing the
free_irq()/request_irq() on suspend/resume however.

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"