[PATCH v3 00/27] PCI: Add for_each_pci_resource and addon_res support

[PATCH v3 00/27] PCI: Add for_each_pci_resource and addon_res support

[Yinghai Lu]

First part is for_each_pci_resource support:
Now pci device resource iteration is done via "for (i=0...)" open code.
That make code hard to read esp when only bridge or sriov resources
are involved.

We want to replace those open code with for_each_pci_resource() to make
the code more readable.
Also will add addon_resource handling, and need to make addon resource
to be treated as normal PCI resources during iteration, add
for_each_pci_resource will make that support transition more simple.

To make for_each_pci_resource more efficient. We will use preset bitmap
of different type for searching next idx.

Second parts is addon_res support:
Now there is some non normal pci resources other than standard,rom,
sriov, bridges.
Some could be same as standard reg, but using different position.
Some could have own way to read/write to them.
Kernel are using different way to hack those resources like abusing
pci bridge resource spot on non bridge pci device.

Add addon_resources list in pci_dev for those non-standard resources.
With this patch, will treat addon-resource like standard resources with
special ops.

At last replace some quirk_io with addon_res.

could get from
        git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-for-each-res-addon-v3
on top of v3.9-rc2

-v3: rebase on of v3.9-rc2

Thanks

Yinghai Lu

Ram Pai (1):
  PCI: pci resource iterator

Yinghai Lu (26):
  PCI: Add pci_dev_resource_n()
  PCI: Add pci_dev_resource_idx() helper
  PCI: Add is_pci_*_resource_idx() helpers
  PCI: Update pci_resource_start etc to use pci_dev_resource_n()
  PCI, x86: Use for_each_pci_resource() with pci_allocate_bridge_resources
  PCI, x86: Use for_each_pci_resource() with pci_allocate_dev_resources
  PCI: Use for_each_pci_resource() with IOV releated functions
  PCI, acpiphp: Use for_each_pci_resource() helper
  PCI, pciehp: Use for_each_pci_resource() helper
  PCI: Use for_each_pci_resource() in pci_enable_dev
  PCI: Use for_each_pci_resource() in pci_reassigndev
  PCI: Use for_each_pci_resource() with pci bar reassign funcs
  PCI: Use for_each_pci_resource() in pci_assign_resource
  PCI, x86: Use for_each_pci_resource() with noassign_bars
  PCI: Use for_each_pci_resource() in pci_dev_driver()
  PCI: Use for_each_pci_resource() in pci resource release
  PCI: Use for_each_pci_resource() in pci bases reading
  PCI, x86: Use for_each_pci_resource() with mrst
  PCI, xen: Use for_each_pci_resource() with xen pci
  PCI: Add addon_resource support for pci devices
  PCI: Add helpers to add addon_resource
  PCI: Update pci_resource_bar() to support addon_resource
  PCI: Assign/update resource to addon_res
  PCI: Make piix4 quirk to use addon_res
  PCI: Make quirk_io_region to use addon_res
  PCI: Use addon_fixed_resource with ati fixed resource

 arch/x86/pci/common.c              |    3 +-
 arch/x86/pci/i386.c                |   58 +++-----
 arch/x86/pci/mrst.c                |    7 +-
 drivers/pci/hotplug/acpiphp_glue.c |    4 +-
 drivers/pci/hotplug/pciehp_hpc.c   |    5 +-
 drivers/pci/iov.c                  |   31 +++--
 drivers/pci/pci-driver.c           |    6 +-
 drivers/pci/pci.c                  |   27 ++--
 drivers/pci/probe.c                |  176 ++++++++++++++++++++++++-
 drivers/pci/quirks.c               |  256 +++++++++++++++++++++---------------
 drivers/pci/remove.c               |    5 +-
 drivers/pci/setup-bus.c            |   28 ++--
 drivers/pci/setup-res.c            |   38 ++++--
 drivers/pci/xen-pcifront.c         |    4 +-
 include/linux/pci.h                |   79 ++++++++++-
 15 files changed, 508 insertions(+), 219 deletions(-)

-- 
1.7.10.4

[PATCH v3 01/27] PCI: Add pci_dev_resource_n()

[Yinghai Lu]

Now pci device resource iteration is done via "for (i=0...)" open code.
That make code hard to read esp when only bridge or sriov resources
are involved.

We want to replace those open code with for_each_pci_resource().
Also want to add addon_resource handling, and need to make addon resource
to be treated as normal PCI resources during iteration.

Add pci_device_resource_n() instead of using dev->resources[n].
Use it with for_each_pci_resource macro and addon_resource support.

-v2: Add EXPORT_SYMBOL to find building error found by Gary Hade

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |    9 +++++++++
 include/linux/pci.h |    2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b494066..9cb3eb3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -105,6 +105,15 @@ static int __init pcibus_class_init(void)
 }
 postcore_initcall(pcibus_class_init);
 
