Re: [PATCH v3 7/8] ACPI, PCI: add hostbridge removal function

From: Bjorn Helgaas <bhelgaas@google.com>
To: Yinghai Lu <yinghai@kernel.org>
Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>,
	linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,
	kaneshige.kenji@jp.fujitsu.com, jiang.liu@huawei.com
Subject: Re: [PATCH v3 7/8] ACPI, PCI: add hostbridge removal function
Date: Fri, 28 Sep 2012 13:44:06 -0600	[thread overview]
Message-ID: <CAErSpo4cR0v0Hy9ujgizDcNOB4wgLrgGrewQH2GewWihHMV9jQ@mail.gmail.com> (raw)
In-Reply-To: <CAE9FiQUbk+Z0OVYMuouEfXTZuwG0ZyPOi=PNwFdmuij_J07-QA@mail.gmail.com>

On Fri, Sep 28, 2012 at 10:19 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 28, 2012 at 9:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Sep 27, 2012 at 2:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> Today we have this, which is more complicated than it should be.  Note
>> how we do some ACPI stuff, some PCI stuff, some more ACPI stuff, then
>> more PCI stuff:
>>
>>     acpi_pci_root_add
>>         pci_acpi_scan_root
>>             pci_scan_child_bus
>>         acpi_pci_irq_add_prt
>>         acpi_pci_osc_control_set
>>     acpi_pci_root_start
>>         pci_bus_add_devices
>>
>> I don't think the ACPI/PCI mixture is anything essential dictated by
>> the way the hardware or firmware works.  I think it's just an artifact
>> of the current design, and it could be changed.  It would be better to
>> have this:
>>
>>     acpi_pci_root_add
>>         acpi_pci_irq_add_prt
>>         acpi_pci_osc_control_set
>>         pci_acpi_scan_root
>>             pci_scan_root_bus
>>                 pci_scan_child_bus
>>                 pci_bus_add_devices
>>
>> We can't get to this latter strategy as long as the ACPI interfaces
>> depend on the struct pci_bus.  So the _PRT change is a small thing in
>> itself, but I do think it helps enable significant improvements in the
>> future.
>
> still to handle to those fallback path like create_bus and scan bus failure.
>
> in my for-pci-next branch, with Jiang's patches and mine, now we achieved at
>
>     acpi_pci_root_add
>         pci_acpi_scan_root
>             pci_scan_root_bus
>                 pci_scan_child_bus
>         acpi_pci_osc_control_set
>         pci_bus_add_devices
>
> acpi_pci_irq_add_prt is called later during acpi binding that is
> triggered by adding to device tree.
> thought os_control set via pci_host_bridge add interface..
>
> with those BUS ADD notification, we can pass bus safely, and without
> considering about cleanup PRT and OSC setting.

I haven't looked at those patches yet.

Is there a reason why acpi_pci_osc_control_set() needs to be done
after pci_scan_child_bus()?  The argument that it might make the error
path somewhat simpler is not very convincing to me.  Having the arch
code call both pci_scan_child_bus() and pci_bus_add_devices() is a
much more fundamental complexity -- it makes x86 and ia64 different
from many architectures, and it exposes the intermediate state where
"devices have been enumerated but not added" to a lot more code.

It doesn't sound like an improvement to call acpi_pci_irq_add_prt()
using a bus add notifier.  At least for the host bridge case, it's
clear, simple, and straightforward to call it in acpi_pci_root_add().
Notifiers are useful in some cases, but they definitely make the code
harder to follow.