On Wed, Aug 15, 2012 at 03:25:06PM -0600, Bjorn Helgaas wrote: > On Mon, Jun 18, 2012 at 12:30 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote: > > On Sun, Jun 17, 2012 at 10:03 PM, Ram Pai <linuxram@xxxxxxxxxx> wrote: > >> PCI: methods to access resources of struct pci_dev > >> > >> 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. > > > > I have patchset on this, please check > > > > git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git > > for-pci-for-each-res-addon > > I don't like the pci_dev.resource[] table very well either, and I > don't like the fact that drivers access it directly. So I do think we > need some sort of abstraction that lets us change the way we store the > resources without affecting the callers. > > Both of these (Ram's patch: > http://marc.info/?l=linux-pci&m=133999604128815&w=2 and Yinghai's > patch: http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=cd192f0ed93203ef6bac2a44c138899190fb5793) > have a similar approach of adding many iterators. > > Yinghai's adds: > for_each_pci_dev_all_resource > for_each_pci_dev_nobridge_resource > for_each_pci_dev_base_resource > for_each_pci_dev_base_norom_resource > for_each_pci_dev_base_iov_norom_resource > for_each_pci_dev_noiov_resource > for_each_pci_dev_std_resource > for_each_pci_dev_iov_resource > for_each_pci_dev_bridge_resource > for_each_pci_dev_addon_resource > > Ram's adds: > for_each_resource > for_each_std_resource > for_each_std_and_rom_resource > for_each_sriov_resource > for_each_bridge_resource > > It seems like we have a combinatorial explosion of iterators based on > various orthogonal selectors (base, rom, iov, bridge window). That > doesn't seem understandable or maintainable to me. > > I wonder if we could get by with *one* iterator, and select the > resources of interest either by supplying a bitmask of things we care > about, or by doing similar filtering in the caller. I am fine with this approach. I have never encountered the need for 'no' based iterator like 'for_each_pci_dev_noiov_resource' or 'for_each_pci_dev_base_norom_resource'. While abstracting the code and replacing explicit references to the resources in various peices of code including the drivers, I just encountered the need for the 'yes' based iterators like the one that I added. However if there is a need for 'no' based iterators, it should be easy to incorporate them using flags. Something like for_each_pci_resource(dev, res, i, flags) where flags can be #define PCI_STD_RES 0x01 #define PCI_ROM_RES 0x02 #define PCI_BRIDGE_RES 0x04 #define PCI_IOV_RES 0x08 #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) so on and so forth Yinghai if you are ok with this approach, let me code up all the iterators. You can incorporate your patches based on those iterators and I can change all my 40+ patches that change various driver sources to use this iterator. agree? 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
