[patch 0/2] PNPACPI: fix IRQ encoding problems

[patch 0/2] PNPACPI: fix IRQ encoding problems

[Bjorn Helgaas]

These patches fix some IRQ encoding problems in PNPACPI.  I
don't have reports of people hitting these specific problems,
but they could easily cause hard-to-debug suspend/resume failures
or failures when enabling devices left disabled by the BIOS.

I think these changes should go into 2.6.26.

Bjorn
-- 

[patch 1/2] PNPACPI: fix IRQ flag decoding

[Bjorn Helgaas]

[-- Attachment #1: pnpacpi-fix-decode-irq --]
[-- Type: text/plain, Size: 2644 bytes --]

When decoding IRQ trigger mode and polarity, it is not enough
to mask by IORESOURCE_BITS because there are now additional
bits defined.  For example, if IORESOURCE_IRQ_SHAREABLE
was set, we failed to set *triggering and *polarity at all.

I can't point to a failure that this patch fixes, but
bugs in this area have caused problems when resuming after
suspend, for example:
    http://bugzilla.kernel.org/show_bug.cgi?id=6316
    http://bugzilla.kernel.org/show_bug.cgi?id=9487
    https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/152187

This is based on a patch by Tom Jaeger <ThJaeger@gmail.com>:
    http://bugzilla.kernel.org/show_bug.cgi?id=9487#c32

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work11/drivers/pnp/pnpacpi/rsparser.c
===================================================================
--- work11.orig/drivers/pnp/pnpacpi/rsparser.c	2008-05-27 15:50:04.000000000 -0600
+++ work11/drivers/pnp/pnpacpi/rsparser.c	2008-05-27 15:53:21.000000000 -0600
@@ -56,9 +56,11 @@ static int irq_flags(int triggering, int
 	return flags;
 }
 
-static void decode_irq_flags(int flag, int *triggering, int *polarity)
+static void decode_irq_flags(struct pnp_dev *dev, int flags, int *triggering,
+			     int *polarity)
 {
-	switch (flag) {
+	switch (flags & (IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ_HIGHLEVEL |
+			 IORESOURCE_IRQ_LOWEDGE  | IORESOURCE_IRQ_HIGHEDGE)) {
 	case IORESOURCE_IRQ_LOWLEVEL:
 		*triggering = ACPI_LEVEL_SENSITIVE;
 		*polarity = ACPI_ACTIVE_LOW;
@@ -75,6 +77,12 @@ static void decode_irq_flags(int flag, i
 		*triggering = ACPI_EDGE_SENSITIVE;
 		*polarity = ACPI_ACTIVE_HIGH;
 		break;
+	default:
+		dev_err(&dev->dev, "can't encode invalid IRQ mode %#x\n",
+			flags);
+		*triggering = ACPI_EDGE_SENSITIVE;
+		*polarity = ACPI_ACTIVE_HIGH;
+		break;
 	}
 }
 
@@ -793,7 +801,7 @@ static void pnpacpi_encode_irq(struct pn
 	struct acpi_resource_irq *irq = &resource->data.irq;
 	int triggering, polarity;
 
-	decode_irq_flags(p->flags & IORESOURCE_BITS, &triggering, &polarity);
+	decode_irq_flags(dev, p->flags, &triggering, &polarity);
 	irq->triggering = triggering;
 	irq->polarity = polarity;
 	if (triggering == ACPI_EDGE_SENSITIVE)
@@ -818,7 +826,7 @@ static void pnpacpi_encode_ext_irq(struc
 	struct acpi_resource_extended_irq *extended_irq = &resource->data.extended_irq;
 	int triggering, polarity;
 
-	decode_irq_flags(p->flags & IORESOURCE_BITS, &triggering, &polarity);
+	decode_irq_flags(dev, p->flags, &triggering, &polarity);
 	extended_irq->producer_consumer = ACPI_CONSUMER;
 	extended_irq->triggering = triggering;
 	extended_irq->polarity = polarity;

-- 

[patch 2/2] PNPACPI: fix shareable IRQ encode/decode

[Bjorn Helgaas]

[-- Attachment #1: pnpacpi-encode-irq --]
[-- Type: text/plain, Size: 2742 bytes --]

When we encode IRQ resources, we should use the "shareable"
flag we got from _PRS rather than guessing based on the
IRQ trigger mode.

This is based on a patch by Tom Jaeger <ThJaeger@gmail.com>:
    http://bugzilla.kernel.org/show_bug.cgi?id=9487#c32

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work11/drivers/pnp/pnpacpi/rsparser.c
===================================================================
--- work11.orig/drivers/pnp/pnpacpi/rsparser.c	2008-05-27 15:53:21.000000000 -0600
+++ work11/drivers/pnp/pnpacpi/rsparser.c	2008-05-27 15:58:29.000000000 -0600
@@ -50,14 +50,14 @@ static int irq_flags(int triggering, int
 			flags = IORESOURCE_IRQ_HIGHEDGE;
 	}
 
-	if (shareable)
+	if (shareable == ACPI_SHARED)
 		flags |= IORESOURCE_IRQ_SHAREABLE;
 
 	return flags;
 }
 
 static void decode_irq_flags(struct pnp_dev *dev, int flags, int *triggering,
-			     int *polarity)
+			     int *polarity, int *shareable)
 {
 	switch (flags & (IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ_HIGHLEVEL |
 			 IORESOURCE_IRQ_LOWEDGE  | IORESOURCE_IRQ_HIGHEDGE)) {
@@ -84,6 +84,11 @@ static void decode_irq_flags(struct pnp_
 		*polarity = ACPI_ACTIVE_HIGH;
 		break;
 	}
+
+	if (flags & IORESOURCE_IRQ_SHAREABLE)
+		*shareable = ACPI_SHARED;
+	else
+		*shareable = ACPI_EXCLUSIVE;
 }
 
 static void pnpacpi_parse_allocated_irqresource(struct pnp_dev *dev,
@@ -799,15 +804,12 @@ static void pnpacpi_encode_irq(struct pn
 			       struct resource *p)
 {
 	struct acpi_resource_irq *irq = &resource->data.irq;
-	int triggering, polarity;
+	int triggering, polarity, shareable;
 
-	decode_irq_flags(dev, p->flags, &triggering, &polarity);
+	decode_irq_flags(dev, p->flags, &triggering, &polarity, &shareable);
 	irq->triggering = triggering;
 	irq->polarity = polarity;
-	if (triggering == ACPI_EDGE_SENSITIVE)
-		irq->sharable = ACPI_EXCLUSIVE;
-	else
-		irq->sharable = ACPI_SHARED;
+	irq->sharable = shareable;
 	irq->interrupt_count = 1;
 	irq->interrupts[0] = p->start;
 
@@ -824,16 +826,13 @@ static void pnpacpi_encode_ext_irq(struc
 				   struct resource *p)
 {
 	struct acpi_resource_extended_irq *extended_irq = &resource->data.extended_irq;
-	int triggering, polarity;
+	int triggering, polarity, shareable;
 
-	decode_irq_flags(dev, p->flags, &triggering, &polarity);
+	decode_irq_flags(dev, p->flags, &triggering, &polarity, &shareable);
 	extended_irq->producer_consumer = ACPI_CONSUMER;
 	extended_irq->triggering = triggering;
 	extended_irq->polarity = polarity;
-	if (triggering == ACPI_EDGE_SENSITIVE)
-		extended_irq->sharable = ACPI_EXCLUSIVE;
-	else
-		extended_irq->sharable = ACPI_SHARED;
+	extended_irq->sharable = shareable;
 	extended_irq->interrupt_count = 1;
 	extended_irq->interrupts[0] = p->start;
 

-- 

Re: [patch 1/2] PNPACPI: fix IRQ flag decoding

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On 28-05-08 00:49, Bjorn Helgaas wrote:

> When decoding IRQ trigger mode and polarity, it is not enough to mask
> by IORESOURCE_BITS because there are now additional bits defined.
> For example, if IORESOURCE_IRQ_SHAREABLE was set, we failed to set
> *triggering and *polarity at all.

Ack. This would also imply the attached is a comment fix...

Rene.


[-- Attachment #2: pnp-non-isa-specific-ioresource_bits.diff --]
[-- Type: text/plain, Size: 1285 bytes --]

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index d5d40a9..c6801bf 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -53,14 +53,14 @@ struct resource_list {
 #define IORESOURCE_AUTO		0x40000000
 #define IORESOURCE_BUSY		0x80000000	/* Driver has marked this resource busy */
 
-/* ISA PnP IRQ specific bits (IORESOURCE_BITS) */
+/* PnP IRQ specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_IRQ_HIGHEDGE		(1<<0)
 #define IORESOURCE_IRQ_LOWEDGE		(1<<1)
 #define IORESOURCE_IRQ_HIGHLEVEL	(1<<2)
 #define IORESOURCE_IRQ_LOWLEVEL		(1<<3)
 #define IORESOURCE_IRQ_SHAREABLE	(1<<4)
 
-/* ISA PnP DMA specific bits (IORESOURCE_BITS) */
+/* PnP DMA specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_DMA_TYPE_MASK	(3<<0)
 #define IORESOURCE_DMA_8BIT		(0<<0)
 #define IORESOURCE_DMA_8AND16BIT	(1<<0)
@@ -76,7 +76,7 @@ struct resource_list {
 #define IORESOURCE_DMA_TYPEB		(2<<6)
 #define IORESOURCE_DMA_TYPEF		(3<<6)
 
-/* ISA PnP memory I/O specific bits (IORESOURCE_BITS) */
+/* PnP memory I/O specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_MEM_WRITEABLE	(1<<0)	/* dup: IORESOURCE_READONLY */
 #define IORESOURCE_MEM_CACHEABLE	(1<<1)	/* dup: IORESOURCE_CACHEABLE */
 #define IORESOURCE_MEM_RANGELENGTH	(1<<2)	/* dup: IORESOURCE_RANGELENGTH */

Re: [patch 2/2] PNPACPI: fix shareable IRQ encode/decode

[Rene Herman]

On 28-05-08 00:49, Bjorn Helgaas wrote:

> When we encode IRQ resources, we should use the "shareable"
> flag we got from _PRS rather than guessing based on the
> IRQ trigger mode.
> 
> This is based on a patch by Tom Jaeger <ThJaeger@gmail.com>:
>     http://bugzilla.kernel.org/show_bug.cgi?id=9487#c32
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Not-commented-on-by: Rene Herman <rene.herman@gmail.com>

Makes sense patchwise but one would expect that it wasn't that way to 
start with due to perhaps some/many/most BIOSen not encoding the flag 
correctly in _PRS. Don't know ACPI though...

Rene.

Re: [patch 2/2] PNPACPI: fix shareable IRQ encode/decode

[Bjorn Helgaas]

On Wednesday 28 May 2008 02:58:47 pm Rene Herman wrote:
> On 28-05-08 00:49, Bjorn Helgaas wrote:
> 
> > When we encode IRQ resources, we should use the "shareable"
> > flag we got from _PRS rather than guessing based on the
> > IRQ trigger mode.
> > 
> > This is based on a patch by Tom Jaeger <ThJaeger@gmail.com>:
> >     http://bugzilla.kernel.org/show_bug.cgi?id=9487#c32
> > 
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Not-commented-on-by: Rene Herman <rene.herman@gmail.com>
> 
> Makes sense patchwise but one would expect that it wasn't that way to 
> start with due to perhaps some/many/most BIOSen not encoding the flag 
> correctly in _PRS. Don't know ACPI though...

I added IORESOURCE_IRQ_SHAREABLE recently:
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c32928c579d88acd43981b59e86900da65f40762

and I just didn't notice these places at the time.  If I had,
I would have made this change then.  It's possible we could
trip over a BIOS issue, but I don't *think* that's why it's
been this way in the past.

Bjorn

Re: [patch 2/2] PNPACPI: fix shareable IRQ encode/decode

[Rene Herman]

On 29-05-08 00:05, Bjorn Helgaas wrote:

> On Wednesday 28 May 2008 02:58:47 pm Rene Herman wrote:
>> On 28-05-08 00:49, Bjorn Helgaas wrote:
>>
>>> When we encode IRQ resources, we should use the "shareable"
>>> flag we got from _PRS rather than guessing based on the
>>> IRQ trigger mode.
>>>
>>> This is based on a patch by Tom Jaeger <ThJaeger@gmail.com>:
>>>     http://bugzilla.kernel.org/show_bug.cgi?id=9487#c32
>>>
>>> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
>> Not-commented-on-by: Rene Herman <rene.herman@gmail.com>
>>
>> Makes sense patchwise but one would expect that it wasn't that way to 
>> start with due to perhaps some/many/most BIOSen not encoding the flag 
>> correctly in _PRS. Don't know ACPI though...
> 
> I added IORESOURCE_IRQ_SHAREABLE recently:
>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c32928c579d88acd43981b59e86900da65f40762
> 
> and I just didn't notice these places at the time.  If I had,
> I would have made this change then.  It's possible we could
> trip over a BIOS issue, but I don't *think* that's why it's
> been this way in the past.

Ah, I see. Was likely just a matter of originally not having a place to 
stash it then (by the way, time flies; "recently" is almost two years 
ago...)

Rene.

Re: [patch 2/2] PNPACPI: fix shareable IRQ encode/decode

[Bjorn Helgaas]

On Wednesday 28 May 2008 05:14:49 pm Rene Herman wrote:
> On 29-05-08 00:05, Bjorn Helgaas wrote:
> 
> > On Wednesday 28 May 2008 02:58:47 pm Rene Herman wrote:
> >> On 28-05-08 00:49, Bjorn Helgaas wrote:
> >>
> >>> When we encode IRQ resources, we should use the "shareable"
> >>> flag we got from _PRS rather than guessing based on the
> >>> IRQ trigger mode.
> >>>
> >>> This is based on a patch by Tom Jaeger <ThJaeger@gmail.com>:
> >>>     http://bugzilla.kernel.org/show_bug.cgi?id=9487#c32
> >>>
> >>> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> >> Not-commented-on-by: Rene Herman <rene.herman@gmail.com>
> >>
> >> Makes sense patchwise but one would expect that it wasn't that way to 
> >> start with due to perhaps some/many/most BIOSen not encoding the flag 
> >> correctly in _PRS. Don't know ACPI though...
> > 
> > I added IORESOURCE_IRQ_SHAREABLE recently:
> >   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c32928c579d88acd43981b59e86900da65f40762
> > 
> > and I just didn't notice these places at the time.  If I had,
> > I would have made this change then.  It's possible we could
> > trip over a BIOS issue, but I don't *think* that's why it's
> > been this way in the past.
> 
> Ah, I see. Was likely just a matter of originally not having a place to 
> stash it then (by the way, time flies; "recently" is almost two years 
> ago...)

Yep.  Time does fly when you're having fun :-)