[PATCH] PNP: make the resource type an unsigned long

[PATCH] PNP: make the resource type an unsigned long

[Rene Herman]

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

Hi Bjorn.

Andrew earlier commented that pci_resourec_flags() returns an unsigned 
long. Had this hanging around a local branch. Useful?

Rene.

[-- Attachment #2: 0001-PNP-make-the-resource-type-an-unsigned-long.patch --]
[-- Type: text/plain, Size: 2113 bytes --]

>From ddab0bc46eb538c957357549ae2ba657db3887a3 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@gmail.com>
Date: Thu, 26 Jun 2008 00:14:08 +0200
Subject: [PATCH] PNP: make the resource type an unsigned long

PnP encodes the resource type directly as its struct resource->flags
value which is an unsigned long. Make it so...

Signed-off-by: Rene Herman <rene.herman@gmail.com>
---
 drivers/pnp/base.h     |    2 +-
 drivers/pnp/resource.c |    4 ++--
 include/linux/pnp.h    |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pnp/base.h b/drivers/pnp/base.h
index 9fd7bb9..7cc7bf5 100644
--- a/drivers/pnp/base.h
+++ b/drivers/pnp/base.h
@@ -147,7 +147,7 @@ char *pnp_resource_type_name(struct resource *res);
 void dbg_pnp_show_resources(struct pnp_dev *dev, char *desc);
 
 void pnp_free_resources(struct pnp_dev *dev);
-int pnp_resource_type(struct resource *res);
+unsigned long pnp_resource_type(struct resource *res);
 
 struct pnp_resource {
 	struct list_head list;
diff --git a/drivers/pnp/resource.c b/drivers/pnp/resource.c
index 4cfe3a1..dbae23a 100644
--- a/drivers/pnp/resource.c
+++ b/drivers/pnp/resource.c
@@ -467,14 +467,14 @@ int pnp_check_dma(struct pnp_dev *dev, struct resource *res)
 #endif
 }
 
-int pnp_resource_type(struct resource *res)
+unsigned long pnp_resource_type(struct resource *res)
 {
 	return res->flags & (IORESOURCE_IO  | IORESOURCE_MEM |
 			     IORESOURCE_IRQ | IORESOURCE_DMA);
 }
 
 struct resource *pnp_get_resource(struct pnp_dev *dev,
-				  unsigned int type, unsigned int num)
+				  unsigned long type, unsigned int num)
 {
 	struct pnp_resource *pnp_res;
 	struct resource *res;
diff --git a/include/linux/pnp.h b/include/linux/pnp.h
index 1ce54b6..97c8022 100644
--- a/include/linux/pnp.h
+++ b/include/linux/pnp.h
@@ -21,7 +21,7 @@ struct pnp_dev;
 /*
  * Resource Management
  */
-struct resource *pnp_get_resource(struct pnp_dev *, unsigned int, unsigned int);
+struct resource *pnp_get_resource(struct pnp_dev *, unsigned long, unsigned int);
 
 static inline int pnp_resource_valid(struct resource *res)
 {
-- 
1.5.5

Re: [PATCH] PNP: make the resource type an unsigned long

[H. Peter Anvin]

Rene Herman wrote:
> Hi Bjorn.
> 
> Andrew earlier commented that pci_resourec_flags() returns an unsigned 
> long. Had this hanging around a local branch. Useful?
> 

-int pnp_resource_type(struct resource *res)
+unsigned long pnp_resource_type(struct resource *res)
  {
  	return res->flags & (IORESOURCE_IO  | IORESOURCE_MEM |
  			     IORESOURCE_IRQ | IORESOURCE_DMA);
  }

Seems a bit pointless ... either one of those flags is >= 32 bits, in 
which case we need u64, or it's not, in which case there is no reason to 
burden the output with bits we don't need.

	-hpa

Re: [PATCH] PNP: make the resource type an unsigned long

[Rene Herman]

On 08-08-08 23:55, H. Peter Anvin wrote:

>> Andrew earlier commented that pci_resourec_flags() returns an unsigned 
>> long. Had this hanging around a local branch. Useful?
>>
> 
> -int pnp_resource_type(struct resource *res)
> +unsigned long pnp_resource_type(struct resource *res)
>  {
>      return res->flags & (IORESOURCE_IO  | IORESOURCE_MEM |
>                   IORESOURCE_IRQ | IORESOURCE_DMA);
>  }
> 
> Seems a bit pointless ... either one of those flags is >= 32 bits, in 
> which case we need u64, or it's not, in which case there is no reason to 
> burden the output with bits we don't need.

Yes, it's a not a functional patch -- only a type-consistency one. Right 
now we're mixing ints (signed ones even) and unsigned longs and while in 
this case that's not a functional problem it's messy and inconsistent.

I agree (as Andrew said earlier as well) that the struct resource flags 
member should probably just be a u32 but it's not. Changing that would 
be a bigger change than just a simple conistency thing.

Rene.

Re: [PATCH] PNP: make the resource type an unsigned long

[H. Peter Anvin]

Rene Herman wrote:
>>
>> Seems a bit pointless ... either one of those flags is >= 32 bits, in 
>> which case we need u64, or it's not, in which case there is no reason 
>> to burden the output with bits we don't need.
> 
> Yes, it's a not a functional patch -- only a type-consistency one. Right 
> now we're mixing ints (signed ones even) and unsigned longs and while in 
> this case that's not a functional problem it's messy and inconsistent.
> 
> I agree (as Andrew said earlier as well) that the struct resource flags 
> member should probably just be a u32 but it's not. Changing that would 
> be a bigger change than just a simple conistency thing.
> 

You're going in the wrong direction for consistency.  long is different 
on 32 and 64 bits, and really should be avoided unless that is intended.

	-hpa

Re: [PATCH] PNP: make the resource type an unsigned long

[Rene Herman]

On 09-08-08 07:25, H. Peter Anvin wrote:

>>> Seems a bit pointless ... either one of those flags is >= 32 bits, in 
>>> which case we need u64, or it's not, in which case there is no reason 
>>> to burden the output with bits we don't need.
>>
>> Yes, it's a not a functional patch -- only a type-consistency one. 
>> Right now we're mixing ints (signed ones even) and unsigned longs and 
>> while in this case that's not a functional problem it's messy and 
>> inconsistent.
>>
>> I agree (as Andrew said earlier as well) that the struct resource 
>> flags member should probably just be a u32 but it's not. Changing that 
>> would be a bigger change than just a simple conistency thing.
>>
> 
> You're going in the wrong direction for consistency.  long is different 
> on 32 and 64 bits, and really should be avoided unless that is intended.

I know and fair enough but changing struct resource is just a bit too 
central for my tastes.

<shrug>

Rene.

Re: [PATCH] PNP: make the resource type an unsigned long

[Bjorn Helgaas]

On Friday 08 August 2008 12:39:45 am Rene Herman wrote:
> Andrew earlier commented that pci_resourec_flags() returns an unsigned 
> long. Had this hanging around a local branch. Useful?

I see hpa's point that it makes no functional difference, but I do
think it's worthwhile to make the types match.  I don't want to
debug the problem that happens when somebody adds an IORESOURCE_*
flag in the upper bits.

You probably want to send this (and the next one, which is really
a pci_resource_flags() usage despite being in drivers/pnp/quirks.c)
to Andi Kleen.

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

Re: [PATCH] PNP: make the resource type an unsigned long

[H. Peter Anvin]

Bjorn Helgaas wrote:
> 
> I see hpa's point that it makes no functional difference, but I do
> think it's worthwhile to make the types match.  I don't want to
> debug the problem that happens when somebody adds an IORESOURCE_*
> flag in the upper bits.
> 

Then half the platforms break anyway.

We should change this to an explicit type.

	-hpa

Re: [PATCH] PNP: make the resource type an unsigned long

[Bjorn Helgaas]

On Monday 11 August 2008 04:08:18 pm H. Peter Anvin wrote:
> Bjorn Helgaas wrote:
> > 
> > I see hpa's point that it makes no functional difference, but I do
> > think it's worthwhile to make the types match.  I don't want to
> > debug the problem that happens when somebody adds an IORESOURCE_*
> > flag in the upper bits.
> 
> Then half the platforms break anyway.
> 
> We should change this to an explicit type.

Are you suggesting a change to the "unsigned long flags" in struct
resource, or a different change to pnp_resource_type()?

The struct resource change sounds reasonable, but I think neither
Rene nor I want to sign up to do that right now.

Whatever we have in struct resource, I think it makes sense to use
the same type everywhere we reference the "flags" element.  I think
you're saying "it doesn't matter in this case that we use different
types."  But I, as a code reader, would rather not have to spend the
mental energy to convince myself that you're right.

Bjorn

Re: [PATCH] PNP: make the resource type an unsigned long

[Rene Herman]

On 11-08-08 23:59, Bjorn Helgaas wrote:

> On Friday 08 August 2008 12:39:45 am Rene Herman wrote:
>> Andrew earlier commented that pci_resourec_flags() returns an unsigned 
>> long. Had this hanging around a local branch. Useful?
> 
> I see hpa's point that it makes no functional difference, but I do
> think it's worthwhile to make the types match.  I don't want to
> debug the problem that happens when somebody adds an IORESOURCE_*
> flag in the upper bits.
> 
> You probably want to send this (and the next one, which is really
> a pci_resource_flags() usage despite being in drivers/pnp/quirks.c)
> to Andi Kleen.
> 
> Acked-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Okay, thanks. I see that Andrew picked these up with Andi in CC so I'll 
assume things will find their way from there.

Rene.