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

From: Ram Pai <linuxram@us.ibm.com>
To: Yinghai Lu <yinghai@kernel.org>
Cc: Ram Pai <linuxram@us.ibm.com>,
	linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [RFC PATCH v2 ]pci: pci resource iterator
Date: Wed, 22 Aug 2012 18:15:33 +0800	[thread overview]
Message-ID: <20120822101533.GA2332@ram-ThinkPad-T61> (raw)
In-Reply-To: <CAE9FiQUW2rB8X18=X6hVb-ZV8AnoL4RA+8P7897vHxwjj63bsg@mail.gmail.com>

On Tue, Aug 21, 2012 at 04:22:52PM -0700, Yinghai Lu wrote:
> On Tue, Aug 21, 2012 at 8:13 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> > PCI: pci resource iterator
> >
> > 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.
> >
> > Once this patch is accepted, I have another 40+ patches that modify all
> > the files that directly access the resource structure, to use the new
> > methods provided in the first step.
> >
> > Finally we can re-organize the resource structure in the pci_dev
> > structure and correspondingly update the methods.
> >
> > This patch is compile tested only.
> >
> > Changelog:
> >         Consolidated iterator interface as per Bjorn's suggestion.
> >
> > +#define for_each_pci_resource(dev, res, flag) \
> > +       for (res = pci_next_resource(dev, NULL, flag); res; \
> > +                       res = pci_next_resource(dev, res, flag))
> > +
> 
> We may need to keep the idx, so we could make the converting more granularity.
> 
> because some loop body is still using the idx.
> 
> also there is some abusing pci bridge resource as addon resources.
> and we need to remove the abusing at first ---
> that is addressed by:
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=5eb48c3c998257386f67a7570778872ec600138f
> 
>  PCI: Add addon_resource support for pci_dev
> 
> and later we may remove the idx in the for_each_pci_resource()
> 
> Please check updated version of your patch that keep the idx.

by exposing idx through the interface, we are exposing the implementation to
the enduser. I want the end user not know that the resources are 
structured as a array. This will help easily restructure resources
in the pci_dev structure to whatever implementation we want, linked list
or hash or whatever...

Why can't the addon resource be hidden behind the interface? something
like this?

static inline struct resource *pci_next_resource(struct pci_dev *pdev,
                       struct resource *res, int flag)
{
       int i = res? pci_resource_number(pdev, res) : -1;

       while (++i < PCI_NUM_RESOURCES) {
               if ((i >= 0 && i < PCI_ROM_RESOURCE) && (flag & PCI_STD_RES))
                       return pci_get_std_resource(pdev, i);
               else if ((i == PCI_ROM_RESOURCE) && (flag & PCI_ROM_RES))
                       return pci_get_rom_resource(pdev);
               else if ((i <= PCI_IOV_RESOURCE_END) && (flag & PCI_IOV_RES))
                       return pci_get_sriov_resource(pdev, i-PCI_IOV_RESOURCES);
               else if ((i <= PCI_BRIDGE_RESOURCE_END) && (flag & PCI_BRIDGE_RES))
                       return pci_get_bridge_resource(pdev, i-PCI_BRIDGE_RESOURCES);
       }

       if (flag & PCI_ADDON_RES) {
       		if ( i == PCI_NUM_RESOURCES) {
       			// return the first element of the addon list;
		} else {
			// return the next element in the list;
		}		
       }
       return NULL;
}