diff mbox

[RFC,v2] pci: pci resource iterator

Message ID 20120821151245.GA2356@ram-ThinkPad-T61
State Superseded
Headers show

Commit Message

Ram Pai Aug. 21, 2012, 3:13 p.m. UTC 
PCI: pci resource iterator
    
Currently pci_dev structure holds an array of 17 PCI resources; six base
BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
A bridge device just needs the 4 bridge resources. A non-bridge device
just needs the six base resources and one ROM resource. The sriov
resources are needed only if the device has SRIOV capability.
    
The pci_dev structure needs to be re-organized to avoid unnecessary
bloating.  However too much code outside the pci-bus driver, assumes the
internal details of the pci_dev structure, thus making it hard to
re-organize the datastructure.
    
As a first step this patch provides generic methods to access the
resource structure of the pci_dev.
    
Once this patch is accepted, I have another 40+ patches that modify all
the files that directly access the resource structure, to use the new
methods provided in the first step.
    
Finally we can re-organize the resource structure in the pci_dev
structure and correspondingly update the methods.

This patch is compile tested only.

Changelog: 
	Consolidated iterator interface as per Bjorn's suggestion.
    
Signed-off-by: Ram Pai <linuxram@us.ibm.com>


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Yinghai Lu Aug. 21, 2012, 11:22 p.m. UTC | #1 
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
Ram Pai Aug. 22, 2012, 10:15 a.m. UTC | #2 
On Tue, Aug 21, 2012 at 04:22:52PM -0700, Yinghai Lu wrote:
> On Tue, Aug 21, 2012 at 8:13 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> > PCI: pci resource iterator
> >
> > Currently pci_dev structure holds an array of 17 PCI resources; six base
> > BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs.  This is wasteful.
> > A bridge device just needs the 4 bridge resources. A non-bridge device
> > just needs the six base resources and one ROM resource. The sriov
> > resources are needed only if the device has SRIOV capability.
> >
> > The pci_dev structure needs to be re-organized to avoid unnecessary
> > bloating.  However too much code outside the pci-bus driver, assumes the
> > internal details of the pci_dev structure, thus making it hard to
> > re-organize the datastructure.
> >
> > As a first step this patch provides generic methods to access the
> > resource structure of the pci_dev.
> >
> > Once this patch is accepted, I have another 40+ patches that modify all
> > the files that directly access the resource structure, to use the new
> > methods provided in the first step.
> >
> > Finally we can re-organize the resource structure in the pci_dev
> > structure and correspondingly update the methods.
> >
> > This patch is compile tested only.
> >
> > Changelog:
> >         Consolidated iterator interface as per Bjorn's suggestion.
> >
> > +#define for_each_pci_resource(dev, res, flag) \
> > +       for (res = pci_next_resource(dev, NULL, flag); res; \
> > +                       res = pci_next_resource(dev, res, flag))
> > +
> 
> We may need to keep the idx, so we could make the converting more granularity.
> 
> because some loop body is still using the idx.
> 
> also there is some abusing pci bridge resource as addon resources.
> and we need to remove the abusing at first ---
> that is addressed by:
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=5eb48c3c998257386f67a7570778872ec600138f
> 
>  PCI: Add addon_resource support for pci_dev
> 
> and later we may remove the idx in the for_each_pci_resource()
> 
> Please check updated version of your patch that keep the idx.

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

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

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

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Aug. 22, 2012, 5:31 p.m. UTC | #3 
On Wed, Aug 22, 2012 at 3:15 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Tue, Aug 21, 2012 at 04:22:52PM -0700, Yinghai Lu wrote:
>> On Tue, Aug 21, 2012 at 8:13 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> by exposing idx through the interface, we are exposing the implementation to
> the enduser. I want the end user not know that the resources are
> structured as a array. This will help easily restructure resources
> in the pci_dev structure to whatever implementation we want, linked list
> or hash or whatever...
>
> Why can't the addon resource be hidden behind the interface? something
> like this?

keep the idx would make thing simple for referencing and converting.

pci_dev_resource_n() would be wrapper to from idx to real resource pointer.
we just need to change &dev->resource[i] referencing to
pci_dev_resource_n(dev, i) instead.

We did that before for irq_desc storage converting, and now we have
     for_each_irq_desc(irq, desc)

later for the resource allocation, I will change resource member from
          struct resource resource[DEVICE_COUNT_RESOURCE];
to
          struct resource *std_res;
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Aug. 22, 2012, 5:35 p.m. UTC | #4 
On Wed, Aug 22, 2012 at 10:31 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Aug 22, 2012 at 3:15 AM, Ram Pai <linuxram@us.ibm.com> wrote:
>> On Tue, Aug 21, 2012 at 04:22:52PM -0700, Yinghai Lu wrote:
>>> On Tue, Aug 21, 2012 at 8:13 AM, Ram Pai <linuxram@us.ibm.com> wrote:
>> by exposing idx through the interface, we are exposing the implementation to
>> the enduser. I want the end user not know that the resources are
>> structured as a array. This will help easily restructure resources
>> in the pci_dev structure to whatever implementation we want, linked list
>> or hash or whatever...
>>
>> Why can't the addon resource be hidden behind the interface? something
>> like this?
>
 keep the idx would make thing simple for referencing and converting.

 pci_dev_resource_n() would be wrapper to from idx to real resource pointer.
 we just need to change &dev->resource[i] referencing to
 pci_dev_resource_n(dev, i) instead.

 We did that before for irq_desc storage converting, and now we have
      for_each_irq_desc(irq, desc)

 later for the resource allocation, I will change resource member from
           struct resource resource[DEVICE_COUNT_RESOURCE];
 to
           struct resource std_res[7];
           struct resource *iov_res;
           struct resource *bridge_res;
and allocate iov_res and bridge array as needed.

anyway keep the overalll idx is good way to go and follow.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Aug. 23, 2012, 12:28 a.m. UTC | #5 
On Wed, Aug 22, 2012 at 10:35 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Aug 22, 2012 at 10:31 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>
>  keep the idx would make thing simple for referencing and converting.
>
>  pci_dev_resource_n() would be wrapper to from idx to real resource pointer.
>  we just need to change &dev->resource[i] referencing to
>  pci_dev_resource_n(dev, i) instead.
>
>  We did that before for irq_desc storage converting, and now we have
>       for_each_irq_desc(irq, desc)
>
>  later for the resource allocation, I will change resource member from
>            struct resource resource[DEVICE_COUNT_RESOURCE];
>  to
>            struct resource std_res[7];
>            struct resource *iov_res;
>            struct resource *bridge_res;
> and allocate iov_res and bridge array as needed.
>
> anyway keep the overalll idx is good way to go and follow.
>

Like to suggest two versions that only handle idx...

Thanks

Yinghai
diff mbox

Patch

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.