Re: [PATCH] ACPI / PCI: Set root bridge ACPI handle in advance

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, December 17, 2012 12:09:46 AM Yinghai Lu wrote:
> On Sun, Dec 16, 2012 at 11:51 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, December 16, 2012 09:27:49 PM Yinghai Lu wrote:
> >> On Sun, Dec 16, 2012 at 2:25 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >
> >> > 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().
> >> > Namely, if pci_acpi_scan_root() could easily pass the root bridge's
> >> > ACPI handle to pci_create_root_bus(), the latter could set the ACPI
> >> > handle in its struct pci_host_bridge object's "dev" member before
> >> > passing it to device_register() and steps 6-8 above wouldn't be
> >> > necessary any more.
> >> >
> >> > To make that happen 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.
> >> >
> >> > Only x86 and ia64 are affected directly, there should be no
> >> > functional changes resulting from this on other architectures.
> >>
> >> that is good one to avoid that find_root_bridge...
> >>
> >> >
> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> > ---
> >> >
> >> > Should apply to the current Linus' tree, boots correctly on x86(-64).
> >
> >> >
> >> > ---
> >> >  arch/ia64/pci/pci.c              |    5 ++++-
> >> >  arch/powerpc/kernel/pci-common.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              |   16 +++++++++++-----
> >> >  include/acpi/acpi_bus.h          |    1 -
> >> >  include/linux/pci.h              |    9 ++++++++-
> >> >  9 files changed, 31 insertions(+), 48 deletions(-)
> >>
> >> you need to update other arch for pci_create_root_bus
> >>
> >> arch/powerpc/kernel/pci-common.c:       bus =
> >> pci_create_root_bus(hose->parent, hose->first_busno,
> >
> > I thought I addressed this one, didn't I?
> >
> >> arch/s390/pci/pci.c:    zdev->bus = pci_create_root_bus(NULL,
> >> ZPCI_BUS_NR, &pci_root_ops,
> >
> > This one appears to have been removed.  There's no pci_create_root_bus()
> > in all arch/s390, as far as I can say.
> 
> at least it is there on linus tree today.
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/s390/pci/pci.c;h=7ed38e5e3028689543c8c6356ef49b3a45546cd6;hb=HEAD
> 
> line 890

Ah, it's been added rather than removed. :-)

OK, will address.

> >> arch/sparc/kernel/pci.c:        bus = pci_create_root_bus(parent,
> >> pbm->pci_first_busno, pbm->pci_ops,
> >
> > I modified this one too, is that not sufficient?
> >
> >> drivers/parisc/dino.c:  dino_dev->hba.hba_bus = bus =
> >> pci_create_root_bus(&dev->dev,
> >> drivers/parisc/lba_pci.c:               pci_create_root_bus(&dev->dev,
> >> lba_dev->hba.bus_num.start,
> >
> > These two pass NULL as the 4th argument to pci_create_root_bus() and don't
> > need to be updated, AFAICS.
> 
> then how could
> -       b->sysdata = sysdata;
> +       b->sysdata = sys_info->sysdata;
> 
> be survived ? need to change to
> 
> +       b->sysdata = sys_info?sys_info->sysdata : NULL;
> 

Good point, I didn't think about that.  Thanks!

> >> >
> >> > Index: linux/arch/x86/pci/acpi.c
> >> > ===================================================================
> >> > --- linux.orig/arch/x86/pci/acpi.c
> >> > +++ linux/arch/x86/pci/acpi.c
> >> > @@ -450,6 +450,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;
> >> > @@ -486,6 +487,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 you can try to have si.acpi_handle directly ?
> >
> > I did it this way for handle to be compiled out when CONFIG_ACPI is not set
> > (struct acpi_dev_node is an empty structure in that case).
> 
> ok.

Thanks for the review!

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.