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

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

[Bjorn Helgaas]

[+cc linux-pci, Myron]

On Mon, Dec 17, 2012 at 4:30 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().

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@intel.com>
> Acked-by: Yinghai Lu <yinghai@kernel.org>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: H. Peter Anvin <hpa@zytor.com>
> ---
>
> 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;
>
>

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

[H. Peter Anvin]

On 12/20/2012 01:13 PM, Bjorn Helgaas wrote:
> 
> - 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.
> 

Currently.  There will be ARM platforms with ACPI soon, if there aren't
already.

	-hpa

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

[Rafael J. Wysocki]

On Thursday, December 20, 2012 02:13:15 PM Bjorn Helgaas wrote:
> [+cc linux-pci, Myron]
> 
> On Mon, Dec 17, 2012 at 4:30 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().
> 
> 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.

Do you mean that pci_scan_root_bus() and friends should take a
struct pci_root_sys_info pointer rather than (void *) as an argument?
That's not too difficult to do on top of my patch.  I can do that if you
want me to, no problem.

> 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().

Well, in my opinion things like that make following the code more difficult.
If you were new to the code in question and wanted to understand what it was
doing, you'd need to inspect all architectures to see (1) if they defined
pcibios_create_root_bus() and (2) what was in there if so.

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

All of the things that use pci_create_root_bus() are still working with my
patch applied, hopefully. :-)

You seem to would like the headers of all the involved functions, including
pci_create_root_bus(), not to change.

Thanks,
Rafael


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

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

[Bjorn Helgaas]

On Thu, Dec 20, 2012 at 3:56 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, December 20, 2012 02:13:15 PM Bjorn Helgaas wrote:
>> [+cc linux-pci, Myron]
>>
>> On Mon, Dec 17, 2012 at 4:30 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().
>>
>> 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.
>
> Do you mean that pci_scan_root_bus() and friends should take a
> struct pci_root_sys_info pointer rather than (void *) as an argument?
> That's not too difficult to do on top of my patch.  I can do that if you
> want me to, no problem.
>
>> 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().
>
> Well, in my opinion things like that make following the code more difficult.
> If you were new to the code in question and wanted to understand what it was
> doing, you'd need to inspect all architectures to see (1) if they defined
> pcibios_create_root_bus() and (2) what was in there if so.

It's a trade-off.  Your approach puts arch-specific ACPI code in the
generic PCI path.  I wouldn't like to see that extended to do
ACPI_HANDLE_SET(), PDC_HPA_SET(), OF_HANDLE_SET(), etc., all in the
generic code.  I guess I'm used to using "make ALLSOURCE_ARCHS=all
cscope" so I see all the architectures all the time, and I actually
like the fact that we have arch-specific hooks (we have too many right
now, but we do need some).

pcibios_create_root_bus() isn't really a good name; it only gives a
hint about where it's called.  Maybe
pcibios_host_bridge_platform_info() or something would make it more
readable.

>> - 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.
>
> All of the things that use pci_create_root_bus() are still working with my
> patch applied, hopefully. :-)

Well, sure, but only because no ACPI architectures use pci_scan_root_bus() yet.

> You seem to would like the headers of all the involved functions, including
> pci_create_root_bus(), not to change.

I think it's OK to change the pci_create_root_bus() signature because
it's not exported to modules.  But yes, I think it will be a problem
to change pci_scan_root_bus(), because it *is* exported.  So distros
won't be able to backport a change there unless you change the name.

Bjorn

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.

Thanks,
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))
 

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

[Bjorn Helgaas]

On Tue, Dec 25, 2012 at 3:42 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

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

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

[Rafael J. Wysocki]

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@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@google.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.

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

[Yinghai Lu]

On Tue, Dec 25, 2012 at 2:42 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.
>
> Thanks,
> 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;
> +}
> +

You may need to put that weak version to another file.

some version gcc/ld will inline the weak version directly if it is in same file.

otherwise

Acked-by: Yinghai Lu <yinghai@kernel.org>

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

[Bjorn Helgaas]

On Wed, Dec 26, 2012 at 1:04 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Dec 25, 2012 at 2:42 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.
>>
>> Thanks,
>> 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;
>> +}
>> +
>
> You may need to put that weak version to another file.
>
> some version gcc/ld will inline the weak version directly if it is in same file.

Do you have a reference for this?  I think this might have been true
in the past, but I don't think it's true for any version of gcc we
support for building Linux.

If it's still true, we have many other places that need to be changed,
e.g., omap_secure_ram_reserve_memblock(), r8a7779_register_twd(),
sh73a0_register_twd(), rtc_mips_set_time(), ...

Bjorn

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

[Yinghai Lu]

On Wed, Dec 26, 2012 at 12:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Do you have a reference for this?  I think this might have been true
> in the past, but I don't think it's true for any version of gcc we
> support for building Linux.

http://lkml.indiana.edu/hypermail/linux/kernel/0804.3/3600.html

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

[Yinghai Lu]

On Wed, Dec 26, 2012 at 12:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Dec 26, 2012 at 12:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Do you have a reference for this?  I think this might have been true
>> in the past, but I don't think it's true for any version of gcc we
>> support for building Linux.
>
> http://lkml.indiana.edu/hypermail/linux/kernel/0804.3/3600.html

the problem is already addressed by:

| commit f9d14250071eda9972e4c9cea745a11185952114
| Author: Linus Torvalds <torvalds@linux-foundation.org>
| Date:   Fri Jan 2 09:29:43 2009 -0800
|
|    Disallow gcc versions 4.1.{0,1}
|
|    These compiler versions are known to miscompile __weak functions and
|    thus generate kernels that don't necessarily work correctly.  If a weak
|    function is int he same compilation unit as a caller, gcc may end up
|    inlining it, and thus binding the weak function too early.
|
|    See
|
|        http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
|
|    for details.

so it is ok to put the __weak in the same file now.

Yinghai

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

[Rafael J. Wysocki]

On Wednesday, December 26, 2012 12:41:05 PM Yinghai Lu wrote:
> On Wed, Dec 26, 2012 at 12:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Wed, Dec 26, 2012 at 12:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> Do you have a reference for this?  I think this might have been true
> >> in the past, but I don't think it's true for any version of gcc we
> >> support for building Linux.
> >
> > http://lkml.indiana.edu/hypermail/linux/kernel/0804.3/3600.html
> 
> the problem is already addressed by:
> 
> | commit f9d14250071eda9972e4c9cea745a11185952114
> | Author: Linus Torvalds <torvalds@linux-foundation.org>
> | Date:   Fri Jan 2 09:29:43 2009 -0800
> |
> |    Disallow gcc versions 4.1.{0,1}
> |
> |    These compiler versions are known to miscompile __weak functions and
> |    thus generate kernels that don't necessarily work correctly.  If a weak
> |    function is int he same compilation unit as a caller, gcc may end up
> |    inlining it, and thus binding the weak function too early.
> |
> |    See
> |
> |        http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
> |
> |    for details.
> 
> so it is ok to put the __weak in the same file now.

Cool, thanks for checking and for the ACK!

Rafael


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

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

[Yinghai Lu]

On Wed, Dec 26, 2012 at 2:36 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, December 26, 2012 12:41:05 PM Yinghai Lu wrote:
>> On Wed, Dec 26, 2012 at 12:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Wed, Dec 26, 2012 at 12:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >> Do you have a reference for this?  I think this might have been true
>> >> in the past, but I don't think it's true for any version of gcc we
>> >> support for building Linux.
>> >
>> > http://lkml.indiana.edu/hypermail/linux/kernel/0804.3/3600.html
>>
>> the problem is already addressed by:
>>
>> | commit f9d14250071eda9972e4c9cea745a11185952114
>> | Author: Linus Torvalds <torvalds@linux-foundation.org>
>> | Date:   Fri Jan 2 09:29:43 2009 -0800
>> |
>> |    Disallow gcc versions 4.1.{0,1}
>> |
>> |    These compiler versions are known to miscompile __weak functions and
>> |    thus generate kernels that don't necessarily work correctly.  If a weak
>> |    function is int he same compilation unit as a caller, gcc may end up
>> |    inlining it, and thus binding the weak function too early.
>> |
>> |    See
>> |
>> |        http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
>> |
>> |    for details.
>>
>> so it is ok to put the __weak in the same file now.
>
> Cool, thanks for checking and for the ACK!

