[PATCH 0/8] PCI, ACPI, x86: pci root bus hotplug support pci_root.c related core changes

[PATCH 0/8] PCI, ACPI, x86: pci root bus hotplug support pci_root.c related core changes

[Yinghai Lu]

based on pci/next

could get from
        git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-root-bus-hotplug

Yinghai Lu (8):
  PCI: Separate out pci_assign_unassigned_bus_resources()
  PCI: Move pci_rescan_bus() back to probe.c
  PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res
  PCI, ACPI: assign unassigned resource for hot add root bus
  PCI: Add pci_stop_and_remove_root_bus()
  PCI, ACPI: Make acpi_pci_root_remove stop/remove pci root bus
  PCI, ACPI: delete root bus prt during hot remove path
  PCI, ACPI: remove acpi_root_driver in reserse order

 drivers/acpi/pci_root.c |   21 ++++++++++++++++++++-
 drivers/pci/probe.c     |   22 ++++++++++++++++++++++
 drivers/pci/remove.c    |   36 ++++++++++++++++++++++++++++++++++++
 drivers/pci/setup-bus.c |   22 +---------------------
 include/linux/pci.h     |    3 +++
 5 files changed, 82 insertions(+), 22 deletions(-)

-- 
1.7.7

[PATCH 1/8] PCI: Separate out pci_assign_unassigned_bus_resources()

[Yinghai Lu]

It is main portion of pci_rescan_bus().

Separate it out and prepare to use it for pci root bus hot add later.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c |   32 ++++++++++++++++++--------------
 include/linux/pci.h     |    1 +
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 1e808ca..1f34929 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1550,25 +1550,12 @@ enable_all:
 }
 EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
 
-#ifdef CONFIG_HOTPLUG
-/**
- * pci_rescan_bus - scan a PCI bus for devices.
- * @bus: PCI bus to scan
- *
- * Scan a PCI bus and child buses for new devices, adds them,
- * and enables them.
- *
- * Returns the max number of subordinate bus discovered.
- */
-unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
+void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 {
-	unsigned int max;
 	struct pci_dev *dev;
 	LIST_HEAD(add_list); /* list of resources that
 					want additional resources */
 
-	max = pci_scan_child_bus(bus);
-
 	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &bus->devices, bus_list)
 		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
@@ -1581,6 +1568,23 @@ unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
 	BUG_ON(!list_empty(&add_list));
 
 	pci_enable_bridges(bus);
+}
+#ifdef CONFIG_HOTPLUG
+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+
+	max = pci_scan_child_bus(bus);
+	pci_assign_unassigned_bus_resources(bus);
 	pci_bus_add_devices(bus);
 
 	return max;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index be1de01..505c05a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -980,6 +980,7 @@ void pci_bus_size_bridges(struct pci_bus *bus);
 int pci_claim_resource(struct pci_dev *, int);
 void pci_assign_unassigned_resources(void);
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge);
+void pci_assign_unassigned_bus_resources(struct pci_bus *bus);
 void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
-- 
1.7.7

[PATCH 2/8] PCI: Move pci_rescan_bus() back to probe.c

[Yinghai Lu]

We have pci_assign_unassigned_bus_resources() in as global function now.

Move back pci_rescan_bus to probe.c where it should be.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c     |   21 +++++++++++++++++++++
 drivers/pci/setup-bus.c |   22 ----------------------
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ec909af..65f62e3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1890,6 +1890,27 @@ unsigned int __ref pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 	return max;
 }
 
+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+
+	max = pci_scan_child_bus(bus);
+	pci_assign_unassigned_bus_resources(bus);
+	pci_bus_add_devices(bus);
+
+	return max;
+}
+EXPORT_SYMBOL_GPL(pci_rescan_bus);
+
 EXPORT_SYMBOL(pci_add_new_bus);
 EXPORT_SYMBOL(pci_scan_slot);
 EXPORT_SYMBOL(pci_scan_bridge);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 1f34929..59e6c55 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1569,25 +1569,3 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 
 	pci_enable_bridges(bus);
 }
-#ifdef CONFIG_HOTPLUG
-/**
- * pci_rescan_bus - scan a PCI bus for devices.
- * @bus: PCI bus to scan
- *
- * Scan a PCI bus and child buses for new devices, adds them,
- * and enables them.
- *
- * Returns the max number of subordinate bus discovered.
- */
-unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
-{
-	unsigned int max;
-
-	max = pci_scan_child_bus(bus);
-	pci_assign_unassigned_bus_resources(bus);
-	pci_bus_add_devices(bus);
-
-	return max;
-}
-EXPORT_SYMBOL_GPL(pci_rescan_bus);
-#endif
-- 
1.7.7

[PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Yinghai Lu]

So could use assign_unassigned_bus_res pci root bus add

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c     |    1 +
 drivers/pci/setup-bus.c |    2 --
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 65f62e3..59cf1ba 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1905,6 +1905,7 @@ unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
 
 	max = pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+	pci_enable_bridges(bus);
 	pci_bus_add_devices(bus);
 
 	return max;
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 59e6c55..6d3591d 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1566,6 +1566,4 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 	up_read(&pci_bus_sem);
 	__pci_bus_assign_resources(bus, &add_list, NULL);
 	BUG_ON(!list_empty(&add_list));
-
-	pci_enable_bridges(bus);
 }
-- 
1.7.7

Re: [PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Bjorn Helgaas]

On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> So could use assign_unassigned_bus_res pci root bus add

After your series, acpi_pci_root_start() looks like this:

    pci_assign_unassigned_bus_resources
    list_for_each_entry(driver, &acpi_pci_drivers, node)
        driver->add(root);
    pci_enable_bridges(root->bus);

so apparently it's important that the driver->add() methods be run
*before* the bridges are enabled.  Why?

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/probe.c     |    1 +
>  drivers/pci/setup-bus.c |    2 --
>  2 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 65f62e3..59cf1ba 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1905,6 +1905,7 @@ unsigned int __ref pci_rescan_bus(struct pci_bus *bus)
>
>         max = pci_scan_child_bus(bus);
>         pci_assign_unassigned_bus_resources(bus);
> +       pci_enable_bridges(bus);
>         pci_bus_add_devices(bus);
>
>         return max;
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 59e6c55..6d3591d 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1566,6 +1566,4 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
>         up_read(&pci_bus_sem);
>         __pci_bus_assign_resources(bus, &add_list, NULL);
>         BUG_ON(!list_empty(&add_list));
> -
> -       pci_enable_bridges(bus);
>  }
> --
> 1.7.7
>

Re: [PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Yinghai Lu]

On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> So could use assign_unassigned_bus_res pci root bus add
>
> After your series, acpi_pci_root_start() looks like this:
>
>     pci_assign_unassigned_bus_resources
>     list_for_each_entry(driver, &acpi_pci_drivers, node)
>         driver->add(root);
>     pci_enable_bridges(root->bus);
>
> so apparently it's important that the driver->add() methods be run
> *before* the bridges are enabled.  Why?

pci_enable_bridge

Re: [PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Yinghai Lu]

On Fri, Sep 28, 2012 at 6:48 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> So could use assign_unassigned_bus_res pci root bus add
>>
>> After your series, acpi_pci_root_start() looks like this:
>>
>>     pci_assign_unassigned_bus_resources
>>     list_for_each_entry(driver, &acpi_pci_drivers, node)
>>         driver->add(root);
>>     pci_enable_bridges(root->bus);
>>
>> so apparently it's important that the driver->add() methods be run
>> *before* the bridges are enabled.  Why?
>

During rebase, the changelog get lost.

 pci_enable_bridges
    ==> pci_enable_device
        ==> ...
           ===> pcibios_enable_device
               ===> pcibios_enable_irq

and one of driver in acpi_pci_drivers, is ioapic drivers and it will
enable ioapic there.

So we need to enable bridges that later.

-Yinghai

Re: [PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Bjorn Helgaas]

On Fri, Sep 28, 2012 at 7:52 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 28, 2012 at 6:48 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> So could use assign_unassigned_bus_res pci root bus add
>>>
>>> After your series, acpi_pci_root_start() looks like this:
>>>
>>>     pci_assign_unassigned_bus_resources
>>>     list_for_each_entry(driver, &acpi_pci_drivers, node)
>>>         driver->add(root);
>>>     pci_enable_bridges(root->bus);
>>>
>>> so apparently it's important that the driver->add() methods be run
>>> *before* the bridges are enabled.  Why?
>>
>
> During rebase, the changelog get lost.
>
>  pci_enable_bridges
>     ==> pci_enable_device
>         ==> ...
>            ===> pcibios_enable_device
>                ===> pcibios_enable_irq
>
> and one of driver in acpi_pci_drivers, is ioapic drivers and it will
> enable ioapic there.
>
> So we need to enable bridges that later.

Is it important that pci_assign_unassigned_bus_resources() be run
before the driver->add() methods?  Why?

Do you have a pointer to this ioapic driver?  I don't think it's in
the tree yet.  I know about drivers/pci/ioapic.c, but it doesn't use
acpi_pci_register_driver().

Re: [PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Yinghai Lu]

On Fri, Sep 28, 2012 at 8:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> Is it important that pci_assign_unassigned_bus_resources() be run
> before the driver->add() methods?  Why?

for ioapic controller that may need kernel to assign base bar.

>
> Do you have a pointer to this ioapic driver?  I don't think it's in
> the tree yet.  I know about drivers/pci/ioapic.c, but it doesn't use
> acpi_pci_register_driver().

it is in my for-x86-irq branch.

http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-x86-irq

http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=c186df98f84336422088740ef8cedc1313ca2485

Re: [PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Bjorn Helgaas]

On Fri, Sep 28, 2012 at 10:04 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 28, 2012 at 8:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> Is it important that pci_assign_unassigned_bus_resources() be run
>> before the driver->add() methods?  Why?
>
> for ioapic controller that may need kernel to assign base bar.

It seems like there are two cases of IOAPICs we need to worry about:

1) An IOAPIC device in the ACPI namespace (ACPI0009, ACPI000A, or
ACPI000B).  This device has a _CRS that contains the device base
address.  Since it's an ACPI device and has _CRS, I expect it should
*not* appear in PCI config space and would have no BARs.

2) An IOAPIC device that appears in PCI config space.  This has a BAR
that we'll need to assign when hot-adding the device.

The spec (ACPI v5.0, sec 9.17) implies that the second type (a
bus-enumerated device) will not have an device in the ACPI namespace.
However, sec 6.2.6 says we need a _GSB for both types of IOAPICs, and
I don't know where the _GSB for bus-enumerated IOAPICs would live if
there's no ACPI device for them.

Since you're talking about assigning a BAR, I assume you're talking
about the second type -- a bus-enumerated IOAPIC.  I would also assume
that a PCI IOAPIC would appear in the hierarchy *above* any devices
that are connected to it, so we'd enumerate it *before* those devices.
 Maybe that will help ensure that we bind the driver to the IOAPIC
before binding drivers to devices that use it.

Can you help me understand this?  What's the topology of the machine
you're working on?  E.g., what does this part of the ACPI namespace
and PCI config space look like?

>> Do you have a pointer to this ioapic driver?  I don't think it's in
>> the tree yet.  I know about drivers/pci/ioapic.c, but it doesn't use
>> acpi_pci_register_driver().
>
> it is in my for-x86-irq branch.
>
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-x86-irq
>
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=c186df98f84336422088740ef8cedc1313ca2485

Re: [PATCH 3/8] PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res

[Yinghai Lu]

On Fri, Sep 28, 2012 at 6:52 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 28, 2012 at 6:48 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> So could use assign_unassigned_bus_res pci root bus add
>>>
>>> After your series, acpi_pci_root_start() looks like this:
>>>
>>>     pci_assign_unassigned_bus_resources
>>>     list_for_each_entry(driver, &acpi_pci_drivers, node)
>>>         driver->add(root);
>>>     pci_enable_bridges(root->bus);
>>>
>>> so apparently it's important that the driver->add() methods be run
>>> *before* the bridges are enabled.  Why?
>>
>
> During rebase, the changelog get lost.
>
>  pci_enable_bridges
>     ==> pci_enable_device
>         ==> ...
>            ===> pcibios_enable_device
>                ===> pcibios_enable_irq
>
> and one of driver in acpi_pci_drivers, is ioapic drivers and it will
> enable ioapic there.
>
> So we need to enable bridges that later.

old changelog before rebase:

-----
Subject: [PATCH] PCI, x86: Move pci_enable_bridges() down

After we get hot-added ioapic registered.
pci_enable_bridges will try to enable ioapic irq for pci bridge.

So need to move it down.

Or We can move out pcibios_enable_irq() out of pci_enable_device()
and call pcibios_enable_irq in pci_bus_add_devices ?
also will need to move ...
        pcibios_resource_survey_bus(root->bus);
        pci_assign_unassigned_bus_resources(root->bus);
to the start add ....
----

[PATCH 4/8] PCI, ACPI: assign unassigned resource for hot add root bus

[Yinghai Lu]

After we get hot-added ioapic registered.
pci_enable_bridges will try to enable ioapic irq for pci bridges.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_root.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index bce469c..27adbfd 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device)
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
 
+	if (system_state != SYSTEM_BOOTING)
+		pci_assign_unassigned_bus_resources(root->bus);
+
 	mutex_lock(&acpi_pci_root_lock);
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
 		if (driver->add)
 			driver->add(root);
 	mutex_unlock(&acpi_pci_root_lock);
 
+	/* need to after hot-added ioapic is registered */
+	if (system_state != SYSTEM_BOOTING)
+		pci_enable_bridges(root->bus);
+
 	pci_bus_add_devices(root->bus);
 
 	return 0;
-- 
1.7.7

Re: [PATCH 4/8] PCI, ACPI: assign unassigned resource for hot add root bus

[Bjorn Helgaas]

On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> After we get hot-added ioapic registered.
> pci_enable_bridges will try to enable ioapic irq for pci bridges.

What makes this specifically related to IOAPICs?

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/acpi/pci_root.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index bce469c..27adbfd 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device)
>         struct acpi_pci_root *root = acpi_driver_data(device);
>         struct acpi_pci_driver *driver;
>
> +       if (system_state != SYSTEM_BOOTING)
> +               pci_assign_unassigned_bus_resources(root->bus);
> +
>         mutex_lock(&acpi_pci_root_lock);
>         list_for_each_entry(driver, &acpi_pci_drivers, node)
>                 if (driver->add)
>                         driver->add(root);
>         mutex_unlock(&acpi_pci_root_lock);
>
> +       /* need to after hot-added ioapic is registered */
> +       if (system_state != SYSTEM_BOOTING)
> +               pci_enable_bridges(root->bus);

Theoretically, we should be able to assign resources and enable
bridges here regardless of the system_state.

I think the reason you don't want to do it while SYSTEM_BOOTING is
because we currently do this at boot-time:

    acpi_pci_root_add
        pci_scan_child_bus
    acpi_pci_root_start
        pci_bus_add_devices
    <account for some motherboard (PNP0C01) resources>
    pcibios_assign_resources            # fs_initcall
        pci_assign_unassigned_resources
            pci_enable_bridges

and without the SYSTEM_BOOTING check, we might assign PCI resources at
boot-time that conflict with motherboard resources.

Is that right?

This is completely non-obvious and future readers deserve a hint about
what's going on here.

The right way to do this would be to pay attention to the host bridge
apertures, and assign resources from within the apertures.  Then we
could always assign PCI resources when adding a host bridge instead of
in an fs_initcall.  I understand we still have legacy issues and
machines where we still don't pay attention to _CRS.  But if we're
doing things to work around a broken design, it's important to be
aware of how the design is broken so we have some hope of eventually
fixing the design.

>         pci_bus_add_devices(root->bus);
>
>         return 0;
> --
> 1.7.7
>

Re: [PATCH 4/8] PCI, ACPI: assign unassigned resource for hot add root bus

[Yinghai Lu]

On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> After we get hot-added ioapic registered.
>> pci_enable_bridges will try to enable ioapic irq for pci bridges.
>
> What makes this specifically related to IOAPICs?
>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/acpi/pci_root.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index bce469c..27adbfd 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device)
>>         struct acpi_pci_root *root = acpi_driver_data(device);
>>         struct acpi_pci_driver *driver;
>>
>> +       if (system_state != SYSTEM_BOOTING)
>> +               pci_assign_unassigned_bus_resources(root->bus);
>> +
>>         mutex_lock(&acpi_pci_root_lock);
>>         list_for_each_entry(driver, &acpi_pci_drivers, node)
>>                 if (driver->add)
>>                         driver->add(root);
>>         mutex_unlock(&acpi_pci_root_lock);
>>
>> +       /* need to after hot-added ioapic is registered */
>> +       if (system_state != SYSTEM_BOOTING)
>> +               pci_enable_bridges(root->bus);
>
> Theoretically, we should be able to assign resources and enable
> bridges here regardless of the system_state.
>
> I think the reason you don't want to do it while SYSTEM_BOOTING is
> because we currently do this at boot-time:
>
>     acpi_pci_root_add
>         pci_scan_child_bus
>     acpi_pci_root_start
>         pci_bus_add_devices
>     <account for some motherboard (PNP0C01) resources>
>     pcibios_assign_resources            # fs_initcall
>         pci_assign_unassigned_resources
>             pci_enable_bridges
>
> and without the SYSTEM_BOOTING check, we might assign PCI resources at
> boot-time that conflict with motherboard resources.
>
> Is that right?

