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-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
