Re: [PATCH v7 4/6] pci: Introduce a domain number for pci_host_bridge.

[Bjorn Helgaas]

On Wed, Apr 9, 2014 at 7:27 PM, Liviu Dudau <liviu@xxxxxxxxxxx> wrote:
> On Wed, Apr 09, 2014 at 08:02:41AM -0600, Bjorn Helgaas wrote:
>> On Wed, Apr 9, 2014 at 6:07 AM, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
>> > On Tue, Apr 08, 2014 at 05:28:39PM +0100, Bjorn Helgaas wrote:
>> >> On Tue, Apr 8, 2014 at 4:20 AM, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
>> >> > On Mon, Apr 07, 2014 at 11:44:51PM +0100, Bjorn Helgaas wrote:
>> >>
>> >> >> Let me try to explain my concern about the
>> >> >> pci_create_root_bus_in_domain() interface.  We currently have these
>> >> >> interfaces:
>> >> >>
>> >> >>   pci_scan_root_bus()
>> >> >>   pci_scan_bus()
>> >> >>   pci_scan_bus_parented()
>> >> >>   pci_create_root_bus()
>> >> >> ...
>> >>
>> >> >> One alternative is to add an _in_domain() variant of each of these
>> >> >> interfaces, but that doesn't seem very convenient either.  My idea of
>> >> >> passing in a structure would also require adding variants, so there's
>> >> >> not really an advantage there, but I am thinking of the next
>> >> >> unification effort, e.g., for NUMA node info.  I don't really want to
>> >> >> have to change all the _in_domain() interfaces to also take yet
>> >> >> another parameter for the node number.
>> >> >
>> >> > OK, what about this: all the functions that you have mentioned take a
>> >> > void *sysdata parameter. Should we convert this opaque pointer into a
>> >> > specific structure that holds the domain_nr and (in future) the NUMA
>> >> > node info?
>> >>
>> >> I doubt if we can make sysdata itself generic because I suspect we
>> >> need a way to have *some* arch-specific data.  But maybe the arch
>> >> could supply a structure containing a struct device *, domain, struct
>> >> pci_ops *, list of resources, aperture info, etc.  I wonder if struct
>> >> pci_host_bridge would be a reasonable place to put this stuff, e.g.,
>> >> something like this:
>> >>
>> >>   struct pci_host_bridge {
>> >>     int domain;
>> >>     int node;
>> >>     struct device *dev;
>> >>     struct pci_ops *ops;
>> >>     struct list_head resources;
>> >>     void *sysdata;
>> >>     struct pci_bus *bus;  /* filled in by core, not by arch */
>> >>     ... /* other existing contents managed by core */
>> >>   };
>> >>
>> >>   struct pci_bus *pci_scan_host_bridge(struct pci_host_bridge *bridge);
>> >
>> > I'm really reluctant to give the arches more rope to hang themselves.
>>
>> If you mean the sysdata pointer is rope to hang themselves, I think it
>> would be great it we didn't need sysdata at all.  But I think it would
>> be a huge amount of work to get rid of it completely, and keeping it
>> would let us work at that incrementally.
>
> Agree. But then your suggestion was to wrap sysdata inside another structure,
> which to me constitutes additional rope.

I'll ponder this more, but I don't see your point here yet.  The arch
already supplies a sysdata pointer to pci_scan_root_bus(), and we
stash it in every struct pci_bus already.  My idea was just to pass it
in differently, as a structure member rather than a separate argument.
 (And I'm not completely attached to my proposal; it was only to
illustrate my concern about the explosion of interfaces if we have to
add *_domain(), *_node(), etc.)

> The "what" for PCI host bridges is defined in the spec. The "how" is implementation
> defined. What I'm trying to get with the cleanup is the ordering of pci_host_bridge
> creation: core creates the structure first ("what"), arch then has the chance
> of adding specific data to it (ops, resources, etc) ("how").
>
> At the moment arm and powerpc do some horrible dances in trying to create their
> local idea of a host bridge before passing it to the core code.
>
> As for the root bus number, maybe we can offer some sensible default strategy
> for numbering them and the arches that don't care too much can use that.

I think we're at the limit of what can be accomplished with the
abstractness of English.

My opinion is that it's reasonable for the arch to discover the host
bridge properties first and pass them to the core, and it doesn't
require unreasonable things of the arch.  I know the arm PCI setup is
complicated, partly because it deals with a huge number of machines
that don't have a consistent firmware interface.  The x86/ACPI setup
is relatively simple because it deals with a simple firmware
interface.  But that's just my opinion, and maybe your code will show
otherwise.

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