[PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[David Howells]

Fix the test pnpacpi_parse_irq_option() makes against PNP_IRQ_NR by sticking
p->interrupt[i] into an unsigned int and then using it in the three places
that want it.  This gets rid of the warning:

drivers/pnp/pnpacpi/rsparser.c:500: warning: comparison is always true due to limited range of data type

Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/pnp/pnpacpi/rsparser.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)


diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
index d2abc87..b0d89eb 100644
--- a/drivers/pnp/pnpacpi/rsparser.c
+++ b/drivers/pnp/pnpacpi/rsparser.c
@@ -497,12 +497,13 @@ static __init void pnpacpi_parse_irq_option(struct pnp_dev *dev,
 	bitmap_zero(map.bits, PNP_IRQ_NR);
 	for (i = 0; i < p->interrupt_count; i++) {
 		if (p->interrupts[i]) {
-			if (p->interrupts[i] < PNP_IRQ_NR)
-				__set_bit(p->interrupts[i], map.bits);
+			unsigned irq = p->interrupts[i];
+			if (irq  < PNP_IRQ_NR)
+				__set_bit(irq, map.bits);
 			else
 				dev_err(&dev->dev, "ignoring IRQ %d option "
 					"(too large for %d entry bitmap)\n",
-					p->interrupts[i], PNP_IRQ_NR);
+					irq, PNP_IRQ_NR);
 		}
 	}
 

Re: [PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[Rene Herman]

On 27-06-08 14:33, David Howells wrote:

> Fix the test pnpacpi_parse_irq_option() makes against PNP_IRQ_NR by sticking
> p->interrupt[i] into an unsigned int and then using it in the three places
> that want it.

Pedantically, a simple unadorned int would be better it seems. The 
#define is an int, __set_bit(_) takes an int and an int is printed.

> This gets rid of the warning:
> 
> drivers/pnp/pnpacpi/rsparser.c:500: warning: comparison is always true due to limited range of data type
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  drivers/pnp/pnpacpi/rsparser.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
> index d2abc87..b0d89eb 100644
> --- a/drivers/pnp/pnpacpi/rsparser.c
> +++ b/drivers/pnp/pnpacpi/rsparser.c
> @@ -497,12 +497,13 @@ static __init void pnpacpi_parse_irq_option(struct pnp_dev *dev,
>  	bitmap_zero(map.bits, PNP_IRQ_NR);
>  	for (i = 0; i < p->interrupt_count; i++) {
>  		if (p->interrupts[i]) {
> -			if (p->interrupts[i] < PNP_IRQ_NR)
> -				__set_bit(p->interrupts[i], map.bits);
> +			unsigned irq = p->interrupts[i];
> +			if (irq  < PNP_IRQ_NR)

Hyper-pedantically, this adds one space too many :-)

> +				__set_bit(irq, map.bits);
>  			else
>  				dev_err(&dev->dev, "ignoring IRQ %d option "
>  					"(too large for %d entry bitmap)\n",
> -					p->interrupts[i], PNP_IRQ_NR);
> +					irq, PNP_IRQ_NR);
>  		}
>  	}
>  

Rene.

Re: [PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[David Howells]

Rene Herman <rene.herman@keyaccess.nl> wrote:

> Pedantically, a simple unadorned int would be better it seems. The #define is
> an int, __set_bit(_) takes an int and an int is printed.

Yes, but the test doesn't check for -ve values.  If it's unsigned, it doesn't
need to.

> Hyper-pedantically, this adds one space too many :-)

Bah.

David

