Re: [RFC PATCH v3 ]pci: pci resource iterator

[Ram Pai <linuxram@us.ibm.com>]

On Mon, Sep 03, 2012 at 01:07:46AM -0700, Yinghai Lu wrote:
> On Mon, Aug 27, 2012 at 12:33 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> > On Thu, Aug 23, 2012 at 12:30:05PM -0700, Yinghai Lu wrote:
> >> Hi, Ram and Bjorn,
> >>
> >> On Wed, Aug 22, 2012 at 10:09 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> >> > +static inline int pci_next_resource_idx(int i, int flag)
> >> > +{
> >> > +       while (++i < PCI_NUM_RESOURCES) {
> >> > +               if ((i >= 0 && i < PCI_ROM_RESOURCE && (flag & PCI_STD_RES)) ||
> >> > +                   (i == PCI_ROM_RESOURCE && (flag & PCI_ROM_RES)) ||
> >> > +#ifdef CONFIG_PCI_IOV
> >> > +                   (i <= PCI_IOV_RESOURCE_END && (flag & PCI_IOV_RES)) ||
> >> > +#endif
> >> > +                   (i <= PCI_BRIDGE_RESOURCE_END && (flag & PCI_BRIDGE_RES)))
> >> > +                       return i;
> >> > +       }
> >> > +       return -1;
> >> > +}
> >>

.snip....

> >
> > Please decide which version you want and lets take the next step. I
> > think we have iterated over this interface quite a lot. Time to move on
> > :)
> >
> >
> > ---------------------------------------------------------------------
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 5e1ca3c..d95a31c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -146,6 +146,54 @@ static int __init pcibus_class_init(void)
> >  }
> >  postcore_initcall(pcibus_class_init);
> >
> > +
> > +#define PCI_RES_SHIFT 4
> > +/*
> > + * capture the starting index of the next resource type
> > + * as well as the type of the resource
> > + */
> > +int res_idx[PCI_NUM_RESOURCES] = {
> > +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES   */
> > +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+1 */
> > +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+2 */
> > +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+3 */
> > +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+4 */
> > +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+5 */
> > +(PCI_IOV_RESOURCES << PCI_RES_SHIFT) | PCI_ROM_RES, /* PCI_ROM_RESOURCE */
> > +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START */
> > +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+1 */
> > +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+2 */
> > +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+3 */
> > +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+4 */
> > +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+5 */
> > +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START */
> > +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START+1 */
> > +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START+2 */
> > +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START+3 */
> > +};
> 
> how about some one change the some type resource number?

She/he will have to change this code correspondingly. :) I can put in
some comments towards that. However this iterator implementation will
eventually change anyway; ones we restructure the resources in pci_dev
structure.

> 
> better have some marco helper.
> 
> > +#define res_type(idx)  (res_idx[idx] &  ((0x1 << PCI_RES_SHIFT) - 1))
> > +#define res_next(idx)  (res_idx[idx] >> PCI_RES_SHIFT)
> > +
> > +int pci_next_resource_idx(int i, int flag)
> > +{
> > +       i++;
> > +       while (i < PCI_NUM_RESOURCES) {
> > +               if (res_type(i) & flag)
> > +                       return i;
> > +               i = res_next(i);
> > +       }
> 
> here still have some two extra loop when only need to loop STD + BRIDGE.

true. its a tradeoff between space; your bitmap patch, and time; my code
above.  The loss of a few cycles or a few bytes of memory should be
highly insignificant in the grand scheme, as long as it is bounded by a
constant.

Anyway I am ok with either patch.
RP