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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
