Re: [PATCH v3 03/27] PCI: pci resource iterator
From: Yinghai Lu
Date: Wed Apr 10 2013 - 12:12:24 EST
On Thu, Apr 4, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> On Wed, Mar 13, 2013 at 5:27 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>> From: Ram Pai <linuxram@xxxxxxxxxx>
>>
>> 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
>> -v8: Fix acpiphp module compiling error that is found by
>> Steven Newbury <steve@xxxxxxxxxxxxxxx> - Yinghai Lu
>>
>> Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
>> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
>> ---
>> drivers/pci/probe.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/pci.h | 24 ++++++++++++++++++++++++
>> 2 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 1df75f7..ac751a6 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -123,6 +123,54 @@ int pci_dev_resource_idx(struct pci_dev *dev, struct resource *res)
>> 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;
>> +}
>> +EXPORT_SYMBOL(pci_next_resource_idx);
>> +
>> 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 aefff8b..127a856 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -341,6 +341,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
>
> I think this turned out to be a disaster, with all the bitmaps and
> helper functions. Filtering in the bodies of the
> for_each_pci_resource() users has *got* to be better. That probably
> requires a wrapper struct around the struct resource. Or possibly
> having a wrapper struct with a "type" or "flags" field would make
> filtering in for_each_pci_resources() itself cleaner to implement.
Do you mean have extra struct that will wrapper resource ?
and the struct will have type that will tell that resource is PCI bridge
resource or other std/sriov resource?
then that struct need to have BAR in it.
that looks like addon_res that is added in this patchset.
but for the std/bridge/sriov, idx is used all the way and in all over
the drivers.
>
> I think it would also help if we got rid of all the "PCI_NO*_RES"
> definitions and just had the users explicitly specify what they *do*
> want.
yes, remove PCI_NO*_RES should be more clean
Thanks
Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/