wait, we have some problem on systems that root bus is not exported via DSDT ...

one of my nehalem system that have uncore cpu devices are not exported via ACPI.

also there will be problem that system is booting with acpi=off.


+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;
+}

will get wrong info...via sd... as their sd is standalone

Thanks

Yinghai

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

[Rafael J. Wysocki]

On Wednesday, December 26, 2012 04:10:32 PM Yinghai Lu wrote:
> On Wed, Dec 26, 2012 at 2:36 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, December 26, 2012 12:41:05 PM Yinghai Lu wrote:
> >> On Wed, Dec 26, 2012 at 12:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> > On Wed, Dec 26, 2012 at 12:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> >> Do you have a reference for this?  I think this might have been true
> >> >> in the past, but I don't think it's true for any version of gcc we
> >> >> support for building Linux.
> >> >
> >> > http://lkml.indiana.edu/hypermail/linux/kernel/0804.3/3600.html
> >>
> >> the problem is already addressed by:
> >>
> >> | commit f9d14250071eda9972e4c9cea745a11185952114
> >> | Author: Linus Torvalds <torvalds@linux-foundation.org>
> >> | Date:   Fri Jan 2 09:29:43 2009 -0800
> >> |
> >> |    Disallow gcc versions 4.1.{0,1}
> >> |
> >> |    These compiler versions are known to miscompile __weak functions and
> >> |    thus generate kernels that don't necessarily work correctly.  If a weak
> >> |    function is int he same compilation unit as a caller, gcc may end up
> >> |    inlining it, and thus binding the weak function too early.
> >> |
> >> |    See
> >> |
> >> |        http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
> >> |
> >> |    for details.
> >>
> >> so it is ok to put the __weak in the same file now.
> >
> > Cool, thanks for checking and for the ACK!
> 
> wait, we have some problem on systems that root bus is not exported via DSDT ...
> 
> one of my nehalem system that have uncore cpu devices are not exported via ACPI.
> 
> also there will be problem that system is booting with acpi=off.
> 
> 
> +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;
> +}
> 
> will get wrong info...via sd... as their sd is standalone

Yes, it will be called in all code paths leading to acpi_create_root_bus(),
not only the ones started by pci_acpi_scan_root().  Well, too bad.

By the way, that illustrates nicely why I generally have concerns about __weak
stuff and similar tricks.

Bjorn, I had tried to use the approach you suggested, but it didn't work.
I thought about fixing that, but everything I could come up with turned out to
be too complicated, so I'm inclined to use the previous version after all:

https://patchwork.kernel.org/patch/1889221/

that has been acked by Yinghai, Greg and Peter already.

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

On Thursday, December 27, 2012 01:47:22 PM Rafael J. Wysocki wrote:
> On Wednesday, December 26, 2012 04:10:32 PM Yinghai Lu wrote:
> > On Wed, Dec 26, 2012 at 2:36 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Wednesday, December 26, 2012 12:41:05 PM Yinghai Lu wrote:
> > >> On Wed, Dec 26, 2012 at 12:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > >> > On Wed, Dec 26, 2012 at 12:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > >> >> Do you have a reference for this?  I think this might have been true
> > >> >> in the past, but I don't think it's true for any version of gcc we
> > >> >> support for building Linux.
> > >> >
> > >> > http://lkml.indiana.edu/hypermail/linux/kernel/0804.3/3600.html
> > >>
> > >> the problem is already addressed by:
> > >>
> > >> | commit f9d14250071eda9972e4c9cea745a11185952114
> > >> | Author: Linus Torvalds <torvalds@linux-foundation.org>
> > >> | Date:   Fri Jan 2 09:29:43 2009 -0800
> > >> |
> > >> |    Disallow gcc versions 4.1.{0,1}
> > >> |
> > >> |    These compiler versions are known to miscompile __weak functions and
> > >> |    thus generate kernels that don't necessarily work correctly.  If a weak
> > >> |    function is int he same compilation unit as a caller, gcc may end up
> > >> |    inlining it, and thus binding the weak function too early.
> > >> |
> > >> |    See
> > >> |
> > >> |        http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
> > >> |
> > >> |    for details.
> > >>
> > >> so it is ok to put the __weak in the same file now.
> > >
> > > Cool, thanks for checking and for the ACK!
> > 
> > wait, we have some problem on systems that root bus is not exported via DSDT ...
> > 
> > one of my nehalem system that have uncore cpu devices are not exported via ACPI.
> > 
> > also there will be problem that system is booting with acpi=off.
> > 
> > 
> > +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;
> > +}
> > 
> > will get wrong info...via sd... as their sd is standalone
> 
> Yes, it will be called in all code paths leading to acpi_create_root_bus(),
> not only the ones started by pci_acpi_scan_root().  Well, too bad.

s/acpi_create_root_bus/pci_create_root_bus/

Sorry.

> By the way, that illustrates nicely why I generally have concerns about __weak
> stuff and similar tricks.
> 
> Bjorn, I had tried to use the approach you suggested, but it didn't work.
> I thought about fixing that, but everything I could come up with turned out to
> be too complicated, so I'm inclined to use the previous version after all:
> 
> https://patchwork.kernel.org/patch/1889221/
> 
> that has been acked by Yinghai, Greg and Peter already.

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

On Thursday, December 27, 2012 02:31:09 PM Rafael J. Wysocki wrote:
> On Thursday, December 27, 2012 01:47:22 PM Rafael J. Wysocki wrote:
> > On Wednesday, December 26, 2012 04:10:32 PM Yinghai Lu wrote:
> > > On Wed, Dec 26, 2012 at 2:36 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Wednesday, December 26, 2012 12:41:05 PM Yinghai Lu wrote:
> > > >> On Wed, Dec 26, 2012 at 12:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > > >> > On Wed, Dec 26, 2012 at 12:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > > >> >> Do you have a reference for this?  I think this might have been true
> > > >> >> in the past, but I don't think it's true for any version of gcc we
> > > >> >> support for building Linux.
> > > >> >
> > > >> > http://lkml.indiana.edu/hypermail/linux/kernel/0804.3/3600.html
> > > >>
> > > >> the problem is already addressed by:
> > > >>
> > > >> | commit f9d14250071eda9972e4c9cea745a11185952114
> > > >> | Author: Linus Torvalds <torvalds@linux-foundation.org>
> > > >> | Date:   Fri Jan 2 09:29:43 2009 -0800
> > > >> |
> > > >> |    Disallow gcc versions 4.1.{0,1}
> > > >> |
> > > >> |    These compiler versions are known to miscompile __weak functions and
> > > >> |    thus generate kernels that don't necessarily work correctly.  If a weak
> > > >> |    function is int he same compilation unit as a caller, gcc may end up
> > > >> |    inlining it, and thus binding the weak function too early.
> > > >> |
> > > >> |    See
> > > >> |
> > > >> |        http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
> > > >> |
> > > >> |    for details.
> > > >>
> > > >> so it is ok to put the __weak in the same file now.
> > > >
> > > > Cool, thanks for checking and for the ACK!
> > > 
> > > wait, we have some problem on systems that root bus is not exported via DSDT ...
> > > 
> > > one of my nehalem system that have uncore cpu devices are not exported via ACPI.
> > > 
> > > also there will be problem that system is booting with acpi=off.
> > > 
> > > 
> > > +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;
> > > +}
> > > 
> > > will get wrong info...via sd... as their sd is standalone
> > 
> > Yes, it will be called in all code paths leading to acpi_create_root_bus(),
> > not only the ones started by pci_acpi_scan_root().  Well, too bad.
> 
> s/acpi_create_root_bus/pci_create_root_bus/
> 
> Sorry.
> 
> > By the way, that illustrates nicely why I generally have concerns about __weak
> > stuff and similar tricks.
> > 
> > Bjorn, I had tried to use the approach you suggested, but it didn't work.
> > I thought about fixing that, but everything I could come up with turned out to
> > be too complicated, so I'm inclined to use the previous version after all:
> > 
> > https://patchwork.kernel.org/patch/1889221/
> > 
> > that has been acked by Yinghai, Greg and Peter already.