+struct resource *pci_dev_resource_n(struct pci_dev *dev, int n)
+{
+	if (n >= 0 && n < PCI_NUM_RESOURCES)
+		return &dev->resource[n];
+
+	return NULL;
+}
+EXPORT_SYMBOL(pci_dev_resource_n);
+
 static u64 pci_size(u64 base, u64 maxbase, u64 mask)
 {
 	u64 size = mask & maxbase;	/* Find the significant bits */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2461033a..00d5367 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -338,6 +338,8 @@ struct pci_dev {
 	size_t romlen; /* Length of ROM if it's not from the BAR */
 };
 
+struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
+
 static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
 {
 #ifdef CONFIG_PCI_IOV
-- 
1.7.10.4

[PATCH v3 02/27] PCI: Add pci_dev_resource_idx() helper

[Yinghai Lu]

Use resource pointer to get index in pci resources array/list.

-v2: export symbol for acpiphp compiling error, found by
       Steven Newbury <steve@snewbury.org.uk>

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |    9 +++++++++
 include/linux/pci.h |    1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9cb3eb3..1df75f7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -114,6 +114,15 @@ struct resource *pci_dev_resource_n(struct pci_dev *dev, int n)
 }
 EXPORT_SYMBOL(pci_dev_resource_n);
 
+int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
+{
+	if (res >= dev->resource &&
+	    res <= dev->resource + (PCI_NUM_RESOURCES - 1))
+		return res - dev->resource;
+
+	return -1;
+}
+
 static u64 pci_size(u64 base, u64 maxbase, u64 mask)
 {
 	u64 size = mask & maxbase;	/* Find the significant bits */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 00d5367..aefff8b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -339,6 +339,7 @@ struct pci_dev {
 };
 
 struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
+int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
 
 static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
 {
-- 
1.7.10.4

Re: [PATCH v3 02/27] PCI: Add pci_dev_resource_idx() helper

[Bjorn Helgaas]

On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> Use resource pointer to get index in pci resources array/list.
>
> -v2: export symbol for acpiphp compiling error, found by
>        Steven Newbury <steve@snewbury.org.uk>
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/probe.c |    9 +++++++++
>  include/linux/pci.h |    1 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 9cb3eb3..1df75f7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -114,6 +114,15 @@ struct resource *pci_dev_resource_n(struct pci_dev *dev, int n)
>  }
>  EXPORT_SYMBOL(pci_dev_resource_n);
>
> +int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
> +{
> +       if (res >= dev->resource &&
> +           res <= dev->resource + (PCI_NUM_RESOURCES - 1))
> +               return res - dev->resource;
> +
> +       return -1;
> +}

I'm dubious about the whole idea of a resource *index*.  I'd like to
get away from that concept completely.

Some uses of this are just for printing and are obviously unnecessary
(get_res_add_size()).

Other places we pass around the index, e.g.,
reassign_resources_sorted() passes it to pci_assign_resource() and
pci_reassign_resource(), and eventually we just pass the index to
pci_dev_resource_n() to get back what we started with.  I'd rather
just pass around a pointer instead of this half pointer/half index
strategy.

We might need a "struct pci_resource" or something that contains the
type (BAR, IOV, bridge window, etc), a BAR number, etc.  There's
already a "struct pci_resource" in cpqphp, but that's isolated to
cpqphp and could be easily changed if you wanted that name.

It's going to be very confusing to have a "struct pci_dev_resource"
and a "pci_dev_resource_n()" that have nothing to do with each other.

>  static u64 pci_size(u64 base, u64 maxbase, u64 mask)
>  {
>         u64 size = mask & maxbase;      /* Find the significant bits */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 00d5367..aefff8b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -339,6 +339,7 @@ struct pci_dev {
>  };
>
>  struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
> +int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
>
>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>  {
> --
> 1.7.10.4
>

[PATCH v3 03/27] PCI: pci resource iterator

[Yinghai Lu]

From: Ram Pai <linuxram@us.ibm.com>

Currently pci_dev structure holds an array of 17 PCI resources; six base
BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
A bridge device just needs the 4 bridge resources. A non-bridge device
just needs the six base resources and one ROM resource. The sriov
resources are needed only if the device has SRIOV capability.

The pci_dev structure needs to be re-organized to avoid unnecessary
bloating.  However too much code outside the pci-bus driver, assumes the
internal details of the pci_dev structure, thus making it hard to
re-organize the datastructure.

As a first step this patch provides generic methods to access the
resource structure of the pci_dev.

Finally we can re-organize the resource structure in the pci_dev
structure and correspondingly update the methods.

-v2: Consolidated iterator interface as per Bjorn's suggestion.
-v3: Add the idx back - Yinghai Lu
-v7: Change to use bitmap for searching - Yinghai Lu
-v8: Fix acpiphp module compiling error that is found by
	Steven Newbury <steve@snewbury.org.uk> - Yinghai Lu

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |   24 ++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 1df75f7..ac751a6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -123,6 +123,54 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
 	return -1;
 }
 
+static void __init_res_idx_mask(unsigned long *mask, int flag)
+{
+	bitmap_zero(mask, PCI_NUM_RESOURCES);
+	if (flag & PCI_STD_RES)
+		bitmap_set(mask, PCI_STD_RESOURCES,
+			PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1);
+	if (flag & PCI_ROM_RES)
+		bitmap_set(mask, PCI_ROM_RESOURCE, 1);
+#ifdef CONFIG_PCI_IOV
+	if (flag & PCI_IOV_RES)
+		bitmap_set(mask, PCI_IOV_RESOURCES,
+			PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1);
+#endif
+	if (flag & PCI_BRIDGE_RES)
+		bitmap_set(mask, PCI_BRIDGE_RESOURCES,
+			PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1);
+}
+
+static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES);
+static int __init pci_res_idx_mask_init(void)
+{
+	int i;
+
+	for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++)
+		__init_res_idx_mask(res_idx_mask[i], i);
+
+	return 0;
+}
+postcore_initcall(pci_res_idx_mask_init);
+
+static inline unsigned long *get_res_idx_mask(int flag)
+{
+	return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)];
+}
+
+int pci_next_resource_idx(int i, int flag)
+{
+	i++;
+	if (i < PCI_NUM_RESOURCES)
+		i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i);
+
+	if (i < PCI_NUM_RESOURCES)
+		return i;
+
+	return -1;
+}
+EXPORT_SYMBOL(pci_next_resource_idx);
+
 static u64 pci_size(u64 base, u64 maxbase, u64 mask)
 {
 	u64 size = mask & maxbase;	/* Find the significant bits */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index aefff8b..127a856 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -341,6 +341,30 @@ struct pci_dev {
 struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
 int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
 
+#define PCI_STD_RES		(1<<0)
+#define PCI_ROM_RES		(1<<1)
+#define PCI_IOV_RES		(1<<2)
+#define PCI_BRIDGE_RES		(1<<3)
+#define PCI_RES_BLOCK_NUM	4
+
+#define PCI_ALL_RES		(PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES)
+#define PCI_NOSTD_RES		(PCI_ALL_RES & ~PCI_STD_RES)
+#define PCI_NOIOV_RES		(PCI_ALL_RES & ~PCI_IOV_RES)
+#define PCI_NOROM_RES		(PCI_ALL_RES & ~PCI_ROM_RES)
+#define PCI_NOBRIDGE_RES	(PCI_ALL_RES & ~PCI_BRIDGE_RES)
+#define PCI_STD_ROM_RES		(PCI_STD_RES | PCI_ROM_RES)
+#define PCI_STD_IOV_RES		(PCI_STD_RES | PCI_IOV_RES)
+#define PCI_STD_ROM_IOV_RES	(PCI_STD_RES | PCI_ROM_RES | PCI_IOV_RES)
+
+int pci_next_resource_idx(int i, int flag);
+
+#define for_each_pci_resource(dev, res, i, flag)	\
+	for (i = pci_next_resource_idx(-1, flag),	\
+		res = pci_dev_resource_n(dev, i);	\
+	     res;					\
+	     i = pci_next_resource_idx(i, flag),	\
+		res = pci_dev_resource_n(dev, i))
+
 static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
 {
 #ifdef CONFIG_PCI_IOV
-- 
1.7.10.4

Re: [PATCH v3 03/27] PCI: pci resource iterator

[Bjorn Helgaas]

On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> From: Ram Pai <linuxram@us.ibm.com>
>
> Currently pci_dev structure holds an array of 17 PCI resources; six base
> BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
> A bridge device just needs the 4 bridge resources. A non-bridge device
> just needs the six base resources and one ROM resource. The sriov
> resources are needed only if the device has SRIOV capability.
>
> The pci_dev structure needs to be re-organized to avoid unnecessary
> bloating.  However too much code outside the pci-bus driver, assumes the
> internal details of the pci_dev structure, thus making it hard to
> re-organize the datastructure.
>
> As a first step this patch provides generic methods to access the
> resource structure of the pci_dev.
>
> Finally we can re-organize the resource structure in the pci_dev
> structure and correspondingly update the methods.
>
> -v2: Consolidated iterator interface as per Bjorn's suggestion.
> -v3: Add the idx back - Yinghai Lu
> -v7: Change to use bitmap for searching - Yinghai Lu
> -v8: Fix acpiphp module compiling error that is found by
>         Steven Newbury <steve@snewbury.org.uk> - Yinghai Lu
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |   24 ++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 1df75f7..ac751a6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -123,6 +123,54 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
>         return -1;
>  }
>
> +static void __init_res_idx_mask(unsigned long *mask, int flag)
> +{
> +       bitmap_zero(mask, PCI_NUM_RESOURCES);
> +       if (flag & PCI_STD_RES)
> +               bitmap_set(mask, PCI_STD_RESOURCES,
> +                       PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1);
> +       if (flag & PCI_ROM_RES)
> +               bitmap_set(mask, PCI_ROM_RESOURCE, 1);
> +#ifdef CONFIG_PCI_IOV
> +       if (flag & PCI_IOV_RES)
> +               bitmap_set(mask, PCI_IOV_RESOURCES,
> +                       PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1);
> +#endif
> +       if (flag & PCI_BRIDGE_RES)
> +               bitmap_set(mask, PCI_BRIDGE_RESOURCES,
> +                       PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1);
> +}
> +
> +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES);
> +static int __init pci_res_idx_mask_init(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++)
> +               __init_res_idx_mask(res_idx_mask[i], i);
> +
> +       return 0;
> +}
> +postcore_initcall(pci_res_idx_mask_init);
> +
> +static inline unsigned long *get_res_idx_mask(int flag)
> +{
> +       return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)];
> +}
> +
> +int pci_next_resource_idx(int i, int flag)
> +{
> +       i++;
> +       if (i < PCI_NUM_RESOURCES)
> +               i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i);
> +
> +       if (i < PCI_NUM_RESOURCES)
> +               return i;
> +
> +       return -1;
> +}
> +EXPORT_SYMBOL(pci_next_resource_idx);
> +
>  static u64 pci_size(u64 base, u64 maxbase, u64 mask)
>  {
>         u64 size = mask & maxbase;      /* Find the significant bits */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aefff8b..127a856 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -341,6 +341,30 @@ struct pci_dev {
>  struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
>  int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
>
> +#define PCI_STD_RES            (1<<0)
> +#define PCI_ROM_RES            (1<<1)
> +#define PCI_IOV_RES            (1<<2)
> +#define PCI_BRIDGE_RES         (1<<3)
> +#define PCI_RES_BLOCK_NUM      4
> +
> +#define PCI_ALL_RES            (PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES)
> +#define PCI_NOSTD_RES          (PCI_ALL_RES & ~PCI_STD_RES)
> +#define PCI_NOIOV_RES          (PCI_ALL_RES & ~PCI_IOV_RES)
> +#define PCI_NOROM_RES          (PCI_ALL_RES & ~PCI_ROM_RES)
> +#define PCI_NOBRIDGE_RES       (PCI_ALL_RES & ~PCI_BRIDGE_RES)
> +#define PCI_STD_ROM_RES                (PCI_STD_RES | PCI_ROM_RES)
> +#define PCI_STD_IOV_RES                (PCI_STD_RES | PCI_IOV_RES)
> +#define PCI_STD_ROM_IOV_RES    (PCI_STD_RES | PCI_ROM_RES | PCI_IOV_RES)
> +
> +int pci_next_resource_idx(int i, int flag);
> +
> +#define for_each_pci_resource(dev, res, i, flag)       \
> +       for (i = pci_next_resource_idx(-1, flag),       \
> +               res = pci_dev_resource_n(dev, i);       \
> +            res;                                       \
> +            i = pci_next_resource_idx(i, flag),        \
> +               res = pci_dev_resource_n(dev, i))
> +
>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>  {
>  #ifdef CONFIG_PCI_IOV

I think this turned out to be a disaster, with all the bitmaps and
helper functions.  Filtering in the bodies of the
for_each_pci_resource() users has *got* to be better.  That probably
requires a wrapper struct around the struct resource.  Or possibly
having a wrapper struct with a "type" or "flags" field would make
filtering in for_each_pci_resources() itself cleaner to implement.

I think it would also help if we got rid of all the "PCI_NO*_RES"
definitions and just had the users explicitly specify what they *do*
want.

Bjorn

Re: [PATCH v3 03/27] PCI: pci resource iterator

[Ram Pai]

On Thu, Apr 04, 2013 at 04:18:01PM -0600, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > From: Ram Pai <linuxram@us.ibm.com>
> >
> > Currently pci_dev structure holds an array of 17 PCI resources; six base
> > BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
> > A bridge device just needs the 4 bridge resources. A non-bridge device
> > just needs the six base resources and one ROM resource. The sriov
> > resources are needed only if the device has SRIOV capability.
> >
> > The pci_dev structure needs to be re-organized to avoid unnecessary
> > bloating.  However too much code outside the pci-bus driver, assumes the
> > internal details of the pci_dev structure, thus making it hard to
> > re-organize the datastructure.
> >
> > As a first step this patch provides generic methods to access the
> > resource structure of the pci_dev.
> >
> > Finally we can re-organize the resource structure in the pci_dev
> > structure and correspondingly update the methods.
> >
> > -v2: Consolidated iterator interface as per Bjorn's suggestion.
> > -v3: Add the idx back - Yinghai Lu
> > -v7: Change to use bitmap for searching - Yinghai Lu
> > -v8: Fix acpiphp module compiling error that is found by
> >         Steven Newbury <steve@snewbury.org.uk> - Yinghai Lu
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > ---
> >  drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h |   24 ++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 1df75f7..ac751a6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -123,6 +123,54 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
> >         return -1;
> >  }
> >
> > +static void __init_res_idx_mask(unsigned long *mask, int flag)
> > +{
> > +       bitmap_zero(mask, PCI_NUM_RESOURCES);
> > +       if (flag & PCI_STD_RES)
> > +               bitmap_set(mask, PCI_STD_RESOURCES,
> > +                       PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1);
> > +       if (flag & PCI_ROM_RES)
> > +               bitmap_set(mask, PCI_ROM_RESOURCE, 1);
> > +#ifdef CONFIG_PCI_IOV
> > +       if (flag & PCI_IOV_RES)
> > +               bitmap_set(mask, PCI_IOV_RESOURCES,
> > +                       PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1);
> > +#endif
> > +       if (flag & PCI_BRIDGE_RES)
> > +               bitmap_set(mask, PCI_BRIDGE_RESOURCES,
> > +                       PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1);
> > +}
> > +
> > +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES);
> > +static int __init pci_res_idx_mask_init(void)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++)
> > +               __init_res_idx_mask(res_idx_mask[i], i);
> > +
> > +       return 0;
> > +}
> > +postcore_initcall(pci_res_idx_mask_init);
> > +
> > +static inline unsigned long *get_res_idx_mask(int flag)
> > +{
> > +       return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)];
> > +}
> > +
> > +int pci_next_resource_idx(int i, int flag)
> > +{
> > +       i++;
> > +       if (i < PCI_NUM_RESOURCES)
> > +               i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i);
> > +
> > +       if (i < PCI_NUM_RESOURCES)
> > +               return i;
> > +
> > +       return -1;
> > +}
> > +EXPORT_SYMBOL(pci_next_resource_idx);
> > +
> >  static u64 pci_size(u64 base, u64 maxbase, u64 mask)
> >  {
> >         u64 size = mask & maxbase;      /* Find the significant bits */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index aefff8b..127a856 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -341,6 +341,30 @@ struct pci_dev {
> >  struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
> >  int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
> >
> > +#define PCI_STD_RES            (1<<0)
> > +#define PCI_ROM_RES            (1<<1)
> > +#define PCI_IOV_RES            (1<<2)
> > +#define PCI_BRIDGE_RES         (1<<3)
> > +#define PCI_RES_BLOCK_NUM      4
> > +
> > +#define PCI_ALL_RES            (PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES)
> > +#define PCI_NOSTD_RES          (PCI_ALL_RES & ~PCI_STD_RES)
> > +#define PCI_NOIOV_RES          (PCI_ALL_RES & ~PCI_IOV_RES)
> > +#define PCI_NOROM_RES          (PCI_ALL_RES & ~PCI_ROM_RES)
> > +#define PCI_NOBRIDGE_RES       (PCI_ALL_RES & ~PCI_BRIDGE_RES)
> > +#define PCI_STD_ROM_RES                (PCI_STD_RES | PCI_ROM_RES)
> > +#define PCI_STD_IOV_RES                (PCI_STD_RES | PCI_IOV_RES)
> > +#define PCI_STD_ROM_IOV_RES    (PCI_STD_RES | PCI_ROM_RES | PCI_IOV_RES)
> > +
> > +int pci_next_resource_idx(int i, int flag);
> > +
> > +#define for_each_pci_resource(dev, res, i, flag)       \
> > +       for (i = pci_next_resource_idx(-1, flag),       \
> > +               res = pci_dev_resource_n(dev, i);       \
> > +            res;                                       \
> > +            i = pci_next_resource_idx(i, flag),        \
> > +               res = pci_dev_resource_n(dev, i))
> > +
> >  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> >  {
> >  #ifdef CONFIG_PCI_IOV
> 
> I think this turned out to be a disaster, with all the bitmaps and
> helper functions.  Filtering in the bodies of the
> for_each_pci_resource() users has *got* to be better.  That probably
> requires a wrapper struct around the struct resource.  Or possibly
> having a wrapper struct with a "type" or "flags" field would make
> filtering in for_each_pci_resources() itself cleaner to implement.

I agree. There are two cleanups needed.

a) pci drivers should not assume the internal organization of the
	resources in the struct pci_dev.

b) The type of a resource has to be determined based on some
   information internal to the resource; possibly a flag, 
   instead of the relative position of the resource in some array.

My original patch was aimed at (a); to make the organization of the
resources transparent to the drivers, and then move to (b) where the
type of the resource would be determined based on some flag, instead
of its relative location. Unfortuately the current newer version has got 
morphed significantly, that I think is not clean enough.

RP

Re: [PATCH v3 03/27] PCI: pci resource iterator

[Bjorn Helgaas]

On Mon, Apr 8, 2013 at 10:51 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Thu, Apr 04, 2013 at 04:18:01PM -0600, Bjorn Helgaas wrote:
>> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > From: Ram Pai <linuxram@us.ibm.com>
>> >
>> > Currently pci_dev structure holds an array of 17 PCI resources; six base
>> > BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
>> > A bridge device just needs the 4 bridge resources. A non-bridge device
>> > just needs the six base resources and one ROM resource. The sriov
>> > resources are needed only if the device has SRIOV capability.
>> >
>> > The pci_dev structure needs to be re-organized to avoid unnecessary
>> > bloating.  However too much code outside the pci-bus driver, assumes the
>> > internal details of the pci_dev structure, thus making it hard to
>> > re-organize the datastructure.
>> >
>> > As a first step this patch provides generic methods to access the
>> > resource structure of the pci_dev.
>> >
>> > Finally we can re-organize the resource structure in the pci_dev
>> > structure and correspondingly update the methods.
>> >
>> > -v2: Consolidated iterator interface as per Bjorn's suggestion.
>> > -v3: Add the idx back - Yinghai Lu
>> > -v7: Change to use bitmap for searching - Yinghai Lu
>> > -v8: Fix acpiphp module compiling error that is found by
>> >         Steven Newbury <steve@snewbury.org.uk> - Yinghai Lu
>> >
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> > ---
>> >  drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  include/linux/pci.h |   24 ++++++++++++++++++++++++
>> >  2 files changed, 72 insertions(+)
>> >
>> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> > index 1df75f7..ac751a6 100644
>> > --- a/drivers/pci/probe.c
>> > +++ b/drivers/pci/probe.c
>> > @@ -123,6 +123,54 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
>> >         return -1;
>> >  }
>> >
>> > +static void __init_res_idx_mask(unsigned long *mask, int flag)
>> > +{
>> > +       bitmap_zero(mask, PCI_NUM_RESOURCES);
>> > +       if (flag & PCI_STD_RES)
>> > +               bitmap_set(mask, PCI_STD_RESOURCES,
>> > +                       PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1);
>> > +       if (flag & PCI_ROM_RES)
>> > +               bitmap_set(mask, PCI_ROM_RESOURCE, 1);
>> > +#ifdef CONFIG_PCI_IOV
>> > +       if (flag & PCI_IOV_RES)
>> > +               bitmap_set(mask, PCI_IOV_RESOURCES,
>> > +                       PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1);
>> > +#endif
>> > +       if (flag & PCI_BRIDGE_RES)
>> > +               bitmap_set(mask, PCI_BRIDGE_RESOURCES,
>> > +                       PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1);
>> > +}
>> > +
>> > +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES);
>> > +static int __init pci_res_idx_mask_init(void)
>> > +{
>> > +       int i;
>> > +
>> > +       for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++)
>> > +               __init_res_idx_mask(res_idx_mask[i], i);
>> > +
>> > +       return 0;
>> > +}
>> > +postcore_initcall(pci_res_idx_mask_init);
>> > +
>> > +static inline unsigned long *get_res_idx_mask(int flag)
>> > +{
>> > +       return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)];
>> > +}
>> > +
>> > +int pci_next_resource_idx(int i, int flag)
>> > +{
>> > +       i++;
>> > +       if (i < PCI_NUM_RESOURCES)
>> > +               i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i);
>> > +
>> > +       if (i < PCI_NUM_RESOURCES)
>> > +               return i;
>> > +
>> > +       return -1;
>> > +}
>> > +EXPORT_SYMBOL(pci_next_resource_idx);
>> > +
>> >  static u64 pci_size(u64 base, u64 maxbase, u64 mask)
>> >  {
>> >         u64 size = mask & maxbase;      /* Find the significant bits */
>> > diff --git a/include/linux/pci.h b/include/linux/pci.h
>> > index aefff8b..127a856 100644
>> > --- a/include/linux/pci.h
>> > +++ b/include/linux/pci.h
>> > @@ -341,6 +341,30 @@ struct pci_dev {
>> >  struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
>> >  int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
>> >
>> > +#define PCI_STD_RES            (1<<0)
>> > +#define PCI_ROM_RES            (1<<1)
>> > +#define PCI_IOV_RES            (1<<2)
>> > +#define PCI_BRIDGE_RES         (1<<3)
>> > +#define PCI_RES_BLOCK_NUM      4
>> > +
>> > +#define PCI_ALL_RES            (PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES)
>> > +#define PCI_NOSTD_RES          (PCI_ALL_RES & ~PCI_STD_RES)
>> > +#define PCI_NOIOV_RES          (PCI_ALL_RES & ~PCI_IOV_RES)
>> > +#define PCI_NOROM_RES          (PCI_ALL_RES & ~PCI_ROM_RES)
>> > +#define PCI_NOBRIDGE_RES       (PCI_ALL_RES & ~PCI_BRIDGE_RES)
>> > +#define PCI_STD_ROM_RES                (PCI_STD_RES | PCI_ROM_RES)
>> > +#define PCI_STD_IOV_RES                (PCI_STD_RES | PCI_IOV_RES)
>> > +#define PCI_STD_ROM_IOV_RES    (PCI_STD_RES | PCI_ROM_RES | PCI_IOV_RES)
>> > +
>> > +int pci_next_resource_idx(int i, int flag);
>> > +
>> > +#define for_each_pci_resource(dev, res, i, flag)       \
>> > +       for (i = pci_next_resource_idx(-1, flag),       \
>> > +               res = pci_dev_resource_n(dev, i);       \
>> > +            res;                                       \
>> > +            i = pci_next_resource_idx(i, flag),        \
>> > +               res = pci_dev_resource_n(dev, i))
>> > +
>> >  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>> >  {
>> >  #ifdef CONFIG_PCI_IOV
>>
>> I think this turned out to be a disaster, with all the bitmaps and
>> helper functions.  Filtering in the bodies of the
>> for_each_pci_resource() users has *got* to be better.  That probably
>> requires a wrapper struct around the struct resource.  Or possibly
>> having a wrapper struct with a "type" or "flags" field would make
>> filtering in for_each_pci_resources() itself cleaner to implement.
>
> I agree. There are two cleanups needed.
>
> a) pci drivers should not assume the internal organization of the
>         resources in the struct pci_dev.

Do you mean that drivers should not use "pci_dev->resource[i]"?  If
so, I agree that it would be great if we had an accessor for BARs, but
it seems impractical to change all the drivers that use the current
style.

The number of places that actually look at *non-BAR* pci_dev
resources, e.g., the places that look at bridge windows and SR-IOV
BARs, should be pretty small, and it seems reasonable to change them.

> b) The type of a resource has to be determined based on some
>    information internal to the resource; possibly a flag,
>    instead of the relative position of the resource in some array.

Yes, I agree.

Bjorn

Re: [PATCH v3 03/27] PCI: pci resource iterator

[Ram Pai]

On Wed, Apr 10, 2013 at 09:22:48AM -0600, Bjorn Helgaas wrote:
> On Mon, Apr 8, 2013 at 10:51 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> > On Thu, Apr 04, 2013 at 04:18:01PM -0600, Bjorn Helgaas wrote:
> >> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> > From: Ram Pai <linuxram@us.ibm.com>
> >> >
> >> > Currently pci_dev structure holds an array of 17 PCI resources; six base
> >> > BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
> >> > A bridge device just needs the 4 bridge resources. A non-bridge device
> >> > just needs the six base resources and one ROM resource. The sriov
> >> > resources are needed only if the device has SRIOV capability.
> >> >
> >> > The pci_dev structure needs to be re-organized to avoid unnecessary
> >> > bloating.  However too much code outside the pci-bus driver, assumes the
> >> > internal details of the pci_dev structure, thus making it hard to
> >> > re-organize the datastructure.
> >> >
> >> > As a first step this patch provides generic methods to access the
> >> > resource structure of the pci_dev.
> >> >
> >> > Finally we can re-organize the resource structure in the pci_dev
> >> > structure and correspondingly update the methods.
> >> >
> >> > -v2: Consolidated iterator interface as per Bjorn's suggestion.
> >> > -v3: Add the idx back - Yinghai Lu
> >> > -v7: Change to use bitmap for searching - Yinghai Lu
> >> > -v8: Fix acpiphp module compiling error that is found by
> >> >         Steven Newbury <steve@snewbury.org.uk> - Yinghai Lu
> >> >
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> > ---
> >> >  drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  include/linux/pci.h |   24 ++++++++++++++++++++++++
> >> >  2 files changed, 72 insertions(+)
> >> >
> >> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> >> > index 1df75f7..ac751a6 100644
> >> > --- a/drivers/pci/probe.c
> >> > +++ b/drivers/pci/probe.c
> >> > @@ -123,6 +123,54 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
> >> >         return -1;
> >> >  }
> >> >
> >> > +static void __init_res_idx_mask(unsigned long *mask, int flag)
> >> > +{
> >> > +       bitmap_zero(mask, PCI_NUM_RESOURCES);
> >> > +       if (flag & PCI_STD_RES)
> >> > +               bitmap_set(mask, PCI_STD_RESOURCES,
> >> > +                       PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1);
> >> > +       if (flag & PCI_ROM_RES)
> >> > +               bitmap_set(mask, PCI_ROM_RESOURCE, 1);
> >> > +#ifdef CONFIG_PCI_IOV
> >> > +       if (flag & PCI_IOV_RES)
> >> > +               bitmap_set(mask, PCI_IOV_RESOURCES,
> >> > +                       PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1);
> >> > +#endif
> >> > +       if (flag & PCI_BRIDGE_RES)
> >> > +               bitmap_set(mask, PCI_BRIDGE_RESOURCES,
> >> > +                       PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1);
> >> > +}
> >> > +
> >> > +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES);
> >> > +static int __init pci_res_idx_mask_init(void)
> >> > +{
> >> > +       int i;
> >> > +
> >> > +       for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++)
> >> > +               __init_res_idx_mask(res_idx_mask[i], i);
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +postcore_initcall(pci_res_idx_mask_init);
> >> > +
> >> > +static inline unsigned long *get_res_idx_mask(int flag)
> >> > +{
> >> > +       return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)];
> >> > +}
> >> > +
> >> > +int pci_next_resource_idx(int i, int flag)
> >> > +{
> >> > +       i++;
> >> > +       if (i < PCI_NUM_RESOURCES)
> >> > +               i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i);
> >> > +
> >> > +       if (i < PCI_NUM_RESOURCES)
> >> > +               return i;
> >> > +
> >> > +       return -1;
> >> > +}
> >> > +EXPORT_SYMBOL(pci_next_resource_idx);
> >> > +
> >> >  static u64 pci_size(u64 base, u64 maxbase, u64 mask)
> >> >  {
> >> >         u64 size = mask & maxbase;      /* Find the significant bits */
> >> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> > index aefff8b..127a856 100644
> >> > --- a/include/linux/pci.h
> >> > +++ b/include/linux/pci.h
> >> > @@ -341,6 +341,30 @@ struct pci_dev {
> >> >  struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
> >> >  int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
> >> >
> >> > +#define PCI_STD_RES            (1<<0)
> >> > +#define PCI_ROM_RES            (1<<1)
> >> > +#define PCI_IOV_RES            (1<<2)
> >> > +#define PCI_BRIDGE_RES         (1<<3)
> >> > +#define PCI_RES_BLOCK_NUM      4
> >> > +
> >> > +#define PCI_ALL_RES            (PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES)
> >> > +#define PCI_NOSTD_RES          (PCI_ALL_RES & ~PCI_STD_RES)
> >> > +#define PCI_NOIOV_RES          (PCI_ALL_RES & ~PCI_IOV_RES)
> >> > +#define PCI_NOROM_RES          (PCI_ALL_RES & ~PCI_ROM_RES)
> >> > +#define PCI_NOBRIDGE_RES       (PCI_ALL_RES & ~PCI_BRIDGE_RES)
> >> > +#define PCI_STD_ROM_RES                (PCI_STD_RES | PCI_ROM_RES)
> >> > +#define PCI_STD_IOV_RES                (PCI_STD_RES | PCI_IOV_RES)
> >> > +#define PCI_STD_ROM_IOV_RES    (PCI_STD_RES | PCI_ROM_RES | PCI_IOV_RES)
> >> > +
> >> > +int pci_next_resource_idx(int i, int flag);
> >> > +
> >> > +#define for_each_pci_resource(dev, res, i, flag)       \
> >> > +       for (i = pci_next_resource_idx(-1, flag),       \
> >> > +               res = pci_dev_resource_n(dev, i);       \
> >> > +            res;                                       \
> >> > +            i = pci_next_resource_idx(i, flag),        \
> >> > +               res = pci_dev_resource_n(dev, i))
> >> > +
> >> >  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> >> >  {
> >> >  #ifdef CONFIG_PCI_IOV
> >>
> >> I think this turned out to be a disaster, with all the bitmaps and
> >> helper functions.  Filtering in the bodies of the
> >> for_each_pci_resource() users has *got* to be better.  That probably
> >> requires a wrapper struct around the struct resource.  Or possibly
> >> having a wrapper struct with a "type" or "flags" field would make
> >> filtering in for_each_pci_resources() itself cleaner to implement.
> >
> > I agree. There are two cleanups needed.
> >
> > a) pci drivers should not assume the internal organization of the
> >         resources in the struct pci_dev.
> 
> Do you mean that drivers should not use "pci_dev->resource[i]"?  If
> so, I agree that it would be great if we had an accessor for BARs, but
> it seems impractical to change all the drivers that use the current
> style.

Sorry for the delay. Was vacationing.  I mean, we cannot let drivers
assume anything about the how the resources are organized.

The only thing the drivers should know is that there are 6 normal
resources, 4 bridge resources, 1 ROM resource and 6 iov resources.

Currently the drivers assume that ROM resource follows normal resources
followed by IOV followed by bridge. These assumptions are making it hard
to re-organize the layout of resources in struct pci_dev.

I think we need to expose the following interfaces to drivers.

a) return the nth normal resource
b) return the nth iov resource
c) return the rom resource
d) return the nth bridge resource
e) return the type and index of a given resource, where 'index' is
	the index w.r.t to that resource type; not w.r.t to all
	the resources of the device.
f) ability to loop through all resources of the given type/types.

Everything else needs to be hidden.

> 
> The number of places that actually look at *non-BAR* pci_dev
> resources, e.g., the places that look at bridge windows and SR-IOV
> BARs, should be pretty small, and it seems reasonable to change them.

Unfortunately, the bridge windows and SRIOV windows are currently 
inter-twined with normal windows. Just cleaning up that code still
leaves some ugly hacks around.

> 
> > b) The type of a resource has to be determined based on some
> >    information internal to the resource; possibly a flag,
> >    instead of the relative position of the resource in some array.
> 
> Yes, I agree.
> 
> Bjorn

-- 
Ram Pai

Re: [PATCH v3 03/27] PCI: pci resource iterator

[Bjorn Helgaas]

On Wed, Apr 24, 2013 at 9:55 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Wed, Apr 10, 2013 at 09:22:48AM -0600, Bjorn Helgaas wrote:
>> On Mon, Apr 8, 2013 at 10:51 PM, Ram Pai <linuxram@us.ibm.com> wrote:
>> > On Thu, Apr 04, 2013 at 04:18:01PM -0600, Bjorn Helgaas wrote:
>> >> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> >> > From: Ram Pai <linuxram@us.ibm.com>
>> >> >
>> >> > Currently pci_dev structure holds an array of 17 PCI resources; six base
>> >> > BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
>> >> > A bridge device just needs the 4 bridge resources. A non-bridge device
>> >> > just needs the six base resources and one ROM resource. The sriov
>> >> > resources are needed only if the device has SRIOV capability.
>> >> >
...
>> > I agree. There are two cleanups needed.
>> >
>> > a) pci drivers should not assume the internal organization of the
>> >         resources in the struct pci_dev.
>>
>> Do you mean that drivers should not use "pci_dev->resource[i]"?  If
>> so, I agree that it would be great if we had an accessor for BARs, but
>> it seems impractical to change all the drivers that use the current
>> style.
>
> Sorry for the delay. Was vacationing.  I mean, we cannot let drivers
> assume anything about the how the resources are organized.
>
> The only thing the drivers should know is that there are 6 normal
> resources, 4 bridge resources, 1 ROM resource and 6 iov resources.
>
> Currently the drivers assume that ROM resource follows normal resources
> followed by IOV followed by bridge. These assumptions are making it hard
> to re-organize the layout of resources in struct pci_dev.
>
> I think we need to expose the following interfaces to drivers.
>
> a) return the nth normal resource

I think this needs to remain "pci_dev->resource[n]", because so many
drivers do this that it would be impractical to change them all.

> b) return the nth iov resource

I could imagine a new interface for this, given that I only see a
dozen SR-IOV drivers in the tree.  There might be a few out-of-tree,
but there probably aren't many.

> c) return the rom resource

There are only about 30 drivers in the tree that reference
PCI_ROM_RESOURCE.  Fewer than I expected, but I'd still be hesitant
about make "pci_dev->resource[PCI_ROM_RESOURCE]" stop working.

> d) return the nth bridge resource

I think it's reasonable to have a new interface for this because
bridges are handled almost entirely in the PCI core and architecture
code, and I doubt there are many, if any, drivers that care.

> e) return the type and index of a given resource, where 'index' is
>         the index w.r.t to that resource type; not w.r.t to all
>         the resources of the device.
> f) ability to loop through all resources of the given type/types.

We do loop through resources in the core when we're assigning, fixing
up, etc., and that makes some sense to me.  But I actually don't see
the use case for *drivers* to loop through resources.  All a driver
knows is "BAR X means Y", and it generally doesn't need to iterate and
do something generic to all of them.

> Everything else needs to be hidden.

