* [RFC PATCH] methods to access resources of a struct pci_dev @ 2012-06-18 5:03 Ram Pai 2012-06-18 18:30 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-06-18 5:03 UTC (permalink / raw) To: linux-pci 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. Signed-off-by: Ram Pai <linuxram@us.ibm.com> diff --git a/include/linux/pci.h b/include/linux/pci.h index e444f5b..475b10d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1350,6 +1350,58 @@ static inline int pci_domain_nr(struct pci_bus *bus) \ (pci_resource_end((dev), (bar)) - \ pci_resource_start((dev), (bar)) + 1)) +#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) + +/* code that assume the current resource layout will continue to operate */ +static inline struct resource *pci_get_resource(struct pci_dev *pdev, int bar) +{ + if (bar >= 0 && bar < PCI_ROM_RESOURCE) + return pci_get_std_resource(pdev, bar); + else if (bar == PCI_ROM_RESOURCE) + return pci_get_rom_resource(pdev); + else if (bar <= PCI_IOV_RESOURCE_END) + return pci_get_sriov_resource(pdev, bar-PCI_IOV_RESOURCES); + else if (bar <= PCI_BRIDGE_RESOURCE_END) + return pci_get_bridge_resource(pdev, bar-PCI_BRIDGE_RESOURCES); + else + return NULL; +} + +#define for_each_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_get_resource(dev, i)) || i < PCI_NUM_RESOURCES; \ + i++) + +#define for_each_std_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_get_std_resource(dev, i)) || i <= PCI_STD_RESOURCE_END; \ + i++) + +#define for_each_std_and_rom_resource(dev, res, i) \ + for (i = 0; \ + (res = (i==PCI_ROM_RESOURCE)? pci_get_rom_resource(dev) : \ + pci_get_std_resource(dev, i)) || \ + i <= PCI_ROM_RESOURCE; \ + i++) + +#define for_each_sriov_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_get_sriov_resource(dev, i)) || i < PCI_SRIOV_NUM_BARS; \ + i++) + +#define for_each_bridge_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_get_bridge_resource(dev, i)) || i < PCI_BRIDGE_RESOURCE_NUM; \ + i++) + +#define pci_get_res_attr(dev, bar) (((dev)->res_attr)[bar]) +#define pci_get_res_attrwc(dev, bar) (((dev)->res_attr_wc)[bar]) +#define pci_set_res_attr(dev, bar, value) (((dev)->res_attr)[bar] = value) +#define pci_set_res_attrwc(dev, bar, value) (((dev)->res_attr_wc)[bar] = value) /* Similar to the helpers above, these manipulate per-pci_dev * driver-specific data. They are really just a wrapper around ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH] methods to access resources of a struct pci_dev 2012-06-18 5:03 [RFC PATCH] methods to access resources of a struct pci_dev Ram Pai @ 2012-06-18 18:30 ` Yinghai Lu 2012-06-19 1:46 ` Ram Pai 2012-08-15 21:25 ` Bjorn Helgaas 0 siblings, 2 replies; 24+ messages in thread From: Yinghai Lu @ 2012-06-18 18:30 UTC (permalink / raw) To: Ram Pai; +Cc: linux-pci On Sun, Jun 17, 2012 at 10:03 PM, Ram Pai <linuxram@us.ibm.com> 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 Thanks Yinghai ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH] methods to access resources of a struct pci_dev 2012-06-18 18:30 ` Yinghai Lu @ 2012-06-19 1:46 ` Ram Pai 2012-06-19 2:57 ` Yinghai Lu 2012-08-15 21:25 ` Bjorn Helgaas 1 sibling, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-06-19 1:46 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ram Pai, linux-pci On Mon, Jun 18, 2012 at 11:30:13AM -0700, Yinghai Lu wrote: > On Sun, Jun 17, 2012 at 10:03 PM, Ram Pai <linuxram@us.ibm.com> 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 > Amazing, when are you pushing those patches in? Looks like you have patches for everything :) Do you also have patches that change all the places that directly access the ->resource structure? Also does Bjorn pull regularly from your tree? Is making patches against your tree the right approach? RP ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH] methods to access resources of a struct pci_dev 2012-06-19 1:46 ` Ram Pai @ 2012-06-19 2:57 ` Yinghai Lu 0 siblings, 0 replies; 24+ messages in thread From: Yinghai Lu @ 2012-06-19 2:57 UTC (permalink / raw) To: Ram Pai; +Cc: linux-pci On Mon, Jun 18, 2012 at 6:46 PM, Ram Pai <linuxram@us.ibm.com> wrote: > On Mon, Jun 18, 2012 at 11:30:13AM -0700, Yinghai Lu wrote: >> 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 >> > > > Amazing, when are you pushing those patches in? Looks like you have > patches for everything :) > > Do you also have patches that change all the places that directly access > the ->resource structure? maybe not. could still have some left over. > > Also does Bjorn pull regularly from your tree? Is making patches > against your tree the right approach? No. Bjorn will get patches from my branch and have to update change log at least. and then into pci/next. current still have several branch in the waiting list for-pci-res-alloc for-pci-busn-alloc for-pci-root-bus-hotplug for-pci-for-each-res-addon also for-x86-irq and for-iommu are for pci root bus hotplug related too, but need to push them to tip pci related get merged. Current status: for-pci-busn-alloc is pending with http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=patch;h=b00469b377231ebe0b76248ec6d1866b51966c97 Bjorn thought that is too complicated. Thanks Yinghai ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH] methods to access resources of a struct pci_dev 2012-06-18 18:30 ` Yinghai Lu 2012-06-19 1:46 ` Ram Pai @ 2012-08-15 21:25 ` Bjorn Helgaas 2012-08-16 3:26 ` Ram Pai 1 sibling, 1 reply; 24+ messages in thread From: Bjorn Helgaas @ 2012-08-15 21:25 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ram Pai, linux-pci On Mon, Jun 18, 2012 at 12:30 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Sun, Jun 17, 2012 at 10:03 PM, Ram Pai <linuxram@us.ibm.com> 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. Bjorn ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH] methods to access resources of a struct pci_dev 2012-08-15 21:25 ` Bjorn Helgaas @ 2012-08-16 3:26 ` Ram Pai 2012-08-16 4:11 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-08-16 3:26 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Yinghai Lu, Ram Pai, linux-pci 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@kernel.org> wrote: > > On Sun, Jun 17, 2012 at 10:03 PM, Ram Pai <linuxram@us.ibm.com> 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH] methods to access resources of a struct pci_dev 2012-08-16 3:26 ` Ram Pai @ 2012-08-16 4:11 ` Yinghai Lu 2012-08-16 4:41 ` Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-08-16 4:11 UTC (permalink / raw) To: Ram Pai; +Cc: Bjorn Helgaas, linux-pci On Wed, Aug 15, 2012 at 8:26 PM, Ram Pai <linuxram@us.ibm.com> wrote: > On Wed, Aug 15, 2012 at 03:25:06PM -0600, Bjorn Helgaas wrote: > > 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. Do you mean that you will have updated patch for http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=cd192f0ed93203ef6bac2a44c138899190fb5793 ? if it is that case, i am ok, and then I could use scripts to update following patches. >From cd192f0ed93203ef6bac2a44c138899190fb5793 Mon Sep 17 00:00:00 2001 From: Yinghai Lu <yinghai@kernel.org> Date: Tue, 26 Jun 2012 17:02:04 -0700 Subject: [PATCH] PCI: Add for_each_resource helpers to make resource loop easier. Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- include/linux/pci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 55 insertions(+), 0 deletions(-) diff --git a/include/linux/pci.h b/include/linux/pci.h index 77778cb..dd577e3 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -363,6 +363,61 @@ struct pci_dev { struct resource *pci_dev_resource_n(struct pci_dev *dev, int n); +#define resno_is_for_bridge(n) \ + ((n) >= PCI_BRIDGE_RESOURCES && (n) <= PCI_BRIDGE_RESOURCE_END) + +/* all (include bridge) resources */ +#define for_each_pci_dev_all_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_dev_resource_n(dev, i)) || i < PCI_NUM_RESOURCES; \ + i++) +/* exclude bridge resources */ +#define for_each_pci_dev_nobridge_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_dev_resource_n(dev, i)) || i < PCI_NUM_RESOURCES; \ + i = (i != (PCI_BRIDGE_RESOURCES - 1)) ? (i+1) : PCI_NUM_RESOURCES) +/* exclude bridge and IOV resources */ +#define for_each_pci_dev_base_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_dev_resource_n(dev, i)) || i < PCI_NUM_RESOURCES; \ + i = (i != PCI_ROM_RESOURCE) ? (i+1) : PCI_NUM_RESOURCES) +/* exclude ROM and bridge and IOV resources */ +#define for_each_pci_dev_base_norom_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_dev_resource_n(dev, i)) || i < PCI_NUM_RESOURCES; \ + i = (i != (PCI_ROM_RESOURCE-1)) ? (i+1) : PCI_NUM_RESOURCES) +/* exclude ROM and bridge resources */ +#define for_each_pci_dev_base_iov_norom_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_dev_resource_n(dev, i)) || i < PCI_NUM_RESOURCES; \ + i = (i != (PCI_ROM_RESOURCE-1) && i != (PCI_BRIDGE_RESOURCES-1)) ? (i+1) : \ + ((i == PCI_ROM_RESOURCE-1) ? (PCI_ROM_RESOURCE+1) : PCI_NUM_RESOURCES)) +/* exclude IOV resources */ +#define for_each_pci_dev_noiov_resource(dev, res, i) \ + for (i = 0; \ + (res = pci_dev_resource_n(dev, i)) || i < PCI_NUM_RESOURCES; \ + i = (i != PCI_ROM_RESOURCE) ? (i+1) : PCI_BRIDGE_RESOURCES) +/* only std resources */ +#define for_each_pci_dev_std_resource(dev, res, i) \ + for (i = PCI_STD_RESOURCES; \ + (res = pci_dev_resource_n(dev, i)) && i < (PCI_STD_RESOURCE_END+1); \ + i++) +/* only IOV resources */ +#define for_each_pci_dev_iov_resource(dev, res, i) \ + for (i = PCI_IOV_RESOURCES; \ + (res = pci_dev_resource_n(dev, i)) && i < (PCI_IOV_RESOURCE_END+1); \ + i++) +/* only bridge resources */ +#define for_each_pci_dev_bridge_resource(dev, res, i) \ + for (i = PCI_BRIDGE_RESOURCES; \ + (res = pci_dev_resource_n(dev, i)) && i < (PCI_BRIDGE_RESOURCE_END+1); \ + i++) +/* only addon resources */ +#define for_each_pci_dev_addon_resource(dev, res, i) \ + for (i = PCI_NUM_RESOURCES; \ + (res = pci_dev_resource_n(dev, i)); \ + i++) + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV -- Actually that for-pci-for-each-res-addon branch depends on other branches like for-pci-res-alloc for-pci-root-bus-hotplug for-pci-busn-alloc for-pci-next I'd like to have busn-alloc and root-bus-hotplug setting down into pci-next tree at first. Hope we can get result in this pci min-summit 8/27. Thanks Yinghai ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH] methods to access resources of a struct pci_dev 2012-08-16 4:11 ` Yinghai Lu @ 2012-08-16 4:41 ` Ram Pai 2012-08-21 15:13 ` [RFC PATCH v2 ]pci: pci resource iterator Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-08-16 4:41 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ram Pai, Bjorn Helgaas, linux-pci On Wed, Aug 15, 2012 at 09:11:03PM -0700, Yinghai Lu wrote: > On Wed, Aug 15, 2012 at 8:26 PM, Ram Pai <linuxram@us.ibm.com> wrote: > > On Wed, Aug 15, 2012 at 03:25:06PM -0600, Bjorn Helgaas wrote: > > > > 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. > > Do you mean that you will have updated patch for > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=cd192f0ed93203ef6bac2a44c138899190fb5793 > ? Yes. > > if it is that case, i am ok, and then I could use scripts to update > following patches. Ok. Will have it your way soon. RP ^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH v2 ]pci: pci resource iterator 2012-08-16 4:41 ` Ram Pai @ 2012-08-21 15:13 ` Ram Pai 2012-08-21 23:22 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-08-21 15:13 UTC (permalink / raw) To: linux-pci; +Cc: Yinghai Lu, Bjorn Helgaas 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> 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. ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 ]pci: pci resource iterator 2012-08-21 15:13 ` [RFC PATCH v2 ]pci: pci resource iterator Ram Pai @ 2012-08-21 23:22 ` Yinghai Lu 2012-08-22 10:15 ` Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-08-21 23:22 UTC (permalink / raw) To: Ram Pai; +Cc: linux-pci, Bjorn Helgaas [-- Attachment #1: Type: text/plain, Size: 2007 bytes --] 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 [-- Attachment #2: ram_pci_it_v3.patch --] [-- Type: application/octet-stream, Size: 3340 bytes --] From: Ram Pai <linuxram@us.ibm.com> To: linux-pci@vger.kernel.org Cc: Yinghai Lu <yinghai@kernel.org>, Bjorn Helgaas <bhelgaas@google.com> Subject: [RFC PATCH v3]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. -v3: add the idx back Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- include/linux/pci.h | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -1410,6 +1410,47 @@ static inline struct pci_dev *pci_dev_ge (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) + +static inline struct resource *pci_next_resource(struct pci_dev *pdev, + struct resource *res, int flag, int *next_i) +{ + int i = *next_i; + + while (++i < PCI_NUM_RESOURCES) { + if ((i >= 0 && i < PCI_ROM_RESOURCE) && (flag & PCI_STD_RES)) + goto found; + else if ((i == PCI_ROM_RESOURCE) && (flag & PCI_ROM_RES)) + goto found; + else if ((i <= PCI_IOV_RESOURCE_END) && (flag & PCI_IOV_RES)) + goto found; + else if ((i <= PCI_BRIDGE_RESOURCE_END) && (flag & PCI_BRIDGE_RES)) + goto found; + } + + *next_id = -1; + return NULL; + +found: + *next_id = i; + return pci_dev_resource_n(i); +} + +#define for_each_pci_resource(dev, res, i, flag) \ + for (i = -1, res = pci_next_resource(dev, NULL, flag, &i); res; \ + res = pci_next_resource(dev, res, flag, &i)) + /* 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 ]pci: pci resource iterator 2012-08-21 23:22 ` Yinghai Lu @ 2012-08-22 10:15 ` Ram Pai 2012-08-22 17:31 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-08-22 10:15 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ram Pai, linux-pci, Bjorn Helgaas 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; } ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 ]pci: pci resource iterator 2012-08-22 10:15 ` Ram Pai @ 2012-08-22 17:31 ` Yinghai Lu 2012-08-22 17:35 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-08-22 17:31 UTC (permalink / raw) To: Ram Pai; +Cc: linux-pci, Bjorn Helgaas 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; ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 ]pci: pci resource iterator 2012-08-22 17:31 ` Yinghai Lu @ 2012-08-22 17:35 ` Yinghai Lu 2012-08-23 0:28 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-08-22 17:35 UTC (permalink / raw) To: Ram Pai; +Cc: linux-pci, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 ]pci: pci resource iterator 2012-08-22 17:35 ` Yinghai Lu @ 2012-08-23 0:28 ` Yinghai Lu 2012-08-23 5:09 ` [RFC PATCH v3 " Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-08-23 0:28 UTC (permalink / raw) To: Ram Pai; +Cc: linux-pci, Bjorn Helgaas [-- Attachment #1: Type: text/plain, Size: 964 bytes --] 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 [-- Attachment #2: ram_pci_it_v4.patch --] [-- Type: application/octet-stream, Size: 2246 bytes --] --- include/linux/pci.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -373,6 +373,60 @@ struct pci_dev { struct resource *pci_dev_resource_n(struct pci_dev *dev, int n); int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res); +#define PCI_STD_RES (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_BRIDGE_RES (1<<2) +#define PCI_IOV_RES (1<<3) + +#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) + +static inline void __prepare_idx_bitmap(unsigned long *mask, int flag) +{ + bitmap_zero(mask, PCI_NUM_RESOURCES); + if (flag & PCI_STD_RES) + bitmap_set(mask, PCI_STD_RESOURCES, + PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1); + if (flag & PCI_ROM_RES) + bitmap_set(mask, PCI_ROM_RESOURCE, 1); +#ifdef CONFIG_PCI_IOV + if (flag & PCI_IOV_RES) + bitmap_set(mask, PCI_IOV_RESOURCES, + PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1); +#endif + if (flag & PCI_BRIDGE_RES) + bitmap_set(mask, PCI_BRIDGE_RESOURCES, + PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1); +}; + +static inline int pci_next_resource_idx(int i, int flag) +{ + DECLARE_BITMAP(mask, PCI_NUM_RESOURCES); + + __prepare_idx_bitmap(mask, flag); + + i++; + i = find_next_bit(mask, PCI_NUM_RESOURCES, i); + + if (i < PCI_NUM_RESOURCES) + return i; + + return -1; +} + +#define for_each_pci_resource(dev, res, i, flag) \ + for (i = pci_next_resource_idx(-1, flag), \ + res = pci_dev_resource_n(dev, i); \ + res; \ + i = pci_next_resource_idx(i, flag), \ + res = pci_dev_resource_n(dev, i)) + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV [-- Attachment #3: ram_pci_it_v4_x.patch --] [-- Type: application/octet-stream, Size: 1862 bytes --] --- include/linux/pci.h | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -373,6 +373,43 @@ struct pci_dev { struct resource *pci_dev_resource_n(struct pci_dev *dev, int n); int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res); +#define PCI_STD_RES (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_BRIDGE_RES (1<<2) +#define PCI_IOV_RES (1<<3) + +#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) + +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)) + return i; + else if ((i == PCI_ROM_RESOURCE) && (flag & PCI_ROM_RES)) + return i; +#ifdef CONFIG_PCI_IOV + else if ((i <= PCI_IOV_RESOURCE_END) && (flag & PCI_IOV_RES)) + return i; +#endif + else if ((i <= PCI_BRIDGE_RESOURCE_END) && (flag & PCI_BRIDGE_RES)) + return i; + } + + return -1; +} + +#define for_each_pci_resource(dev, res, i, flag) \ + for (i = pci_next_resource_idx(-1, flag), res = pci_dev_resource_n(dev, i);\ + res; \ + i = pci_next_resource_idx(i, flag), res = pci_dev_resource_n(dev, i)) + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV ^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH v3 ]pci: pci resource iterator 2012-08-23 0:28 ` Yinghai Lu @ 2012-08-23 5:09 ` Ram Pai 2012-08-23 19:30 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-08-23 5:09 UTC (permalink / raw) To: linux-pci; +Cc: Bjorn Helgaas, Yinghai Lu, linuxram 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. Next step, change the code in all the drivers that access the resource structure to use the new iterator. 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 v2: Consolidated iterator interface as per Bjorn's suggestion. Changelog v3: ability to get index of the returned resource. Suggested by Yinghai. Incorporates Yinghai's code snippets. <TODO get Yinghai's SOB> Signed-off-by: Ram Pai <linuxram@us.ibm.com> diff --git a/include/linux/pci.h b/include/linux/pci.h index e444f5b..7101da2 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1351,6 +1351,48 @@ 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 (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_BRIDGE_RES (1<<2) +#define PCI_IOV_RES (1<<3) +#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_NOBRIDGE_RES) + +static inline struct resource *pci_dev_resource_n(struct pci_dev *dev, int n) +{ + if (n >= 0 && n < PCI_NUM_RESOURCES) + return &dev->resource[n]; + + return NULL; +} + +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; +} + +#define for_each_pci_resource(dev, res, flag) \ + for (i = pci_next_resource_idx(-1, flag), \ + res = pci_dev_resource_n(dev, i); \ + res; \ + i = pci_next_resource_idx(i, flag), \ + res = pci_dev_resource_n(dev, i)) + /* 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. ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-08-23 5:09 ` [RFC PATCH v3 " Ram Pai @ 2012-08-23 19:30 ` Yinghai Lu 2012-08-27 7:33 ` Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-08-23 19:30 UTC (permalink / raw) To: Ram Pai, Bjorn Helgaas; +Cc: linux-pci [-- Attachment #1: Type: text/plain, Size: 5709 bytes --] 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; > +} no, you can not merge them. when it start as -1, and user only need the bridge resource, it will loop from 0 to 16. I optimized it more to skip some searching. please check v5 and v6. v5 will store aside the mask, and use the bit map later. v6 will still to do the local checking, but will skip some ++i loop. it seems v5 is more efficient while it will need 16*8 bytes for mask storage. v5: pci_next_resource_idx: 000000000000098c <pci_next_resource_idx>: 98c: 55 push %rbp 98d: 48 89 e5 mov %rsp,%rbp 990: 53 push %rbx 991: 41 50 push %r8 993: e8 00 00 00 00 callq 998 <pci_next_resource_idx+0xc> 998: 89 fb mov %edi,%ebx 99a: 89 f7 mov %esi,%edi 99c: ff c3 inc %ebx 99e: e8 d1 fe ff ff callq 874 <get_res_idx_mask> 9a3: 48 89 c7 mov %rax,%rdi 9a6: 83 c8 ff or $0xffffffffffffffff,%eax 9a9: 83 fb 10 cmp $0x10,%ebx 9ac: 7f 18 jg 9c6 <pci_next_resource_idx+0x3a> 9ae: 48 63 d3 movslq %ebx,%rdx 9b1: be 11 00 00 00 mov $0x11,%esi 9b6: e8 00 00 00 00 callq 9bb <pci_next_resource_idx+0x2f> 9bb: ba ff ff ff ff mov $0xffffffff,%edx 9c0: 83 f8 11 cmp $0x11,%eax 9c3: 0f 4d c2 cmovge %edx,%eax 9c6: 59 pop %rcx 9c7: 5b pop %rbx 9c8: 5d pop %rbp 9c9: c3 retq v6: would need to more code in pci_next_resource_idx: 00000000000008e1 <pci_next_resource_idx>: 8e1: 55 push %rbp 8e2: 48 89 e5 mov %rsp,%rbp 8e5: e8 00 00 00 00 callq 8ea <pci_next_resource_idx+0x9> 8ea: 89 f1 mov %esi,%ecx 8ec: 89 f2 mov %esi,%edx 8ee: 89 f8 mov %edi,%eax 8f0: 89 f7 mov %esi,%edi 8f2: 83 e1 04 and $0x4,%ecx 8f5: 83 e2 02 and $0x2,%edx 8f8: 83 e7 08 and $0x8,%edi 8fb: 83 e6 01 and $0x1,%esi 8fe: eb 35 jmp 935 <pci_next_resource_idx+0x54> 900: 85 c0 test %eax,%eax 902: 78 1c js 920 <pci_next_resource_idx+0x3f> 904: 83 f8 05 cmp $0x5,%eax 907: 7f 07 jg 910 <pci_next_resource_idx+0x2f> 909: 40 84 f6 test %sil,%sil 90c: 75 38 jne 946 <pci_next_resource_idx+0x65> 90e: eb 05 jmp 915 <pci_next_resource_idx+0x34> 910: 83 f8 06 cmp $0x6,%eax 913: 75 0b jne 920 <pci_next_resource_idx+0x3f> 915: 85 d2 test %edx,%edx 917: 75 28 jne 941 <pci_next_resource_idx+0x60> 919: b8 07 00 00 00 mov $0x7,%eax 91e: eb 0a jmp 92a <pci_next_resource_idx+0x49> 920: 83 f8 06 cmp $0x6,%eax 923: 7e 10 jle 935 <pci_next_resource_idx+0x54> 925: 83 f8 0c cmp $0xc,%eax 928: 7f 1e jg 948 <pci_next_resource_idx+0x67> 92a: 85 c9 test %ecx,%ecx 92c: 75 18 jne 946 <pci_next_resource_idx+0x65> 92e: b8 0d 00 00 00 mov $0xd,%eax 933: eb 13 jmp 948 <pci_next_resource_idx+0x67> 935: ff c0 inc %eax 937: 83 f8 10 cmp $0x10,%eax 93a: 7e c4 jle 900 <pci_next_resource_idx+0x1f> 93c: 83 c8 ff or $0xffffffffffffffff,%eax 93f: eb 05 jmp 946 <pci_next_resource_idx+0x65> 941: b8 06 00 00 00 mov $0x6,%eax 946: 5d pop %rbp 947: c3 retq 948: 85 ff test %edi,%edi 94a: 75 fa jne 946 <pci_next_resource_idx+0x65> 94c: b8 11 00 00 00 mov $0x11,%eax 951: eb e2 jmp 935 <pci_next_resource_idx+0x54> So just -v5 from now? Please let me know which one is good, so I could rebase for-pci-for-each-res-addon branch, and then Ram could rebase his patches to top of new for-pci-each-res-addon branch. Thanks Yinghai [-- Attachment #2: ram_pci_it_v5.patch --] [-- Type: application/octet-stream, Size: 3131 bytes --] --- drivers/pci/probe.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 24 ++++++++++++++++++++++++ 2 files changed, 72 insertions(+) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -373,6 +373,30 @@ struct pci_dev { struct resource *pci_dev_resource_n(struct pci_dev *dev, int n); int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res); +#define PCI_STD_RES (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_IOV_RES (1<<2) +#define PCI_BRIDGE_RES (1<<3) +#define PCI_RES_BLOCK_NUM 4 + +#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) + +int pci_next_resource_idx(int i, int flag); + +#define for_each_pci_resource(dev, res, i, flag) \ + for (i = pci_next_resource_idx(-1, flag), \ + res = pci_dev_resource_n(dev, i); \ + res; \ + i = pci_next_resource_idx(i, flag), \ + res = pci_dev_resource_n(dev, i)) + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV Index: linux-2.6/drivers/pci/probe.c =================================================================== --- linux-2.6.orig/drivers/pci/probe.c +++ linux-2.6/drivers/pci/probe.c @@ -124,6 +124,54 @@ int pci_dev_resource_idx(struct pci_dev return -1; } +static void __init_res_idx_mask(unsigned long *mask, int flag) +{ + bitmap_zero(mask, PCI_NUM_RESOURCES); + if (flag & PCI_STD_RES) + bitmap_set(mask, PCI_STD_RESOURCES, + PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1); + if (flag & PCI_ROM_RES) + bitmap_set(mask, PCI_ROM_RESOURCE, 1); +#ifdef CONFIG_PCI_IOV + if (flag & PCI_IOV_RES) + bitmap_set(mask, PCI_IOV_RESOURCES, + PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1); +#endif + if (flag & PCI_BRIDGE_RES) + bitmap_set(mask, PCI_BRIDGE_RESOURCES, + PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1); +} + +static bool res_idx_mask_inited; +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES); +static unsigned long *get_res_idx_mask(int flag) +{ + int i; + + if (!res_idx_mask_inited) { + for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++) + __init_res_idx_mask(res_idx_mask[i], i); + + res_idx_mask_inited = true; + } + + return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)]; +} + +int pci_next_resource_idx(int i, int flag) +{ + unsigned long *mask = get_res_idx_mask(flag); + + i++; + if (i < PCI_NUM_RESOURCES) + i = find_next_bit(mask, PCI_NUM_RESOURCES, i); + + if (i < PCI_NUM_RESOURCES) + return i; + + return -1; +} + static u64 pci_size(u64 base, u64 maxbase, u64 mask) { u64 size = mask & maxbase; /* Find the significant bits */ [-- Attachment #3: ram_pci_it_v6.patch --] [-- Type: application/octet-stream, Size: 2658 bytes --] --- drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pci.h | 23 +++++++++++++++++++++++ 2 files changed, 57 insertions(+) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -373,6 +373,29 @@ struct pci_dev { struct resource *pci_dev_resource_n(struct pci_dev *dev, int n); int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res); +#define PCI_STD_RES (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_IOV_RES (1<<2) +#define PCI_BRIDGE_RES (1<<3) + +#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) + +int pci_next_resource_idx(int i, int flag); + +#define for_each_pci_resource(dev, res, i, flag) \ + for (i = pci_next_resource_idx(-1, flag), \ + res = pci_dev_resource_n(dev, i); \ + res; \ + i = pci_next_resource_idx(i, flag), \ + res = pci_dev_resource_n(dev, i)) + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV Index: linux-2.6/drivers/pci/probe.c =================================================================== --- linux-2.6.orig/drivers/pci/probe.c +++ linux-2.6/drivers/pci/probe.c @@ -124,6 +124,40 @@ int pci_dev_resource_idx(struct pci_dev return -1; } +static bool check_resource_idx(int *i, int start, int end, bool match) +{ + if (*i >= start && *i <= end) { + if (match) + return true; + else + *i = end + 1; + } + return false; +} + +int pci_next_resource_idx(int i, int flag) +{ + while (++i < PCI_NUM_RESOURCES) { + if (check_resource_idx(&i, PCI_STD_RESOURCES, + PCI_STD_RESOURCE_END, !!(flag & PCI_STD_RES))) + return i; + if (check_resource_idx(&i, PCI_ROM_RESOURCE, + PCI_ROM_RESOURCE, !!(flag & PCI_ROM_RES))) + return i; +#ifdef CONFIG_PCI_IOV + if (check_resource_idx(&i, PCI_IOV_RESOURCES, + PCI_IOV_RESOURCE_END, !!(flag & PCI_IOV_RES))) + return i; +#endif + if (check_resource_idx(&i, PCI_BRIDGE_RESOURCES, + PCI_BRIDGE_RESOURCE_END, + !!(flag & PCI_BRIDGE_RES))) + return i; + } + + return -1; +} + static u64 pci_size(u64 base, u64 maxbase, u64 mask) { u64 size = mask & maxbase; /* Find the significant bits */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-08-23 19:30 ` Yinghai Lu @ 2012-08-27 7:33 ` Ram Pai 2012-09-03 8:07 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-08-27 7:33 UTC (permalink / raw) To: Yinghai Lu; +Cc: Bjorn Helgaas, linux-pci 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; > > +} > > no, you can not merge them. > when it start as -1, and user only need the bridge resource, it will > loop from 0 to 16. True. But the logic by itself is not wrong. It is sub-optimal. > > I optimized it more to skip some searching. please check v5 and v6. > v5 will store aside the mask, and use the bit map later. > v6 will still to do the local checking, but will skip some ++i loop. :) The v5 patch; the bitmask one, took me some time to understand. But yes it does a good job. There is one thing I dont like in that patch. You have to call to pci_dev_resource_n(), but that function is not there anywhere in upstream tree. Its only in your tree. Can we bring pci_dev_resource_n() into this patch and make it self-sustained? Also, please find below, one other patch proposal, which improves upon your v5 patch. Instead of having a array of bitmask, this patch maintains a single dimensional array with an entry for each resource. Each entry captures its type; ie PCI_RES_*, and index of the starting location of the next resource type. Using this information one can quickly cut down on the number of useless iterations. It also does compile time initialization of the array, unlike the v5 patch which does runtime initialization. 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 */ +}; +#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); + } + return -1; +} + +struct resource *pci_dev_resource_n(struct pci_dev *dev, int n) +{ + if (n >= 0 && n < PCI_NUM_RESOURCES) + return &dev->resource[n]; + + return NULL; +} + + static u64 pci_size(u64 base, u64 maxbase, u64 mask) { u64 size = mask & maxbase; /* Find the significant bits */ diff --git a/include/linux/pci.h b/include/linux/pci.h index e444f5b..adae706 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1351,6 +1351,25 @@ 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 (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_BRIDGE_RES (1<<2) +#define PCI_IOV_RES (1<<3) +#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_NOBRIDGE_RES) +#define for_each_pci_resource(dev, res, flag) \ + for (i = pci_next_resource_idx(-1, flag), \ + res = pci_dev_resource_n(dev, i); \ + res; \ + i = pci_next_resource_idx(i, flag), \ + res = pci_dev_resource_n(dev, i)) + /* 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. ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-08-27 7:33 ` Ram Pai @ 2012-09-03 8:07 ` Yinghai Lu 2012-09-03 9:08 ` Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-09-03 8:07 UTC (permalink / raw) To: Ram Pai; +Cc: Bjorn Helgaas, linux-pci 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; >> > +} >> >> no, you can not merge them. >> when it start as -1, and user only need the bridge resource, it will >> loop from 0 to 16. > > True. But the logic by itself is not wrong. It is sub-optimal. > >> >> I optimized it more to skip some searching. please check v5 and v6. >> v5 will store aside the mask, and use the bit map later. >> v6 will still to do the local checking, but will skip some ++i loop. > > :) The v5 patch; the bitmask one, took me some time to understand. > But yes it does a good job. > > There is one thing I dont like in that patch. You have to call to > pci_dev_resource_n(), but that function is not there anywhere in > upstream tree. Its only in your tree. Can we bring pci_dev_resource_n() > into this patch and make it self-sustained? I'd like to keep every patch to be smaller. > > Also, please find below, one other patch proposal, which improves upon > your v5 patch. Instead of having a array of bitmask, this patch > maintains a single dimensional array with an entry for each resource. > Each entry captures its type; ie PCI_RES_*, and index of the starting location > of the next resource type. Using this information one can quickly cut down on > the number of useless iterations. It also does compile time > initialization of the array, unlike the v5 patch which does runtime > initialization. only one time. and could move the initialization call to other place. > > 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? 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. > + return -1; > +} > + > +struct resource *pci_dev_resource_n(struct pci_dev *dev, int n) > +{ > + if (n >= 0 && n < PCI_NUM_RESOURCES) > + return &dev->resource[n]; > + > + return NULL; > +} > + > + > static u64 pci_size(u64 base, u64 maxbase, u64 mask) > { > u64 size = mask & maxbase; /* Find the significant bits */ > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e444f5b..adae706 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1351,6 +1351,25 @@ 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 (1<<0) > +#define PCI_ROM_RES (1<<1) > +#define PCI_BRIDGE_RES (1<<2) > +#define PCI_IOV_RES (1<<3) > +#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_NOBRIDGE_RES) > +#define for_each_pci_resource(dev, res, flag) \ > + for (i = pci_next_resource_idx(-1, flag), \ > + res = pci_dev_resource_n(dev, i); \ > + res; \ > + i = pci_next_resource_idx(i, flag), \ > + res = pci_dev_resource_n(dev, i)) > + > /* 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. > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-09-03 8:07 ` Yinghai Lu @ 2012-09-03 9:08 ` Ram Pai 2012-09-03 18:20 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-09-03 9:08 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ram Pai, Bjorn Helgaas, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-09-03 9:08 ` Ram Pai @ 2012-09-03 18:20 ` Yinghai Lu 2012-09-04 3:27 ` Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-09-03 18:20 UTC (permalink / raw) To: Ram Pai; +Cc: Bjorn Helgaas, linux-pci [-- Attachment #1: Type: text/plain, Size: 631 bytes --] On Mon, Sep 3, 2012 at 2:08 AM, Ram Pai <linuxram@us.ibm.com> wrote: > On Mon, Sep 03, 2012 at 01:07:46AM -0700, Yinghai Lu wrote: > > Anyway I am ok with either patch. please check -v7. final next_idx finding will be like: static inline unsigned long *get_res_idx_mask(int flag) { return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)]; } int pci_next_resource_idx(int i, int flag) { i++; if (i < PCI_NUM_RESOURCES) i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i); if (i < PCI_NUM_RESOURCES) return i; return -1; } Thanks Yinghai [-- Attachment #2: ram_pci_it_v7.patch --] [-- Type: application/octet-stream, Size: 4273 bytes --] From: Ram Pai <linuxram@us.ibm.com> To: linux-pci@vger.kernel.org Subject: [PATCH v7]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. Finally we can re-organize the resource structure in the pci_dev structure and correspondingly update the methods. -v2: Consolidated iterator interface as per Bjorn's suggestion. -v3: Add the idx back - Yinghai Lu -v7: Change to use bitmap for searching - Yinghai Lu Signed-off-by: Ram Pai <linuxram@us.ibm.com> Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/probe.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 24 ++++++++++++++++++++++++ 2 files changed, 71 insertions(+) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -360,6 +360,30 @@ struct pci_dev { struct resource *pci_dev_resource_n(struct pci_dev *dev, int n); int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res); +#define PCI_STD_RES (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_IOV_RES (1<<2) +#define PCI_BRIDGE_RES (1<<3) +#define PCI_RES_BLOCK_NUM 4 + +#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) + +int pci_next_resource_idx(int i, int flag); + +#define for_each_pci_resource(dev, res, i, flag) \ + for (i = pci_next_resource_idx(-1, flag), \ + res = pci_dev_resource_n(dev, i); \ + res; \ + i = pci_next_resource_idx(i, flag), \ + res = pci_dev_resource_n(dev, i)) + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV Index: linux-2.6/drivers/pci/probe.c =================================================================== --- linux-2.6.orig/drivers/pci/probe.c +++ linux-2.6/drivers/pci/probe.c @@ -123,6 +123,53 @@ int pci_dev_resource_idx(struct pci_dev return -1; } +static void __init_res_idx_mask(unsigned long *mask, int flag) +{ + bitmap_zero(mask, PCI_NUM_RESOURCES); + if (flag & PCI_STD_RES) + bitmap_set(mask, PCI_STD_RESOURCES, + PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1); + if (flag & PCI_ROM_RES) + bitmap_set(mask, PCI_ROM_RESOURCE, 1); +#ifdef CONFIG_PCI_IOV + if (flag & PCI_IOV_RES) + bitmap_set(mask, PCI_IOV_RESOURCES, + PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1); +#endif + if (flag & PCI_BRIDGE_RES) + bitmap_set(mask, PCI_BRIDGE_RESOURCES, + PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1); +} + +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES); +static int __init pci_res_idx_mask_init(void) +{ + int i; + + for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++) + __init_res_idx_mask(res_idx_mask[i], i); + + return 0; +} +postcore_initcall(pci_res_idx_mask_init); + +static inline unsigned long *get_res_idx_mask(int flag) +{ + return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)]; +} + +int pci_next_resource_idx(int i, int flag) +{ + i++; + if (i < PCI_NUM_RESOURCES) + i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i); + + if (i < PCI_NUM_RESOURCES) + return i; + + return -1; +} + static u64 pci_size(u64 base, u64 maxbase, u64 mask) { u64 size = mask & maxbase; /* Find the significant bits */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-09-03 18:20 ` Yinghai Lu @ 2012-09-04 3:27 ` Ram Pai 2012-09-18 0:03 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-09-04 3:27 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ram Pai, Bjorn Helgaas, linux-pci On Mon, Sep 03, 2012 at 11:20:45AM -0700, Yinghai Lu wrote: > On Mon, Sep 3, 2012 at 2:08 AM, Ram Pai <linuxram@us.ibm.com> wrote: > > On Mon, Sep 03, 2012 at 01:07:46AM -0700, Yinghai Lu wrote: > > > > Anyway I am ok with either patch. > > please check -v7. Looks good to me. I am inlining the patch for others to comment. BTW: your patch that introduces pci_dev_resource_n() will have to be applied before applying this patch. From: Ram Pai <linuxram@us.ibm.com> To: linux-pci@vger.kernel.org Subject: [PATCH v7]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. Finally we can re-organize the resource structure in the pci_dev structure and correspondingly update the methods. -v2: Consolidated iterator interface as per Bjorn's suggestion. -v3: Add the idx back - Yinghai Lu -v7: Change to use bitmap for searching - Yinghai Lu Signed-off-by: Ram Pai <linuxram@us.ibm.com> Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/probe.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 24 ++++++++++++++++++++++++ 2 files changed, 71 insertions(+) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -360,6 +360,30 @@ struct pci_dev { struct resource *pci_dev_resource_n(struct pci_dev *dev, int n); int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res); +#define PCI_STD_RES (1<<0) +#define PCI_ROM_RES (1<<1) +#define PCI_IOV_RES (1<<2) +#define PCI_BRIDGE_RES (1<<3) +#define PCI_RES_BLOCK_NUM 4 + +#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) + +int pci_next_resource_idx(int i, int flag); + +#define for_each_pci_resource(dev, res, i, flag) \ + for (i = pci_next_resource_idx(-1, flag), \ + res = pci_dev_resource_n(dev, i); \ + res; \ + i = pci_next_resource_idx(i, flag), \ + res = pci_dev_resource_n(dev, i)) + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV Index: linux-2.6/drivers/pci/probe.c =================================================================== --- linux-2.6.orig/drivers/pci/probe.c +++ linux-2.6/drivers/pci/probe.c @@ -123,6 +123,53 @@ int pci_dev_resource_idx(struct pci_dev return -1; } +static void __init_res_idx_mask(unsigned long *mask, int flag) +{ + bitmap_zero(mask, PCI_NUM_RESOURCES); + if (flag & PCI_STD_RES) + bitmap_set(mask, PCI_STD_RESOURCES, + PCI_STD_RESOURCE_END - PCI_STD_RESOURCES + 1); + if (flag & PCI_ROM_RES) + bitmap_set(mask, PCI_ROM_RESOURCE, 1); +#ifdef CONFIG_PCI_IOV + if (flag & PCI_IOV_RES) + bitmap_set(mask, PCI_IOV_RESOURCES, + PCI_IOV_RESOURCE_END - PCI_IOV_RESOURCES + 1); +#endif + if (flag & PCI_BRIDGE_RES) + bitmap_set(mask, PCI_BRIDGE_RESOURCES, + PCI_BRIDGE_RESOURCE_END - PCI_BRIDGE_RESOURCES + 1); +} + +static DECLARE_BITMAP(res_idx_mask[1 << PCI_RES_BLOCK_NUM], PCI_NUM_RESOURCES); +static int __init pci_res_idx_mask_init(void) +{ + int i; + + for (i = 0; i < (1 << PCI_RES_BLOCK_NUM); i++) + __init_res_idx_mask(res_idx_mask[i], i); + + return 0; +} +postcore_initcall(pci_res_idx_mask_init); + +static inline unsigned long *get_res_idx_mask(int flag) +{ + return res_idx_mask[flag & ((1 << PCI_RES_BLOCK_NUM) - 1)]; +} + +int pci_next_resource_idx(int i, int flag) +{ + i++; + if (i < PCI_NUM_RESOURCES) + i = find_next_bit(get_res_idx_mask(flag), PCI_NUM_RESOURCES, i); + + if (i < PCI_NUM_RESOURCES) + return i; + + return -1; +} + static u64 pci_size(u64 base, u64 maxbase, u64 mask) { u64 size = mask & maxbase; /* Find the significant bits */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-09-04 3:27 ` Ram Pai @ 2012-09-18 0:03 ` Yinghai Lu 2012-09-21 6:18 ` Ram Pai 0 siblings, 1 reply; 24+ messages in thread From: Yinghai Lu @ 2012-09-18 0:03 UTC (permalink / raw) To: Ram Pai; +Cc: Bjorn Helgaas, linux-pci On Mon, Sep 3, 2012 at 8:27 PM, Ram Pai <linuxram@us.ibm.com> wrote: > On Mon, Sep 03, 2012 at 11:20:45AM -0700, Yinghai Lu wrote: >> On Mon, Sep 3, 2012 at 2:08 AM, Ram Pai <linuxram@us.ibm.com> wrote: >> > On Mon, Sep 03, 2012 at 01:07:46AM -0700, Yinghai Lu wrote: >> > >> > Anyway I am ok with either patch. >> >> please check -v7. > > Looks good to me. I am inlining the patch for others to comment. > BTW: your patch that introduces pci_dev_resource_n() will have > to be applied before applying this patch. > > just updated for-pci-for-each-res-addon branch... http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-for-each-res-addon now it only depends on pci-root-bus-hotplug now, and pci-root-bus-hotplug only depends on pci/next without busn-alloc. So after Bjorn accept pci-root-bus-hotplug, we could push that to him. Please check for-pci-for-each-addon branch. git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-for-each-res-addon Thanks Yinghai ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-09-18 0:03 ` Yinghai Lu @ 2012-09-21 6:18 ` Ram Pai 2012-09-21 6:27 ` Yinghai Lu 0 siblings, 1 reply; 24+ messages in thread From: Ram Pai @ 2012-09-21 6:18 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ram Pai, Bjorn Helgaas, linux-pci On Mon, Sep 17, 2012 at 05:03:56PM -0700, Yinghai Lu wrote: > On Mon, Sep 3, 2012 at 8:27 PM, Ram Pai <linuxram@us.ibm.com> wrote: > > On Mon, Sep 03, 2012 at 11:20:45AM -0700, Yinghai Lu wrote: > >> On Mon, Sep 3, 2012 at 2:08 AM, Ram Pai <linuxram@us.ibm.com> wrote: > >> > On Mon, Sep 03, 2012 at 01:07:46AM -0700, Yinghai Lu wrote: > >> > > >> > Anyway I am ok with either patch. > >> > >> please check -v7. > > > > Looks good to me. I am inlining the patch for others to comment. > > BTW: your patch that introduces pci_dev_resource_n() will have > > to be applied before applying this patch. > > > > > > just updated for-pci-for-each-res-addon branch... > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-for-each-res-addon > > now it only depends on pci-root-bus-hotplug now, and > pci-root-bus-hotplug only depends on pci/next without busn-alloc. > > So after Bjorn accept pci-root-bus-hotplug, we could push that to him. > > Please check for-pci-for-each-addon branch. > git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git > for-pci-for-each-res-addon Checked and the patch looks good. But later you have added another patch which introduces lot many macros ... Will that patch also be pushed to Bjorn? I thought Bjorn resisted the idea of having multiple such iterators.. This is the patch I am talking about: commit a37fbc4685ab086581f9842630c997ca78186ea4 Author: Yinghai Lu <yinghai@kernel.org> Date: Mon Sep 3 00:32:00 2012 -0700 PCI: Add for_each_resource helpers to make resource loop easier. +#define resno_is_for_bridge(n) \ + ((n) >= PCI_BRIDGE_RESOURCES && (n) <= PCI_BRIDGE_RESOURCE_END) + +/* all (include bridge) resources */ +#define for_each_pci_dev_all_resource(dev, res, i) \ + for_each_pci_resource(dev, res, i, PCI_ALL_RES) ....... ...... -- Ram Pai ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v3 ]pci: pci resource iterator 2012-09-21 6:18 ` Ram Pai @ 2012-09-21 6:27 ` Yinghai Lu 0 siblings, 0 replies; 24+ messages in thread From: Yinghai Lu @ 2012-09-21 6:27 UTC (permalink / raw) To: Ram Pai; +Cc: Bjorn Helgaas, linux-pci On Thu, Sep 20, 2012 at 11:18 PM, Ram Pai <linuxram@us.ibm.com> wrote: > Checked and the patch looks good. > > But later you have added another patch which introduces lot many macros ... > > Will that patch also be pushed to Bjorn? I thought Bjorn resisted the > idea of having multiple such iterators.. no. I removed that patches 3 days ago... please check it again. http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-for-each-res-addon -Yinghai ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2012-09-21 6:27 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-06-18 5:03 [RFC PATCH] methods to access resources of a struct pci_dev Ram Pai 2012-06-18 18:30 ` Yinghai Lu 2012-06-19 1:46 ` Ram Pai 2012-06-19 2:57 ` Yinghai Lu 2012-08-15 21:25 ` Bjorn Helgaas 2012-08-16 3:26 ` Ram Pai 2012-08-16 4:11 ` Yinghai Lu 2012-08-16 4:41 ` Ram Pai 2012-08-21 15:13 ` [RFC PATCH v2 ]pci: pci resource iterator Ram Pai 2012-08-21 23:22 ` Yinghai Lu 2012-08-22 10:15 ` Ram Pai 2012-08-22 17:31 ` Yinghai Lu 2012-08-22 17:35 ` Yinghai Lu 2012-08-23 0:28 ` Yinghai Lu 2012-08-23 5:09 ` [RFC PATCH v3 " Ram Pai 2012-08-23 19:30 ` Yinghai Lu 2012-08-27 7:33 ` Ram Pai 2012-09-03 8:07 ` Yinghai Lu 2012-09-03 9:08 ` Ram Pai 2012-09-03 18:20 ` Yinghai Lu 2012-09-04 3:27 ` Ram Pai 2012-09-18 0:03 ` Yinghai Lu 2012-09-21 6:18 ` Ram Pai 2012-09-21 6:27 ` Yinghai Lu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.