diff mbox

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

Message ID 5457615.T2WjKPXXC4@vostro.rjw.lan
State Not Applicable
Headers show

Commit Message

Rafael J. Wysocki Dec. 25, 2012, 10:42 p.m. UTC
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(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Helgaas Dec. 26, 2012, 6:14 p.m. UTC | #1
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))
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Dec. 26, 2012, 8:04 p.m. UTC | #2
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>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Dec. 26, 2012, 8:10 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Dec. 26, 2012, 8:16 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Dec. 26, 2012, 8:41 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Dec. 26, 2012, 10:35 p.m. UTC | #6
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))
> >
> >
Rafael J. Wysocki Dec. 26, 2012, 10:36 p.m. UTC | #7
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
Yinghai Lu Dec. 27, 2012, 12:10 a.m. UTC | #8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Dec. 27, 2012, 12:47 p.m. UTC | #9
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
Rafael J. Wysocki Dec. 27, 2012, 1:31 p.m. UTC | #10
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
Rafael J. Wysocki Dec. 27, 2012, 9:25 p.m. UTC | #11
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
diff mbox

Patch

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