| Message ID | 20130102230732.GB31813@google.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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. > > 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 Thanks, Rafael
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. I'm actually unsure why reducing should mean just leaving one function that will do the allocation and registration in one piece? Why don't we have, for example, struct pci_root { struct pci_host_bridge bridge; struct pci_bus bus; }; that's supposed to be allocated in advance by the platform and then pci_root_init(struct pci_root *); pci_root_add(struct pci_root *); pci_root_register(struct pci_root *); that will do the common initialization, the registration with the driver core and both these things at a time, respectively? Rafael
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c index 0c01261..1cfde12 100644 --- a/arch/x86/pci/acpi.c +++ b/arch/x86/pci/acpi.c @@ -552,8 +552,9 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root) 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); + bus = pci_create_root_bus(&device->dev, busnum, + &pci_root_ops, sd, + &resources); if (bus) { pci_scan_child_bus(bus); @@ -593,6 +594,15 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root) return bus; } +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) +{ + struct device *parent = bridge->dev.parent; + + if (parent) + ACPI_HANDLE_SET(&bridge->dev, to_acpi_device(parent)->handle); + return 0; +} + int __init pci_acpi_init(void) { struct pci_dev *dev = NULL; diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 1af4008..d4516c4 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -303,28 +303,9 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) 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) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 6186f03..3575b2b 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1633,6 +1633,11 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus) return max; } +int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) +{ + return 0; +} + struct pci_bus *pci_create_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources) { @@ -1645,7 +1650,6 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, char bus_addr[64]; char *fmt; - b = pci_alloc_bus(); if (!b) return NULL; @@ -1666,6 +1670,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, 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); + error = pcibios_root_bridge_prepare(bridge); + if (error) + goto bridge_dev_reg_err; error = device_register(&bridge->dev); if (error) goto bridge_dev_reg_err; diff --git a/include/linux/pci.h b/include/linux/pci.h index 15472d6..238438c 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -674,6 +674,7 @@ extern struct list_head pci_root_buses; /* list of all known PCI buses */ /* Some device drivers need know if pci is initiated */ extern int no_pci_devices(void); +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); void pcibios_fixup_bus(struct pci_bus *); int __must_check pcibios_enable_device(struct pci_dev *, int mask); /* Architecture specific versions may override this (weak) */