I think I know how to avoid the __weak thing without doing what the first
version did.  I'll post a new version of the patch shortly.

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

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().

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.

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().

Finally, remove acpi_pci_find_root_bridge() and
acpi_get_pci_rootbridge_handle() that aren't necessary any more
and modify acpi_pci_bus accordingly.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Bjorn,

The approach using the __weak function doesn't really work, but there is one
more thing we can do to avoid modifying architectures that don't care about
ACPI, which is implemented by this patch.  I think I prefer it to the first
one, but YMMV. :-)

Thanks,
Rafael

---
 arch/ia64/pci/pci.c     |   10 +++++--
 arch/x86/pci/acpi.c     |   12 ++++++--
 drivers/acpi/pci_root.c |   18 -------------
 drivers/pci/pci-acpi.c  |   19 -------------
 drivers/pci/probe.c     |   66 ++++++++++++++++++++++++++++++++++++------------
 include/acpi/acpi_bus.h |    1 
 include/linux/pci.h     |    5 +++
 7 files changed, 72 insertions(+), 59 deletions(-)

Index: linux/drivers/pci/probe.c
===================================================================
--- linux.orig/drivers/pci/probe.c
+++ linux/drivers/pci/probe.c
@@ -1632,18 +1632,17 @@ unsigned int pci_scan_child_bus(struct p
 	return max;
 }
 
-struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+/**
+ * pci_alloc_root - Allocate memory for PCI root bus and bridge representations.
+ * @bus: Bus number of the root bus.
+ * @ops: Root bus operations.
+ * @sysdata: Architecture-specific data.
+ */
+struct pci_host_bridge *pci_alloc_root(int bus, struct pci_ops *ops,
+				       void *sysdata)
 {
-	int error;
 	struct pci_host_bridge *bridge;
 	struct pci_bus *b, *b2;
-	struct pci_host_bridge_window *window, *n;
-	struct resource *res;
-	resource_size_t offset;
-	char bus_addr[64];
-	char *fmt;
-
 
 	b = pci_alloc_bus();
 	if (!b)
@@ -1651,16 +1650,43 @@ struct pci_bus *pci_create_root_bus(stru
 
 	b->sysdata = sysdata;
 	b->ops = ops;
+	b->number = bus;
 	b2 = pci_find_bus(pci_domain_nr(b), bus);
 	if (b2) {
-		/* If we already got to this bus through a different bridge, ignore it */
+		/*
+		 * If we already got to this bus through a different bridge,
+		 * ignore it.
+		 */
 		dev_dbg(&b2->dev, "bus already known\n");
-		goto err_out;
+	} else {
+		bridge = pci_alloc_host_bridge(b);
+		if (bridge)
+			return bridge;
 	}
 
-	bridge = pci_alloc_host_bridge(b);
-	if (!bridge)
-		goto err_out;
+	kfree(b);
+	return NULL;
+}
+
+/**
+ * pci_add_root - Register PCI root bridge and root bus.
+ * @bridge: Bridge to register.
+ * @parent: Parent device of the bridge.
+ * @resources: Bridge resources.
+ *
+ * Register PCI root bridge and bus allocated by %pci_alloc_root().
+ */
+struct pci_bus *pci_add_root(struct pci_host_bridge *bridge,
+			     struct device *parent, struct list_head *resources)
+{
+	int error;
+	struct pci_bus *b = bridge->bus;
+	struct pci_host_bridge_window *window, *n;
+	struct resource *res;
+	resource_size_t offset;
+	char bus_addr[64];
+	int bus = b->number;
+	char *fmt;
 
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_bus_bridge_dev;
@@ -1668,6 +1694,7 @@ struct pci_bus *pci_create_root_bus(stru
 	error = device_register(&bridge->dev);
 	if (error)
 		goto bridge_dev_reg_err;
+
 	b->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(b->bridge);
 	pci_set_bus_of_node(b);
@@ -1685,7 +1712,7 @@ struct pci_bus *pci_create_root_bus(stru
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(b);
 
-	b->number = b->busn_res.start = bus;
+	b->busn_res.start = bus;
 
 	if (parent)
 		dev_info(parent, "PCI host bridge to bus %s\n", dev_name(&b->dev));
@@ -1725,11 +1752,18 @@ class_dev_reg_err:
 	device_unregister(&bridge->dev);
 bridge_dev_reg_err:
 	kfree(bridge);
-err_out:
 	kfree(b);
 	return NULL;
 }
 
+struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	struct pci_host_bridge *bridge = pci_alloc_root(bus, ops, sysdata);
+
+	return bridge ? pci_add_root(bridge, parent, resources) : NULL;
+}
+
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
 {
 	struct resource *res = &b->busn_res;
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))
 
Index: linux/include/linux/pci.h
===================================================================
--- linux.orig/include/linux/pci.h
+++ linux/include/linux/pci.h
@@ -700,6 +700,11 @@ 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_host_bridge *pci_alloc_root(int bus, struct pci_ops *ops,
+				       void *sysdata);
+struct pci_bus *pci_add_root(struct pci_host_bridge *bridge,
+			     struct device *parent,
+			     struct list_head *resources);
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 				    struct pci_ops *ops, void *sysdata,
 				    struct list_head *resources);
Index: linux/arch/x86/pci/acpi.c
===================================================================
--- linux.orig/arch/x86/pci/acpi.c
+++ linux/arch/x86/pci/acpi.c
@@ -551,9 +551,15 @@ 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);
+				    (u8)root->secondary.end, root->mcfg_addr)) {
+			struct pci_host_bridge *bridge;
+
+			bridge = pci_alloc_root(busnum, &pci_root_ops, sd);
+			if (bridge) {
+				ACPI_HANDLE_SET(&bridge->dev, device->handle);
+				bus = pci_add_root(bridge, NULL, &resources);
+			}
+		}
 
 		if (bus) {
 			pci_scan_child_bus(bus);
Index: linux/arch/ia64/pci/pci.c
===================================================================
--- linux.orig/arch/ia64/pci/pci.c
+++ linux/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_host_bridge *bridge;
 	struct pci_bus *pbus;
 	char *name;
 	int pxm;
@@ -378,8 +379,13 @@ 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,
-				   &info.resources);
+	bridge = pci_alloc_root(bus, &pci_root_ops, controller);
+	if (bridge) {
+		ACPI_HANDLE_SET(&bridge->dev, device->handle);
+		pbus = pci_add_root(bridge, NULL, &info.resources);
+	} else {
+		pbus = NULL;
+	}
 	if (!pbus) {
 		pci_free_resource_list(&info.resources);
 		return NULL;

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

[Bjorn Helgaas]

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.

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

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.  That seems like an improvement to me,
but it *is* different.

Bjorn

commit 5dee5f2f4fefbe4888939871c2252299067123bf
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Wed Jan 2 13:47:02 2013 -0700

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) */

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

