Re: [Alternative 2][PATCH] ACPI / PCI: Set root bridge ACPI handle in advance

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, January 03, 2013 01:11:30 PM Rafael J. Wysocki wrote:
> On Thursday, January 03, 2013 01:40:52 AM Rafael J. Wysocki wrote:
> > On Wednesday, January 02, 2013 04:07:32 PM Bjorn Helgaas wrote:
> > > On Thu, Dec 27, 2012 at 10:32:13PM +0100, Rafael J. Wysocki wrote:
> > > > To that end, split pci_create_root_bus() into two functions,
> > > > pci_alloc_root() and pci_add_root(), that will allocate memory for
> > > > the new PCI bus and bridge representations and register them with
> > > > the driver core, respectively, and that may be called directly by
> > > > the architectures that need to set the root bridge's ACPI handle
> > > > before registering it.
> > > 
> > > I'm trying to *reduce* the interfaces for creating and scanning PCI
> > > host bridges, and this is a step in the opposite direction.
> > 
> > Yes it is.
> > 
> > The alternative is to make the root bridge initialization code more complex.
> 
> Well, maybe not so much.
> 
> What about adding an extra arg to pci_create_root_bus(), ie. the patch below
> (changelog skipped for now)?
> 
> I admit that having two void * args there is a little awkward, but at least
> it's totally generic.
> 
> > > > Next, Make both x86 and ia64 (the only architectures using ACPI at
> > > > the moment) call pci_alloc_root(), set the root bridge's ACPI handle
> > > > and then call pci_add_root() in their pci_acpi_scan_root() routines
> > > > instead of calling pci_create_root_bus().  For the other code paths
> > > > adding PCI root bridges define a new pci_create_root_bus() as a
> > > > simple combination of pci_alloc_root() and pci_add_root().
> > > 
> > > pci_create_root_bus() takes a "struct device *parent" argument.  That
> > > seems like a logical place to tell the PCI core about the host bridge
> > > device, but x86 and ia64 currently pass NULL there.
> > 
> > And there's a reason for that.  Namely, on these architectures PCI host
> > bridges have no physical parents (well, at least in current practice).
> > 
> > > The patch below shows what I'm thinking.  It does have the side-effect
> > > of changing the sysfs topology from this:
> > > 
> > >     /sys/devices/pci0000:00
> > >     /sys/devices/pci0000:00/0000:00:00.0
> > > 
> > > to this:
> > > 
> > >     /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/pci0000:00
> > >     /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/pci0000:00/0000:00:00.0
> > > 
> > > because it puts the PCI root bus (pci0000:00) under the PNP0A08 device
> > > rather than at the top level.
> > 
> > Which is wrong.
> > 
> > PNP0A08 is not a parent of the host bridge, but its ACPI "companion" (ie. ACPI
> > namespace node representing the host bridge itself).
> > 
> > > That seems like an improvement to me, but it *is* different.
> > 
> > Well, then we should make every ACPI device node corresponding to a PCI device
> > be a parent of that device's struct pci_dev and so on for other bus types.  It
> > doesn't sound like an attractive idea. :-)  Moreover, it is impossible, because
> > those things generally already have parents (struct pci_dev objects have them
> > at least).
> > 
> > That said the idea to pass something meaningful in the parent argument
> > of pci_create_root_bus() can be implemented if we create a "physical" device
> > object corresponding to "device:00" (which is an ACPI namespace node) in your
> > example.
> > 
> > From what I can tell, "device:00" always corresponds to the ACPI _SB scope
> > (which is mandatory), so in principle we can create an abstract "physical"
> > device object for it and call it something like "system_root".  Then, if we
> > use it as the parent of pci0000:00 (the host bridge), then we'll have
> > 
> >      /sys/devices/system_root/pci0000:00
> >      /sys/devices/system_root/pci0000:00/0000:00:00.0
> 
> Having considered that a little more I don't really think it's a good idea.
> It still would be going a little backwards, because we'd need to use the parent
> to get an ACPI handle known already beforehand.

One more thing.

The sysfs locations of PCI devices shouldn't change if acpi=off is passed to
the kernel, so the above is not a good idea at all, I'm afraid.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.