Patchwork [RFC,v3] pci: pci resource iterator

login
register
mail settings
Submitter Ram Pai
Date Aug. 23, 2012, 5:09 a.m.
Message ID <20120823050958.GB2332@ram-ThinkPad-T61>
Download mbox | patch
Permalink /patch/179501/
State Superseded
Headers show

Comments

Ram Pai - Aug. 23, 2012, 5:09 a.m.
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>


--
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, 7:30 p.m.
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

Patch

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.