yes.

>
> This is completely non-obvious and future readers deserve a hint about
> what's going on here.
>
> The right way to do this would be to pay attention to the host bridge
> apertures, and assign resources from within the apertures.  Then we
> could always assign PCI resources when adding a host bridge instead of
> in an fs_initcall.  I understand we still have legacy issues and
> machines where we still don't pay attention to _CRS.  But if we're
> doing things to work around a broken design, it's important to be
> aware of how the design is broken so we have some hope of eventually
> fixing the design.

that could be another big topic.

Re: [PATCH 4/8] PCI, ACPI: assign unassigned resource for hot add root bus

[Bjorn Helgaas]

On Fri, Sep 28, 2012 at 7:56 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> After we get hot-added ioapic registered.
>>> pci_enable_bridges will try to enable ioapic irq for pci bridges.
>>
>> What makes this specifically related to IOAPICs?

I think you missed this question.

>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> ---
>>>  drivers/acpi/pci_root.c |    7 +++++++
>>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>> index bce469c..27adbfd 100644
>>> --- a/drivers/acpi/pci_root.c
>>> +++ b/drivers/acpi/pci_root.c
>>> @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device)
>>>         struct acpi_pci_root *root = acpi_driver_data(device);
>>>         struct acpi_pci_driver *driver;
>>>
>>> +       if (system_state != SYSTEM_BOOTING)
>>> +               pci_assign_unassigned_bus_resources(root->bus);
>>> +
>>>         mutex_lock(&acpi_pci_root_lock);
>>>         list_for_each_entry(driver, &acpi_pci_drivers, node)
>>>                 if (driver->add)
>>>                         driver->add(root);
>>>         mutex_unlock(&acpi_pci_root_lock);
>>>
>>> +       /* need to after hot-added ioapic is registered */
>>> +       if (system_state != SYSTEM_BOOTING)
>>> +               pci_enable_bridges(root->bus);
>>
>> Theoretically, we should be able to assign resources and enable
>> bridges here regardless of the system_state.
>>
>> I think the reason you don't want to do it while SYSTEM_BOOTING is
>> because we currently do this at boot-time:
>>
>>     acpi_pci_root_add
>>         pci_scan_child_bus
>>     acpi_pci_root_start
>>         pci_bus_add_devices
>>     <account for some motherboard (PNP0C01) resources>
>>     pcibios_assign_resources            # fs_initcall
>>         pci_assign_unassigned_resources
>>             pci_enable_bridges
>>
>> and without the SYSTEM_BOOTING check, we might assign PCI resources at
>> boot-time that conflict with motherboard resources.
>>
>> Is that right?
>
> yes.
>
>>
>> This is completely non-obvious and future readers deserve a hint about
>> what's going on here.
>>
>> The right way to do this would be to pay attention to the host bridge
>> apertures, and assign resources from within the apertures.  Then we
>> could always assign PCI resources when adding a host bridge instead of
>> in an fs_initcall.  I understand we still have legacy issues and
>> machines where we still don't pay attention to _CRS.  But if we're
>> doing things to work around a broken design, it's important to be
>> aware of how the design is broken so we have some hope of eventually
>> fixing the design.
>
> that could be another big topic.

I agree, and I'm not asking you to fix that.  I'm just trying to
understand what's going on and write some comments and changelogs that
will help make this maintainable in the future.

Also note the question at the top -- your changelog calls out IOAPICs,
but I'm still not sure what this patch has to do with IOAPICs.

Re: [PATCH 4/8] PCI, ACPI: assign unassigned resource for hot add root bus

[Jiang Liu]

On 2012-9-29 11:31, Bjorn Helgaas wrote:
> On Fri, Sep 28, 2012 at 7:56 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> After we get hot-added ioapic registered.
>>>> pci_enable_bridges will try to enable ioapic irq for pci bridges.
>>>
>>> What makes this specifically related to IOAPICs?
> 
> I think you missed this question.
> 
>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>> ---
>>>>  drivers/acpi/pci_root.c |    7 +++++++
>>>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>>> index bce469c..27adbfd 100644
>>>> --- a/drivers/acpi/pci_root.c
>>>> +++ b/drivers/acpi/pci_root.c
>>>> @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device)
>>>>         struct acpi_pci_root *root = acpi_driver_data(device);
>>>>         struct acpi_pci_driver *driver;
>>>>
>>>> +       if (system_state != SYSTEM_BOOTING)
>>>> +               pci_assign_unassigned_bus_resources(root->bus);
>>>> +
>>>>         mutex_lock(&acpi_pci_root_lock);
>>>>         list_for_each_entry(driver, &acpi_pci_drivers, node)
>>>>                 if (driver->add)
>>>>                         driver->add(root);
>>>>         mutex_unlock(&acpi_pci_root_lock);
>>>>
>>>> +       /* need to after hot-added ioapic is registered */
>>>> +       if (system_state != SYSTEM_BOOTING)
>>>> +               pci_enable_bridges(root->bus);
>>>
>>> Theoretically, we should be able to assign resources and enable
>>> bridges here regardless of the system_state.
>>>
>>> I think the reason you don't want to do it while SYSTEM_BOOTING is
>>> because we currently do this at boot-time:
>>>
>>>     acpi_pci_root_add
>>>         pci_scan_child_bus
>>>     acpi_pci_root_start
>>>         pci_bus_add_devices
>>>     <account for some motherboard (PNP0C01) resources>
>>>     pcibios_assign_resources            # fs_initcall
>>>         pci_assign_unassigned_resources
>>>             pci_enable_bridges
>>>
>>> and without the SYSTEM_BOOTING check, we might assign PCI resources at
>>> boot-time that conflict with motherboard resources.
>>>
>>> Is that right?
>>
>> yes.
>>
>>>
>>> This is completely non-obvious and future readers deserve a hint about
>>> what's going on here.
>>>
>>> The right way to do this would be to pay attention to the host bridge
>>> apertures, and assign resources from within the apertures.  Then we
>>> could always assign PCI resources when adding a host bridge instead of
>>> in an fs_initcall.  I understand we still have legacy issues and
>>> machines where we still don't pay attention to _CRS.  But if we're
>>> doing things to work around a broken design, it's important to be
>>> aware of how the design is broken so we have some hope of eventually
>>> fixing the design.
>>
>> that could be another big topic.
> 
> I agree, and I'm not asking you to fix that.  I'm just trying to
> understand what's going on and write some comments and changelogs that
> will help make this maintainable in the future.
> 
> Also note the question at the top -- your changelog calls out IOAPICs,
> but I'm still not sure what this patch has to do with IOAPICs.
Hi Bjorn，
	Another weay is to add an ACPI driver for IOAPIC, so we can control