[Rafael J. Wysocki]

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


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

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

[Rafael J. Wysocki]

On Thursday, January 03, 2013 01:40:52 AM Rafael J. Wysocki wrote:
> 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.

Well, maybe not so much.

What about adding an extra arg to pci_create_root_bus(), ie. the patch below
(changelog skipped for now)?

I admit that having two void * args there is a little awkward, but at least
it's totally generic.

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

Having considered that a little more I don't really think it's a good idea.
It still would be going a little backwards, because we'd need to use the parent
to get an ACPI handle known already beforehand.

Please tell me what you think of the one below.

Thanks,
Rafael


Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/ia64/pci/pci.c              |    8 +++++++-
 arch/powerpc/kernel/pci-common.c |    2 +-
 arch/s390/pci/pci.c              |    2 +-
 arch/sparc/kernel/pci.c          |    2 +-
 arch/x86/pci/acpi.c              |    9 ++++++++-
 drivers/acpi/pci_root.c          |   18 ------------------
 drivers/pci/pci-acpi.c           |   19 -------------------
 drivers/pci/probe.c              |   23 +++++++++++++++++++----
 include/acpi/acpi_bus.h          |    1 -
 include/linux/pci.h              |    6 ++++--
 10 files changed, 41 insertions(+), 49 deletions(-)

Index: linux/drivers/pci/probe.c
===================================================================
--- linux.orig/drivers/pci/probe.c
+++ linux/drivers/pci/probe.c
@@ -1632,8 +1632,22 @@ unsigned int pci_scan_child_bus(struct p
 	return max;
 }
 
+/**
+ * pcibios_root_bridge_setup - Platform-specific host bridge setup.
+ * @bridge: Host bridge to set up.
+ * @rootdata: Additional data to use for the host bridge setup.
+ *
+ * Default empty implementation.  Replace with an architecture-specific setup
+ * routine, if necessary.
+ */
+void __weak pcibios_root_bridge_setup(struct pci_host_bridge *bridge,
+				      void *rootdata)
+{
+}
+
 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, void *sysdata,
+				    struct list_head *resources, void *rootdata)
 {
 	int error;
 	struct pci_host_bridge *bridge;
@@ -1665,6 +1679,7 @@ 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);
+	pcibios_root_bridge_setup(bridge, rootdata);
 	error = device_register(&bridge->dev);
 	if (error)
 		goto bridge_dev_reg_err;
@@ -1807,7 +1822,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, sysdata, resources, NULL);
 	if (!b)
 		return NULL;
 
@@ -1838,7 +1853,7 @@ struct pci_bus *pci_scan_bus_parented(st
 	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, sysdata, &resources, NULL);
 	if (b)
 		pci_scan_child_bus(b);
 	else
@@ -1856,7 +1871,7 @@ struct pci_bus *pci_scan_bus(int bus, st
 	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, sysdata, &resources, NULL);
 	if (b) {
 		pci_scan_child_bus(b);
 		pci_bus_add_devices(b);
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);
 
+void pcibios_root_bridge_setup(struct pci_host_bridge *bridge, void *rootdata);
+
 /*
  * 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
@@ -701,8 +703,8 @@ struct pci_bus *pci_scan_bus_parented(st
 				      struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
 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, void *sysdata,
+				struct list_head *resources, void *rootdata);
 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);
 void pci_bus_release_busn_res(struct pci_bus *b);
Index: linux/arch/x86/pci/acpi.c
===================================================================
--- linux.orig/arch/x86/pci/acpi.c
+++ linux/arch/x86/pci/acpi.c
@@ -553,7 +553,8 @@ 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);
+						  sd, &resources,
+						  device->handle);
 
 		if (bus) {
 			pci_scan_child_bus(bus);
@@ -593,6 +594,12 @@ struct pci_bus * __devinit pci_acpi_scan
 	return bus;
 }
 
+void pcibios_root_bridge_setup(struct pci_host_bridge *bridge, void *rootdata)
+{
+	if (rootdata)
+		ACPI_HANDLE_SET(&bridge->dev, (acpi_handle)rootdata);
+}
+
 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
@@ -379,7 +379,7 @@ pci_acpi_scan_root(struct acpi_pci_root
 	 * such quirk. So we just ignore the case now.
 	 */
 	pbus = pci_create_root_bus(NULL, bus, &pci_root_ops, controller,
-				   &info.resources);
+				   &info.resources, controller->acpi_handle);
 	if (!pbus) {
 		pci_free_resource_list(&info.resources);
 		return NULL;
@@ -396,6 +396,12 @@ out1:
 	return NULL;
 }
 
+void pcibios_root_bridge_setup(struct pci_host_bridge *bridge, void *rootdata)
+{
+	if (rootdata)
+		ACPI_HANDLE_SET(&bridge->dev, (acpi_handle)rootdata);
+}
+
 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
@@ -402,7 +402,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/arch/powerpc/kernel/pci-common.c
===================================================================
--- linux.orig/arch/powerpc/kernel/pci-common.c
+++ linux/arch/powerpc/kernel/pci-common.c
@@ -1661,7 +1661,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, hose, &resources, NULL);
 	if (bus == NULL) {
 		pr_err("Failed to create bus for PCI domain %04x\n",
 			hose->global_number);
Index: linux/arch/s390/pci/pci.c
===================================================================
--- linux.orig/arch/s390/pci/pci.c
+++ linux/arch/s390/pci/pci.c
@@ -940,7 +940,7 @@ static int zpci_create_device_bus(struct
 	}
 
 	zdev->bus = pci_create_root_bus(NULL, ZPCI_BUS_NR, &pci_root_ops,
-					zdev, &resources);
+					zdev, &resources, NULL);
 	if (!zdev->bus)
 		return -EIO;
 
Index: linux/arch/sparc/kernel/pci.c
===================================================================
--- linux.orig/arch/sparc/kernel/pci.c
+++ linux/arch/sparc/kernel/pci.c
@@ -603,7 +603,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);
+				  pbm, &resources, NULL);
 	if (!bus) {
 		printk(KERN_ERR "Failed to create bus for %s\n",
 		       node->full_name);


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

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

[Rafael J. Wysocki]

On Thursday, January 03, 2013 01:11:30 PM Rafael J. Wysocki wrote:
> On Thursday, January 03, 2013 01:40:52 AM Rafael J. Wysocki wrote:
> > 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.
> 
> Well, maybe not so much.
> 
> What about adding an extra arg to pci_create_root_bus(), ie. the patch below
> (changelog skipped for now)?
> 
> I admit that having two void * args there is a little awkward, but at least
> it's totally generic.
> 
> > > > 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
> 
> Having considered that a little more I don't really think it's a good idea.
> It still would be going a little backwards, because we'd need to use the parent
> to get an ACPI handle known already beforehand.

One more thing.

The sysfs locations of PCI devices shouldn't change if acpi=off is passed to
the kernel, so the above is not a good idea at all, I'm afraid.

Thanks,
Rafael


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

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

[Bjorn Helgaas]

On Thu, Jan 3, 2013 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 03, 2013 01:11:30 PM Rafael J. Wysocki wrote:
>> On Thursday, January 03, 2013 01:40:52 AM Rafael J. Wysocki wrote:
>> > 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.
>>
>> Well, maybe not so much.
>>
>> What about adding an extra arg to pci_create_root_bus(), ie. the patch below
>> (changelog skipped for now)?
>>
>> I admit that having two void * args there is a little awkward, but at least
>> it's totally generic.
>>
>> > > > 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
>>
>> Having considered that a little more I don't really think it's a good idea.
>> It still would be going a little backwards, because we'd need to use the parent
>> to get an ACPI handle known already beforehand.
>
> One more thing.
>
> The sysfs locations of PCI devices shouldn't change if acpi=off is passed to
> the kernel, so the above is not a good idea at all, I'm afraid.

I don't think there's a requirement that sysfs with acpi=off be the
same as with it on.  acpi=off is only a debugging tool.  With acpi=off
we ignore all the topology information we get from ACPI, and the
location of PCI host bridges is part of that topology.  We shouldn't
cripple sysfs when we're using ACPI just so it matches the acpi=off
case.

Of course, we might not want to change this in sysfs anyway; I just
don't think acpi=off is a valid reason to avoid a change.

Bjorn

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

[Rafael J. Wysocki]

On Thursday, January 03, 2013 03:13:31 PM Bjorn Helgaas wrote:
> On Thu, Jan 3, 2013 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 03, 2013 01:11:30 PM Rafael J. Wysocki wrote:
> >> On Thursday, January 03, 2013 01:40:52 AM Rafael J. Wysocki wrote:
> >> > 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.
> >>
> >> Well, maybe not so much.
> >>
> >> What about adding an extra arg to pci_create_root_bus(), ie. the patch below
> >> (changelog skipped for now)?
> >>
> >> I admit that having two void * args there is a little awkward, but at least
> >> it's totally generic.
> >>
> >> > > > 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
> >>
> >> Having considered that a little more I don't really think it's a good idea.
> >> It still would be going a little backwards, because we'd need to use the parent
> >> to get an ACPI handle known already beforehand.
> >
> > One more thing.
> >
> > The sysfs locations of PCI devices shouldn't change if acpi=off is passed to
> > the kernel, so the above is not a good idea at all, I'm afraid.
> 
> I don't think there's a requirement that sysfs with acpi=off be the
> same as with it on.  acpi=off is only a debugging tool.  With acpi=off
> we ignore all the topology information we get from ACPI, and the
> location of PCI host bridges is part of that topology.  We shouldn't
> cripple sysfs when we're using ACPI just so it matches the acpi=off
> case.
> 
> Of course, we might not want to change this in sysfs anyway; I just
> don't think acpi=off is a valid reason to avoid a change.

Well, I don't think we'll agree on that one, sorry.

OK, I now have sent no less than three working version of the patch that fixes
the current code which _is_ insane.  You haven't even responded to the last
one, but for the first two the reason why you didn't like them was something
similar to "it may conflict with some future changes I'm planning".  Well,
that might be used to reject prety much any change and I'm not considering it
as a good enough reason for blocking a fix.  Sorry about that.

I easily could say I didn't care, but I feel I've spent too much time on that
pretty much for no reason.

Please tell me, honestly, do you want the current code to be fixed at all?

Disappointed,
Rafael


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

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

[Bjorn Helgaas]

On Thu, Jan 03, 2013 at 11:56:55PM +0100, Rafael J. Wysocki wrote:

> OK, I now have sent no less than three working version of the patch that fixes
> the current code which _is_ insane.  You haven't even responded to the last
> one, but for the first two the reason why you didn't like them was something
> similar to "it may conflict with some future changes I'm planning".  Well,
> that might be used to reject prety much any change and I'm not considering it
> as a good enough reason for blocking a fix.  Sorry about that.

I think your memory is faulty.  My response to the first
(https://lkml.org/lkml/2012/12/20/407) was "Thanks for cleaning this up, I
have an interface concern, here's an outline of a possible alternative."

My response to the second (https://lkml.org/lkml/2012/12/26/72) was "I like
this much better, Acked-by: Bjorn Helgaas."  Then Yinghai noticed the issue
with non-ACPI host bridges, and you abandoned that approach.

I took a few days of vacation, then spent the better part of yesterday
exploring the reasons why x86 and ia64 don't use the "parent" argument when
several other arches do, and worked up a patch
(https://lkml.org/lkml/2013/1/2/285).  It turned out to have a fatal flaw,
but was done in good faith.

It's true I haven't responded to the third one, posted about 12 hours ago.

I still like the approach of the second patch.  What would you think
of the following incremental change to it?  I did reproduce Yinghai's
issue with non-ACPI host bridges, and this change resolves it for me.

diff -u b/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
--- b/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -522,6 +522,7 @@
 	sd = &info->sd;
 	sd->domain = domain;
 	sd->node = node;
+	sd->acpi_handle = device->handle;
 	/*
 	 * 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.
@@ -596,9 +597,8 @@
 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);
+	ACPI_HANDLE_SET(&bridge->dev, sd->acpi_handle);
 	return 0;
 }
 
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -14,6 +14,7 @@
 struct pci_sysdata {
 	int		domain;		/* PCI domain */
 	int		node;		/* NUMA node */
+	void		*acpi_handle;
 #ifdef CONFIG_X86_64
 	void		*iommu;		/* IOMMU private data */
 #endif

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

[Rafael J. Wysocki]

On Thursday, January 03, 2013 06:00:38 PM Bjorn Helgaas wrote:
> On Thu, Jan 03, 2013 at 11:56:55PM +0100, Rafael J. Wysocki wrote:
> 
> > OK, I now have sent no less than three working version of the patch that fixes
> > the current code which _is_ insane.  You haven't even responded to the last
> > one, but for the first two the reason why you didn't like them was something
> > similar to "it may conflict with some future changes I'm planning".  Well,
> > that might be used to reject prety much any change and I'm not considering it
> > as a good enough reason for blocking a fix.  Sorry about that.
> 
> I think your memory is faulty.  My response to the first
> (https://lkml.org/lkml/2012/12/20/407) was "Thanks for cleaning this up, I
> have an interface concern, here's an outline of a possible alternative."
> 
> My response to the second (https://lkml.org/lkml/2012/12/26/72) was "I like
> this much better, Acked-by: Bjorn Helgaas."  Then Yinghai noticed the issue
> with non-ACPI host bridges, and you abandoned that approach.

I thought it was helpelss, but I was clearly wrong.  I should have spent more
time on figuring out why it failed, so thank for taking the time to do that.

> I took a few days of vacation, then spent the better part of yesterday
> exploring the reasons why x86 and ia64 don't use the "parent" argument when
> several other arches do, and worked up a patch
> (https://lkml.org/lkml/2013/1/2/285).  It turned out to have a fatal flaw,
> but was done in good faith.

I know.

> It's true I haven't responded to the third one, posted about 12 hours ago.

Oh, that's a simple patch. ;-)

But you're right, I should be more patient.  Sorry about that.

> I still like the approach of the second patch.  What would you think
> of the following incremental change to it?

I'm fine with it.

> I did reproduce Yinghai's
> issue with non-ACPI host bridges, and this change resolves it for me.

If you don't mind, I'll fold the patch below into

https://patchwork.kernel.org/patch/1910181/

and post the complete patch.

Thanks,
Rafael


> diff -u b/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> --- b/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -522,6 +522,7 @@
>  	sd = &info->sd;
>  	sd->domain = domain;
>  	sd->node = node;
> +	sd->acpi_handle = device->handle;
>  	/*
>  	 * 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.
> @@ -596,9 +597,8 @@
>  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);
> +	ACPI_HANDLE_SET(&bridge->dev, sd->acpi_handle);
>  	return 0;
>  }
>  
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -14,6 +14,7 @@
>  struct pci_sysdata {
>  	int		domain;		/* PCI domain */
>  	int		node;		/* NUMA node */
> +	void		*acpi_handle;
>  #ifdef CONFIG_X86_64
>  	void		*iommu;		/* IOMMU private data */
>  #endif
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Yinghai Lu]

On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> --- a/arch/x86/include/asm/pci.h
>> +++ b/arch/x86/include/asm/pci.h
>> @@ -14,6 +14,7 @@
>>  struct pci_sysdata {
>>       int             domain;         /* PCI domain */
>>       int             node;           /* NUMA node */
>> +     void            *acpi_handle;
>>  #ifdef CONFIG_X86_64
>>       void            *iommu;         /* IOMMU private data */
>>  #endif
>>

acpi_handle is not good name and it is confusing.

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

[Rafael J. Wysocki]

On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> --- a/arch/x86/include/asm/pci.h
> >> +++ b/arch/x86/include/asm/pci.h
> >> @@ -14,6 +14,7 @@
> >>  struct pci_sysdata {
> >>       int             domain;         /* PCI domain */
> >>       int             node;           /* NUMA node */
> >> +     void            *acpi_handle;
> >>  #ifdef CONFIG_X86_64
> >>       void            *iommu;         /* IOMMU private data */
> >>  #endif
> >>
> 
> acpi_handle is not good name and it is confusing.

Well, what would be a better name in your opinion?

I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
calling it acpi_data?

Rafael


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

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

[Yinghai Lu]

On Fri, Jan 4, 2013 at 4:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
>> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> --- a/arch/x86/include/asm/pci.h
>> >> +++ b/arch/x86/include/asm/pci.h
>> >> @@ -14,6 +14,7 @@
>> >>  struct pci_sysdata {
>> >>       int             domain;         /* PCI domain */
>> >>       int             node;           /* NUMA node */
>> >> +     void            *acpi_handle;
>> >>  #ifdef CONFIG_X86_64
>> >>       void            *iommu;         /* IOMMU private data */
>> >>  #endif
>> >>
>>
>> acpi_handle is not good name and it is confusing.
>
> Well, what would be a better name in your opinion?
>
> I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
> calling it acpi_data?

yes, with #ifdef, you can use acpi_handle type directly.

it is acpi handle for pci_root.

so would call int pci_root_acpi_handle ?

Thanks

Yinghai

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

[Bjorn Helgaas]

On Fri, Jan 4, 2013 at 5:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Jan 4, 2013 at 4:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
>>> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> >> --- a/arch/x86/include/asm/pci.h
>>> >> +++ b/arch/x86/include/asm/pci.h
>>> >> @@ -14,6 +14,7 @@
>>> >>  struct pci_sysdata {
>>> >>       int             domain;         /* PCI domain */
>>> >>       int             node;           /* NUMA node */
>>> >> +     void            *acpi_handle;
>>> >>  #ifdef CONFIG_X86_64
>>> >>       void            *iommu;         /* IOMMU private data */
>>> >>  #endif
>>> >>
>>>
>>> acpi_handle is not good name and it is confusing.
>>
>> Well, what would be a better name in your opinion?
>>
>> I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
>> calling it acpi_data?
>
> yes, with #ifdef, you can use acpi_handle type directly.
>
> it is acpi handle for pci_root.
>
> so would call int pci_root_acpi_handle ?

I just copied the name from the corresponding ia64 code.  I don't care
if you want to change it, but I think there is *some* value in keeping
the x86 and ia64 code as similar as possible because it would be nice
to converge it some day.

Bjorn

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

[Rafael J. Wysocki]

On Friday, January 04, 2013 05:36:55 PM Bjorn Helgaas wrote:
> On Fri, Jan 4, 2013 at 5:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Fri, Jan 4, 2013 at 4:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
> >>> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>> >> --- a/arch/x86/include/asm/pci.h
> >>> >> +++ b/arch/x86/include/asm/pci.h
> >>> >> @@ -14,6 +14,7 @@
> >>> >>  struct pci_sysdata {
> >>> >>       int             domain;         /* PCI domain */
> >>> >>       int             node;           /* NUMA node */
> >>> >> +     void            *acpi_handle;
> >>> >>  #ifdef CONFIG_X86_64
> >>> >>       void            *iommu;         /* IOMMU private data */
> >>> >>  #endif
> >>> >>
> >>>
> >>> acpi_handle is not good name and it is confusing.
> >>
> >> Well, what would be a better name in your opinion?
> >>
> >> I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
> >> calling it acpi_data?
> >
> > yes, with #ifdef, you can use acpi_handle type directly.
> >
> > it is acpi handle for pci_root.
> >
> > so would call int pci_root_acpi_handle ?
> 
> I just copied the name from the corresponding ia64 code.  I don't care
> if you want to change it, but I think there is *some* value in keeping
> the x86 and ia64 code as similar as possible because it would be nice
> to converge it some day.

Well, the corresponding data structure for ia64 is called struct pci_controller,
so it is quite obvious what acpi_handle in there means. :-)

Since the data structure for x86 is called struct pci_sysdata and the data
type for the field in question may be acpi_handle, perhaps we can call that
field simply "root_handle"?

Alternatively, in analogy with the iommu we could use void * as its data
type and call it simply "acpi".

That said I'm fine with using just "void *acpi_handle" as you did, but I would
do the #ifdef CONFIG_ACPI / #endif around it anyway.

I wonder what Peter thinks?

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

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


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

[Alternative][PATCH] ACPI / PCI: Set root bridge ACPI handle in advance, v2

[Rafael J. Wysocki]

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().

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.

Included is a fix for breakage on systems with non-ACPI PCI host
bridges from Bjorn Helgaas.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Well, I guess it's time to resend this one before everybody forgets it. :-)

Bjorn, Yinghai,

For arch/x86/include/asm/pci.h is used the iommu convention and called the
new pointer "acpi".

Thanks,
Rafael

---
 arch/ia64/pci/pci.c        |    8 ++++++++
 arch/x86/include/asm/pci.h |    3 +++
 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 ++
 8 files changed, 38 insertions(+), 38 deletions(-)

Index: linux-pm/drivers/pci/probe.c
===================================================================
--- linux-pm.orig/drivers/pci/probe.c
+++ linux-pm/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-pm/include/linux/pci.h
===================================================================
--- linux-pm.orig/include/linux/pci.h
+++ linux-pm/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-pm/arch/x86/pci/acpi.c
===================================================================
--- linux-pm.orig/arch/x86/pci/acpi.c
+++ linux-pm/arch/x86/pci/acpi.c
@@ -521,6 +521,7 @@ struct pci_bus *pci_acpi_scan_root(struc
 	sd = &info->sd;
 	sd->domain = domain;
 	sd->node = node;
+	sd->acpi = device->handle;
 	/*
 	 * 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.
@@ -592,6 +593,14 @@ struct pci_bus *pci_acpi_scan_root(struc
 	return bus;
 }
 
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	struct pci_sysdata *sd = bridge->bus->sysdata;
+
+	ACPI_HANDLE_SET(&bridge->dev, sd->acpi);
+	return 0;
+}
+
 int __init pci_acpi_init(void)
 {
 	struct pci_dev *dev = NULL;
Index: linux-pm/arch/ia64/pci/pci.c
===================================================================
--- linux-pm.orig/arch/ia64/pci/pci.c
+++ linux-pm/arch/ia64/pci/pci.c
@@ -393,6 +393,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 is_valid_resource(struct pci_dev *dev, int idx)
 {
 	unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/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);
@@ -376,7 +358,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-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
@@ -441,7 +441,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/x86/include/asm/pci.h
===================================================================
--- linux-pm.orig/arch/x86/include/asm/pci.h
+++ linux-pm/arch/x86/include/asm/pci.h
@@ -14,6 +14,9 @@
 struct pci_sysdata {
 	int		domain;		/* PCI domain */
 	int		node;		/* NUMA node */
+#ifdef CONFIG_ACPI
+	void		*acpi;		/* ACPI-specific data */
+#endif
 #ifdef CONFIG_X86_64
 	void		*iommu;		/* IOMMU private data */
 #endif

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

[Bjorn Helgaas]

On Wed, Jan 9, 2013 at 2:33 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().
>
> 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.
>
> Included is a fix for breakage on systems with non-ACPI PCI host
> bridges from Bjorn Helgaas.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Looks good to me!

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>
> Well, I guess it's time to resend this one before everybody forgets it. :-)
>
> Bjorn, Yinghai,
>
> For arch/x86/include/asm/pci.h is used the iommu convention and called the
> new pointer "acpi".
>
> Thanks,
> Rafael
>
> ---
>  arch/ia64/pci/pci.c        |    8 ++++++++
>  arch/x86/include/asm/pci.h |    3 +++
>  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 ++
>  8 files changed, 38 insertions(+), 38 deletions(-)
>
> Index: linux-pm/drivers/pci/probe.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/probe.c
> +++ linux-pm/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-pm/include/linux/pci.h
> ===================================================================
> --- linux-pm.orig/include/linux/pci.h
> +++ linux-pm/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-pm/arch/x86/pci/acpi.c
> ===================================================================
> --- linux-pm.orig/arch/x86/pci/acpi.c
> +++ linux-pm/arch/x86/pci/acpi.c
> @@ -521,6 +521,7 @@ struct pci_bus *pci_acpi_scan_root(struc
>         sd = &info->sd;
>         sd->domain = domain;
>         sd->node = node;
> +       sd->acpi = device->handle;
>         /*
>          * 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.
> @@ -592,6 +593,14 @@ struct pci_bus *pci_acpi_scan_root(struc
>         return bus;
>  }
>
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +       struct pci_sysdata *sd = bridge->bus->sysdata;
> +
> +       ACPI_HANDLE_SET(&bridge->dev, sd->acpi);
> +       return 0;
> +}
> +
>  int __init pci_acpi_init(void)
>  {
>         struct pci_dev *dev = NULL;
> Index: linux-pm/arch/ia64/pci/pci.c
> ===================================================================
> --- linux-pm.orig/arch/ia64/pci/pci.c
> +++ linux-pm/arch/ia64/pci/pci.c
> @@ -393,6 +393,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 is_valid_resource(struct pci_dev *dev, int idx)
>  {
>         unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/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);
> @@ -376,7 +358,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-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
> @@ -441,7 +441,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/x86/include/asm/pci.h
> ===================================================================
> --- linux-pm.orig/arch/x86/include/asm/pci.h
> +++ linux-pm/arch/x86/include/asm/pci.h
> @@ -14,6 +14,9 @@
>  struct pci_sysdata {
>         int             domain;         /* PCI domain */
>         int             node;           /* NUMA node */
> +#ifdef CONFIG_ACPI
> +       void            *acpi;          /* ACPI-specific data */
> +#endif
>  #ifdef CONFIG_X86_64
>         void            *iommu;         /* IOMMU private data */
>  #endif
>

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

[Rafael J. Wysocki]

On Wednesday, January 09, 2013 03:16:59 PM Bjorn Helgaas wrote:
> On Wed, Jan 9, 2013 at 2:33 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().
> >
> > 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.
> >
> > Included is a fix for breakage on systems with non-ACPI PCI host
> > bridges from Bjorn Helgaas.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Looks good to me!
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks!

I'll wait for comments from the others, if any, and put it into my acpi-scan
branch after a couple of days.  It doesn't need to be there technically, but
it's kind of related.

Thanks,
Rafael


> > ---
> >
> > Well, I guess it's time to resend this one before everybody forgets it. :-)
> >
> > Bjorn, Yinghai,
> >
> > For arch/x86/include/asm/pci.h is used the iommu convention and called the
> > new pointer "acpi".
> >
> > Thanks,
> > Rafael
> >
> > ---
> >  arch/ia64/pci/pci.c        |    8 ++++++++
> >  arch/x86/include/asm/pci.h |    3 +++
> >  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 ++
> >  8 files changed, 38 insertions(+), 38 deletions(-)
> >
> > Index: linux-pm/drivers/pci/probe.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/probe.c
> > +++ linux-pm/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-pm/include/linux/pci.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/pci.h
> > +++ linux-pm/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-pm/arch/x86/pci/acpi.c
> > ===================================================================
> > --- linux-pm.orig/arch/x86/pci/acpi.c
> > +++ linux-pm/arch/x86/pci/acpi.c
> > @@ -521,6 +521,7 @@ struct pci_bus *pci_acpi_scan_root(struc
> >         sd = &info->sd;
> >         sd->domain = domain;
> >         sd->node = node;
> > +       sd->acpi = device->handle;
> >         /*
> >          * 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.
> > @@ -592,6 +593,14 @@ struct pci_bus *pci_acpi_scan_root(struc
> >         return bus;
> >  }
> >
> > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> > +{
> > +       struct pci_sysdata *sd = bridge->bus->sysdata;
> > +
> > +       ACPI_HANDLE_SET(&bridge->dev, sd->acpi);
> > +       return 0;
> > +}
> > +
> >  int __init pci_acpi_init(void)
> >  {
> >         struct pci_dev *dev = NULL;
> > Index: linux-pm/arch/ia64/pci/pci.c
> > ===================================================================
> > --- linux-pm.orig/arch/ia64/pci/pci.c
> > +++ linux-pm/arch/ia64/pci/pci.c
> > @@ -393,6 +393,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 is_valid_resource(struct pci_dev *dev, int idx)
> >  {
> >         unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
> > Index: linux-pm/drivers/pci/pci-acpi.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci-acpi.c
> > +++ linux-pm/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);
> > @@ -376,7 +358,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-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
> > @@ -441,7 +441,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/x86/include/asm/pci.h
> > ===================================================================
> > --- linux-pm.orig/arch/x86/include/asm/pci.h
> > +++ linux-pm/arch/x86/include/asm/pci.h
> > @@ -14,6 +14,9 @@
> >  struct pci_sysdata {
> >         int             domain;         /* PCI domain */
> >         int             node;           /* NUMA node */
> > +#ifdef CONFIG_ACPI
> > +       void            *acpi;          /* ACPI-specific data */
> > +#endif
> >  #ifdef CONFIG_X86_64
> >         void            *iommu;         /* IOMMU private data */
> >  #endif
> >
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Yinghai Lu]

On Wed, Jan 9, 2013 at 3:06 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, January 09, 2013 03:16:59 PM Bjorn Helgaas wrote:
>> On Wed, Jan 9, 2013 at 2:33 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().
>> >
>> > 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.
>> >
>> > Included is a fix for breakage on systems with non-ACPI PCI host
>> > bridges from Bjorn Helgaas.
>> >
>> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Looks good to me!
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Thanks!
>
> I'll wait for comments from the others, if any, and put it into my acpi-scan
> branch after a couple of days.  It doesn't need to be there technically, but
> it's kind of related.

I tested it with acpi-scan and pci-root-bus-hotplug patchset, and it works well.
so
Acked-by: Yinghai Lu <yinghai@kernel.org>

BTW, found one left over of change about ->acpi_handle==> ACPI_HANDLE..

Index: linux-2.6/drivers/acpi/glue.c
===================================================================
--- linux-2.6.orig/drivers/acpi/glue.c
+++ linux-2.6/drivers/acpi/glue.c
@@ -311,7 +311,7 @@ static int acpi_platform_notify(struct d
        if (!ret) {
                struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };

-               acpi_get_name(dev->acpi_handle, ACPI_FULL_PATHNAME, &buffer);
+               acpi_get_name(ACPI_HANDLE(dev), ACPI_FULL_PATHNAME, &buffer);
                DBG("Device %s -> %s\n", dev_name(dev), (char *)buffer.pointer);
                kfree(buffer.pointer);
        } else


otherwise will get compiling error when enable acpi glue debug.

Thanks

Yinghai

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

[Rafael J. Wysocki]

On Wednesday, January 09, 2013 03:27:29 PM Yinghai Lu wrote:
> On Wed, Jan 9, 2013 at 3:06 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, January 09, 2013 03:16:59 PM Bjorn Helgaas wrote:
> >> On Wed, Jan 9, 2013 at 2:33 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().
> >> >
> >> > 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.
> >> >
> >> > Included is a fix for breakage on systems with non-ACPI PCI host
> >> > bridges from Bjorn Helgaas.
> >> >
> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Looks good to me!
> >>
> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Thanks!
> >
> > I'll wait for comments from the others, if any, and put it into my acpi-scan
> > branch after a couple of days.  It doesn't need to be there technically, but
> > it's kind of related.
> 
> I tested it with acpi-scan and pci-root-bus-hotplug patchset, and it works well.
> so
> Acked-by: Yinghai Lu <yinghai@kernel.org>

Thanks!

> BTW, found one left over of change about ->acpi_handle==> ACPI_HANDLE..

Can you please submit that patch with a proper changelog?

It is a mainline bug now (and a build regression for that matter), so I'd like to
push the fix to Linus for v3.8-rc4.

> Index: linux-2.6/drivers/acpi/glue.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/glue.c
> +++ linux-2.6/drivers/acpi/glue.c
> @@ -311,7 +311,7 @@ static int acpi_platform_notify(struct d
>         if (!ret) {
>                 struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> 
> -               acpi_get_name(dev->acpi_handle, ACPI_FULL_PATHNAME, &buffer);
> +               acpi_get_name(ACPI_HANDLE(dev), ACPI_FULL_PATHNAME, &buffer);
>                 DBG("Device %s -> %s\n", dev_name(dev), (char *)buffer.pointer);
>                 kfree(buffer.pointer);
>         } else
> 
> 
> otherwise will get compiling error when enable acpi glue debug.

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

On Thursday, January 10, 2013 01:05:24 AM Rafael J. Wysocki wrote:
> On Wednesday, January 09, 2013 03:27:29 PM Yinghai Lu wrote:
> > On Wed, Jan 9, 2013 at 3:06 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Wednesday, January 09, 2013 03:16:59 PM Bjorn Helgaas wrote:
> > >> On Wed, Jan 9, 2013 at 2:33 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().
> > >> >
> > >> > 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.
> > >> >
> > >> > Included is a fix for breakage on systems with non-ACPI PCI host
> > >> > bridges from Bjorn Helgaas.
> > >> >
> > >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >>
> > >> Looks good to me!
> > >>
> > >> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > >
> > > Thanks!
> > >
> > > I'll wait for comments from the others, if any, and put it into my acpi-scan
> > > branch after a couple of days.  It doesn't need to be there technically, but
> > > it's kind of related.
> > 
> > I tested it with acpi-scan and pci-root-bus-hotplug patchset, and it works well.
> > so
> > Acked-by: Yinghai Lu <yinghai@kernel.org>
> 
> Thanks!
> 
> > BTW, found one left over of change about ->acpi_handle==> ACPI_HANDLE..
> 
> Can you please submit that patch with a proper changelog?
> 
> It is a mainline bug now (and a build regression for that matter), so I'd like to
> push the fix to Linus for v3.8-rc4.

May I have your sign-off for the patch below at least?

Rafael


> > Index: linux-2.6/drivers/acpi/glue.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/glue.c
> > +++ linux-2.6/drivers/acpi/glue.c
> > @@ -311,7 +311,7 @@ static int acpi_platform_notify(struct d
> >         if (!ret) {
> >                 struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > 
> > -               acpi_get_name(dev->acpi_handle, ACPI_FULL_PATHNAME, &buffer);
> > +               acpi_get_name(ACPI_HANDLE(dev), ACPI_FULL_PATHNAME, &buffer);
> >                 DBG("Device %s -> %s\n", dev_name(dev), (char *)buffer.pointer);
> >                 kfree(buffer.pointer);
> >         } else
> > 
> > 


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

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

[Yinghai Lu]

On Fri, Jan 11, 2013 at 1:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> May I have your sign-off for the patch below at least?

Yes. please.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

>
>
> Rafael
>
>
>> > Index: linux-2.6/drivers/acpi/glue.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/glue.c
>> > +++ linux-2.6/drivers/acpi/glue.c
>> > @@ -311,7 +311,7 @@ static int acpi_platform_notify(struct d
>> >         if (!ret) {
>> >                 struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> >
>> > -               acpi_get_name(dev->acpi_handle, ACPI_FULL_PATHNAME, &buffer);
>> > +               acpi_get_name(ACPI_HANDLE(dev), ACPI_FULL_PATHNAME, &buffer);
>> >                 DBG("Device %s -> %s\n", dev_name(dev), (char *)buffer.pointer);
>> >                 kfree(buffer.pointer);
>> >         } else
>> >

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

[Yinghai Lu]

On Wed, Jan 9, 2013 at 3:06 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:

 > arch/ia64/pci/pci.c        |    8 ++++++++
 > arch/x86/include/asm/pci.h |    3 +++
 >  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 ++
 >  8 files changed, 38 insertions(+), 38 deletions
...
> I'll wait for comments from the others, if any, and put it into my acpi-scan
> branch after a couple of days.  It doesn't need to be there technically, but
> it's kind of related.

looks like you put it into acpi-scan-next instead.

and it does touch pci code.

can you put it in to acpi-scan ? so Bjorn could pull it again to pci/next.

I like to rebase my patches on top of pci/next.

Thanks

Yinghai

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

[Rafael J. Wysocki]

On Thursday, January 10, 2013 02:54:00 PM Yinghai Lu wrote:
> On Wed, Jan 9, 2013 at 3:06 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
>  > arch/ia64/pci/pci.c        |    8 ++++++++
>  > arch/x86/include/asm/pci.h |    3 +++
>  >  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 ++
>  >  8 files changed, 38 insertions(+), 38 deletions
> ...
> > I'll wait for comments from the others, if any, and put it into my acpi-scan
> > branch after a couple of days.  It doesn't need to be there technically, but
> > it's kind of related.
> 
> looks like you put it into acpi-scan-next instead.
> 
> and it does touch pci code.
> 
> can you put it in to acpi-scan ? so Bjorn could pull it again to pci/next.

I will tomorrow.

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

On Friday, January 11, 2013 12:40:30 AM Rafael J. Wysocki wrote:
> On Thursday, January 10, 2013 02:54:00 PM Yinghai Lu wrote:
> > On Wed, Jan 9, 2013 at 3:06 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> >  > arch/ia64/pci/pci.c        |    8 ++++++++
> >  > arch/x86/include/asm/pci.h |    3 +++
> >  >  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 ++
> >  >  8 files changed, 38 insertions(+), 38 deletions
> > ...
> > > I'll wait for comments from the others, if any, and put it into my acpi-scan
> > > branch after a couple of days.  It doesn't need to be there technically, but
> > > it's kind of related.
> > 
> > looks like you put it into acpi-scan-next instead.
> > 
> > and it does touch pci code.
> > 
> > can you put it in to acpi-scan ? so Bjorn could pull it again to pci/next.
> 
> I will tomorrow.

There is a problem with this, because the acpi-scan branch is based on v3.8-rc2
that doesn't the contain __devinit removal in arch/ia64/pci/pci.c which conflicts
with the version of the $subject patch I have in linux-next.  If I put this
patch into acpi-scan and Bjorn pulls from it, he will need to resolve that
conflict going forward and I will have to resolve it too, so we'll end up with
two merge commits resolving the same conflict.

For this reason, I think it would be better if Bjorn took the $subject patch
directly to his tree, so I've dropped it from my linux-next branch.

Bjorn, can you please apply https://patchwork.kernel.org/patch/1956491/ directly?

Rafael


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