[+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.
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().
- 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.
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Acked-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Acked-by: H. Peter Anvin <hpa@xxxxxxxxx>
> ---
>
> Peter Anvin pointed out to me that it's better to make it clear in the
> changelog what the patch actually does versus what might be left as future
> work, so here's another update with a slightly modified (and hopefully better)
> changelog. The patch itself hasn't been changed.
>
> Thanks,
> Rafael
>
> ---
> arch/ia64/pci/pci.c | 5 ++++-
> arch/powerpc/kernel/pci-common.c | 3 ++-
> arch/s390/pci/pci.c | 3 ++-
> arch/sparc/kernel/pci.c | 3 ++-
> arch/x86/pci/acpi.c | 5 ++++-
> drivers/acpi/pci_root.c | 18 ------------------
> drivers/pci/pci-acpi.c | 19 -------------------
> drivers/pci/probe.c | 17 ++++++++++++-----
> include/acpi/acpi_bus.h | 1 -
> include/linux/pci.h | 9 ++++++++-
> 10 files changed, 34 insertions(+), 49 deletions(-)
>
> Index: linux-pm/arch/x86/pci/acpi.c
> ===================================================================
> --- linux-pm.orig/arch/x86/pci/acpi.c
> +++ linux-pm/arch/x86/pci/acpi.c
> @@ -483,6 +483,7 @@ struct pci_bus * __devinit pci_acpi_scan
> LIST_HEAD(resources);
> struct pci_bus *bus = NULL;
> struct pci_sysdata *sd;
> + struct pci_root_sys_info si;
> int node;
> #ifdef CONFIG_ACPI_NUMA
> int pxm;
> @@ -522,6 +523,8 @@ struct pci_bus * __devinit pci_acpi_scan
> sd = &info->sd;
> sd->domain = domain;
> sd->node = node;
> + si.acpi_node.handle = device->handle;
> + si.sysdata = sd;
> /*
> * Maybe the desired pci bus has been already scanned. In such case
> * it is unnecessary to scan the pci bus with the given domain,busnum.
> @@ -553,7 +556,7 @@ struct pci_bus * __devinit pci_acpi_scan
> if (!setup_mcfg_map(info, domain, (u8)root->secondary.start,
> (u8)root->secondary.end, root->mcfg_addr))
> bus = pci_create_root_bus(NULL, busnum, &pci_root_ops,
> - sd, &resources);
> + &si, &resources);
>
> if (bus) {
> pci_scan_child_bus(bus);
> Index: linux-pm/drivers/pci/probe.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/probe.c
> +++ linux-pm/drivers/pci/probe.c
> @@ -1634,7 +1634,9 @@ unsigned int pci_scan_child_bus(struct p
> }
>
> struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> - struct pci_ops *ops, void *sysdata, struct list_head *resources)
> + struct pci_ops *ops,
> + struct pci_root_sys_info *sys_info,
> + struct list_head *resources)
> {
> int error;
> struct pci_host_bridge *bridge;
> @@ -1650,7 +1652,7 @@ struct pci_bus *pci_create_root_bus(stru
> if (!b)
> return NULL;
>
> - b->sysdata = sysdata;
> + b->sysdata = sys_info ? sys_info->sysdata : NULL;
> b->ops = ops;
> b2 = pci_find_bus(pci_domain_nr(b), bus);
> if (b2) {
> @@ -1666,6 +1668,8 @@ struct pci_bus *pci_create_root_bus(stru
> bridge->dev.parent = parent;
> bridge->dev.release = pci_release_bus_bridge_dev;
> dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> + ACPI_HANDLE_SET(&bridge->dev,
> + sys_info ? sys_info->acpi_node.handle : NULL);
> error = device_register(&bridge->dev);
> if (error)
> goto bridge_dev_reg_err;
> @@ -1798,6 +1802,7 @@ struct pci_bus *pci_scan_root_bus(struct
> struct pci_ops *ops, void *sysdata, struct list_head *resources)
> {
> struct pci_host_bridge_window *window;
> + struct pci_root_sys_info si = { .sysdata = sysdata, };
> bool found = false;
> struct pci_bus *b;
> int max;
> @@ -1808,7 +1813,7 @@ struct pci_bus *pci_scan_root_bus(struct
> break;
> }
>
> - b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
> + b = pci_create_root_bus(parent, bus, ops, &si, resources);
> if (!b)
> return NULL;
>
> @@ -1833,13 +1838,14 @@ EXPORT_SYMBOL(pci_scan_root_bus);
> struct pci_bus *pci_scan_bus_parented(struct device *parent,
> int bus, struct pci_ops *ops, void *sysdata)
> {
> + struct pci_root_sys_info si = { .sysdata = sysdata, };
> LIST_HEAD(resources);
> struct pci_bus *b;
>
> pci_add_resource(&resources, &ioport_resource);
> pci_add_resource(&resources, &iomem_resource);
> pci_add_resource(&resources, &busn_resource);
> - b = pci_create_root_bus(parent, bus, ops, sysdata, &resources);
> + b = pci_create_root_bus(parent, bus, ops, &si, &resources);
> if (b)
> pci_scan_child_bus(b);
> else
> @@ -1851,13 +1857,14 @@ EXPORT_SYMBOL(pci_scan_bus_parented);
> struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops,
> void *sysdata)
> {
> + struct pci_root_sys_info si = { .sysdata = sysdata, };
> LIST_HEAD(resources);
> struct pci_bus *b;
>
> pci_add_resource(&resources, &ioport_resource);
> pci_add_resource(&resources, &iomem_resource);
> pci_add_resource(&resources, &busn_resource);
> - b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources);
> + b = pci_create_root_bus(NULL, bus, ops, &si, &resources);
> if (b) {
> pci_scan_child_bus(b);
> pci_bus_add_devices(b);
> Index: linux-pm/include/linux/pci.h
> ===================================================================
> --- linux-pm.orig/include/linux/pci.h
> +++ linux-pm/include/linux/pci.h
> @@ -700,8 +700,15 @@ void pci_bus_add_devices(const struct pc
> struct pci_bus *pci_scan_bus_parented(struct device *parent, int bus,
> struct pci_ops *ops, void *sysdata);
> struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
> +
> +struct pci_root_sys_info {
> + void *sysdata;
> + struct acpi_dev_node acpi_node;
> +};
> +
> struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> - struct pci_ops *ops, void *sysdata,
> + struct pci_ops *ops,
> + struct pci_root_sys_info *sys_info,
> struct list_head *resources);
> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> Index: linux-pm/arch/ia64/pci/pci.c
> ===================================================================
> --- linux-pm.orig/arch/ia64/pci/pci.c
> +++ linux-pm/arch/ia64/pci/pci.c
> @@ -333,6 +333,7 @@ pci_acpi_scan_root(struct acpi_pci_root
> struct pci_controller *controller;
> unsigned int windows = 0;
> struct pci_root_info info;
> + struct pci_root_sys_info si;
> struct pci_bus *pbus;
> char *name;
> int pxm;
> @@ -378,7 +379,9 @@ pci_acpi_scan_root(struct acpi_pci_root
> * should handle the case here, but it appears that IA64 hasn't
> * such quirk. So we just ignore the case now.
> */
> - pbus = pci_create_root_bus(NULL, bus, &pci_root_ops, controller,
> + si.sysdata = controller;
> + si.acpi_node.handle = controller->acpi_handle;
> + pbus = pci_create_root_bus(NULL, bus, &pci_root_ops, &si,
> &info.resources);
> if (!pbus) {
> pci_free_resource_list(&info.resources);
> Index: linux-pm/arch/powerpc/kernel/pci-common.c
> ===================================================================
> --- linux-pm.orig/arch/powerpc/kernel/pci-common.c
> +++ linux-pm/arch/powerpc/kernel/pci-common.c
> @@ -1644,6 +1644,7 @@ void __devinit pcibios_scan_phb(struct p
> LIST_HEAD(resources);
> struct pci_bus *bus;
> struct device_node *node = hose->dn;
> + struct pci_root_sys_info si = { .sysdata = hose, };
> int mode;
>
> pr_debug("PCI: Scanning PHB %s\n", of_node_full_name(node));
> @@ -1661,7 +1662,7 @@ void __devinit pcibios_scan_phb(struct p
>
> /* Create an empty bus for the toplevel */
> bus = pci_create_root_bus(hose->parent, hose->first_busno,
> - hose->ops, hose, &resources);
> + hose->ops, &si, &resources);
> if (bus == NULL) {
> pr_err("Failed to create bus for PCI domain %04x\n",
> hose->global_number);
> Index: linux-pm/arch/sparc/kernel/pci.c
> ===================================================================
> --- linux-pm.orig/arch/sparc/kernel/pci.c
> +++ linux-pm/arch/sparc/kernel/pci.c
> @@ -590,6 +590,7 @@ struct pci_bus * __devinit pci_scan_one_
> {
> LIST_HEAD(resources);
> struct device_node *node = pbm->op->dev.of_node;
> + struct pci_root_sys_info si = { .sysdata = pbm, };
> struct pci_bus *bus;
>
> printk("PCI: Scanning PBM %s\n", node->full_name);
> @@ -603,7 +604,7 @@ struct pci_bus * __devinit pci_scan_one_
> pbm->busn.flags = IORESOURCE_BUS;
> pci_add_resource(&resources, &pbm->busn);
> bus = pci_create_root_bus(parent, pbm->pci_first_busno, pbm->pci_ops,
> - pbm, &resources);
> + &si, &resources);
> if (!bus) {
> printk(KERN_ERR "Failed to create bus for %s\n",
> node->full_name);
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -303,28 +303,9 @@ static int acpi_pci_find_device(struct d
> return 0;
> }
>
> -static int acpi_pci_find_root_bridge(struct device *dev, acpi_handle *handle)
> -{
> - int num;
> - unsigned int seg, bus;
> -
> - /*
> - * The string should be the same as root bridge's name
> - * Please look at 'pci_scan_bus_parented'
> - */
> - num = sscanf(dev_name(dev), "pci%04x:%02x", &seg, &bus);
> - if (num != 2)
> - return -ENODEV;
> - *handle = acpi_get_pci_rootbridge_handle(seg, bus);
> - if (!*handle)
> - return -ENODEV;
> - return 0;
> -}
> -
> static struct acpi_bus_type acpi_pci_bus = {
> .bus = &pci_bus_type,
> .find_device = acpi_pci_find_device,
> - .find_bridge = acpi_pci_find_root_bridge,
> };
>
> static int __init acpi_pci_init(void)
> Index: linux-pm/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_root.c
> +++ linux-pm/drivers/acpi/pci_root.c
> @@ -107,24 +107,6 @@ void acpi_pci_unregister_driver(struct a
> }
> EXPORT_SYMBOL(acpi_pci_unregister_driver);
>
> -acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
> -{
> - struct acpi_pci_root *root;
> - acpi_handle handle = NULL;
> -
> - mutex_lock(&acpi_pci_root_lock);
> - list_for_each_entry(root, &acpi_pci_roots, node)
> - if ((root->segment == (u16) seg) &&
> - (root->secondary.start == (u16) bus)) {
> - handle = root->device->handle;
> - break;
> - }
> - mutex_unlock(&acpi_pci_root_lock);
> - return handle;
> -}
> -
> -EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
> -
> /**
> * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
> * @handle - the ACPI CA node in question.
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -443,7 +443,6 @@ struct acpi_pci_root {
> /* helper */
> acpi_handle acpi_get_child(acpi_handle, u64);
> int acpi_is_root_bridge(acpi_handle);
> -acpi_handle acpi_get_pci_rootbridge_handle(unsigned int, unsigned int);
> struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
> #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)ACPI_HANDLE(dev))
>
> Index: linux-pm/arch/s390/pci/pci.c
> ===================================================================
> --- linux-pm.orig/arch/s390/pci/pci.c
> +++ linux-pm/arch/s390/pci/pci.c
> @@ -852,6 +852,7 @@ static void zpci_free_iomap(struct zpci_
>
> static int zpci_create_device_bus(struct zpci_dev *zdev)
> {
> + struct pci_root_sys_info si = { .sysdata = zdev, };
> struct resource *res;
> LIST_HEAD(resources);
> int i;
> @@ -888,7 +889,7 @@ static int zpci_create_device_bus(struct
> }
>
> zdev->bus = pci_create_root_bus(NULL, ZPCI_BUS_NR, &pci_root_ops,
> - zdev, &resources);
> + &si, &resources);
> if (!zdev->bus)
> return -EIO;
>
>
--
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