the order between IOAPIC and PCI host bridge.
	Thanks!
	Gerry

Re: [PATCH 4/8] PCI, ACPI: assign unassigned resource for hot add root bus

[Yinghai Lu]

On Fri, Sep 28, 2012 at 8:37 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2012-9-29 11:31, Bjorn Helgaas wrote:
>> I agree, and I'm not asking you to fix that.  I'm just trying to
>> understand what's going on and write some comments and changelogs that
>> will help make this maintainable in the future.
>>
>> Also note the question at the top -- your changelog calls out IOAPICs,
>> but I'm still not sure what this patch has to do with IOAPICs.
> Hi Bjorn，
>         Another weay is to add an ACPI driver for IOAPIC, so we can control
> the order between IOAPIC and PCI host bridge.

use acpi_driver instead of acpi_pci_driver ?

Re: [PATCH 4/8] PCI, ACPI: assign unassigned resource for hot add root bus

[Yinghai Lu]

On Fri, Sep 28, 2012 at 8:31 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Sep 28, 2012 at 7:56 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> After we get hot-added ioapic registered.
>>>> pci_enable_bridges will try to enable ioapic irq for pci bridges.
>>>
>>> What makes this specifically related to IOAPICs?
>
> I think you missed this question.

in another response:

 pci_enable_bridges
    ==> pci_enable_device
        ==> ...
           ===> pcibios_enable_device
               ===> pcibios_enable_irq

and one of driver in acpi_pci_drivers, is ioapic drivers and it will
enable ioapic there.

So we need to enable bridges that later.

[PATCH 5/8] PCI: Add pci_stop_and_remove_root_bus()

[Yinghai Lu]

It supports both pci root bus and pci bus under pci bridge.

-v2: clear pci_bridge's subordinate.
-v3: only handle root bus. and also put Jiang's get/put pair in
-v4: fold pci_stop/remove_bus_devices in... reducing confusing.
-v5: split device_register/unregister to avoid extra get...
     also remove extra blank line.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/remove.c |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  |    2 ++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 513972f..7c0fd92 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -111,3 +111,39 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
 	pci_remove_bus_device(dev);
 }
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
+
+void pci_stop_root_bus(struct pci_bus *bus)
+{
+	struct pci_dev *child, *tmp;
+	struct pci_host_bridge *host_bridge;
+
+	if (!pci_is_root_bus(bus))
+		return;
+
+	host_bridge = to_pci_host_bridge(bus->bridge);
+	list_for_each_entry_safe_reverse(child, tmp,
+					 &bus->devices, bus_list)
+		pci_stop_bus_device(child);
+
+	/* stop the host bridge */
+	device_del(&host_bridge->dev);
+}
+
+void pci_remove_root_bus(struct pci_bus *bus)
+{
+	struct pci_dev *child, *tmp;
+	struct pci_host_bridge *host_bridge;
+
+	if (!pci_is_root_bus(bus))
+		return;
+
+	host_bridge = to_pci_host_bridge(bus->bridge);
+	list_for_each_entry_safe(child, tmp,
+				 &bus->devices, bus_list)
+		pci_remove_bus_device(child);
+	pci_remove_bus(bus);
+	host_bridge->bus = NULL;
+
+	/* remove the host bridge */
+	put_device(&host_bridge->dev);
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 505c05a..a5cd03b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -734,6 +734,8 @@ extern struct pci_dev *pci_dev_get(struct pci_dev *dev);
 extern void pci_dev_put(struct pci_dev *dev);
 extern void pci_remove_bus(struct pci_bus *b);
 extern void pci_stop_and_remove_bus_device(struct pci_dev *dev);
+void pci_stop_root_bus(struct pci_bus *bus);
+void pci_remove_root_bus(struct pci_bus *bus);
 void pci_setup_cardbus(struct pci_bus *bus);
 extern void pci_sort_breadthfirst(void);
 #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
-- 
1.7.7

Re: [PATCH 5/8] PCI: Add pci_stop_and_remove_root_bus()

[Bjorn Helgaas]

On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> It supports both pci root bus and pci bus under pci bridge.
>
> -v2: clear pci_bridge's subordinate.
> -v3: only handle root bus. and also put Jiang's get/put pair in
> -v4: fold pci_stop/remove_bus_devices in... reducing confusing.
> -v5: split device_register/unregister to avoid extra get...
>      also remove extra blank line.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/remove.c |   36 ++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h  |    2 ++
>  2 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 513972f..7c0fd92 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -111,3 +111,39 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
>         pci_remove_bus_device(dev);
>  }
>  EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
> +
> +void pci_stop_root_bus(struct pci_bus *bus)
> +{
> +       struct pci_dev *child, *tmp;
> +       struct pci_host_bridge *host_bridge;
> +
> +       if (!pci_is_root_bus(bus))
> +               return;
> +
> +       host_bridge = to_pci_host_bridge(bus->bridge);

What if we made these functions just take a "struct pci_host_bridge *"
directly instead of a "struct pci_bus *"?  Then the caller
(acpi_pci_root_remove()) could just look up the pci_host_bridge
pointer itself, or even keep that pointer in struct acpi_pci_root
instead of keeping the pci_bus pointer.

> +       list_for_each_entry_safe_reverse(child, tmp,
> +                                        &bus->devices, bus_list)
> +               pci_stop_bus_device(child);
> +
> +       /* stop the host bridge */
> +       device_del(&host_bridge->dev);
> +}
> +
> +void pci_remove_root_bus(struct pci_bus *bus)
> +{
> +       struct pci_dev *child, *tmp;
> +       struct pci_host_bridge *host_bridge;
> +
> +       if (!pci_is_root_bus(bus))
> +               return;
> +
> +       host_bridge = to_pci_host_bridge(bus->bridge);
> +       list_for_each_entry_safe(child, tmp,
> +                                &bus->devices, bus_list)
> +               pci_remove_bus_device(child);
> +       pci_remove_bus(bus);
> +       host_bridge->bus = NULL;
> +
> +       /* remove the host bridge */
> +       put_device(&host_bridge->dev);
> +}
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 505c05a..a5cd03b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -734,6 +734,8 @@ extern struct pci_dev *pci_dev_get(struct pci_dev *dev);
>  extern void pci_dev_put(struct pci_dev *dev);
>  extern void pci_remove_bus(struct pci_bus *b);
>  extern void pci_stop_and_remove_bus_device(struct pci_dev *dev);
> +void pci_stop_root_bus(struct pci_bus *bus);
> +void pci_remove_root_bus(struct pci_bus *bus);
>  void pci_setup_cardbus(struct pci_bus *bus);
>  extern void pci_sort_breadthfirst(void);
>  #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
> --
> 1.7.7
>

