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

[Bjorn Helgaas]

On Thu, Dec 20, 2012 at 3:56 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> On Thursday, December 20, 2012 02:13:15 PM Bjorn Helgaas wrote:
>> [+cc linux-pci, Myron]
>>
>> On Mon, Dec 17, 2012 at 4:30 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> >
>> > The ACPI handles of PCI root bridges need to be known to
>> > acpi_bind_one(), so that it can create the appropriate
>> > "firmware_node" and "physical_node" files for them, but currently
>> > the way it gets to know those handles is not exactly straightforward
>> > (to put it lightly).
>> >
>> > This is how it works, roughly:
>> >
>> >   1. acpi_bus_scan() finds the handle of a PCI root bridge,
>> >      creates a struct acpi_device object for it and passes that
>> >      object to acpi_pci_root_add().
>> >
>> >   2. acpi_pci_root_add() creates a struct acpi_pci_root object,
>> >      populates its "device" field with its argument's address
>> >      (device->handle is the ACPI handle found in step 1).
>> >
>> >   3. The struct acpi_pci_root object created in step 2 is passed
>> >      to pci_acpi_scan_root() and used to get resources that are
>> >      passed to pci_create_root_bus().
>> >
>> >   4. pci_create_root_bus() creates a struct pci_host_bridge object
>> >      and passes its "dev" member to device_register().
>> >
>> >   5. platform_notify(), which for systems with ACPI is set to
>> >      acpi_platform_notify(), is called.
>> >
>> > So far, so good.  Now it starts to be "interesting".
>> >
>> >   6. acpi_find_bridge_device() is used to find the ACPI handle of
>> >      the given device (which is the PCI root bridge) and executes
>> >      acpi_pci_find_root_bridge(), among other things, for the
>> >      given device object.
>> >
>> >   7. acpi_pci_find_root_bridge() uses the name (sic!) of the given
>> >      device object to extract the segment and bus numbers of the PCI
>> >      root bridge and passes them to acpi_get_pci_rootbridge_handle().
>> >
>> >   8. acpi_get_pci_rootbridge_handle() browses the list of ACPI PCI
>> >      root bridges and finds the one that matches the given segment
>> >      and bus numbers.  Its handle is then used to initialize the
>> >      ACPI handle of the PCI root bridge's device object by
>> >      acpi_bind_one().  However, this is *exactly* the ACPI handle we
>> >      started with in step 1.
>> >
>> > Needless to say, this is quite embarassing, but it may be avoided
>> > thanks to commit f3fd0c8 (ACPI: Allow ACPI handles of devices to be
>> > initialized in advance), which makes it possible to initialize the
>> > ACPI handle of a device before passing it to device_register().
>>
>> This was a mess.  Thanks for cleaning it up.
>>
>> > Accordingly, make pci_acpi_scan_root() pass the root bridge's ACPI
>> > handle to pci_create_root_bus() and make the latter set the ACPI
>> > handle in its struct pci_host_bridge object's "dev" member before
>> > passing it to device_register(), so that steps 6-8 above aren't
>> > necessary any more.
>> >
>> > To implement that, I decided to repurpose the 4th argument of
>> > pci_create_root_bus(), because that allowed me to avoid defining
>> > additional callbacks or similar things and didn't seem to impact
>> > architectures without ACPI substantially.
>> >
>> > All architectures using pci_create_root_bus() directly are updated
>> > as needed, but only x86 and ia64 are affected as far as the behavior
>> > is concerned (no one else uses ACPI).  There should be no changes in
>> > behavior resulting from this on the other architectures.
>>
>> I'd like to converge all architectures on a single higher-level
>> interface, pci_scan_root_bus(), then deprecate and remove
>> pci_create_root_bus(), pci_scan_bus_parented(), and pci_scan_bus().
>> You're changing the underlying pci_create_root_bus(), but not the
>> higher-level interfaces that use it, which will make converging a bit
>> harder.
>
> Do you mean that pci_scan_root_bus() and friends should take a
> struct pci_root_sys_info pointer rather than (void *) as an argument?
> That's not too difficult to do on top of my patch.  I can do that if you
> want me to, no problem.
>
>> Here's an alternate implementation strategy; see what you think:
>>
>> - Add "struct acpi_dev_node acpi_node" to struct pci_sysdata (x86) and
>> struct pci_controller (ia64).  These are the only two arches that use
>> ACPI.
>>
>> - Add an empty generic (weak) pcibios_create_root_ bus().
>
> Well, in my opinion things like that make following the code more difficult.
> If you were new to the code in question and wanted to understand what it was
> doing, you'd need to inspect all architectures to see (1) if they defined
> pcibios_create_root_bus() and (2) what was in there if so.

It's a trade-off.  Your approach puts arch-specific ACPI code in the
generic PCI path.  I wouldn't like to see that extended to do
ACPI_HANDLE_SET(), PDC_HPA_SET(), OF_HANDLE_SET(), etc., all in the
generic code.  I guess I'm used to using "make ALLSOURCE_ARCHS=all
cscope" so I see all the architectures all the time, and I actually
like the fact that we have arch-specific hooks (we have too many right
now, but we do need some).

pcibios_create_root_bus() isn't really a good name; it only gives a
hint about where it's called.  Maybe
pcibios_host_bridge_platform_info() or something would make it more
readable.

>> - Add pcibios_create_root_bus() for x86 and ia64 that does the
>> ACPI_HANDLE_SET().
>>
>> It does add a pcibios callback, which you were trying to avoid, but it
>> does have the advantages that all the higher-level interfaces that use
>> pci_create_root_bus() will keep working and only the ACPI arches have
>> the acpi_dev_node member and associated code.
>
> All of the things that use pci_create_root_bus() are still working with my
> patch applied, hopefully. :-)

Well, sure, but only because no ACPI architectures use pci_scan_root_bus() yet.

> You seem to would like the headers of all the involved functions, including
> pci_create_root_bus(), not to change.

I think it's OK to change the pci_create_root_bus() signature because
it's not exported to modules.  But yes, I think it will be a problem
to change pci_scan_root_bus(), because it *is* exported.  So distros
won't be able to backport a change there unless you change the name.

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/