Re: [PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[Rene Herman]

On 27-06-08 15:16, David Howells wrote:

> Rene Herman <rene.herman@keyaccess.nl> wrote:
> 
>> Pedantically, a simple unadorned int would be better it seems. The #define is
>> an int, __set_bit(_) takes an int and an int is printed.
> 
> Yes, but the test doesn't check for -ve values.  If it's unsigned, it doesn't
> need to.

Well, it's been promoted from a u8, so no need for that anyway, but <shrug>.

Rene.

Re: [PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[David Howells]

Rene Herman <rene.herman@keyaccess.nl> wrote:

> Well, it's been promoted from a u8, so no need for that anyway, but <shrug>.

My logic is that in commit 95b24192cf27631dc11541e97c430389320e7a93 it says
the following:

	ACPI Extended Interrupt Descriptors can encode 32-bit interrupt
	numbers, so an interrupt number may exceed the size of the bitmap
	we use to track possible IRQ settings.
 
so the field in 'struct acpi_resource_irq' might at some point increase to be
a 32-bit unsigned value.  Otherwise there's no point having the check at all,
right?

David

Re: [PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[Bjorn Helgaas]

On Friday 27 June 2008 08:54:04 am David Howells wrote:
> Rene Herman <rene.herman@keyaccess.nl> wrote:
> 
> > Well, it's been promoted from a u8, so no need for that anyway, but <shrug>.
> 
> My logic is that in commit 95b24192cf27631dc11541e97c430389320e7a93 it says
> the following:
> 
> 	ACPI Extended Interrupt Descriptors can encode 32-bit interrupt
> 	numbers, so an interrupt number may exceed the size of the bitmap
> 	we use to track possible IRQ settings.
>  
> so the field in 'struct acpi_resource_irq' might at some point increase to be
> a 32-bit unsigned value.  Otherwise there's no point having the check at all,
> right?

There are two flavors of ACPI IRQ descriptors, which the CA puts
into acpi_resource_irq and acpi_resource_extended_irq.

struct acpi_resource_irq will never have an IRQ number larger than
255.  PNP_IRQ_NR is currently 256, so we don't really have a problem
there.

struct acpi_resource_extended_irq has a u32 IRQ value, and that's
where I'm worried.  We already have plenty of devices with GSIs in
the 2000 range.  In most cases, I think these are PCI devices, so
these GSIs appear in _PRT (PCI routing tables) rather than in _PRS.
But there is the possibility that we could see a configurable ACPI
device with a large GSI, and that's why I added the check.

I think what happened here is that I meant to put this test in
pnpacpi_parse_ext_irq_option(), but somehow it got into
pnpacpi_parse_irq_option() instead.  I'll try to sort this out
today.

Bjorn

Re: [PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[Rene Herman]

On 27-06-08 16:54, David Howells wrote:

> Rene Herman <rene.herman@keyaccess.nl> wrote:
> 
>> Well, it's been promoted from a u8, so no need for that anyway, but <shrug>.
> 
> My logic is that in commit 95b24192cf27631dc11541e97c430389320e7a93 it says
> the following:
> 
> 	ACPI Extended Interrupt Descriptors can encode 32-bit interrupt
> 	numbers, so an interrupt number may exceed the size of the bitmap
> 	we use to track possible IRQ settings.
>  
> so the field in 'struct acpi_resource_irq' might at some point increase to be
> a 32-bit unsigned value.  Otherwise there's no point having the check at all,
> right?

Ah, how lovely, there has been a merge error at some point...

No, that larger value would live in a struct acpi_resource_extended_irq. 
This code was supposed to go in pnpacpi_parse_ext_irq_option() instead.

Here's the original posting of this patch:

http://lkml.org/lkml/2008/5/30/390

where it indeed is. Here is the last one, where it has mistakingly 
shifted position to pnpacpi_parse_irq_option():

http://lkml.org/lkml/2008/6/17/337

I was already wondering why I hadn't see that warning myself while I was 
testing things...

Bjorn?

Rene.

Re: [PATCH] Fix pnpacpi_parse_irq_option()'s test against PNP_IRQ_NR

[Rene Herman]

On 27-06-08 17:26, Rene Herman wrote:

> On 27-06-08 16:54, David Howells wrote:
> 
>> Rene Herman <rene.herman@keyaccess.nl> wrote:
>> 
>>> Well, it's been promoted from a u8, so no need for that anyway,
>>> but <shrug>.
>> 
>> My logic is that in commit 95b24192cf27631dc11541e97c430389320e7a93
>> it says the following:
>> 
>> ACPI Extended Interrupt Descriptors can encode 32-bit interrupt 
>> numbers, so an interrupt number may exceed the size of the bitmap 
>> we use to track possible IRQ settings.
>> 
>> so the field in 'struct acpi_resource_irq' might at some point 
>> increase to be a 32-bit unsigned value.  Otherwise there's no point
>> having the check at all, right?

(as an aside, we conceptually don't know what PNP_IRQ_NR is -- why the 
define otherwise -- so the check in itself still makes some sense here 
as well).

> Ah, how lovely, there has been a merge error at some point...
> 
> No, that larger value would live in a struct acpi_resource_extended_irq. 
> This code was supposed to go in pnpacpi_parse_ext_irq_option() instead.
> 
> Here's the original posting of this patch:
> 
> http://lkml.org/lkml/2008/5/30/390
> 
> where it indeed is. Here is the last one, where it has mistakingly 
> shifted position to pnpacpi_parse_irq_option():
> 
> http://lkml.org/lkml/2008/6/17/337
> 
> I was already wondering why I hadn't see that warning myself while I was 
> testing things...
> 
> Bjorn?

Rene.