Date:	Wed, 26 Dec 2012 13:10:23 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Tony Luck <tony.luck@...el.com>,
	"H. Peter Anvin" <hpa@...or.com>, Jiang Liu <liuj97@...il.com>,
	Myron Stowe <myron.stowe@...hat.com>
Subject: Re: [Alternative][PATCH] ACPI / PCI: Set root bridge ACPI handle in advance

On Wed, Dec 26, 2012 at 1:04 PM, Yinghai Lu <yinghai@...nel.org> wrote:
> On Tue, Dec 25, 2012 at 2:42 PM, Rafael J. Wysocki <rjw@...k.pl> wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> Subject: PCI / ACPI: Set root bridge ACPI handle in advance
>>
>> The ACPI handles of PCI root bridges need to be known to
>> acpi_bind_one(), so that it can create the appropriate
>> "firmware_node" and "physical_node" files for them, but currently
>> the way it gets to know those handles is not exactly straightforward
>> (to put it lightly).
>>
>> This is how it works, roughly:
>>
>>   1. acpi_bus_scan() finds the handle of a PCI root bridge,
>>      creates a struct acpi_device object for it and passes that
>>      object to acpi_pci_root_add().
>>
>>   2. acpi_pci_root_add() creates a struct acpi_pci_root object,
>>      populates its "device" field with its argument's address
>>      (device->handle is the ACPI handle found in step 1).
>>
>>   3. The struct acpi_pci_root object created in step 2 is passed
>>      to pci_acpi_scan_root() and used to get resources that are
>>      passed to pci_create_root_bus().
>>
>>   4. pci_create_root_bus() creates a struct pci_host_bridge object
>>      and passes its "dev" member to device_register().
>>
>>   5. platform_notify(), which for systems with ACPI is set to
>>      acpi_platform_notify(), is called.
>>
>> So far, so good.  Now it starts to be "interesting".
>>
>>   6. acpi_find_bridge_device() is used to find the ACPI handle of
>>      the given device (which is the PCI root bridge) and executes
>>      acpi_pci_find_root_bridge(), among other things, for the
>>      given device object.
>>
>>   7. acpi_pci_find_root_bridge() uses the name (sic!) of the given
>>      device object to extract the segment and bus numbers of the PCI
>>      root bridge and passes them to acpi_get_pci_rootbridge_handle().
>>
>>   8. acpi_get_pci_rootbridge_handle() browses the list of ACPI PCI
>>      root bridges and finds the one that matches the given segment
>>      and bus numbers.  Its handle is then used to initialize the
>>      ACPI handle of the PCI root bridge's device object by
>>      acpi_bind_one().  However, this is *exactly* the ACPI handle we
>>      started with in step 1.
>>
>> Needless to say, this is quite embarassing, but it may be avoided
>> thanks to commit f3fd0c8 (ACPI: Allow ACPI handles of devices to be
>> initialized in advance), which makes it possible to initialize the
>> ACPI handle of a device before passing it to device_register().
>>
>> Accordingly, add a new __weak routine, pcibios_root_bridge_prepare(),
>> defaulting to an empty implementation that can be replaced by the
>> interested architecutres (x86 and ia64 at the moment) with functions
>> that will set the root bridge's ACPI handle before its dev member is
>> passed to device_register().  Make both x86 and ia64 provide such
>> implementations of pcibios_root_bridge_prepare() and remove
>> acpi_pci_find_root_bridge() and acpi_get_pci_rootbridge_handle() that aren't
>> necessary any more.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> ---
>>
>> Bjorn,
>>
>> Since you didn't like the implementation used in the previous patch, here's
>> an alternative one using a __weak function.
>>
>> I don't really strongly prefer any of them.  The advantage of the present one
>> is that it changes fewer files and directly affects fewer architectures.  The
>> disadvantage of it is the addition of the __weak "callback".
>>
>> I wonder what the maintainers of the architectures in question (Peter, Tony)
>> think.
>>
>> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists