diff mbox

[RFC] methods to access resources of a struct pci_dev

Message ID 20120618050333.GA13469@ram-ThinkPad-T61
State Rejected
Headers show

Commit Message

Ram Pai June 18, 2012, 5:03 a.m. UTC 
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>


--
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

Comments

Yinghai Lu June 18, 2012, 6:30 p.m. UTC | #1 
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
--
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
Ram Pai June 19, 2012, 1:46 a.m. UTC | #2 
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

--
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
Yinghai Lu June 19, 2012, 2:57 a.m. UTC | #3 
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
--
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
Bjorn Helgaas Aug. 15, 2012, 9:25 p.m. UTC | #4 
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
--
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
Ram Pai Aug. 16, 2012, 3:26 a.m. UTC | #5 
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

--
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
diff mbox

Patch

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