Bjorn

Re: [PATCH v3 03/27] PCI: pci resource iterator

[Ram Pai]

On Thu, Apr 25, 2013 at 11:22:59AM -0600, Bjorn Helgaas wrote:
> On Wed, Apr 24, 2013 at 9:55 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> > On Wed, Apr 10, 2013 at 09:22:48AM -0600, Bjorn Helgaas wrote:
> >> On Mon, Apr 8, 2013 at 10:51 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> >> > On Thu, Apr 04, 2013 at 04:18:01PM -0600, Bjorn Helgaas wrote:
> >> >> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> >> > From: Ram Pai <linuxram@us.ibm.com>
> >> >> >
> >> >> > Currently pci_dev structure holds an array of 17 PCI resources; six base
> >> >> > BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
> >> >> > A bridge device just needs the 4 bridge resources. A non-bridge device
> >> >> > just needs the six base resources and one ROM resource. The sriov
> >> >> > resources are needed only if the device has SRIOV capability.
> >> >> >
> ...
> >> > I agree. There are two cleanups needed.
> >> >
> >> > a) pci drivers should not assume the internal organization of the
> >> >         resources in the struct pci_dev.
> >>
> >> Do you mean that drivers should not use "pci_dev->resource[i]"?  If
> >> so, I agree that it would be great if we had an accessor for BARs, but
> >> it seems impractical to change all the drivers that use the current
> >> style.
> >
> > Sorry for the delay. Was vacationing.  I mean, we cannot let drivers
> > assume anything about the how the resources are organized.
> >
> > The only thing the drivers should know is that there are 6 normal
> > resources, 4 bridge resources, 1 ROM resource and 6 iov resources.
> >
> > Currently the drivers assume that ROM resource follows normal resources
> > followed by IOV followed by bridge. These assumptions are making it hard
> > to re-organize the layout of resources in struct pci_dev.
> >
> > I think we need to expose the following interfaces to drivers.
> >
> > a) return the nth normal resource
> 
> I think this needs to remain "pci_dev->resource[n]", because so many
> drivers do this that it would be impractical to change them all.

Scanning through the entire kernel tree, I did find about 40 different
drivers that are accessing pci_dev->resource[n]. These drivers can
be changed to use the newer interface. Out-of-tree drivers can continue
to access it directly, but they will break, when the
datastructure is eventually re-organized.

I was thinking of a interface something like

pci_get_std_resource(dev,i) which is implemented internally as

#define pci_get_std_resource(dev,i) dev->resource[i]

> 
> > b) return the nth iov resource
> 
> I could imagine a new interface for this, given that I only see a
> dozen SR-IOV drivers in the tree.  There might be a few out-of-tree,
> but there probably aren't many.
> 
> > c) return the rom resource
> 
> There are only about 30 drivers in the tree that reference
> PCI_ROM_RESOURCE.  Fewer than I expected, but I'd still be hesitant
> about make "pci_dev->resource[PCI_ROM_RESOURCE]" stop working.

It will work till someday when the datastructure is re-organized.

Again the interface will be something like

pci_get_rom_resource(dev) which is implemented internally as

#define pci_get_std_resource(dev)  pci_dev->resource[PCI_ROM_RESOURCE]

> 
> > d) return the nth bridge resource
> 
> I think it's reasonable to have a new interface for this because
> bridges are handled almost entirely in the PCI core and architecture
> code, and I doubt there are many, if any, drivers that care.
> 
> > e) return the type and index of a given resource, where 'index' is
> >         the index w.r.t to that resource type; not w.r.t to all
> >         the resources of the device.
> > f) ability to loop through all resources of the given type/types.
> 
> We do loop through resources in the core when we're assigning, fixing
> up, etc., and that makes some sense to me.  But I actually don't see
> the use case for *drivers* to loop through resources.  All a driver
> knows is "BAR X means Y", and it generally doesn't need to iterate and
> do something generic to all of them.

Yes mostly true. However I have seen a couple of drivers looping through
the resources. An examples is ..

yenta_free_resources()

RP

Re: [PATCH v3 03/27] PCI: pci resource iterator

[Yinghai Lu]

On Thu, Apr 4, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> From: Ram Pai <linuxram@us.ibm.com>
>>
>> Currently pci_dev structure holds an array of 17 PCI resources; six base
>> BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
>> A bridge device just needs the 4 bridge resources. A non-bridge device
>> just needs the six base resources and one ROM resource. The sriov
>> resources are needed only if the device has SRIOV capability.
>>
>> The pci_dev structure needs to be re-organized to avoid unnecessary
>> bloating.  However too much code outside the pci-bus driver, assumes the
>> internal details of the pci_dev structure, thus making it hard to
>> re-organize the datastructure.
>>
>> As a first step this patch provides generic methods to access the
>> resource structure of the pci_dev.
>>
>> Finally we can re-organize the resource structure in the pci_dev
>> structure and correspondingly update the methods.
>>
>> -v2: Consolidated iterator interface as per Bjorn's suggestion.
>> -v3: Add the idx back - Yinghai Lu
>> -v7: Change to use bitmap for searching - Yinghai Lu
>> -v8: Fix acpiphp module compiling error that is found by
>>         Steven Newbury <steve@snewbury.org.uk> - Yinghai Lu
>>
>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pci.h |   24 ++++++++++++++++++++++++
>>  2 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 1df75f7..ac751a6 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -123,6 +123,54 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
>>         return -1;
>>  }
>>
>> +static void __init_res_idx_mask(unsigned long *mask, int flag)
>> +{
>> +       bitmap_zero(mask, PCI_NUM_RESOURCES);
>> +       if (flag & PCI_STD_RES)
>> +               bitmap_set(mask, PCI_STD_RESOURCES,
>> +                       PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1);
>> +       if (flag & PCI_ROM_RES)
>> +               bitmap_set(mask, PCI_ROM_RESOURCE, 1);
>> +#ifdef CONFIG_PCI_IOV
>> +       if (flag & PCI_IOV_RES)
>> +               bitmap_set(mask, PCI_IOV_RESOURCES,
>> +                       PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1);
>> +#endif
>> +       if (flag & PCI_BRIDGE_RES)
>> +               bitmap_set(mask, PCI_BRIDGE_RESOURCES,
>> +                       PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1);
>> +}
>> +
>> +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES);
>> +static int __init pci_res_idx_mask_init(void)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++)
>> +               __init_res_idx_mask(res_idx_mask[i], i);
>> +
>> +       return 0;
>> +}
>> +postcore_initcall(pci_res_idx_mask_init);
>> +
>> +static inline unsigned long *get_res_idx_mask(int flag)
>> +{
>> +       return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)];
>> +}
>> +
>> +int pci_next_resource_idx(int i, int flag)
>> +{
>> +       i++;
>> +       if (i < PCI_NUM_RESOURCES)
>> +               i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i);
>> +
>> +       if (i < PCI_NUM_RESOURCES)
>> +               return i;
>> +
>> +       return -1;
>> +}
>> +EXPORT_SYMBOL(pci_next_resource_idx);
>> +
>>  static u64 pci_size(u64 base, u64 maxbase, u64 mask)
>>  {
>>         u64 size = mask & maxbase;      /* Find the significant bits */
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index aefff8b..127a856 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -341,6 +341,30 @@ struct pci_dev {
>>  struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
>>  int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
>>
>> +#define PCI_STD_RES            (1<<0)
>> +#define PCI_ROM_RES            (1<<1)
>> +#define PCI_IOV_RES            (1<<2)
>> +#define PCI_BRIDGE_RES         (1<<3)
>> +#define PCI_RES_BLOCK_NUM      4
>> +
>> +#define PCI_ALL_RES            (PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES)
>> +#define PCI_NOSTD_RES          (PCI_ALL_RES & ~PCI_STD_RES)
>> +#define PCI_NOIOV_RES          (PCI_ALL_RES & ~PCI_IOV_RES)
>> +#define PCI_NOROM_RES          (PCI_ALL_RES & ~PCI_ROM_RES)
>> +#define PCI_NOBRIDGE_RES       (PCI_ALL_RES & ~PCI_BRIDGE_RES)
>> +#define PCI_STD_ROM_RES                (PCI_STD_RES | PCI_ROM_RES)
>> +#define PCI_STD_IOV_RES                (PCI_STD_RES | PCI_IOV_RES)
>> +#define PCI_STD_ROM_IOV_RES    (PCI_STD_RES | PCI_ROM_RES | PCI_IOV_RES)
>> +
>> +int pci_next_resource_idx(int i, int flag);
>> +
>> +#define for_each_pci_resource(dev, res, i, flag)       \
>> +       for (i = pci_next_resource_idx(-1, flag),       \
>> +               res = pci_dev_resource_n(dev, i);       \
>> +            res;                                       \
>> +            i = pci_next_resource_idx(i, flag),        \
>> +               res = pci_dev_resource_n(dev, i))
>> +
>>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>>  {
>>  #ifdef CONFIG_PCI_IOV
>
> I think this turned out to be a disaster, with all the bitmaps and
> helper functions.  Filtering in the bodies of the
> for_each_pci_resource() users has *got* to be better.  That probably
> requires a wrapper struct around the struct resource.  Or possibly
> having a wrapper struct with a "type" or "flags" field would make
> filtering in for_each_pci_resources() itself cleaner to implement.

Do you mean have extra struct that will wrapper resource ?
and the struct will have type that will tell that resource is PCI bridge
resource or other std/sriov resource?

then that struct need to have BAR in it.

that looks like addon_res that is added in this patchset.

but for the std/bridge/sriov, idx is used all the way and in all over
the drivers.

>
> I think it would also help if we got rid of all the "PCI_NO*_RES"
> definitions and just had the users explicitly specify what they *do*
> want.

yes, remove PCI_NO*_RES should be more clean

Thanks

Yinghai

[PATCH v3 04/27] PCI: Add is_pci_*_resource_idx() helpers

[Yinghai Lu]

According to resource pointer find out if the resource is some type resource
like bridge, sriov, or std.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 include/linux/pci.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 127a856..efb348b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -92,6 +92,29 @@ enum {
 	DEVICE_COUNT_RESOURCE = PCI_NUM_RESOURCES,
 };
 
+static inline bool is_pci_std_resource_idx(int i)
+{
+	return i >= PCI_STD_RESOURCES && i <= PCI_STD_RESOURCE_END;
+}
+
+static inline bool is_pci_rom_resource_idx(int i)
+{
+	return i == PCI_ROM_RESOURCE;
+}
+
+static inline bool is_pci_iov_resource_idx(int i)
+{
+#ifdef CONFIG_PCI_IOV
+	return i >= PCI_IOV_RESOURCES && i <= PCI_IOV_RESOURCE_END;
+#endif
+	return false;
+}
+
+static inline bool is_pci_bridge_resource_idx(int i)
+{
+	return i >= PCI_BRIDGE_RESOURCES && i <= PCI_BRIDGE_RESOURCE_END;
+}
+
 typedef int __bitwise pci_power_t;
 
 #define PCI_D0		((pci_power_t __force) 0)
-- 
1.7.10.4

Re: [PATCH v3 04/27] PCI: Add is_pci_*_resource_idx() helpers

[Bjorn Helgaas]

On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> According to resource pointer find out if the resource is some type resource
> like bridge, sriov, or std.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  include/linux/pci.h |   23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 127a856..efb348b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -92,6 +92,29 @@ enum {
>         DEVICE_COUNT_RESOURCE = PCI_NUM_RESOURCES,
>  };
>
> +static inline bool is_pci_std_resource_idx(int i)
> +{
> +       return i >= PCI_STD_RESOURCES && i <= PCI_STD_RESOURCE_END;
> +}
> +
> +static inline bool is_pci_rom_resource_idx(int i)
> +{
> +       return i == PCI_ROM_RESOURCE;
> +}
> +
> +static inline bool is_pci_iov_resource_idx(int i)
> +{
> +#ifdef CONFIG_PCI_IOV
> +       return i >= PCI_IOV_RESOURCES && i <= PCI_IOV_RESOURCE_END;
> +#endif
> +       return false;
> +}
> +
> +static inline bool is_pci_bridge_resource_idx(int i)
> +{
> +       return i >= PCI_BRIDGE_RESOURCES && i <= PCI_BRIDGE_RESOURCE_END;
> +}
> +
>  typedef int __bitwise pci_power_t;
>
>  #define PCI_D0         ((pci_power_t __force) 0)

1) I don't like adding more "_idx()" interfaces.

2) The only one of these that's even used is "is_pci_bridge_resource_idx()"

3) I think adding a wrapper struct with a "type" or "flags" field
would make this trivial, e.g., "pres->flags & PCI_RESOURCE_WINDOW" or
something.

[PATCH v3 05/27] PCI: Update pci_resource_start etc to use pci_dev_resource_n()

[Yinghai Lu]

Now pci_resource_start is using dev->resource[n] directly.
Replace it with pci_dev_resource[n] to prepare for addon resource support.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 include/linux/pci.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index efb348b..94935fc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1488,9 +1488,9 @@ static inline struct pci_dev *pci_dev_get(struct pci_dev *dev)
 
 /* these helpers provide future and backwards compatibility
  * for accessing popular PCI BAR info */
-#define pci_resource_start(dev, bar)	((dev)->resource[(bar)].start)
-#define pci_resource_end(dev, bar)	((dev)->resource[(bar)].end)
-#define pci_resource_flags(dev, bar)	((dev)->resource[(bar)].flags)
+#define pci_resource_start(dev, bar)	(pci_dev_resource_n(dev, (bar))->start)
+#define pci_resource_end(dev, bar)	(pci_dev_resource_n(dev, (bar))->end)
+#define pci_resource_flags(dev, bar)	(pci_dev_resource_n(dev, (bar))->flags)
 #define pci_resource_len(dev,bar) \
 	((pci_resource_start((dev), (bar)) == 0 &&	\
 	  pci_resource_end((dev), (bar)) ==		\
-- 
1.7.10.4

[PATCH v3 06/27] PCI, x86: Use for_each_pci_resource() with pci_allocate_bridge_resources

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 arch/x86/pci/i386.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 94919e3..7c3395a 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -206,8 +206,7 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
 	int idx;
 	struct resource *r;
 
-	for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_NUM_RESOURCES; idx++) {
-		r = &dev->resource[idx];
+	for_each_pci_resource(dev, r, idx, PCI_BRIDGE_RES) {
 		if (!r->flags)
 			continue;
 		if (!r->start || pci_claim_resource(dev, idx) < 0) {
-- 
1.7.10.4

[PATCH v3 07/27] PCI, x86: Use for_each_pci_resource() with pci_allocate_dev_resources

[Yinghai Lu]

We can replace two layer loop with for_each_pci_resource(), and make
the code more readable.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: x86@kernel.org
---
 arch/x86/pci/i386.c |   55 +++++++++++++++++++--------------------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 7c3395a..ed219c7 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -233,49 +233,34 @@ static void pcibios_allocate_bus_resources(struct pci_bus *bus)
 		pcibios_allocate_bus_resources(child);
 }
 
-struct pci_check_idx_range {
-	int start;
-	int end;
-};
-
 static void pcibios_allocate_dev_resources(struct pci_dev *dev, int pass)
 {
-	int idx, disabled, i;
+	int idx, disabled;
 	u16 command;
 	struct resource *r;
 
-	struct pci_check_idx_range idx_range[] = {
-		{ PCI_STD_RESOURCES, PCI_STD_RESOURCE_END },
-#ifdef CONFIG_PCI_IOV
-		{ PCI_IOV_RESOURCES, PCI_IOV_RESOURCE_END },
-#endif
-	};
-
 	pci_read_config_word(dev, PCI_COMMAND, &command);
-	for (i = 0; i < ARRAY_SIZE(idx_range); i++)
-		for (idx = idx_range[i].start; idx <= idx_range[i].end; idx++) {
-			r = &dev->resource[idx];
-			if (r->parent)	/* Already allocated */
-				continue;
-			if (!r->start)	/* Address not assigned at all */
-				continue;
-			if (r->flags & IORESOURCE_IO)
-				disabled = !(command & PCI_COMMAND_IO);
-			else
-				disabled = !(command & PCI_COMMAND_MEMORY);
-			if (pass == disabled) {
-				dev_dbg(&dev->dev,
-					"BAR %d: reserving %pr (d=%d, p=%d)\n",
-					idx, r, disabled, pass);
-				if (pci_claim_resource(dev, idx) < 0) {
-					/* We'll assign a new address later */
-					pcibios_save_fw_addr(dev,
-							idx, r->start);
-					r->end -= r->start;
-					r->start = 0;
-				}
+	for_each_pci_resource(dev, r, idx, PCI_NOROM_RES & ~PCI_BRIDGE_RES) {
+		if (r->parent)	/* Already allocated */
+			continue;
+		if (!r->start)	/* Address not assigned at all */
+			continue;
+		if (r->flags & IORESOURCE_IO)
+			disabled = !(command & PCI_COMMAND_IO);
+		else
+			disabled = !(command & PCI_COMMAND_MEMORY);
+		if (pass == disabled) {
+			dev_dbg(&dev->dev,
+				"BAR %d: reserving %pr (d=%d, p=%d)\n",
+				idx, r, disabled, pass);
+			if (pci_claim_resource(dev, idx) < 0) {
+				/* We'll assign a new address later */
+				pcibios_save_fw_addr(dev, idx, r->start);
+				r->end -= r->start;
+				r->start = 0;
 			}
 		}
+	}
 	if (!pass) {
 		r = &dev->resource[PCI_ROM_RESOURCE];
 		if (r->flags & IORESOURCE_ROM_ENABLE) {
-- 
1.7.10.4

[PATCH v3 08/27] PCI: Use for_each_pci_resource() with IOV releated functions

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/iov.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index ee599f2..baf6c80 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -93,17 +93,20 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
 	pci_setup_device(virtfn);
 	virtfn->dev.parent = dev->dev.parent;
 
-	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		res = dev->resource + PCI_IOV_RESOURCES + i;
+	for_each_pci_resource(dev, res, i, PCI_IOV_RES) {
+		struct resource *virtfn_res;
+
 		if (!res->parent)
 			continue;
-		virtfn->resource[i].name = pci_name(virtfn);
-		virtfn->resource[i].flags = res->flags;
+
+		virtfn_res = pci_dev_resource_n(virtfn, i - PCI_IOV_RESOURCES);
+		virtfn_res->name = pci_name(virtfn);
+		virtfn_res->flags = res->flags;
 		size = resource_size(res);
 		do_div(size, iov->total_VFs);
-		virtfn->resource[i].start = res->start + size * id;
-		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
-		rc = request_resource(res, &virtfn->resource[i]);
+		virtfn_res->start = res->start + size * id;
+		virtfn_res->end = virtfn_res->start + size - 1;
+		rc = request_resource(res, virtfn_res);
 		BUG_ON(rc);
 	}
 
@@ -305,9 +308,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 		return -EIO;
 
 	nres = 0;
-	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		bars |= (1 << (i + PCI_IOV_RESOURCES));
-		res = dev->resource + PCI_IOV_RESOURCES + i;
+	for_each_pci_resource(dev, res, i, PCI_IOV_RES) {
+		bars |= 1 << i;
 		if (res->parent)
 			nres++;
 	}
@@ -465,10 +467,9 @@ found:
 	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
 
 	nres = 0;
-	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		res = dev->resource + PCI_IOV_RESOURCES + i;
+	for_each_pci_resource(dev, res, i, PCI_IOV_RES) {
 		i += __pci_read_base(dev, pci_bar_unknown, res,
-				     pos + PCI_SRIOV_BAR + i * 4);
+			     pos + PCI_SRIOV_BAR + (i - PCI_IOV_RESOURCES) * 4);
 		if (!res->flags)
 			continue;
 		if (resource_size(res) & (PAGE_SIZE - 1)) {
@@ -511,10 +512,8 @@ found:
 	return 0;
 
 failed:
-	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		res = dev->resource + PCI_IOV_RESOURCES + i;
+	for_each_pci_resource(dev, res, i, PCI_IOV_RES)
 		res->flags = 0;
-	}
 
 	return rc;
 }
-- 
1.7.10.4

[PATCH v3 09/27] PCI, acpiphp: Use for_each_pci_resource() helper

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 270fdba..b37d54b 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -1085,10 +1085,10 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus)
 	struct pci_dev *dev;
 	int i;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM;
+	struct resource *res;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
-		for (i=0; i<PCI_BRIDGE_RESOURCES; i++) {
-			struct resource *res = &dev->resource[i];
+		for_each_pci_resource(dev, res, i, PCI_NOBRIDGE_RES) {
 			if ((res->flags & type_mask) && !res->start &&
 					res->end) {
 				/* Could not assign a required resources
-- 
1.7.10.4

[PATCH v3 10/27] PCI, pciehp: Use for_each_pci_resource() helper

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Cc: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 5127f3f..2df1ae3 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -807,6 +807,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
 	int i;
 	u16 reg16;
 	struct pci_dev *pdev = ctrl->pcie->port;
+	struct resource *res;
 
 	if (!pciehp_debug)
 		return;
@@ -822,11 +823,11 @@ static inline void dbg_ctrl(struct controller *ctrl)
 		  pdev->subsystem_vendor);
 	ctrl_info(ctrl, "  PCIe Cap offset      : 0x%02x\n",
 		  pci_pcie_cap(pdev));
-	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+	for_each_pci_resource(pdev, res, i, PCI_ALL_RES) {
 		if (!pci_resource_len(pdev, i))
 			continue;
 		ctrl_info(ctrl, "  PCI resource [%d]     : %pR\n",
-			  i, &pdev->resource[i]);
+			  i, res);
 	}
 	ctrl_info(ctrl, "Slot Capabilities      : 0x%08x\n", ctrl->slot_cap);
 	ctrl_info(ctrl, "  Physical Slot Number : %d\n", PSN(ctrl));
-- 
1.7.10.4

[PATCH v3 11/27] PCI: Use for_each_pci_resource() in pci_enable_dev

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b099e00..c0473dc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -440,8 +440,9 @@ static void
 pci_restore_bars(struct pci_dev *dev)
 {
 	int i;
+	struct resource *res;
 
-	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
+	for_each_pci_resource(dev, res, i, PCI_NOBRIDGE_RES)
 		pci_update_resource(dev, i);
 }
 
@@ -1153,6 +1154,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
 {
 	int err;
 	int i, bars = 0;
+	struct resource *res;
 
 	/*
 	 * Power state could be unknown at this point, either due to a fresh
@@ -1170,12 +1172,11 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
 		return 0;		/* already enabled */
 
 	/* only skip sriov related */
-	for (i = 0; i <= PCI_ROM_RESOURCE; i++)
-		if (dev->resource[i].flags & flags)
-			bars |= (1 << i);
-	for (i = PCI_BRIDGE_RESOURCES; i < DEVICE_COUNT_RESOURCE; i++)
-		if (dev->resource[i].flags & flags)
+	for_each_pci_resource(dev, res, i, PCI_NOIOV_RES) {
+		/* TODO: check i with bits of bars */
+		if (res->flags & flags)
 			bars |= (1 << i);
+	}
 
 	err = do_pci_enable_device(dev, bars);
 	if (err < 0)
@@ -2557,7 +2558,7 @@ static int __pci_request_region(struct pci_dev *pdev, int bar, const char *res_n
 
 err_out:
 	dev_warn(&pdev->dev, "BAR %d: can't reserve %pR\n", bar,
-		 &pdev->resource[bar]);
+		 pci_dev_resource_n(pdev, bar));
 	return -EBUSY;
 }
 
-- 
1.7.10.4

[PATCH v3 12/27] PCI: Use for_each_pci_resource() in pci_reassigndev

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c0473dc..dca6c1a1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3782,7 +3782,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	command &= ~PCI_COMMAND_MEMORY;
 	pci_write_config_word(dev, PCI_COMMAND, command);
 
-	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
+	for_each_pci_resource(dev, r, i, PCI_NOBRIDGE_RES) {
 		r = &dev->resource[i];
 		if (!(r->flags & IORESOURCE_MEM))
 			continue;
@@ -3802,8 +3802,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	 */
 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
 	    (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
-		for (i = PCI_BRIDGE_RESOURCES; i < PCI_NUM_RESOURCES; i++) {
-			r = &dev->resource[i];
+		for_each_pci_resource(dev, r, i, PCI_BRIDGE_RES) {
 			if (!(r->flags & IORESOURCE_MEM))
 				continue;
 			r->end = resource_size(r) - 1;
-- 
1.7.10.4

[PATCH v3 13/27] PCI: Use for_each_pci_resource() with pci bar reassign funcs

[Yinghai Lu]

Replace those open code, and make code more readable.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 7e8739e..b23ef4c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -106,7 +106,7 @@ static resource_size_t get_res_add_size(struct list_head *head,
 
 	list_for_each_entry(dev_res, head, list) {
 		if (dev_res->res == res) {
-			int idx = res - &dev_res->dev->resource[0];
+			int idx = pci_dev_resource_idx(dev_res->dev, res);
 
 			dev_printk(KERN_DEBUG, &dev_res->dev->dev,
 				 "res[%d]=%pR get_res_add_size add_size %llx\n",
@@ -124,15 +124,13 @@ static resource_size_t get_res_add_size(struct list_head *head,
 static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
 {
 	int i;
+	struct resource *r;
 
-	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		struct resource *r;
+	for_each_pci_resource(dev, r, i, PCI_ALL_RES) {
 		struct pci_dev_resource *dev_res, *tmp;
 		resource_size_t r_align;
 		struct list_head *n;
 
-		r = &dev->resource[i];
-
 		if (r->flags & IORESOURCE_PCI_FIXED)
 			continue;
 
@@ -237,7 +235,7 @@ static void reassign_resources_sorted(struct list_head *realloc_head,
 		if (!found_match)/* just skip */
 			continue;
 
-		idx = res - &add_res->dev->resource[0];
+		idx = pci_dev_resource_idx(add_res->dev, res);
 		add_size = add_res->add_size;
 		if (!resource_size(res)) {
 			res->start = add_res->start;
@@ -280,7 +278,7 @@ static void assign_requested_resources_sorted(struct list_head *head,
 
 	list_for_each_entry(dev_res, head, list) {
 		res = dev_res->res;
-		idx = res - &dev_res->dev->resource[0];
+		idx = pci_dev_resource_idx(dev_res->dev, res);
 		if (resource_size(res) &&
 		    pci_assign_resource(dev_res->dev, idx)) {
 			if (fail_head) {
@@ -757,9 +755,9 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
+		struct resource *r;
 
-		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-			struct resource *r = &dev->resource[i];
+		for_each_pci_resource(dev, r, i, PCI_ALL_RES) {
 			unsigned long r_size;
 
 			if (r->parent || !(r->flags & IORESOURCE_IO))
@@ -870,9 +868,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
+		struct resource *r;
 
-		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-			struct resource *r = &dev->resource[i];
+		for_each_pci_resource(dev, r, i, PCI_ALL_RES) {
 			resource_size_t r_size;
 
 			if (r->parent || (r->flags & mask) != type)
@@ -1196,9 +1194,7 @@ static void pci_bridge_release_resources(struct pci_bus *bus,
 				  IORESOURCE_PREFETCH;
 
 	dev = bus->self;
-	for (idx = PCI_BRIDGE_RESOURCES; idx <= PCI_BRIDGE_RESOURCE_END;
-	     idx++) {
-		r = &dev->resource[idx];
+	for_each_pci_resource(dev, r, idx, PCI_BRIDGE_RES) {
 		if ((r->flags & type_mask) != type)
 			continue;
 		if (!r->parent)
@@ -1369,9 +1365,9 @@ static void __init pci_realloc_detect(void)
 
 	for_each_pci_dev(dev) {
 		int i;
+		struct resource *r;
 
-		for (i = PCI_IOV_RESOURCES; i <= PCI_IOV_RESOURCE_END; i++) {
-			struct resource *r = &dev->resource[i];
+		for_each_pci_resource(dev, r, i, PCI_IOV_RES) {
 
 			/* Not assigned, or rejected by kernel ? */
 			if (r->flags && !r->start) {
-- 
1.7.10.4

[PATCH v3 14/27] PCI: Use for_each_pci_resource() in pci_assign_resource

[Yinghai Lu]

Also replace dev->resource[n] with pci_dev_resource_n().

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-res.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 81b88bd..94ef232 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -110,7 +110,7 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 
 int pci_claim_resource(struct pci_dev *dev, int resource)
 {
-	struct resource *res = &dev->resource[resource];
+	struct resource *res = pci_dev_resource_n(dev, resource);
 	struct resource *root, *conflict;
 
 	root = pci_find_parent_resource(dev, res);
@@ -200,7 +200,7 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 		int resno, resource_size_t size, resource_size_t align)
 {
-	struct resource *res = dev->resource + resno;
+	struct resource *res = pci_dev_resource_n(dev, resno);
 	resource_size_t min;
 	int ret;
 
@@ -227,7 +227,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 static int _pci_assign_resource(struct pci_dev *dev, int resno,
 				resource_size_t size, resource_size_t min_align)
 {
-	struct resource *res = dev->resource + resno;
+	struct resource *res = pci_dev_resource_n(dev, resno);
 	struct pci_bus *bus;
 	int ret;
 	char *type;
@@ -259,7 +259,7 @@ static int _pci_assign_resource(struct pci_dev *dev, int resno,
 
 int pci_assign_resource(struct pci_dev *dev, int resno)
 {
-	struct resource *res = dev->resource + resno;
+	struct resource *res = pci_dev_resource_n(dev, resno);
 	resource_size_t align, size;
 	struct pci_bus *bus;
 	int ret;
@@ -295,7 +295,7 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 int pci_reassign_resource(struct pci_dev *dev, int resno, resource_size_t addsize,
 			resource_size_t min_align)
 {
-	struct resource *res = dev->resource + resno;
+	struct resource *res = pci_dev_resource_n(dev, resno);
 	resource_size_t new_size;
 	int ret;
 
@@ -326,12 +326,10 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
 	pci_read_config_word(dev, PCI_COMMAND, &cmd);
 	old_cmd = cmd;
 
-	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+	for_each_pci_resource(dev, r, i, PCI_ALL_RES) {
 		if (!(mask & (1 << i)))
 			continue;
 
-		r = &dev->resource[i];
-
 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
 			continue;
 		if ((i == PCI_ROM_RESOURCE) &&
-- 
1.7.10.4

[PATCH v3 15/27] PCI, x86: Use for_each_pci_resource() with noassign_bars

[Yinghai Lu]

Replace those open code, and make code more readable.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: x86@kernel.org
Cc: Myron Stowe <myron.stowe@redhat.com>
---
 arch/x86/pci/common.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 901177d..fe85836 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -136,8 +136,7 @@ static void pcibios_fixup_device_resources(struct pci_dev *dev)
 		* resource so the kernel doesn't attmept to assign
 		* it later on in pci_assign_unassigned_resources
 		*/
-		for (bar = 0; bar <= PCI_STD_RESOURCE_END; bar++) {
-			bar_r = &dev->resource[bar];
+		for_each_pci_resource(dev, bar_r, bar, PCI_NOIOV_RES & ~PCI_BRIDGE_RES & ~PCI_ROM_RES) {
 			if (bar_r->start == 0 && bar_r->end != 0) {
 				bar_r->flags = 0;
 				bar_r->end = 0;
-- 
1.7.10.4

[PATCH v3 16/27] PCI: Use for_each_pci_resource() in pci_dev_driver()

[Yinghai Lu]

Replace those open code, and make code more readable.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-driver.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 1fa1e48..866149a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1170,9 +1170,11 @@ pci_dev_driver(const struct pci_dev *dev)
 	if (dev->driver)
 		return dev->driver;
 	else {
+		struct resource *res;
 		int i;
-		for(i=0; i<=PCI_ROM_RESOURCE; i++)
-			if (dev->resource[i].flags & IORESOURCE_BUSY)
+
+		for_each_pci_resource((struct pci_dev *)dev, res, i, PCI_NOIOV_RES & ~PCI_BRIDGE_RES)
+			if (res->flags & IORESOURCE_BUSY)
 				return &pci_compat_driver;
 	}
 	return NULL;
-- 
1.7.10.4

[PATCH v3 17/27] PCI: Use for_each_pci_resource() in pci resource release

[Yinghai Lu]

Replace those open code, and make code more readable.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/remove.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index cc875e6..af8ace9 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -6,15 +6,14 @@
 static void pci_free_resources(struct pci_dev *dev)
 {
 	int i;
+	struct resource *res;
 
  	msi_remove_pci_irq_vectors(dev);
 
 	pci_cleanup_rom(dev);
-	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		struct resource *res = dev->resource + i;
+	for_each_pci_resource(dev, res, i, PCI_ALL_RES)
 		if (res->parent)
 			release_resource(res);
-	}
 }
 
 static void pci_stop_dev(struct pci_dev *dev)
-- 
1.7.10.4

[PATCH v3 18/27] PCI: Use for_each_pci_resource() in pci bases reading

[Yinghai Lu]

Replace those open code, and make code more readable.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac751a6..263a575 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -354,9 +354,11 @@ out:
 static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 {
 	unsigned int pos, reg;
+	struct resource *res;
 
-	for (pos = 0; pos < howmany; pos++) {
-		struct resource *res = &dev->resource[pos];
+	for_each_pci_resource(dev, res, pos, PCI_STD_RES) {
+		if (pos >= howmany)
+			break;
 		reg = PCI_BASE_ADDRESS_0 + (pos << 2);
 		pos += __pci_read_base(dev, pci_bar_unknown, res, reg);
 	}
-- 
1.7.10.4

[PATCH v3 19/27] PCI, x86: Use for_each_pci_resource() with mrst

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: x86@kernel.org
Cc: Huang Ying <ying.huang@intel.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 arch/x86/pci/mrst.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/pci/mrst.c b/arch/x86/pci/mrst.c
index 6eb18c4..884ea69 100644
--- a/arch/x86/pci/mrst.c
+++ b/arch/x86/pci/mrst.c
@@ -280,6 +280,7 @@ static void pci_fixed_bar_fixup(struct pci_dev *dev)
 	unsigned long offset;
 	u32 size;
 	int i;
+	struct resource *res;
 
 	if (!pci_soc_mode)
 		return;
@@ -294,10 +295,10 @@ static void pci_fixed_bar_fixup(struct pci_dev *dev)
 	    PCI_DEVFN(2, 2) == dev->devfn)
 		return;
 
-	for (i = 0; i < PCI_ROM_RESOURCE; i++) {
+	for_each_pci_resource(dev, res, i, PCI_STD_RES) {
 		pci_read_config_dword(dev, offset + 8 + (i * 4), &size);
-		dev->resource[i].end = dev->resource[i].start + size - 1;
-		dev->resource[i].flags |= IORESOURCE_PCI_FIXED;
+		res->end = res->start + size - 1;
+		res->flags |= IORESOURCE_PCI_FIXED;
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_fixed_bar_fixup);
-- 
1.7.10.4

[PATCH v3 20/27] PCI, xen: Use for_each_pci_resource() with xen pci

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: virtualization@lists.linux-foundation.org
---
 drivers/pci/xen-pcifront.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 966abc6..c90835f 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -395,9 +395,7 @@ static int pcifront_claim_resource(struct pci_dev *dev, void *data)
 	int i;
 	struct resource *r;
 
-	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		r = &dev->resource[i];
-
+	for_each_pci_resource(dev, r, i, PCI_ALL_RES) {
 		if (!r->parent && r->start && r->flags) {
 			dev_info(&pdev->xdev->dev, "claiming resource %s/%d\n",
 				pci_name(dev), i);
-- 
1.7.10.4

Re: [PATCH v3 20/27] PCI, xen: Use for_each_pci_resource() with xen pci

[Konrad Rzeszutek Wilk]

On Wed, Mar 13, 2013 at 04:28:15PM -0700, Yinghai Lu wrote:
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


Reviewed-by: Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: virtualization@lists.linux-foundation.org
> ---
>  drivers/pci/xen-pcifront.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 966abc6..c90835f 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -395,9 +395,7 @@ static int pcifront_claim_resource(struct pci_dev *dev, void *data)
>  	int i;
>  	struct resource *r;
>  
> -	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> -		r = &dev->resource[i];
> -
> +	for_each_pci_resource(dev, r, i, PCI_ALL_RES) {
>  		if (!r->parent && r->start && r->flags) {
>  			dev_info(&pdev->xdev->dev, "claiming resource %s/%d\n",
>  				pci_name(dev), i);
> -- 
> 1.7.10.4
> 

[PATCH v3 21/27] PCI: Add addon_resource support for pci devices

[Yinghai Lu]

Now there is some non normal pci resources other than standard,rom,
sriov, bridges.
Some could be same as standard reg, but using different position.
Some could have own way to read/write to them.

Kernel are using different way to hack those resources like abusing
pci bridge resource spot on non bridge pci device.

Add addon_resources list in pci_dev for those non-standard resources.
With this patch, will treat addon-resource like standard resources with
special ops.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |   38 +++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h |   20 +++++++++++++++++++-
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 263a575..9e659c7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -107,22 +107,53 @@ postcore_initcall(pcibus_class_init);
 
 struct resource *pci_dev_resource_n(struct pci_dev *dev, int n)
 {
-	if (n >= 0 && n < PCI_NUM_RESOURCES)
+	struct pci_dev_addon_resource *addon_res;
+
+	if (n < 0)
+		return NULL;
+
+	if (n < PCI_NUM_RESOURCES)
 		return &dev->resource[n];
 
+	n -= PCI_NUM_RESOURCES;
+	list_for_each_entry(addon_res, &dev->addon_resources, list) {
+		if (n-- == 0)
+			return &addon_res->res;
+	}
+
 	return NULL;
 }
 EXPORT_SYMBOL(pci_dev_resource_n);
 
 int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
 {
+	struct pci_dev_addon_resource *addon_res;
+	int n;
+
 	if (res >= dev->resource &&
 	    res <= dev->resource + (PCI_NUM_RESOURCES - 1))
 		return res - dev->resource;
 
+	n = PCI_NUM_RESOURCES;
+	list_for_each_entry(addon_res, &dev->addon_resources, list) {
+		if (res == &addon_res->res)
+			return n;
+		n++;
+	}
+
 	return -1;
 }
 
+static void pci_release_dev_addon_resource(struct pci_dev *dev)
+{
+	struct pci_dev_addon_resource *addon_res, *tmp;
+
+	list_for_each_entry_safe(addon_res, tmp, &dev->addon_resources, list) {
+		list_del(&addon_res->list);
+		kfree(addon_res);
+	}
+}
+
 static void __init_res_idx_mask(unsigned long *mask, int flag)
 {
 	bitmap_zero(mask, PCI_NUM_RESOURCES);
@@ -167,6 +198,9 @@ int pci_next_resource_idx(int i, int flag)
 	if (i < PCI_NUM_RESOURCES)
 		return i;
 
+	if (flag & PCI_ADDON_RES)
+		return i;
+
 	return -1;
 }
 EXPORT_SYMBOL(pci_next_resource_idx);
@@ -1199,6 +1233,7 @@ static void pci_release_dev(struct device *dev)
 	pci_dev = to_pci_dev(dev);
 	pci_release_capabilities(pci_dev);
 	pci_release_of_node(pci_dev);
+	pci_release_dev_addon_resource(pci_dev);
 	kfree(pci_dev);
 }
 
@@ -1276,6 +1311,7 @@ struct pci_dev *alloc_pci_dev(void)
 		return NULL;
 
 	INIT_LIST_HEAD(&dev->bus_list);
+	INIT_LIST_HEAD(&dev->addon_resources);
 
 	return dev;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 94935fc..b73245b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -308,6 +308,7 @@ struct pci_dev {
 	 */
 	unsigned int	irq;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
+	struct list_head addon_resources; /* addon I/O and memory resource */
 
 	bool match_driver;		/* Skip attaching driver */
 	/* These fields are used by common fixups */
@@ -361,6 +362,21 @@ struct pci_dev {
 	size_t romlen; /* Length of ROM if it's not from the BAR */
 };
 
+struct resource_ops {
+	int (*read)(struct pci_dev *dev, struct resource *res, int addr);
+	int (*write)(struct pci_dev *dev, struct resource *res, int addr);
+};
+
+struct pci_dev_addon_resource {
+	struct list_head list;
+	int reg_addr;
+	int size;
+	struct resource res;
+	struct resource_ops *ops;
+};
+#define	to_pci_dev_addon_resource(n)					\
+	container_of(n, struct pci_dev_addon_resource, res)
+
 struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
 int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
 
@@ -369,8 +385,10 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
 #define PCI_IOV_RES		(1<<2)
 #define PCI_BRIDGE_RES		(1<<3)
 #define PCI_RES_BLOCK_NUM	4
+/* addon does not need use bitmap ...*/
+#define PCI_ADDON_RES		(1<<4)
 
-#define PCI_ALL_RES		(PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES)
+#define PCI_ALL_RES		(PCI_STD_RES | PCI_ROM_RES | PCI_BRIDGE_RES | PCI_IOV_RES | PCI_ADDON_RES)
 #define PCI_NOSTD_RES		(PCI_ALL_RES & ~PCI_STD_RES)
 #define PCI_NOIOV_RES		(PCI_ALL_RES & ~PCI_IOV_RES)
 #define PCI_NOROM_RES		(PCI_ALL_RES & ~PCI_ROM_RES)
-- 
1.7.10.4

[PATCH v3 22/27] PCI: Add helpers to add addon_resource

[Yinghai Lu]

Add helper to put add_on resources into pci devices resource list.

-v2: change one function to static according to Fengguang.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |    5 ++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9e659c7..f3038c2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -144,6 +144,74 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
 	return -1;
 }
 
+static struct pci_dev_addon_resource *pci_alloc_dev_addon_resource(
+		 struct pci_dev *dev, int addr, char *name)
+{
+	struct pci_dev_addon_resource *addon_res;
+
+	addon_res = kzalloc(sizeof(*addon_res), GFP_KERNEL);
+
+	if (!addon_res)
+		return NULL;
+
+	addon_res->reg_addr = addr;
+
+	if (name)
+		addon_res->res.name = name;
+	else
+		addon_res->res.name = pci_name(dev);
+
+	list_add_tail(&addon_res->list, &dev->addon_resources);
+
+	return addon_res;
+}
+
+struct pci_dev_addon_resource *pci_add_dev_addon_fixed_resource(
+		 struct pci_dev *dev, int start, int size, int flags,
+		 int addr, char *name)
+{
+	struct pci_dev_addon_resource *addon_res;
+	struct resource *res;
+
+	addon_res = pci_alloc_dev_addon_resource(dev, addr, name);
+	if (addon_res)
+		return NULL;
+
+	res = &addon_res->res;
+	res->start = start;
+	res->end = start + size - 1;
+	res->flags = flags | IORESOURCE_PCI_FIXED;
+
+	dev_printk(KERN_DEBUG, &dev->dev,
+		   "addon fixed resource %s %pR added\n", res->name, res);
+
+	return addon_res;
+}
+
+struct pci_dev_addon_resource *pci_add_dev_addon_resource(struct pci_dev *dev,
+		 int addr, int size, struct resource_ops *ops, char *name)
+{
+	struct pci_dev_addon_resource *addon_res;
+	struct resource *res;
+
+	addon_res = pci_alloc_dev_addon_resource(dev, addr, name);
+	if (!addon_res)
+		return NULL;
+
+	res = &addon_res->res;
+	if (ops) {
+		addon_res->ops = ops;
+		addon_res->size = size;
+		ops->read(dev, res, addr);
+	} else
+		__pci_read_base(dev, pci_bar_unknown, res, addr);
+
+	dev_printk(KERN_DEBUG, &dev->dev,
+		   "addon resource %s %pR added\n", res->name, res);
+
+	return addon_res;
+}
+
 static void pci_release_dev_addon_resource(struct pci_dev *dev)
 {
 	struct pci_dev_addon_resource *addon_res, *tmp;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b73245b..624c62f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -379,6 +379,11 @@ struct pci_dev_addon_resource {
 
 struct resource *pci_dev_resource_n(struct pci_dev *dev, int n);
 int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res);
+struct pci_dev_addon_resource *pci_add_dev_addon_fixed_resource(
+		 struct pci_dev *dev, int start, int size, int flags,
+		 int addr, char *name);
+struct pci_dev_addon_resource *pci_add_dev_addon_resource(struct pci_dev *dev,
+		 int addr, int size, struct resource_ops *ops, char *name);
 
 #define PCI_STD_RES		(1<<0)
 #define PCI_ROM_RES		(1<<1)
-- 
1.7.10.4

[PATCH v3 23/27] PCI: Update pci_resource_bar() to support addon_resource

[Yinghai Lu]

Need to loop addon resource list to retrieve reg_addr in it.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index dca6c1a1..7dca4f1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3613,6 +3613,13 @@ int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type)
 		reg = pci_iov_resource_bar(dev, resno, type);
 		if (reg)
 			return reg;
+	} else if (resno >= PCI_NUM_RESOURCES) {
+		struct resource *res = pci_dev_resource_n(dev, resno);
+
+		if (res) {
+			*type = pci_bar_unknown;
+			return to_pci_dev_addon_resource(res)->reg_addr;
+		}
 	}
 
 	dev_err(&dev->dev, "BAR %d: invalid resource\n", resno);
-- 
1.7.10.4

[PATCH v3 24/27] PCI: Assign/update resource to addon_res

[Yinghai Lu]

Extend pci_update_resource to check resno and call addon resources
ops to update bar for it.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-res.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 94ef232..6a85dcf 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -35,7 +35,7 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 	u32 new, check, mask;
 	int reg;
 	enum pci_bar_type type;
-	struct resource *res = dev->resource + resno;
+	struct resource *res = pci_dev_resource_n(dev, resno);
 
 	/*
 	 * Ignore resources for unimplemented BARs and unused resource slots
@@ -52,6 +52,21 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 	if (res->flags & IORESOURCE_PCI_FIXED)
 		return;
 
+	if (resno >= PCI_NUM_RESOURCES) {
+		struct pci_dev_addon_resource *addon_res;
+
+		addon_res = to_pci_dev_addon_resource(res);
+		reg = addon_res->reg_addr;
+		if (addon_res->ops) {
+			addon_res->ops->write(dev, res, reg);
+			return;
+		}
+	} else
+		reg = pci_resource_bar(dev, resno, &type);
+
+	if (!reg)
+		return;
+
 	pcibios_resource_to_bus(dev, &region, res);
 
 	new = region.start | (res->flags & PCI_REGION_FLAG_MASK);
@@ -60,9 +75,6 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 	else
 		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
 
-	reg = pci_resource_bar(dev, resno, &type);
-	if (!reg)
-		return;
 	if (type != pci_bar_unknown) {
 		if (!(res->flags & IORESOURCE_ROM_ENABLE))
 			return;
@@ -286,7 +298,7 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	if (!ret) {
 		res->flags &= ~IORESOURCE_STARTALIGN;
 		dev_info(&dev->dev, "BAR %d: assigned %pR\n", resno, res);
-		if (resno < PCI_BRIDGE_RESOURCES)
+		if (!is_pci_bridge_resource_idx(resno))
 			pci_update_resource(dev, resno);
 	}
 	return ret;
@@ -311,7 +323,7 @@ int pci_reassign_resource(struct pci_dev *dev, int resno, resource_size_t addsiz
 	if (!ret) {
 		res->flags &= ~IORESOURCE_STARTALIGN;
 		dev_info(&dev->dev, "BAR %d: reassigned %pR\n", resno, res);
-		if (resno < PCI_BRIDGE_RESOURCES)
+		if (!is_pci_bridge_resource_idx(resno))
 			pci_update_resource(dev, resno);
 	}
 	return ret;
-- 
1.7.10.4

[PATCH v3 25/27] PCI: Make piix4 quirk to use addon_res

[Yinghai Lu]

After they are put in add-on resources, they will be safely claimed later.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/quirks.c |   93 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 77 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0369fb6..7e5392c 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -383,14 +383,14 @@ static void quirk_ali7101_acpi(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL,	PCI_DEVICE_ID_AL_M7101,		quirk_ali7101_acpi);
 
-static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
+static int piix4_read_io(struct pci_dev *dev, struct resource *res, int port)
 {
 	u32 devres;
 	u32 mask, size, base;
+	struct pci_bus_region bus_region;
 
 	pci_read_config_dword(dev, port, &devres);
-	if ((devres & enable) != enable)
-		return;
+
 	mask = (devres >> 16) & 15;
 	base = devres & 0xffff;
 	size = 16;
@@ -400,23 +400,53 @@ static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int p
 			break;
 		size = bit;
 	}
-	/*
-	 * For now we only print it out. Eventually we'll want to
-	 * reserve it (at least if it's in the 0x1000+ range), but
-	 * let's get enough confirmation reports first. 
-	 */
 	base &= -size;
-	dev_info(&dev->dev, "%s PIO at %04x-%04x\n", name, base, base + size - 1);
+
+	bus_region.start = base;
+	bus_region.end = base + size - 1;
+	res->flags |= IORESOURCE_IO;
+	pcibios_bus_to_resource(dev, res, &bus_region);
+	dev_info(&dev->dev, "PIO at %pR\n", res);
+
+	return 0;
 }
+static int piix4_write_io(struct pci_dev *dev, struct resource *res, int port)
+{
+	u32 devres;
+	struct pci_bus_region bus_region;
+
+	pcibios_resource_to_bus(dev, &bus_region, res);
 
-static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
+	pci_read_config_dword(dev, port, &devres);
+	devres &= 0xffff0000;
+	devres |= bus_region.start & 0xffff;
+	pci_write_config_dword(dev, port, devres);
+
+	return 0;
+}
+static struct resource_ops piix4_io_ops = {
+	.read = piix4_read_io,
+	.write = piix4_write_io,
+};
+static void piix4_io_quirk(struct pci_dev *dev, char *name, unsigned int port,
+				unsigned int enable)
 {
 	u32 devres;
-	u32 mask, size, base;
 
 	pci_read_config_dword(dev, port, &devres);
 	if ((devres & enable) != enable)
 		return;
+
+	pci_add_dev_addon_resource(dev, port, 0, &piix4_io_ops, name);
+}
+
+static int piix4_read_mem(struct pci_dev *dev, struct resource *res, int port)
+{
+	u32 devres;
+	u32 mask, size, base;
+	struct pci_bus_region bus_region;
+
+	pci_read_config_dword(dev, port, &devres);
 	base = devres & 0xffff0000;
 	mask = (devres & 0x3f) << 16;
 	size = 128 << 16;
@@ -426,12 +456,43 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
 			break;
 		size = bit;
 	}
-	/*
-	 * For now we only print it out. Eventually we'll want to
-	 * reserve it, but let's get enough confirmation reports first. 
-	 */
 	base &= -size;
-	dev_info(&dev->dev, "%s MMIO at %04x-%04x\n", name, base, base + size - 1);
+	bus_region.start = base;
+	bus_region.end = base + size - 1;
+	res->flags |= IORESOURCE_MEM;
+	pcibios_bus_to_resource(dev, res, &bus_region);
+	dev_info(&dev->dev, "MMIO at %pR\n", res);
+
+	return 0;
+}
+static int piix4_write_mem(struct pci_dev *dev, struct resource *res, int port)
+{
+	u32 devres;
+	struct pci_bus_region bus_region;
+
+	pcibios_resource_to_bus(dev, &bus_region, res);
+
+	pci_read_config_dword(dev, port, &devres);
+	devres &= 0x0000ffff;
+	devres |= bus_region.start & 0xffff0000;
+	pci_write_config_dword(dev, port, devres);
+
+	return 0;
+}
+static struct resource_ops piix4_mem_ops = {
+	.read = piix4_read_mem,
+	.write = piix4_write_mem,
+};
+static void piix4_mem_quirk(struct pci_dev *dev, char *name, unsigned int port,
+				unsigned int enable)
+{
+	u32 devres;
+
+	pci_read_config_dword(dev, port, &devres);
+	if ((devres & enable) != enable)
+		return;
+
+	pci_add_dev_addon_resource(dev, port, 0, &piix4_mem_ops, name);
 }
 
 /*
-- 
1.7.10.4

[PATCH v3 26/27] PCI: Make quirk_io_region to use addon_res

[Yinghai Lu]

After they are put in add-on resources, they will be safely claimed later.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/quirks.c |  155 +++++++++++++++++++++++---------------------------
 1 file changed, 70 insertions(+), 85 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7e5392c..6d379d6 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -324,29 +324,61 @@ static void quirk_cs5536_vsa(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, quirk_cs5536_vsa);
 
-static void quirk_io_region(struct pci_dev *dev, unsigned region,
-	unsigned size, int nr, const char *name)
+static int quirk_read_io(struct pci_dev *dev, struct resource *res, int port)
 {
-	region &= ~(size-1);
-	if (region) {
-		struct pci_bus_region bus_region;
-		struct resource *res = dev->resource + nr;
+	struct pci_bus_region bus_region;
+	u16 region;
+	unsigned size = to_pci_dev_addon_resource(res)->size;
 
-		res->name = pci_name(dev);
-		res->start = region;
-		res->end = region + size - 1;
-		res->flags = IORESOURCE_IO;
+	pci_read_config_word(dev, port, &region);
+	region &= ~(size - 1);
 
-		/* Convert from PCI bus to resource space.  */
-		bus_region.start = res->start;
-		bus_region.end = res->end;
-		pcibios_bus_to_resource(dev, res, &bus_region);
+	/* Convert from PCI bus to resource space.  */
+	bus_region.start = region;
+	bus_region.end = region + size - 1;
 
-		if (pci_claim_resource(dev, nr) == 0)
-			dev_info(&dev->dev, "quirk: %pR claimed by %s\n",
-				 res, name);
-	}
-}	
+	pcibios_bus_to_resource(dev, res, &bus_region);
+
+	res->flags |= IORESOURCE_IO;
+	dev_info(&dev->dev, "PIO at %pR\n", res);
+
+	return 0;
+}
+static int quirk_write_io(struct pci_dev *dev, struct resource *res, int port)
+{
+	struct pci_bus_region bus_region;
+	u16 region;
+	unsigned size = to_pci_dev_addon_resource(res)->size;
+
+	pci_read_config_word(dev, port, &region);
+	region &= size - 1;
+
+	/* Convert to PCI bus space.  */
+	pcibios_resource_to_bus(dev, &bus_region, res);
+	region |= bus_region.start & (~(size - 1));
+
+	pci_write_config_word(dev, port, region);
+
+	return 0;
+}
+static struct resource_ops quirk_io_ops = {
+	.read = quirk_read_io,
+	.write = quirk_write_io,
+};
+
+static void quirk_io_region(struct pci_dev *dev, int port,
+					unsigned size, char *name)
+{
+	u16 region;
+
+	pci_read_config_word(dev, port, &region);
+	region &= size - 1;
+
+	if (!region)
+		return;
+
+	pci_add_dev_addon_resource(dev, port, size, &quirk_io_ops, name);
+}
 
 /*
  *	ATI Northbridge setups MCE the processor if you even
@@ -374,12 +406,8 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI,	PCI_DEVICE_ID_ATI_RS100,   quirk_ati_
  */
 static void quirk_ali7101_acpi(struct pci_dev *dev)
 {
-	u16 region;
-
-	pci_read_config_word(dev, 0xE0, &region);
-	quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES, "ali7101 ACPI");
-	pci_read_config_word(dev, 0xE2, &region);
-	quirk_io_region(dev, region, 32, PCI_BRIDGE_RESOURCES+1, "ali7101 SMB");
+	quirk_io_region(dev, 0xE0, 64, "ali7101 ACPI");
+	quirk_io_region(dev, 0xE2, 32, "ali7101 SMB");
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL,	PCI_DEVICE_ID_AL_M7101,		quirk_ali7101_acpi);
 
@@ -503,12 +531,10 @@ static void piix4_mem_quirk(struct pci_dev *dev, char *name, unsigned int port,
  */
 static void quirk_piix4_acpi(struct pci_dev *dev)
 {
-	u32 region, res_a;
+	u32 res_a;
 
-	pci_read_config_dword(dev, 0x40, &region);
-	quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES, "PIIX4 ACPI");
-	pci_read_config_dword(dev, 0x90, &region);
-	quirk_io_region(dev, region, 16, PCI_BRIDGE_RESOURCES+1, "PIIX4 SMB");
+	quirk_io_region(dev, 0x40, 64, "PIIX4 ACPI");
+	quirk_io_region(dev, 0x90, 16, "PIIX4 SMB");
 
 	/* Device resource A has enables for some of the other ones */
 	pci_read_config_dword(dev, 0x5c, &res_a);
@@ -552,7 +578,6 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82443MX_3,	qui
  */
 static void quirk_ich4_lpc_acpi(struct pci_dev *dev)
 {
-	u32 region;
 	u8 enable;
 
 	/*
@@ -564,22 +589,12 @@ static void quirk_ich4_lpc_acpi(struct pci_dev *dev)
 	*/
 
 	pci_read_config_byte(dev, ICH_ACPI_CNTL, &enable);
-	if (enable & ICH4_ACPI_EN) {
-		pci_read_config_dword(dev, ICH_PMBASE, &region);
-		region &= PCI_BASE_ADDRESS_IO_MASK;
-		if (region >= PCIBIOS_MIN_IO)
-			quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
-					"ICH4 ACPI/GPIO/TCO");
-	}
+	if (enable & ICH4_ACPI_EN)
+		quirk_io_region(dev, ICH_PMBASE, 128, "ICH4 ACPI/GPIO/TCO");
 
 	pci_read_config_byte(dev, ICH4_GPIO_CNTL, &enable);
-	if (enable & ICH4_GPIO_EN) {
-		pci_read_config_dword(dev, ICH4_GPIOBASE, &region);
-		region &= PCI_BASE_ADDRESS_IO_MASK;
-		if (region >= PCIBIOS_MIN_IO)
-			quirk_io_region(dev, region, 64,
-					PCI_BRIDGE_RESOURCES + 1, "ICH4 GPIO");
-	}
+	if (enable & ICH4_GPIO_EN)
+		quirk_io_region(dev, ICH4_GPIOBASE, 64, "ICH4 GPIO");
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,    PCI_DEVICE_ID_INTEL_82801AA_0,		quirk_ich4_lpc_acpi);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,    PCI_DEVICE_ID_INTEL_82801AB_0,		quirk_ich4_lpc_acpi);
@@ -594,26 +609,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,    PCI_DEVICE_ID_INTEL_ESB_1,		qui
 
 static void ich6_lpc_acpi_gpio(struct pci_dev *dev)
 {
-	u32 region;
 	u8 enable;
 
 	pci_read_config_byte(dev, ICH_ACPI_CNTL, &enable);
-	if (enable & ICH6_ACPI_EN) {
-		pci_read_config_dword(dev, ICH_PMBASE, &region);
-		region &= PCI_BASE_ADDRESS_IO_MASK;
-		if (region >= PCIBIOS_MIN_IO)
-			quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
-					"ICH6 ACPI/GPIO/TCO");
-	}
+	if (enable & ICH6_ACPI_EN)
+		quirk_io_region(dev, ICH_PMBASE, 128, "ICH6 ACPI/GPIO/TCO");
 
 	pci_read_config_byte(dev, ICH6_GPIO_CNTL, &enable);
-	if (enable & ICH6_GPIO_EN) {
-		pci_read_config_dword(dev, ICH6_GPIOBASE, &region);
-		region &= PCI_BASE_ADDRESS_IO_MASK;
-		if (region >= PCIBIOS_MIN_IO)
-			quirk_io_region(dev, region, 64,
-					PCI_BRIDGE_RESOURCES + 1, "ICH6 GPIO");
-	}
+	if (enable & ICH6_GPIO_EN)
+		quirk_io_region(dev, ICH6_GPIOBASE, 64, "ICH6 GPIO");
 }
 
 static void ich6_lpc_generic_decode(struct pci_dev *dev, unsigned reg, const char *name, int dynsize)
@@ -711,13 +715,8 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,   PCI_DEVICE_ID_INTEL_ICH10_1, qui
  */
 static void quirk_vt82c586_acpi(struct pci_dev *dev)
 {
-	u32 region;
-
-	if (dev->revision & 0x10) {
-		pci_read_config_dword(dev, 0x48, &region);
-		region &= PCI_BASE_ADDRESS_IO_MASK;
-		quirk_io_region(dev, region, 256, PCI_BRIDGE_RESOURCES, "vt82c586 ACPI");
-	}
+	if (dev->revision & 0x10)
+		quirk_io_region(dev, 0x48, 256, "vt82c586 ACPI");
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_VIA,	PCI_DEVICE_ID_VIA_82C586_3,	quirk_vt82c586_acpi);
 
@@ -729,18 +728,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_VIA,	PCI_DEVICE_ID_VIA_82C586_3,	quirk_vt
  */
 static void quirk_vt82c686_acpi(struct pci_dev *dev)
 {
-	u16 hm;
-	u32 smb;
-
 	quirk_vt82c586_acpi(dev);
 
-	pci_read_config_word(dev, 0x70, &hm);
-	hm &= PCI_BASE_ADDRESS_IO_MASK;
-	quirk_io_region(dev, hm, 128, PCI_BRIDGE_RESOURCES + 1, "vt82c686 HW-mon");
+	quirk_io_region(dev, 0x70, 128, "vt82c686 HW-mon");
 
-	pci_read_config_dword(dev, 0x90, &smb);
-	smb &= PCI_BASE_ADDRESS_IO_MASK;
-	quirk_io_region(dev, smb, 16, PCI_BRIDGE_RESOURCES + 2, "vt82c686 SMB");
+	quirk_io_region(dev, 0x90, 16, "vt82c686 SMB");
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_VIA,	PCI_DEVICE_ID_VIA_82C686_4,	quirk_vt82c686_acpi);
 
@@ -751,15 +743,8 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_VIA,	PCI_DEVICE_ID_VIA_82C686_4,	quirk_vt
  */
 static void quirk_vt8235_acpi(struct pci_dev *dev)
 {
-	u16 pm, smb;
-
-	pci_read_config_word(dev, 0x88, &pm);
-	pm &= PCI_BASE_ADDRESS_IO_MASK;
-	quirk_io_region(dev, pm, 128, PCI_BRIDGE_RESOURCES, "vt8235 PM");
-
-	pci_read_config_word(dev, 0xd0, &smb);
-	smb &= PCI_BASE_ADDRESS_IO_MASK;
-	quirk_io_region(dev, smb, 16, PCI_BRIDGE_RESOURCES + 1, "vt8235 SMB");
+	quirk_io_region(dev, 0x88, 128, "vt8235 PM");
+	quirk_io_region(dev, 0xd0, 16, "vt8235 SMB");
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_VIA,	PCI_DEVICE_ID_VIA_8235,	quirk_vt8235_acpi);
 
-- 
1.7.10.4

Re: [PATCH v3 26/27] PCI: Make quirk_io_region to use addon_res

[Bjorn Helgaas]

On Wed, Mar 13, 2013 at 5:28 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> After they are put in add-on resources, they will be safely claimed later.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/quirks.c |  155 +++++++++++++++++++++++---------------------------
>  1 file changed, 70 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 7e5392c..6d379d6 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -324,29 +324,61 @@ static void quirk_cs5536_vsa(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, quirk_cs5536_vsa);
>
> -static void quirk_io_region(struct pci_dev *dev, unsigned region,
> -       unsigned size, int nr, const char *name)
> +static int quirk_read_io(struct pci_dev *dev, struct resource *res, int port)
>  {
> -       region &= ~(size-1);
> -       if (region) {
> -               struct pci_bus_region bus_region;
> -               struct resource *res = dev->resource + nr;
> +       struct pci_bus_region bus_region;
> +       u16 region;
> +       unsigned size = to_pci_dev_addon_resource(res)->size;
>
> -               res->name = pci_name(dev);
> -               res->start = region;
> -               res->end = region + size - 1;
> -               res->flags = IORESOURCE_IO;
> +       pci_read_config_word(dev, port, &region);
> +       region &= ~(size - 1);
>
> -               /* Convert from PCI bus to resource space.  */
> -               bus_region.start = res->start;
> -               bus_region.end = res->end;
> -               pcibios_bus_to_resource(dev, res, &bus_region);
> +       /* Convert from PCI bus to resource space.  */
> +       bus_region.start = region;
> +       bus_region.end = region + size - 1;
>
> -               if (pci_claim_resource(dev, nr) == 0)
> -                       dev_info(&dev->dev, "quirk: %pR claimed by %s\n",
> -                                res, name);
> -       }
> -}
> +       pcibios_bus_to_resource(dev, res, &bus_region);
> +
> +       res->flags |= IORESOURCE_IO;
> +       dev_info(&dev->dev, "PIO at %pR\n", res);
> +
> +       return 0;
> +}
> +static int quirk_write_io(struct pci_dev *dev, struct resource *res, int port)
> +{
> +       struct pci_bus_region bus_region;
> +       u16 region;
> +       unsigned size = to_pci_dev_addon_resource(res)->size;
> +
> +       pci_read_config_word(dev, port, &region);
> +       region &= size - 1;
> +
> +       /* Convert to PCI bus space.  */
> +       pcibios_resource_to_bus(dev, &bus_region, res);
> +       region |= bus_region.start & (~(size - 1));
> +
> +       pci_write_config_word(dev, port, region);
> +
> +       return 0;
> +}
> +static struct resource_ops quirk_io_ops = {
> +       .read = quirk_read_io,
> +       .write = quirk_write_io,
> +};
> +
> +static void quirk_io_region(struct pci_dev *dev, int port,
> +                                       unsigned size, char *name)
> +{
> +       u16 region;
> +
> +       pci_read_config_word(dev, port, &region);
> +       region &= size - 1;
> +
> +       if (!region)
> +               return;
> +
> +       pci_add_dev_addon_resource(dev, port, size, &quirk_io_ops, name);
> +}
>
>  /*
>   *     ATI Northbridge setups MCE the processor if you even
> @@ -374,12 +406,8 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_RS100,   quirk_ati_
>   */
>  static void quirk_ali7101_acpi(struct pci_dev *dev)
>  {
> -       u16 region;
> -
> -       pci_read_config_word(dev, 0xE0, &region);
> -       quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES, "ali7101 ACPI");
> -       pci_read_config_word(dev, 0xE2, &region);
> -       quirk_io_region(dev, region, 32, PCI_BRIDGE_RESOURCES+1, "ali7101 SMB");
> +       quirk_io_region(dev, 0xE0, 64, "ali7101 ACPI");
> +       quirk_io_region(dev, 0xE2, 32, "ali7101 SMB");

This patch has two changes that need to be separated:

  1) Refactoring quirk_io_region() so the pci_read_config_word() is
done by quirk_io_region() rather than the caller.
  2) Whatever pci_dev resource changes we end up making.

I think part 1 can be done at any time and is probably not controversial.

Bjorn

Re: [PATCH v3 26/27] PCI: Make quirk_io_region to use addon_res

[Yinghai Lu]

On Thu, Apr 4, 2013 at 2:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> This patch has two changes that need to be separated:
>
>   1) Refactoring quirk_io_region() so the pci_read_config_word() is
> done by quirk_io_region() rather than the caller.
>   2) Whatever pci_dev resource changes we end up making.
>
> I think part 1 can be done at any time and is probably not controversial.

ok, will put that separated one to be first one in the next reversion
of patchset.

Thanks

Yinghai

[PATCH v3 27/27] PCI: Use addon_fixed_resource with ati fixed resource

[Yinghai Lu]

After they are put in addon_res list, they will be safely claimed later.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/quirks.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6d379d6..2b4fbad 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -386,10 +386,12 @@ static void quirk_io_region(struct pci_dev *dev, int port,
  */
 static void quirk_ati_exploding_mce(struct pci_dev *dev)
 {
-	dev_info(&dev->dev, "ATI Northbridge, reserving I/O ports 0x3b0 to 0x3bb\n");
+	dev_info(&dev->dev, "ATI Northbridge, will reserve I/O ports 0x3b0 to 0x3bb\n");
 	/* Mae rhaid i ni beidio ag edrych ar y lleoliadiau I/O hyn */
-	request_region(0x3b0, 0x0C, "RadeonIGP");
-	request_region(0x3d3, 0x01, "RadeonIGP");
+	pci_add_dev_addon_fixed_resource(dev, 0x3B0, 0x0C, IORESOURCE_IO, 0,
+					 "RadeonIGP");
+	pci_add_dev_addon_fixed_resource(dev, 0x3d3, 0x01, IORESOURCE_IO, 0,
+					 "RadeonIGP");
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI,	PCI_DEVICE_ID_ATI_RS100,   quirk_ati_exploding_mce);
 
-- 
1.7.10.4