lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <3773945.VhvRYnl41d@vostro.rjw.lan> Date: Wed, 26 Dec 2012 23:35:08 +0100 From: "Rafael J. Wysocki" <rjw@...k.pl> To: Bjorn Helgaas <bhelgaas@...gle.com>, Tony Luck <tony.luck@...el.com>, "H. Peter Anvin" <hpa@...or.com> Cc: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>, ACPI Devel Maling List <linux-acpi@...r.kernel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, LKML <linux-kernel@...r.kernel.org>, Yinghai Lu <yinghai@...nel.org>, Jiang Liu <liuj97@...il.com>, Myron Stowe <myron.stowe@...hat.com> Subject: Re: [Alternative][PATCH] ACPI / PCI: Set root bridge ACPI handle in advance On Wednesday, December 26, 2012 11:14:22 AM Bjorn Helgaas wrote: > On Tue, Dec 25, 2012 at 3:42 PM, Rafael J. Wysocki <rjw@...k.pl> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com> > > Subject: PCI / ACPI: Set root bridge ACPI handle in advance > > > > 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(). > > > > Accordingly, add a new __weak routine, pcibios_root_bridge_prepare(), > > defaulting to an empty implementation that can be replaced by the > > interested architecutres (x86 and ia64 at the moment) with functions > > that will set the root bridge's ACPI handle before its dev member is > > passed to device_register(). Make both x86 and ia64 provide such > > implementations of pcibios_root_bridge_prepare() and remove > > acpi_pci_find_root_bridge() and acpi_get_pci_rootbridge_handle() that aren't > > necessary any more. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com> > > --- > > > > Bjorn, > > > > Since you didn't like the implementation used in the previous patch, here's > > an alternative one using a __weak function. > > > > I don't really strongly prefer any of them. The advantage of the present one > > is that it changes fewer files and directly affects fewer architectures. The > > disadvantage of it is the addition of the __weak "callback". > > > > I wonder what the maintainers of the architectures in question (Peter, Tony) > > think. > > I like this one much better because it doesn't change any of the PCI > interfaces (pci_create_root_bus()). > > I don't see __weak as a disadvantage. We need some way of having > arch-specific hooks, and I don't think __weak is uglier than the other > techniques, e.g., #ifdefs. The original patch uses ACPI_HANDLE_SET() > in generic code and uses #ifdefs to define ACPI_HANDLE_SET() to > nothing on non-ACPI architectures. OK > Acked-by: Bjorn Helgaas <bhelgaas@...gle.com> Thanks! Peter, Tony, is this version fine with you? Rafael > > --- > > arch/ia64/pci/pci.c | 8 ++++++++ > > arch/x86/pci/acpi.c | 9 +++++++++ > > drivers/acpi/pci_root.c | 18 ------------------ > > drivers/pci/pci-acpi.c | 19 ------------------- > > drivers/pci/probe.c | 16 ++++++++++++++++ > > include/acpi/acpi_bus.h | 1 - > > include/linux/pci.h | 2 ++ > > 7 files changed, 35 insertions(+), 38 deletions(-) > > > > Index: linux/drivers/pci/probe.c > > =================================================================== > > --- linux.orig/drivers/pci/probe.c > > +++ linux/drivers/pci/probe.c > > @@ -1632,6 +1632,18 @@ unsigned int pci_scan_child_bus(struct p > > return max; > > } > > > > +/** > > + * pcibios_root_bridge_prepare - Platform-specific host bridge setup. > > + * @bridge: Host bridge to set up. > > + * > > + * Default empty implementation. Replace with an architecture-specific setup > > + * routine, if necessary. > > + */ > > +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) > > { > > @@ -1665,6 +1677,10 @@ 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); > > + 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; > > Index: linux/include/linux/pci.h > > =================================================================== > > --- linux.orig/include/linux/pci.h > > +++ linux/include/linux/pci.h > > @@ -378,6 +378,8 @@ void pci_set_host_bridge_release(struct > > void (*release_fn)(struct pci_host_bridge *), > > void *release_data); > > > > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); > > + > > /* > > * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond > > * to P2P or CardBus bridge windows) go in a table. Additional ones (for > > Index: linux/arch/x86/pci/acpi.c > > =================================================================== > > --- linux.orig/arch/x86/pci/acpi.c > > +++ linux/arch/x86/pci/acpi.c > > @@ -593,6 +593,15 @@ struct pci_bus * __devinit pci_acpi_scan > > return bus; > > } > > > > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > > +{ > > + struct pci_sysdata *sd = bridge->bus->sysdata; > > + struct pci_root_info *info = container_of(sd, struct pci_root_info, sd); > > + > > + ACPI_HANDLE_SET(&bridge->dev, info->bridge->handle); > > + return 0; > > +} > > + > > int __init pci_acpi_init(void) > > { > > struct pci_dev *dev = NULL; > > Index: linux/arch/ia64/pci/pci.c > > =================================================================== > > --- linux.orig/arch/ia64/pci/pci.c > > +++ linux/arch/ia64/pci/pci.c > > @@ -396,6 +396,14 @@ out1: > > return NULL; > > } > > > > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > > +{ > > + struct pci_controller *controller = bridge->bus->sysdata; > > + > > + ACPI_HANDLE_SET(&bridge->dev, controller->acpi_handle); > > + return 0; > > +} > > + > > static int __devinit is_valid_resource(struct pci_dev *dev, int idx) > > { > > unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM; > > Index: linux/drivers/pci/pci-acpi.c > > =================================================================== > > --- linux.orig/drivers/pci/pci-acpi.c > > +++ linux/drivers/pci/pci-acpi.c > > @@ -302,24 +302,6 @@ 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 void pci_acpi_setup(struct device *dev) > > { > > struct pci_dev *pci_dev = to_pci_dev(dev); > > @@ -378,7 +360,6 @@ static void pci_acpi_cleanup(struct devi > > 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, > > .setup = pci_acpi_setup, > > .cleanup = pci_acpi_cleanup, > > }; > > Index: linux/drivers/acpi/pci_root.c > > =================================================================== > > --- linux.orig/drivers/acpi/pci_root.c > > +++ linux/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/include/acpi/acpi_bus.h > > =================================================================== > > --- linux.orig/include/acpi/acpi_bus.h > > +++ linux/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)) > > > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists