Re: [PATCH 2/16 v6] PCI: define PCI resource names in an 'enum'

2008-11-13 Thread Simon Horman 
On Wed, Oct 22, 2008 at 04:40:41PM +0800, Yu Zhao wrote:
> This patch moves all definitions of the PCI resource names to an 'enum',
> and also replaces some hard-coded resource variables with symbol
> names. This change eases introduction of device specific resources.
> 
> Cc: Alex Chiang <[EMAIL PROTECTED]>
> Cc: Grant Grundler <[EMAIL PROTECTED]>
> Cc: Greg KH <[EMAIL PROTECTED]>
> Cc: Ingo Molnar <[EMAIL PROTECTED]>
> Cc: Jesse Barnes <[EMAIL PROTECTED]>
> Cc: Matthew Wilcox <[EMAIL PROTECTED]>
> Cc: Randy Dunlap <[EMAIL PROTECTED]>
> Cc: Roland Dreier <[EMAIL PROTECTED]>
> Signed-off-by: Yu Zhao <[EMAIL PROTECTED]>
> 
> ---
>  drivers/pci/pci-sysfs.c |4 +++-
>  drivers/pci/pci.c   |   19 ++-
>  drivers/pci/probe.c |2 +-
>  drivers/pci/proc.c  |7 ---
>  include/linux/pci.h |   37 -
>  5 files changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 110022d..5c456ab 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -101,11 +101,13 @@ resource_show(struct device * dev, struct 
> device_attribute *attr, char * buf)
>   struct pci_dev * pci_dev = to_pci_dev(dev);
>   char * str = buf;
>   int i;
> - int max = 7;
> + int max;
>   resource_size_t start, end;
>  
>   if (pci_dev->subordinate)
>   max = DEVICE_COUNT_RESOURCE;
> + else
> + max = PCI_BRIDGE_RESOURCES;
>  
>   for (i = 0; i < max; i++) {
>   struct resource *res =  &pci_dev->resource[i];
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ae62f01..40284dc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -359,24 +359,9 @@ pci_find_parent_resource(const struct pci_dev *dev, 
> struct resource *res)
>  static void
>  pci_restore_bars(struct pci_dev *dev)
>  {
> - int i, numres;
> -
> - switch (dev->hdr_type) {
> - case PCI_HEADER_TYPE_NORMAL:
> - numres = 6;
> - break;
> - case PCI_HEADER_TYPE_BRIDGE:
> - numres = 2;
> - break;
> - case PCI_HEADER_TYPE_CARDBUS:
> - numres = 1;
> - break;
> - default:
> - /* Should never get here, but just in case... */
> - return;
> - }
> + int i;
>  
> - for (i = 0; i < numres; i++)
> + for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
>   pci_update_resource(dev, i);
>  }
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index aaaf0a1..a52784c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -426,7 +426,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus 
> *parent,
>   child->subordinate = 0xff;
>  
>   /* Set up default resource pointers and names.. */
> - for (i = 0; i < 4; i++) {
> + for (i = 0; i < PCI_BRIDGE_RES_NUM; i++) {
>   child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
>   child->resource[i]->name = child->name;
>   }
> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index e1098c3..f6f2a59 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -352,15 +352,16 @@ static int show_device(struct seq_file *m, void *v)
>   dev->vendor,
>   dev->device,
>   dev->irq);
> - /* Here should be 7 and not PCI_NUM_RESOURCES as we need to preserve 
> compatibility */
> - for (i=0; i<7; i++) {
> +
> + /* only print standard and ROM resources to preserve compatibility */
> + for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>   resource_size_t start, end;
>   pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
>   seq_printf(m, "\t%16llx",
>   (unsigned long long)(start |
>   (dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
>   }
> - for (i=0; i<7; i++) {
> + for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>   resource_size_t start, end;
>   pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
>   seq_printf(m, "\t%16llx",
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 43e1fc1..2ada2b6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -76,7 +76,30 @@ enum pci_mmap_state {
>  #define PCI_DMA_FROMDEVICE   2
>  #define PCI_DMA_NONE 3
>  
> -#define DEVICE_COUNT_RESOURCE12
> +/*
> + *  For PCI devices, the region numbers are assigned this way:
> + */
> +enum {
> + /* #0-5: standard PCI regions */
> + PCI_STD_RESOURCES,
> + PCI_STD_RESOURCES_END = 5,
> +
> + /* #6: expansion ROM */
> + PCI_ROM_RESOURCE,
> +
> + /* address space assigned to buses behind the bridge */
> +#ifndef PCI_BRIDGE_RES_NUM
> +#define PCI_BRIDGE_RES_NUM 4
> +#endif


  Is there any intention to ever set PCI_BRIDGE_RES_NUM to any
  value other than 4? I'm

Re: [PATCH 2/16 v6] PCI: define PCI resource names in an 'enum'

2008-10-22 Thread Yu Zhao 

Bjorn Helgaas wrote:

On Wednesday 22 October 2008 08:44:24 am Yu Zhao wrote:

Bjorn Helgaas wrote:

On Wednesday 22 October 2008 02:40:41 am Yu Zhao wrote:

This patch moves all definitions of the PCI resource names to an 'enum',
and also replaces some hard-coded resource variables with symbol
names. This change eases introduction of device specific resources.

Thanks for removing a bunch of magic numbers from the code.


 static void
 pci_restore_bars(struct pci_dev *dev)
 {
-   int i, numres;
-
-   switch (dev->hdr_type) {
-   case PCI_HEADER_TYPE_NORMAL:
-   numres = 6;
-   break;
-   case PCI_HEADER_TYPE_BRIDGE:
-   numres = 2;
-   break;
-   case PCI_HEADER_TYPE_CARDBUS:
-   numres = 1;
-   break;
-   default:
-   /* Should never get here, but just in case... */
-   return;
-   }
+   int i;
 
-	for (i = 0; i < numres; i++)

+   for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
pci_update_resource(dev, i);
 }

The behavior of this function used to depend on dev->hdr_type.  Now
we don't look at hdr_type at all, so we do the same thing for all
devices.

For example, for a CardBus device, we used to call pci_update_resource()
only for BAR 0; now we call it for BARs 0-6.

Maybe this is safe, but I can't tell from the patch, so I think you
should explain *why* it's safe in the changelog.
It's safe because pci_update_resource() will ignore unused resources. 
E.g., for a Cardbus, only BAR 0 is used and its 'flags' is set, then 
pci_update_resource() only updates it. BAR 1-6 are ignored since their 
'flags' are 0.


I'll put more explanation in the changelog.


This is a logically separate change from merely substituting enum
names for magic numbers, so you might even consider splitting it
into a separate patch.  Better bisection and all that, you know :-)


Will do.

Thanks,
Yu
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/16 v6] PCI: define PCI resource names in an 'enum'

2008-10-22 Thread Yu Zhao 

Bjorn Helgaas wrote:

On Wednesday 22 October 2008 02:40:41 am Yu Zhao wrote:

This patch moves all definitions of the PCI resource names to an 'enum',
and also replaces some hard-coded resource variables with symbol
names. This change eases introduction of device specific resources.


Thanks for removing a bunch of magic numbers from the code.


 static void
 pci_restore_bars(struct pci_dev *dev)
 {
-   int i, numres;
-
-   switch (dev->hdr_type) {
-   case PCI_HEADER_TYPE_NORMAL:
-   numres = 6;
-   break;
-   case PCI_HEADER_TYPE_BRIDGE:
-   numres = 2;
-   break;
-   case PCI_HEADER_TYPE_CARDBUS:
-   numres = 1;
-   break;
-   default:
-   /* Should never get here, but just in case... */
-   return;
-   }
+   int i;
 
-	for (i = 0; i < numres; i++)

+   for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
pci_update_resource(dev, i);
 }


The behavior of this function used to depend on dev->hdr_type.  Now
we don't look at hdr_type at all, so we do the same thing for all
devices.

For example, for a CardBus device, we used to call pci_update_resource()
only for BAR 0; now we call it for BARs 0-6.

Maybe this is safe, but I can't tell from the patch, so I think you
should explain *why* it's safe in the changelog.


It's safe because pci_update_resource() will ignore unused resources. 
E.g., for a Cardbus, only BAR 0 is used and its 'flags' is set, then 
pci_update_resource() only updates it. BAR 1-6 are ignored since their 
'flags' are 0.


I'll put more explanation in the changelog.

Thanks,
Yu
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/16 v6] PCI: define PCI resource names in an 'enum'

2008-10-22 Thread Bjorn Helgaas 
On Wednesday 22 October 2008 08:44:24 am Yu Zhao wrote:
> Bjorn Helgaas wrote:
> > On Wednesday 22 October 2008 02:40:41 am Yu Zhao wrote:
> >> This patch moves all definitions of the PCI resource names to an 'enum',
> >> and also replaces some hard-coded resource variables with symbol
> >> names. This change eases introduction of device specific resources.
> > 
> > Thanks for removing a bunch of magic numbers from the code.
> > 
> >>  static void
> >>  pci_restore_bars(struct pci_dev *dev)
> >>  {
> >> -  int i, numres;
> >> -
> >> -  switch (dev->hdr_type) {
> >> -  case PCI_HEADER_TYPE_NORMAL:
> >> -  numres = 6;
> >> -  break;
> >> -  case PCI_HEADER_TYPE_BRIDGE:
> >> -  numres = 2;
> >> -  break;
> >> -  case PCI_HEADER_TYPE_CARDBUS:
> >> -  numres = 1;
> >> -  break;
> >> -  default:
> >> -  /* Should never get here, but just in case... */
> >> -  return;
> >> -  }
> >> +  int i;
> >>  
> >> -  for (i = 0; i < numres; i++)
> >> +  for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
> >>pci_update_resource(dev, i);
> >>  }
> > 
> > The behavior of this function used to depend on dev->hdr_type.  Now
> > we don't look at hdr_type at all, so we do the same thing for all
> > devices.
> > 
> > For example, for a CardBus device, we used to call pci_update_resource()
> > only for BAR 0; now we call it for BARs 0-6.
> > 
> > Maybe this is safe, but I can't tell from the patch, so I think you
> > should explain *why* it's safe in the changelog.
> 
> It's safe because pci_update_resource() will ignore unused resources. 
> E.g., for a Cardbus, only BAR 0 is used and its 'flags' is set, then 
> pci_update_resource() only updates it. BAR 1-6 are ignored since their 
> 'flags' are 0.
> 
> I'll put more explanation in the changelog.

This is a logically separate change from merely substituting enum
names for magic numbers, so you might even consider splitting it
into a separate patch.  Better bisection and all that, you know :-)

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/16 v6] PCI: define PCI resource names in an 'enum'

2008-10-22 Thread Bjorn Helgaas 
On Wednesday 22 October 2008 02:40:41 am Yu Zhao wrote:
> This patch moves all definitions of the PCI resource names to an 'enum',
> and also replaces some hard-coded resource variables with symbol
> names. This change eases introduction of device specific resources.

Thanks for removing a bunch of magic numbers from the code.

>  static void
>  pci_restore_bars(struct pci_dev *dev)
>  {
> - int i, numres;
> -
> - switch (dev->hdr_type) {
> - case PCI_HEADER_TYPE_NORMAL:
> - numres = 6;
> - break;
> - case PCI_HEADER_TYPE_BRIDGE:
> - numres = 2;
> - break;
> - case PCI_HEADER_TYPE_CARDBUS:
> - numres = 1;
> - break;
> - default:
> - /* Should never get here, but just in case... */
> - return;
> - }
> + int i;
>  
> - for (i = 0; i < numres; i++)
> + for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
>   pci_update_resource(dev, i);
>  }

The behavior of this function used to depend on dev->hdr_type.  Now
we don't look at hdr_type at all, so we do the same thing for all
devices.

For example, for a CardBus device, we used to call pci_update_resource()
only for BAR 0; now we call it for BARs 0-6.

Maybe this is safe, but I can't tell from the patch, so I think you
should explain *why* it's safe in the changelog.

> +/*
> + *  For PCI devices, the region numbers are assigned this way:
> + */
> +enum {
> + /* #0-5: standard PCI regions */
> + PCI_STD_RESOURCES,
> + PCI_STD_RESOURCES_END = 5,
> +
> + /* #6: expansion ROM */
> + PCI_ROM_RESOURCE,
> +
> + /* address space assigned to buses behind the bridge */
> +#ifndef PCI_BRIDGE_RES_NUM
> +#define PCI_BRIDGE_RES_NUM 4
> +#endif
> + PCI_BRIDGE_RESOURCES,
> + PCI_BRIDGE_RES_END = PCI_BRIDGE_RESOURCES + PCI_BRIDGE_RES_NUM - 1,

Since you used "PCI_STD_RESOURCES_END" above, maybe you should use
"PCI_BRIDGE_RESOURCES_END" instead of "PCI_BRIDGE_RES_END".

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/16 v6] PCI: define PCI resource names in an 'enum'

2008-10-22 Thread Yu Zhao 
This patch moves all definitions of the PCI resource names to an 'enum',
and also replaces some hard-coded resource variables with symbol
names. This change eases introduction of device specific resources.

Cc: Alex Chiang <[EMAIL PROTECTED]>
Cc: Grant Grundler <[EMAIL PROTECTED]>
Cc: Greg KH <[EMAIL PROTECTED]>
Cc: Ingo Molnar <[EMAIL PROTECTED]>
Cc: Jesse Barnes <[EMAIL PROTECTED]>
Cc: Matthew Wilcox <[EMAIL PROTECTED]>
Cc: Randy Dunlap <[EMAIL PROTECTED]>
Cc: Roland Dreier <[EMAIL PROTECTED]>
Signed-off-by: Yu Zhao <[EMAIL PROTECTED]>

---
 drivers/pci/pci-sysfs.c |4 +++-
 drivers/pci/pci.c   |   19 ++-
 drivers/pci/probe.c |2 +-
 drivers/pci/proc.c  |7 ---
 include/linux/pci.h |   37 -
 5 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 110022d..5c456ab 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -101,11 +101,13 @@ resource_show(struct device * dev, struct 
device_attribute *attr, char * buf)
struct pci_dev * pci_dev = to_pci_dev(dev);
char * str = buf;
int i;
-   int max = 7;
+   int max;
resource_size_t start, end;
 
if (pci_dev->subordinate)
max = DEVICE_COUNT_RESOURCE;
+   else
+   max = PCI_BRIDGE_RESOURCES;
 
for (i = 0; i < max; i++) {
struct resource *res =  &pci_dev->resource[i];
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ae62f01..40284dc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -359,24 +359,9 @@ pci_find_parent_resource(const struct pci_dev *dev, struct 
resource *res)
 static void
 pci_restore_bars(struct pci_dev *dev)
 {
-   int i, numres;
-
-   switch (dev->hdr_type) {
-   case PCI_HEADER_TYPE_NORMAL:
-   numres = 6;
-   break;
-   case PCI_HEADER_TYPE_BRIDGE:
-   numres = 2;
-   break;
-   case PCI_HEADER_TYPE_CARDBUS:
-   numres = 1;
-   break;
-   default:
-   /* Should never get here, but just in case... */
-   return;
-   }
+   int i;
 
-   for (i = 0; i < numres; i++)
+   for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
pci_update_resource(dev, i);
 }
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index aaaf0a1..a52784c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -426,7 +426,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus 
*parent,
child->subordinate = 0xff;
 
/* Set up default resource pointers and names.. */
-   for (i = 0; i < 4; i++) {
+   for (i = 0; i < PCI_BRIDGE_RES_NUM; i++) {
child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
child->resource[i]->name = child->name;
}
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index e1098c3..f6f2a59 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -352,15 +352,16 @@ static int show_device(struct seq_file *m, void *v)
dev->vendor,
dev->device,
dev->irq);
-   /* Here should be 7 and not PCI_NUM_RESOURCES as we need to preserve 
compatibility */
-   for (i=0; i<7; i++) {
+
+   /* only print standard and ROM resources to preserve compatibility */
+   for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
resource_size_t start, end;
pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
seq_printf(m, "\t%16llx",
(unsigned long long)(start |
(dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
}
-   for (i=0; i<7; i++) {
+   for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
resource_size_t start, end;
pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
seq_printf(m, "\t%16llx",
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 43e1fc1..2ada2b6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -76,7 +76,30 @@ enum pci_mmap_state {
 #define PCI_DMA_FROMDEVICE 2
 #define PCI_DMA_NONE   3
 
-#define DEVICE_COUNT_RESOURCE  12
+/*
+ *  For PCI devices, the region numbers are assigned this way:
+ */
+enum {
+   /* #0-5: standard PCI regions */
+   PCI_STD_RESOURCES,
+   PCI_STD_RESOURCES_END = 5,
+
+   /* #6: expansion ROM */
+   PCI_ROM_RESOURCE,
+
+   /* address space assigned to buses behind the bridge */
+#ifndef PCI_BRIDGE_RES_NUM
+#define PCI_BRIDGE_RES_NUM 4
+#endif
+   PCI_BRIDGE_RESOURCES,
+   PCI_BRIDGE_RES_END = PCI_BRIDGE_RESOURCES + PCI_BRIDGE_RES_NUM - 1,
+
+   /* total resources associated with a PCI device */
+   PCI_NUM_RESOURCES,
+
+   /* preserve this for compatibility */
+   DEVICE_COUNT_RESOURCE
+};
 
 typedef int __bitwis