Re: [PATCH 5/8] PCI: Add pci_stop_and_remove_root_bus()

[Yinghai Lu]

On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> It supports both pci root bus and pci bus under pci bridge.
>>
>> -v2: clear pci_bridge's subordinate.
>> -v3: only handle root bus. and also put Jiang's get/put pair in
>> -v4: fold pci_stop/remove_bus_devices in... reducing confusing.
>> -v5: split device_register/unregister to avoid extra get...
>>      also remove extra blank line.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/pci/remove.c |   36 ++++++++++++++++++++++++++++++++++++
>>  include/linux/pci.h  |    2 ++
>>  2 files changed, 38 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 513972f..7c0fd92 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -111,3 +111,39 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
>>         pci_remove_bus_device(dev);
>>  }
>>  EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
>> +
>> +void pci_stop_root_bus(struct pci_bus *bus)
>> +{
>> +       struct pci_dev *child, *tmp;
>> +       struct pci_host_bridge *host_bridge;
>> +
>> +       if (!pci_is_root_bus(bus))
>> +               return;
>> +
>> +       host_bridge = to_pci_host_bridge(bus->bridge);
>
> What if we made these functions just take a "struct pci_host_bridge *"
> directly instead of a "struct pci_bus *"?  Then the caller
> (acpi_pci_root_remove()) could just look up the pci_host_bridge
> pointer itself,

yes, that could save pci_is_root_bus checking.

> or even keep that pointer in struct acpi_pci_root
> instead of keeping the pci_bus pointer.

that could be a lot of change. could be next one stage.

actually acpi root is bound to pci host bridge instead of root bus.
so it is more reasonable to save host_bridge pointer.

we also can think in another way.
host_bridge and pci bus should be one part.
we may could just embed pci bus into pci_host_bridge instead.

-Yinghai

[PATCH 6/8] PCI, ACPI: Make acpi_pci_root_remove stop/remove pci root bus

[Yinghai Lu]

It will call new added pci_stop_and_remove_bus() to stop/remove pci root bus.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_root.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 27adbfd..aed0953 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -667,14 +667,20 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
 
+	pci_stop_root_bus(root->bus);
+
 	mutex_lock(&acpi_pci_root_lock);
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
 		if (driver->remove)
 			driver->remove(root);
+	mutex_unlock(&acpi_pci_root_lock);
 
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	pci_remove_root_bus(root->bus);
+
+	mutex_lock(&acpi_pci_root_lock);
 	list_del(&root->node);
 	mutex_unlock(&acpi_pci_root_lock);
 	kfree(root);
-- 
1.7.7

[PATCH 7/8] PCI, ACPI: delete root bus prt during hot remove path

[Yinghai Lu]

corresponding to add path.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_root.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index aed0953..a27cbb57 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -664,6 +664,8 @@ static int acpi_pci_root_start(struct acpi_device *device)
 
 static int acpi_pci_root_remove(struct acpi_device *device, int type)
 {
+	acpi_status status;
+	acpi_handle handle;
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
 
@@ -678,6 +680,10 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
+	if (ACPI_SUCCESS(status))
+		acpi_pci_irq_del_prt(root->bus);
+
 	pci_remove_root_bus(root->bus);
 
 	mutex_lock(&acpi_pci_root_lock);
-- 
1.7.7

[PATCH 8/8] PCI, ACPI: remove acpi_root_driver in reserse order

[Yinghai Lu]

aka stop acpi root driver in this sequence: acpiphp, iommu, ioapic.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_root.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index a27cbb57..012f40d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -672,7 +672,7 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
 	pci_stop_root_bus(root->bus);
 
 	mutex_lock(&acpi_pci_root_lock);
-	list_for_each_entry(driver, &acpi_pci_drivers, node)
+	list_for_each_entry_reverse(driver, &acpi_pci_drivers, node)
 		if (driver->remove)
 			driver->remove(root);
 	mutex_unlock(&acpi_pci_root_lock);
-- 
1.7.7

Re: [PATCH 8/8] PCI, ACPI: remove acpi_root_driver in reserse order

[Bjorn Helgaas]

On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> aka stop acpi root driver in this sequence: acpiphp, iommu, ioapic.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/acpi/pci_root.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index a27cbb57..012f40d 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -672,7 +672,7 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
>         pci_stop_root_bus(root->bus);
>
>         mutex_lock(&acpi_pci_root_lock);
> -       list_for_each_entry(driver, &acpi_pci_drivers, node)
> +       list_for_each_entry_reverse(driver, &acpi_pci_drivers, node)
>                 if (driver->remove)
>                         driver->remove(root);

This seems sort of OK just because we call the ->add() methods in
forward order, so it is symmetric to call the ->remove() methods in
reverse order.

But I'm uncomfortable with using this as a way to enforce ->remove()
ordering across acpiphp, iommu, and ioapic.  That seems fragile.

What are the dependencies between these drivers that require this ordering?

>         mutex_unlock(&acpi_pci_root_lock);
> --
> 1.7.7
>

Re: [PATCH 8/8] PCI, ACPI: remove acpi_root_driver in reserse order

[Yinghai Lu]

On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> aka stop acpi root driver in this sequence: acpiphp, iommu, ioapic.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/acpi/pci_root.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index a27cbb57..012f40d 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -672,7 +672,7 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
>>         pci_stop_root_bus(root->bus);
>>
>>         mutex_lock(&acpi_pci_root_lock);
>> -       list_for_each_entry(driver, &acpi_pci_drivers, node)
>> +       list_for_each_entry_reverse(driver, &acpi_pci_drivers, node)
>>                 if (driver->remove)
>>                         driver->remove(root);
>
> This seems sort of OK just because we call the ->add() methods in
> forward order, so it is symmetric to call the ->remove() methods in
> reverse order.
>
> But I'm uncomfortable with using this as a way to enforce ->remove()
> ordering across acpiphp, iommu, and ioapic.  That seems fragile.
>
> What are the dependencies between these drivers that require this ordering?

ioapic should come at first, and iommu and last is acpiphp.

iommu will check if intr-mapping is corresponding to ioapic controller number.
also dmar controller will need one normal irq without intr-remapping
for error handling.

need to use link sequence in Makefile to make sure iommu come at first.

-Yinghai

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

[Bjorn Helgaas]

I think I'm missing patches 7/8 and 8/8 from this series.  Can
you repost them to make sure I have the latest versions?

Note the comments below:

On Fri, Sep 28, 2012 at 06:46:27PM +0900, Taku Izumi wrote:
> 
> Currently there's no PCI-related clean-up code
> in acpi_pci_root_remove() function.
> This patch introduces function for hostbridge removal,
> and brings back pci_stop_bus_devices() function.
> 
> diff: rebased against current next
>       updated according to Bjorn's comment
> 
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/acpi/pci_bind.c     |    7 +++++++
>  drivers/acpi/pci_root.c     |    6 ++++++
>  drivers/pci/remove.c        |   28 ++++++++++++++++++++++++++++
>  include/acpi/acpi_drivers.h |    1 +
>  include/linux/pci.h         |    2 ++
>  5 files changed, 44 insertions(+)
> 
> Index: Bjorn-next-0925-configchange/drivers/pci/remove.c
> ===================================================================
> --- Bjorn-next-0925-configchange.orig/drivers/pci/remove.c
> +++ Bjorn-next-0925-configchange/drivers/pci/remove.c
> @@ -111,3 +111,31 @@ void pci_stop_and_remove_bus_device(stru
>  	pci_remove_bus_device(dev);
>  }
>  EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
> +
> +void pci_stop_bus_devices(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev, *tmp;
> +
> +	list_for_each_entry_safe_reverse(dev, tmp,
> +					 &bus->devices, bus_list) {
> +		pci_stop_bus_device(dev);
> +	}
> +
> +}
> +EXPORT_SYMBOL(pci_stop_bus_devices);

I'm hesitant to introduce pci_stop_bus_devices() again, particularly
when it is exported.  The stop/remove split introduces the state where
devices are "stopped" but haven't been "removed" yet.

In this state, the driver .remove() method has been called, sysfs has
been cleaned up, and the struct device has been unregistered, but the 
struct pci_dev itself still exists.  Obviously, this state *must*
exist internally in the PCI core as we remove the PCI device.

The problem is that we have non-core code that *depends* on being
run while in this transitory state.  I think this is a design mistake.

The code that depends on this state is basically just the stuff in the
acpi_pci_drivers list, namely, acpi_pci_hp_driver and acpi_pci_slot_driver.
I suspect that the main reason we have the acpi_pci_drivers list and the
whole acpi_pci_register_driver() infrastructure is so that these PCI
host bridge "sub-drivers" can be built as modules.

I don't think there's really any value in having these sub-drivers as
modules, and it leads to a lot of complication in the code.  I'm pretty
sure that forcing them to be selected at build-time will let us make
things much simpler.

If we have to have pci_stop_bus_devices() as an interim measure, I can
live with it, but it doesn't need to be exported, does it?

> +void pci_remove_host_bridge(struct pci_host_bridge *bridge)
> +{
> +	struct pci_bus *root = bridge->bus;
> +	struct pci_dev *dev, *tmp;
> +
> +	list_for_each_entry_safe_reverse(dev, tmp,
> +					 &root->devices, bus_list) {
> +		pci_remove_bus_device(dev);
> +	}
> +
> +	pci_remove_bus(root);
> +
> +	device_unregister(&bridge->dev);
> +}
> +EXPORT_SYMBOL(pci_remove_host_bridge);
> Index: Bjorn-next-0925-configchange/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next-0925-configchange.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next-0925-configchange/drivers/acpi/pci_root.c
> @@ -659,8 +659,10 @@ static int acpi_pci_root_remove(struct a
>  {
>  	struct acpi_pci_root *root = acpi_driver_data(device);
>  	struct acpi_pci_driver *driver;
> +	struct pci_host_bridge *bridge = to_pci_host_bridge(root->bus->bridge);
>  
>  	mutex_lock(&acpi_pci_root_lock);
> +	pci_stop_bus_devices(root->bus);
>  	list_for_each_entry(driver, &acpi_pci_drivers, node)
>  		if (driver->remove)
>  			driver->remove(root);
> @@ -668,6 +670,10 @@ static int acpi_pci_root_remove(struct a
>  	device_set_run_wake(root->bus->bridge, false);
>  	pci_acpi_remove_bus_pm_notifier(device);
>  
> +	acpi_pci_unbind_root(device);
> +
> +	pci_remove_host_bridge(bridge);
> +
>  	list_del(&root->node);
>  	mutex_unlock(&acpi_pci_root_lock);
>  	kfree(root);
> Index: Bjorn-next-0925-configchange/include/linux/pci.h
> ===================================================================
> --- Bjorn-next-0925-configchange.orig/include/linux/pci.h
> +++ Bjorn-next-0925-configchange/include/linux/pci.h
> @@ -734,6 +734,8 @@ extern struct pci_dev *pci_dev_get(struc
>  extern void pci_dev_put(struct pci_dev *dev);
>  extern void pci_remove_bus(struct pci_bus *b);
>  extern void pci_stop_and_remove_bus_device(struct pci_dev *dev);
> +extern void pci_stop_bus_devices(struct pci_bus *bus);
> +extern void pci_remove_host_bridge(struct pci_host_bridge *bridge);
>  void pci_setup_cardbus(struct pci_bus *bus);
>  extern void pci_sort_breadthfirst(void);
>  #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
> Index: Bjorn-next-0925-configchange/drivers/acpi/pci_bind.c
> ===================================================================
> --- Bjorn-next-0925-configchange.orig/drivers/acpi/pci_bind.c
> +++ Bjorn-next-0925-configchange/drivers/acpi/pci_bind.c
> @@ -118,3 +118,10 @@ int acpi_pci_bind_root(struct acpi_devic
>  
>  	return 0;
>  }
> +
> +void  acpi_pci_unbind_root(struct acpi_device *device)
> +{
> +	device->ops.bind = NULL;
> +	device->ops.unbind = NULL;
> +}
> +
> Index: Bjorn-next-0925-configchange/include/acpi/acpi_drivers.h
> ===================================================================
> --- Bjorn-next-0925-configchange.orig/include/acpi/acpi_drivers.h
> +++ Bjorn-next-0925-configchange/include/acpi/acpi_drivers.h
> @@ -101,6 +101,7 @@ struct pci_bus;
>  
>  struct pci_dev *acpi_get_pci_dev(acpi_handle);
>  int acpi_pci_bind_root(struct acpi_device *device);
> +void acpi_pci_unbind_root(struct acpi_device *device);
>  
>  /* Arch-defined function to add a bus to the system */
>  
> 

(no subject)

[Yinghai Lu]

Subject: [PATCH resend 0/8] PCI, ACPI, x86: pci root bus hotplug support resources assign and remove path

1. add support for assign resource for hot add path.
2. stop and remove root bus during acpi root remove.


could get from
        git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-root-bus-hotplug

Yinghai Lu (8):
  PCI: Separate out pci_assign_unassigned_bus_resources()
  PCI: Move pci_rescan_bus() back to probe.c
  PCI: Move out pci_enable_bridges out of assign_unsigned_bus_res
  PCI, ACPI: assign unassigned resource for hot add root bus
  PCI: Add pci_stop/remove_root_bus()
  PCI, ACPI: Make acpi_pci_root_remove stop/remove pci root bus
  PCI, ACPI: delete root bus prt during hot remove path
  PCI, ACPI: remove acpi_root_driver in reserse order

 drivers/acpi/pci_root.c |   21 ++++++++++++++++++++-
 drivers/pci/probe.c     |   22 ++++++++++++++++++++++
 drivers/pci/remove.c    |   36 ++++++++++++++++++++++++++++++++++++
 drivers/pci/setup-bus.c |   22 +---------------------
 include/linux/pci.h     |    3 +++
 5 files changed, 82 insertions(+), 22 deletions(-)

-- 
1.7.7

[PATCH 8/8] PCI, ACPI: remove acpi_root_driver in reserse order

[Yinghai Lu]

aka stop acpi root driver in this sequence: acpiphp, iommu, ioapic.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_root.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index a27cbb57..012f40d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -672,7 +672,7 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
 	pci_stop_root_bus(root->bus);
 
 	mutex_lock(&acpi_pci_root_lock);
-	list_for_each_entry(driver, &acpi_pci_drivers, node)
+	list_for_each_entry_reverse(driver, &acpi_pci_drivers, node)
 		if (driver->remove)
 			driver->remove(root);
 	mutex_unlock(&acpi_pci_root_lock);
-- 
1.7.7