Message ID | 20120821151245.GA2356@ram-ThinkPad-T61 (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Bjorn Helgaas |
Headers | show |
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. Thanks Yinghai
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; } -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 22, 2012 at 3:15 AM, Ram Pai <linuxram@us.ibm.com> wrote: > 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: > 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? keep the idx would make thing simple for referencing and converting. pci_dev_resource_n() would be wrapper to from idx to real resource pointer. we just need to change &dev->resource[i] referencing to pci_dev_resource_n(dev, i) instead. We did that before for irq_desc storage converting, and now we have for_each_irq_desc(irq, desc) later for the resource allocation, I will change resource member from struct resource resource[DEVICE_COUNT_RESOURCE]; to struct resource *std_res; -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 22, 2012 at 10:31 AM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Aug 22, 2012 at 3:15 AM, Ram Pai <linuxram@us.ibm.com> wrote: >> 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: >> 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? > keep the idx would make thing simple for referencing and converting. pci_dev_resource_n() would be wrapper to from idx to real resource pointer. we just need to change &dev->resource[i] referencing to pci_dev_resource_n(dev, i) instead. We did that before for irq_desc storage converting, and now we have for_each_irq_desc(irq, desc) later for the resource allocation, I will change resource member from struct resource resource[DEVICE_COUNT_RESOURCE]; to struct resource std_res[7]; struct resource *iov_res; struct resource *bridge_res; and allocate iov_res and bridge array as needed. anyway keep the overalll idx is good way to go and follow. Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 22, 2012 at 10:35 AM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Aug 22, 2012 at 10:31 AM, Yinghai Lu <yinghai@kernel.org> wrote: >> > keep the idx would make thing simple for referencing and converting. > > pci_dev_resource_n() would be wrapper to from idx to real resource pointer. > we just need to change &dev->resource[i] referencing to > pci_dev_resource_n(dev, i) instead. > > We did that before for irq_desc storage converting, and now we have > for_each_irq_desc(irq, desc) > > later for the resource allocation, I will change resource member from > struct resource resource[DEVICE_COUNT_RESOURCE]; > to > struct resource std_res[7]; > struct resource *iov_res; > struct resource *bridge_res; > and allocate iov_res and bridge array as needed. > > anyway keep the overalll idx is good way to go and follow. > Like to suggest two versions that only handle idx... Thanks Yinghai
diff --git a/include/linux/pci.h b/include/linux/pci.h index e444f5b..ab897fd 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1351,6 +1351,49 @@ static inline int pci_domain_nr(struct pci_bus *bus) (pci_resource_end((dev), (bar)) - \ pci_resource_start((dev), (bar)) + 1)) +#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) +#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 + +#define pci_get_std_resource(dev, bar) (((dev)->resource)+bar) +#define pci_get_sriov_resource(dev, bar) (((dev)->resource)+bar+PCI_IOV_RESOURCES) +#define pci_get_bridge_resource(dev, bar) (((dev)->resource)+bar+PCI_BRIDGE_RESOURCES) +#define pci_get_rom_resource(dev) (((dev)->resource)+PCI_ROM_RESOURCE) +#define pci_resource_number(dev, res) (res - (dev)->resource) + + +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); + } + return NULL; +} + + +#define for_each_pci_resource(dev, res, flag) \ + for (res = pci_next_resource(dev, NULL, flag); res; \ + res = pci_next_resource(dev, res, flag)) + /* Similar to the helpers above, these manipulate per-pci_dev * driver-specific data. They are really just a wrapper around * the generic device structure functions of these calls.
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. Signed-off-by: Ram Pai <linuxram@us.ibm.com> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html