PNP: make the resource type an unsigned long

PATCH In review Rene Herman 3 participants 9 messages Aug 8, 2008 – Aug 12, 2008 Pnp support

lore

Share & tools

lore

Rene Herman

author

2008-08-08 06:39 UTC #0 lore raw

PATCH 1/1 PNP: make the resource type an unsigned long

Hi Bjorn.

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

Rene.

Quoted 1 line

From ddab0bc46eb538c957357549ae2ba657db3887a3 Mon Sep 17 00:00:00 2001

From: Rene Herman <[email protected]>
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 <[email protected]>

---
 drivers/pnp/base.h     |    2 +-
 drivers/pnp/resource.c |    4 ++--
 include/linux/pnp.h    |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

drivers/pnp/base.h +1 -1

index 9fd7bb9..7cc7bf5 100644
@@ -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;

drivers/pnp/resource.c +2 -2

index 4cfe3a1..dbae23a 100644
@@ -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;

include/linux/pnp.h +1 -1

index 1ce54b6..97c8022 100644
@@ -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)
 {

Signature · 1 line

1.5.5

H. Peter Anvin

2008-08-08 22:11 UTC #1 lore raw

Quoted 4 lines · Rene Herman

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

Rene Herman

author

2008-08-09 05:21 UTC #2 lore raw

Quoted 11 lines · H. Peter Anvin

Quoted 2 lines
Andrew earlier commented that pci_resourec_flags() returns an unsigned long. Had this hanging around a local branch. Useful?
drivers/pnp/resource.c +1 -1
-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.

H. Peter Anvin

2008-08-09 05:25 UTC #3 lore raw

Quoted 7 lines · Rene Herman

Quoted 2 lines
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

Rene Herman

author

2008-08-09 05:32 UTC #4 lore raw

Quoted 8 lines · H. Peter Anvin

Quoted 6 lines
Quoted 1 line
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.

Bjorn Helgaas

Acked-by on v1 1/1

2008-08-11 22:00 UTC #5 lore raw

In reply to Rene Herman

Quoted 2 lines · Rene Herman

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 <[email protected]>

H. Peter Anvin

2008-08-11 22:09 UTC #6 lore raw

Quoted 6 lines · Bjorn Helgaas

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

Bjorn Helgaas

2008-08-11 22:19 UTC #7 lore raw

Quoted 10 lines · H. Peter Anvin

Quoted 5 lines · Bjorn Helgaas
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

Rene Herman

author

2008-08-12 04:14 UTC #8 lore raw

In reply to Bjorn Helgaas

Quoted 8 lines · Bjorn Helgaas

Quoted 1 line · Rene Herman
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.

Quoted 5 lines

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 <[email protected]>

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.