Re: [PATCH v3 03/27] PCI: pci resource iterator

[Ram Pai]

On Thu, Apr 25, 2013 at 11:22:59AM -0600, Bjorn Helgaas wrote:
> On Wed, Apr 24, 2013 at 9:55 PM, Ram Pai <linuxram@xxxxxxxxxx> wrote:
> > On Wed, Apr 10, 2013 at 09:22:48AM -0600, Bjorn Helgaas wrote:
> >> On Mon, Apr 8, 2013 at 10:51 PM, Ram Pai <linuxram@xxxxxxxxxx> wrote:
> >> > On Thu, Apr 04, 2013 at 04:18:01PM -0600, Bjorn Helgaas 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.
> >> >> >
> ...
> >> > I agree. There are two cleanups needed.
> >> >
> >> > a) pci drivers should not assume the internal organization of the
> >> >         resources in the struct pci_dev.
> >>
> >> Do you mean that drivers should not use "pci_dev->resource[i]"?  If
> >> so, I agree that it would be great if we had an accessor for BARs, but
> >> it seems impractical to change all the drivers that use the current
> >> style.
> >
> > Sorry for the delay. Was vacationing.  I mean, we cannot let drivers
> > assume anything about the how the resources are organized.
> >
> > The only thing the drivers should know is that there are 6 normal
> > resources, 4 bridge resources, 1 ROM resource and 6 iov resources.
> >
> > Currently the drivers assume that ROM resource follows normal resources
> > followed by IOV followed by bridge. These assumptions are making it hard
> > to re-organize the layout of resources in struct pci_dev.
> >
> > I think we need to expose the following interfaces to drivers.
> >
> > a) return the nth normal resource
> 
> I think this needs to remain "pci_dev->resource[n]", because so many
> drivers do this that it would be impractical to change them all.

Scanning through the entire kernel tree, I did find about 40 different
drivers that are accessing pci_dev->resource[n]. These drivers can
be changed to use the newer interface. Out-of-tree drivers can continue
to access it directly, but they will break, when the
datastructure is eventually re-organized.

I was thinking of a interface something like

pci_get_std_resource(dev,i) which is implemented internally as

#define pci_get_std_resource(dev,i) dev->resource[i]

> 
> > b) return the nth iov resource
> 
> I could imagine a new interface for this, given that I only see a
> dozen SR-IOV drivers in the tree.  There might be a few out-of-tree,
> but there probably aren't many.
> 
> > c) return the rom resource
> 
> There are only about 30 drivers in the tree that reference
> PCI_ROM_RESOURCE.  Fewer than I expected, but I'd still be hesitant
> about make "pci_dev->resource[PCI_ROM_RESOURCE]" stop working.

It will work till someday when the datastructure is re-organized.

Again the interface will be something like

pci_get_rom_resource(dev) which is implemented internally as

#define pci_get_std_resource(dev)  pci_dev->resource[PCI_ROM_RESOURCE]

> 
> > d) return the nth bridge resource
> 
> I think it's reasonable to have a new interface for this because
> bridges are handled almost entirely in the PCI core and architecture
> code, and I doubt there are many, if any, drivers that care.
> 
> > e) return the type and index of a given resource, where 'index' is
> >         the index w.r.t to that resource type; not w.r.t to all
> >         the resources of the device.
> > f) ability to loop through all resources of the given type/types.
> 
> We do loop through resources in the core when we're assigning, fixing
> up, etc., and that makes some sense to me.  But I actually don't see
> the use case for *drivers* to loop through resources.  All a driver
> knows is "BAR X means Y", and it generally doesn't need to iterate and
> do something generic to all of them.

Yes mostly true. However I have seen a couple of drivers looping through
the resources. An examples is ..

yenta_free_resources()

RP

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