fix suspend/resume irq request free for yenta..

fix suspend/resume irq request free for yenta..

[Dave Airlie]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 265 bytes --]


Without this patch my laptop fails to resume from suspend to RAM...

It applies against a pretty recent 2.6.13-rc3 from git..

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
Linux kernel - DRI, VAX / pam_smb / ILUG

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1875 bytes --]

diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -872,10 +872,20 @@ static irqreturn_t yenta_probe_handler(i
 	return IRQ_NONE;
 }
 
+static int yenta_request_irq(struct yenta_socket *socket)
+{
+	if (request_irq(socket->cb_irq, yenta_probe_handler, SA_SHIRQ, "yenta", socket)) {
+		printk(KERN_WARNING "Yenta: request_irq() in yenta_probe_cb_irq() failed!\n");
+		return -1;
+	}
+	return 0;
+}
+
 /* probes the PCI interrupt, use only on override functions */
 static int yenta_probe_cb_irq(struct yenta_socket *socket)
 {
 	u16 bridge_ctrl;
+	int ret;
 
 	if (!socket->cb_irq)
 		return -1;
@@ -887,10 +897,9 @@ static int yenta_probe_cb_irq(struct yen
 	bridge_ctrl &= ~CB_BRIDGE_INTR;
 	config_writew(socket, CB_BRIDGE_CONTROL, bridge_ctrl);
 
-	if (request_irq(socket->cb_irq, yenta_probe_handler, SA_SHIRQ, "yenta", socket)) {
-		printk(KERN_WARNING "Yenta: request_irq() in yenta_probe_cb_irq() failed!\n");
-		return -1;
-	}
+	ret = yenta_request_irq(socket);
+	if (ret==-1)
+		return ret;
 
 	/* generate interrupt, wait */
 	exca_writeb(socket, I365_CSCINT, I365_CSC_STSCHG);
@@ -1101,6 +1110,7 @@ static int yenta_dev_suspend (struct pci
 		if (socket->type && socket->type->save_state)
 			socket->type->save_state(socket);
 
+		free_irq(dev->irq, socket);
 		/* FIXME: pci_save_state needs to have a better interface */
 		pci_save_state(dev);
 		pci_read_config_dword(dev, 16*4, &socket->saved_state[0]);
@@ -1131,6 +1141,7 @@ static int yenta_dev_resume (struct pci_
 		pci_write_config_dword(dev, 17*4, socket->saved_state[1]);
 		pci_enable_device(dev);
 		pci_set_master(dev);
+		yenta_request_irq(socket);
 
 		if (socket->type && socket->type->restore_state)
 			socket->type->restore_state(socket);

Re: fix suspend/resume irq request free for yenta..

[Dmitry Torokhov]

On Friday 22 July 2005 17:33, Dave Airlie wrote:
> 
> Without this patch my laptop fails to resume from suspend to RAM...
> 
> It applies against a pretty recent 2.6.13-rc3 from git..
> 

Hi,

Is it necessary to do free_irq for suspend? Shouldn't disable_irq
be enough?

-- 
Dmitry

Re: fix suspend/resume irq request free for yenta..

[Pavel Machek]

Hi!

> > Without this patch my laptop fails to resume from suspend to RAM...
> > 
> > It applies against a pretty recent 2.6.13-rc3 from git..
> > 
> 
> Hi,
> 
> Is it necessary to do free_irq for suspend? Shouldn't disable_irq
> be enough?

Due to recent changes in ACPI, yes, it is neccessary.
								Pavel
-- 
teflon -- maybe it is a trademark, but it should not be.

Re: fix suspend/resume irq request free for yenta..

[Russell King]

On Sat, Jul 23, 2005 at 02:29:24AM +0200, Pavel Machek wrote:
> > Is it necessary to do free_irq for suspend? Shouldn't disable_irq
> > be enough?
> 
> Due to recent changes in ACPI, yes, it is neccessary.

What if some other driver is sharing the IRQ, and requires IRQs to be
enabled for the resume to complete?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: fix suspend/resume irq request free for yenta..

[Zwane Mwaikambo]

On Sat, 23 Jul 2005, Russell King wrote:

> On Sat, Jul 23, 2005 at 02:29:24AM +0200, Pavel Machek wrote:
> > > Is it necessary to do free_irq for suspend? Shouldn't disable_irq
> > > be enough?
> > 
> > Due to recent changes in ACPI, yes, it is neccessary.
> 
> What if some other driver is sharing the IRQ, and requires IRQs to be
> enabled for the resume to complete?

This certainly is the case on many laptops.

Re: fix suspend/resume irq request free for yenta..

[Dave Airlie]

.
> >
> > What if some other driver is sharing the IRQ, and requires IRQs to be
> > enabled for the resume to complete?

All drivers re-enable IRQs on their way back up in their resume code,
they shouldn't be doing anything before that point..

> This certainly is the case on many laptops.

Well at the moment on my laptop without this patch we don't make it
back, and we've done a lot of talking at OLS about this.. patches like
this will be popping up for most drivers soon....

Dave.

Re: fix suspend/resume irq request free for yenta..

[Daniel Ritz]

hi

the patch is wrong. yenta_request_irq() registers the wrong handler.
plus yenta_probe_cb_irq() has nothing to do with suspend/resume
(besides it frees the irq in the very same function). correct patch below.

somebody cares to explain me why the free_irq() is necessary before
a suspend?

rgds
-daniel

---------------

[PATCH] yenta: free_irq() on suspend.

Resume doesn't seem to work without.

Signed-off-by: Daniel Ritz <daniel.ritz@gmx.ch>

diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -1107,6 +1107,8 @@ static int yenta_dev_suspend (struct pci
 		pci_read_config_dword(dev, 17*4, &socket->saved_state[1]);
 		pci_disable_device(dev);
 
+		free_irq(dev->irq, socket);
+
 		/*
 		 * Some laptops (IBM T22) do not like us putting the Cardbus
 		 * bridge into D3.  At a guess, some other laptop will
@@ -1132,6 +1134,13 @@ static int yenta_dev_resume (struct pci_
 		pci_enable_device(dev);
 		pci_set_master(dev);
 
+		if (socket->cb_irq)
+			if (request_irq(socket->cb_irq, yenta_interrupt,
+			                SA_SHIRQ, "yenta", socket)) {
+				printk(KERN_WARNING "Yenta: request_irq() failed on resume!\n");
+				socket->cb_irq = 0;
+			}
+
 		if (socket->type && socket->type->restore_state)
 			socket->type->restore_state(socket);
 	}

Re: fix suspend/resume irq request free for yenta..

[Russell King]

On Sun, Jul 24, 2005 at 08:40:00AM +1000, Dave Airlie wrote:
> > > What if some other driver is sharing the IRQ, and requires IRQs to be
> > > enabled for the resume to complete?
> 
> All drivers re-enable IRQs on their way back up in their resume code,
> they shouldn't be doing anything before that point..

I think you missed the point.  If a driver resume method requires
to send some commands to the chip to restore it to the state it was
before it was suspended, and requires interrupts to complete that
operation.

This is quite possible if a device has child devices which will be
resumed after it has been resumed, and they share this interrupt.

This is why I think request_irq/free_irq is a better solution.

Alternatively, we need to go to a two stage resume model - 1st
stage to re-setup the devices such that they are in a quiescent
state, 2nd stage to complete.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

RE: fix suspend/resume irq request free for yenta..

[Brown, Len]

>On Sat, Jul 23, 2005 at 02:29:24AM +0200, Pavel Machek wrote:
>> > Is it necessary to do free_irq for suspend? Shouldn't disable_irq
>> > be enough?
>> 
>> Due to recent changes in ACPI, yes, it is neccessary.
>
>What if some other driver is sharing the IRQ, and requires IRQs to be
>enabled for the resume to complete?

IRQ sharing is an excellent example, not a counter-example,
of why it is necessary to disable devices and free IRQs
on suspend, and acquire them again on resume.

eg. if a device is suspended, but the hardware still causes
an interrupt on a shared IRQ, another device can
suffer a screaming IRQ failure.

Documentation/power/pci.txt has as much as we know about
how to address this -- but I'm certainly open to suggestions
on how to be less invasive to the drivers while having some
chance of being robust.

thanks,
-Len