| 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
| ||
|
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