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

Commit Message

Yinghai Lu March 13, 2013, 11:27 p.m. UTC

From: Ram Pai <[e-mail]>

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 <[e-mail]> - Yinghai Lu

Signed-off-by: Ram Pai <[e-mail]>
Signed-off-by: Yinghai Lu <[e-mail]>
---
 drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |   24 ++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

Comments

Bjorn Helgaas April 4, 2013, 10:18 p.m. UTC | #1

On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <[e-mail]> wrote:
> From: Ram Pai <[e-mail]>
>
> 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 <[e-mail]> - Yinghai Lu
>
> Signed-off-by: Ram Pai <[e-mail]>
> Signed-off-by: Yinghai Lu <[e-mail]>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ram Pai April 9, 2013, 4:51 a.m. UTC | #2

On Thu, Apr 04, 2013 at 04:18:01PM -0600, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <[e-mail]> wrote:
> > From: Ram Pai <[e-mail]>
> >
> > 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 <[e-mail]> - Yinghai Lu
> >
> > Signed-off-by: Ram Pai <[e-mail]>
> > Signed-off-by: Yinghai Lu <[e-mail]>
> > ---
> >  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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas April 10, 2013, 3:22 p.m. UTC | #3

On Mon, Apr 8, 2013 at 10:51 PM, Ram Pai <[e-mail]> 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 <[e-mail]> wrote:
>> > From: Ram Pai <[e-mail]>
>> >
>> > 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 <[e-mail]> - Yinghai Lu
>> >
>> > Signed-off-by: Ram Pai <[e-mail]>
>> > Signed-off-by: Yinghai Lu <[e-mail]>
>> > ---
>> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yinghai Lu April 10, 2013, 4:12 p.m. UTC | #4

On Thu, Apr 4, 2013 at 3:18 PM, Bjorn Helgaas <[e-mail]> wrote:
> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <[e-mail]> wrote:
>> From: Ram Pai <[e-mail]>
>>
>> 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 <[e-mail]> - Yinghai Lu
>>
>> Signed-off-by: Ram Pai <[e-mail]>
>> Signed-off-by: Yinghai Lu <[e-mail]>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ram Pai April 25, 2013, 3:55 a.m. UTC | #5

On Wed, Apr 10, 2013 at 09:22:48AM -0600, Bjorn Helgaas wrote:
> On Mon, Apr 8, 2013 at 10:51 PM, Ram Pai <[e-mail]> 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 <[e-mail]> wrote:
> >> > From: Ram Pai <[e-mail]>
> >> >
> >> > 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 <[e-mail]> - Yinghai Lu
> >> >
> >> > Signed-off-by: Ram Pai <[e-mail]>
> >> > Signed-off-by: Yinghai Lu <[e-mail]>
> >> > ---
> >> >  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

Bjorn Helgaas April 25, 2013, 5:22 p.m. UTC | #6

On Wed, Apr 24, 2013 at 9:55 PM, Ram Pai <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
>> >> > From: Ram Pai <[e-mail]>
>> >> >
>> >> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ram Pai April 28, 2013, 6:08 a.m. UTC | #7

On Thu, Apr 25, 2013 at 11:22:59AM -0600, Bjorn Helgaas wrote:
> On Wed, Apr 24, 2013 at 9:55 PM, Ram Pai <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> >> >> > From: Ram Pai <[e-mail]>
> >> >> >
> >> >> > 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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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