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

login
register
mail settings
Submitter Taku Izumi
Date Sept. 18, 2012, 6:25 a.m.
Message ID <20120918152553.8c8390d8.izumi.taku@jp.fujitsu.com>
Download mbox | patch
Permalink /patch/184623/
State Superseded
Headers show

Comments

Taku Izumi - Sept. 18, 2012, 6:25 a.m.
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.

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_bind.c     |    7 +++++++
 drivers/acpi/pci_root.c     |    7 +++++++
 drivers/pci/remove.c        |   27 +++++++++++++++++++++++++++
 include/acpi/acpi_drivers.h |    1 +
 include/linux/pci.h         |    2 ++
 5 files changed, 44 insertions(+)


--
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 - Sept. 21, 2012, 8:09 p.m.
On Tue, Sep 18, 2012 at 12:25 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> 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.
>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/acpi/pci_bind.c     |    7 +++++++
>  drivers/acpi/pci_root.c     |    7 +++++++
>  drivers/pci/remove.c        |   27 +++++++++++++++++++++++++++
>  include/acpi/acpi_drivers.h |    1 +
>  include/linux/pci.h         |    2 ++
>  5 files changed, 44 insertions(+)
>
> Index: Bjorn-next-0903/drivers/pci/remove.c
> ===================================================================
> --- Bjorn-next-0903.orig/drivers/pci/remove.c
> +++ Bjorn-next-0903/drivers/pci/remove.c
> @@ -92,3 +92,30 @@ void pci_stop_and_remove_bus_device(stru
>         pci_destroy_dev(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) {
> +               if (dev->subordinate)
> +                       pci_stop_bus_devices(dev->subordinate);
> +               pci_stop_dev(dev);
> +       }
> +
> +}
> +EXPORT_SYMBOL(pci_stop_bus_devices);
> +
> +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_stop_and_remove_bus_device(dev);
> +
> +       pci_remove_bus(root);
> +
> +       device_unregister(&bridge->dev);
> +}
> +EXPORT_SYMBOL(pci_remove_host_bridge);
> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> @@ -652,8 +652,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);
> @@ -661,6 +663,11 @@ 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_irq_del_prt(root->bus);

acpi_pci_irq_del_prt() does not actually have a dependency on the
struct pci_bus, so I think its interface should be changed so it takes
a segment number and a bus number instead of the "struct pci_bus *".
The same applies to acpi_pci_irq_add_prt().

This basically boils down to reverting 859a3f86ca8 and d9efae3688a.  I
acked those changes at the time, but I think they were a mistake.  The
reason is that passing in the struct pci_bus * ties them into the host
bridge add/remove flow in a way that's not necessary.

If we get rid of the struct pci_bus * dependency, then we can easily
add the _PRT before doing PCI enumeration behind the bridge, and we
can remove the _PRT after removing the PCI devices.  I think this is
one small step toward getting rid of the add/start and stop/remove
split.

> +       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-0903/include/linux/pci.h
> ===================================================================
> --- Bjorn-next-0903.orig/include/linux/pci.h
> +++ Bjorn-next-0903/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-0903/drivers/acpi/pci_bind.c
> ===================================================================
> --- Bjorn-next-0903.orig/drivers/acpi/pci_bind.c
> +++ Bjorn-next-0903/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-0903/include/acpi/acpi_drivers.h
> ===================================================================
> --- Bjorn-next-0903.orig/include/acpi/acpi_drivers.h
> +++ Bjorn-next-0903/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 */
>
>
--
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 - Sept. 27, 2012, 4:48 p.m.
On Fri, Sep 21, 2012 at 2:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Sep 18, 2012 at 12:25 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> 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.
>>
>> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
>> ---
>>  drivers/acpi/pci_bind.c     |    7 +++++++
>>  drivers/acpi/pci_root.c     |    7 +++++++
>>  drivers/pci/remove.c        |   27 +++++++++++++++++++++++++++
>>  include/acpi/acpi_drivers.h |    1 +
>>  include/linux/pci.h         |    2 ++
>>  5 files changed, 44 insertions(+)
>>
>> Index: Bjorn-next-0903/drivers/pci/remove.c
>> ===================================================================
>> --- Bjorn-next-0903.orig/drivers/pci/remove.c
>> +++ Bjorn-next-0903/drivers/pci/remove.c
>> @@ -92,3 +92,30 @@ void pci_stop_and_remove_bus_device(stru
>>         pci_destroy_dev(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) {
>> +               if (dev->subordinate)
>> +                       pci_stop_bus_devices(dev->subordinate);
>> +               pci_stop_dev(dev);
>> +       }
>> +
>> +}
>> +EXPORT_SYMBOL(pci_stop_bus_devices);
>> +
>> +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_stop_and_remove_bus_device(dev);
>> +
>> +       pci_remove_bus(root);
>> +
>> +       device_unregister(&bridge->dev);
>> +}
>> +EXPORT_SYMBOL(pci_remove_host_bridge);
>> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
>> ===================================================================
>> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
>> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
>> @@ -652,8 +652,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);
>> @@ -661,6 +663,11 @@ 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_irq_del_prt(root->bus);
>
> acpi_pci_irq_del_prt() does not actually have a dependency on the
> struct pci_bus, so I think its interface should be changed so it takes
> a segment number and a bus number instead of the "struct pci_bus *".
> The same applies to acpi_pci_irq_add_prt().
>
> This basically boils down to reverting 859a3f86ca8 and d9efae3688a.  I
> acked those changes at the time, but I think they were a mistake.  The
> reason is that passing in the struct pci_bus * ties them into the host
> bridge add/remove flow in a way that's not necessary.
>
> If we get rid of the struct pci_bus * dependency, then we can easily
> add the _PRT before doing PCI enumeration behind the bridge, and we
> can remove the _PRT after removing the PCI devices.  I think this is
> one small step toward getting rid of the add/start and stop/remove
> split.

I'm going to work on doing this if nobody else is interested.

>> +       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-0903/include/linux/pci.h
>> ===================================================================
>> --- Bjorn-next-0903.orig/include/linux/pci.h
>> +++ Bjorn-next-0903/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-0903/drivers/acpi/pci_bind.c
>> ===================================================================
>> --- Bjorn-next-0903.orig/drivers/acpi/pci_bind.c
>> +++ Bjorn-next-0903/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-0903/include/acpi/acpi_drivers.h
>> ===================================================================
>> --- Bjorn-next-0903.orig/include/acpi/acpi_drivers.h
>> +++ Bjorn-next-0903/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 */
>>
>>
--
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 - Sept. 27, 2012, 5:23 p.m.
On Thu, Sep 27, 2012 at 9:48 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Sep 21, 2012 at 2:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> If we get rid of the struct pci_bus * dependency, then we can easily
>> add the _PRT before doing PCI enumeration behind the bridge, and we
>> can remove the _PRT after removing the PCI devices.  I think this is
>> one small step toward getting rid of the add/start and stop/remove
>> split.
>
> I'm going to work on doing this if nobody else is interested.

I'm not sure if that is needed.

-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
Yinghai Lu - Sept. 27, 2012, 5:59 p.m.
On Thu, Sep 27, 2012 at 10:23 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Sep 27, 2012 at 9:48 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Fri, Sep 21, 2012 at 2:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> If we get rid of the struct pci_bus * dependency, then we can easily
>>> add the _PRT before doing PCI enumeration behind the bridge, and we
>>> can remove the _PRT after removing the PCI devices.  I think this is
>>> one small step toward getting rid of the add/start and stop/remove
>>> split.
>>
>> I'm going to work on doing this if nobody else is interested.
>
> I'm not sure if that is needed.

After patches that is using pci_host_bridge_bus_type and pci_bus_type
notification, we can remove
acpi_pci_root_start.

please check patches in my for-pci-next branch: it include jiang's 7
patches about notifier, and my kill pci_root_buses and etc.

git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
for-pci-next

-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
Bjorn Helgaas - Sept. 27, 2012, 6:44 p.m.
On Thu, Sep 27, 2012 at 11:23 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Sep 27, 2012 at 9:48 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Fri, Sep 21, 2012 at 2:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> If we get rid of the struct pci_bus * dependency, then we can easily
>>> add the _PRT before doing PCI enumeration behind the bridge, and we
>>> can remove the _PRT after removing the PCI devices.  I think this is
>>> one small step toward getting rid of the add/start and stop/remove
>>> split.
>>
>> I'm going to work on doing this if nobody else is interested.
>
> I'm not sure if that is needed.

That's a useless response.  Do you want to elaborate on *why* you
think this is a bad idea?

I explained the reasons why I think it's a good idea above, but just
to expand on it, we currently have to create the struct pci_bus before
we can add _PRT information.   But adding the _PRT info doesn't
actually depend on the struct pci_bus; it only requires the segment
number and the bus number.  We have that information before we scan
the bus .

I think it's useful to disentangle _PRT interface from the specifics
of PCI (in this case, the struct pci_bus).  We're currently using the
struct pci_bus here just as a convenient way to pass around the
segment/bus number, but I don't think it's the appropriate abstraction
for that.

Do you see a technical problem with it?  Even if it's not *necessary*
in order to make host bridge hotplug work, I think it's worth doing to
make the code more understandable.

Do you see a problem with adding the _PRT info before scanning the bus
or with removing it after deleting the bus?  I'd like the bus scan
code to be able to scan/add/bind drivers all at once in the PCI core.
Today I think we have scan/add _PRT/device_add, where we have to do
this _PRT stuff in the middle, so we have to use two PCI interfaces
rather than one.

It's great if you see a way to remove acpi_pci_root_start() -- that
will be tremendous.  I think the current PCI stop/remove split is a
similar issue, and I hope you can dream up a way to consolidate those,
too.

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 - Sept. 27, 2012, 8:17 p.m.
On Thu, Sep 27, 2012 at 11:44 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> I'm not sure if that is needed.
>
> That's a useless response.  Do you want to elaborate on *why* you
> think this is a bad idea?

I did not say that is bad idea.

it is just not needed to waste your time.

>
> I explained the reasons why I think it's a good idea above, but just
> to expand on it, we currently have to create the struct pci_bus before
> we can add _PRT information.   But adding the _PRT info doesn't
> actually depend on the struct pci_bus; it only requires the segment
> number and the bus number.  We have that information before we scan
> the bus .
>
> I think it's useful to disentangle _PRT interface from the specifics
> of PCI (in this case, the struct pci_bus).  We're currently using the
> struct pci_bus here just as a convenient way to pass around the
> segment/bus number, but I don't think it's the appropriate abstraction
> for that.
>
> Do you see a technical problem with it?  Even if it's not *necessary*
> in order to make host bridge hotplug work, I think it's worth doing to
> make the code more understandable.
>
> Do you see a problem with adding the _PRT info before scanning the bus
> or with removing it after deleting the bus?

If the bus is not there, do not need that prt.

So if you find the prt and add it to the list before, later if
scanning fail etc failing path
you will need to clean that prt in the list.


> I'd like the bus scan
> code to be able to scan/add/bind drivers all at once in the PCI core.
> Today I think we have scan/add _PRT/device_add, where we have to do
> this _PRT stuff in the middle, so we have to use two PCI interfaces
> rather than one.

looks like we handle _PRT in pci_host_bridge add/remove notification,
just like Jiang's put that for pci device bus notifier.

>
> It's great if you see a way to remove acpi_pci_root_start() -- that
> will be tremendous.

that will need to more test.

> I think the current PCI stop/remove split is a
> similar issue, and I hope you can dream up a way to consolidate those,
> too.

that is SRIOV handling resign.

I would suggest following way:
use pf as parent for one new domain. and put all VFs on that domain.
and the domain could have 256 buses for those VFs.
and we have 16 bits domain number that could be used.

current got too many patches in my quilt... hate to keep rebasing again.

after pci root bus hotplug and for_each_res topic, will work on that SRIOV one.

-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
Taku Izumi - Sept. 28, 2012, 12:15 a.m.
On Thu, 27 Sep 2012 10:48:09 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Fri, Sep 21, 2012 at 2:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Tue, Sep 18, 2012 at 12:25 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> 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.
> >>
> >> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> >> ---
> >>  drivers/acpi/pci_bind.c     |    7 +++++++
> >>  drivers/acpi/pci_root.c     |    7 +++++++
> >>  drivers/pci/remove.c        |   27 +++++++++++++++++++++++++++
> >>  include/acpi/acpi_drivers.h |    1 +
> >>  include/linux/pci.h         |    2 ++
> >>  5 files changed, 44 insertions(+)
> >>
> >> Index: Bjorn-next-0903/drivers/pci/remove.c
> >> ===================================================================
> >> --- Bjorn-next-0903.orig/drivers/pci/remove.c
> >> +++ Bjorn-next-0903/drivers/pci/remove.c
> >> @@ -92,3 +92,30 @@ void pci_stop_and_remove_bus_device(stru
> >>         pci_destroy_dev(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) {
> >> +               if (dev->subordinate)
> >> +                       pci_stop_bus_devices(dev->subordinate);
> >> +               pci_stop_dev(dev);
> >> +       }
> >> +
> >> +}
> >> +EXPORT_SYMBOL(pci_stop_bus_devices);
> >> +
> >> +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_stop_and_remove_bus_device(dev);
> >> +
> >> +       pci_remove_bus(root);
> >> +
> >> +       device_unregister(&bridge->dev);
> >> +}
> >> +EXPORT_SYMBOL(pci_remove_host_bridge);
> >> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> >> ===================================================================
> >> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> >> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> >> @@ -652,8 +652,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);
> >> @@ -661,6 +663,11 @@ 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_irq_del_prt(root->bus);
> >
> > acpi_pci_irq_del_prt() does not actually have a dependency on the
> > struct pci_bus, so I think its interface should be changed so it takes
> > a segment number and a bus number instead of the "struct pci_bus *".
> > The same applies to acpi_pci_irq_add_prt().
> >
> > This basically boils down to reverting 859a3f86ca8 and d9efae3688a.  I
> > acked those changes at the time, but I think they were a mistake.  The
> > reason is that passing in the struct pci_bus * ties them into the host
> > bridge add/remove flow in a way that's not necessary.
> >
> > If we get rid of the struct pci_bus * dependency, then we can easily
> > add the _PRT before doing PCI enumeration behind the bridge, and we
> > can remove the _PRT after removing the PCI devices.  I think this is
> > one small step toward getting rid of the add/start and stop/remove
> > split.
> 
> I'm going to work on doing this if nobody else is interested.

  I'll do that. Maybee that is separeted from this patchset.

  Best regards,
  Taku Izumi

> 
> >> +       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-0903/include/linux/pci.h
> >> ===================================================================
> >> --- Bjorn-next-0903.orig/include/linux/pci.h
> >> +++ Bjorn-next-0903/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-0903/drivers/acpi/pci_bind.c
> >> ===================================================================
> >> --- Bjorn-next-0903.orig/drivers/acpi/pci_bind.c
> >> +++ Bjorn-next-0903/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-0903/include/acpi/acpi_drivers.h
> >> ===================================================================
> >> --- Bjorn-next-0903.orig/include/acpi/acpi_drivers.h
> >> +++ Bjorn-next-0903/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 */
> >>
> >>
>
Bjorn Helgaas - Sept. 28, 2012, 12:23 a.m.
On Thu, Sep 27, 2012 at 6:15 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> On Thu, 27 Sep 2012 10:48:09 -0600
> Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> On Fri, Sep 21, 2012 at 2:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > On Tue, Sep 18, 2012 at 12:25 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> 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.
>> >>
>> >> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
>> >> ---
>> >>  drivers/acpi/pci_bind.c     |    7 +++++++
>> >>  drivers/acpi/pci_root.c     |    7 +++++++
>> >>  drivers/pci/remove.c        |   27 +++++++++++++++++++++++++++
>> >>  include/acpi/acpi_drivers.h |    1 +
>> >>  include/linux/pci.h         |    2 ++
>> >>  5 files changed, 44 insertions(+)
>> >>
>> >> Index: Bjorn-next-0903/drivers/pci/remove.c
>> >> ===================================================================
>> >> --- Bjorn-next-0903.orig/drivers/pci/remove.c
>> >> +++ Bjorn-next-0903/drivers/pci/remove.c
>> >> @@ -92,3 +92,30 @@ void pci_stop_and_remove_bus_device(stru
>> >>         pci_destroy_dev(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) {
>> >> +               if (dev->subordinate)
>> >> +                       pci_stop_bus_devices(dev->subordinate);
>> >> +               pci_stop_dev(dev);
>> >> +       }
>> >> +
>> >> +}
>> >> +EXPORT_SYMBOL(pci_stop_bus_devices);
>> >> +
>> >> +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_stop_and_remove_bus_device(dev);
>> >> +
>> >> +       pci_remove_bus(root);
>> >> +
>> >> +       device_unregister(&bridge->dev);
>> >> +}
>> >> +EXPORT_SYMBOL(pci_remove_host_bridge);
>> >> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
>> >> ===================================================================
>> >> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
>> >> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
>> >> @@ -652,8 +652,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);
>> >> @@ -661,6 +663,11 @@ 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_irq_del_prt(root->bus);
>> >
>> > acpi_pci_irq_del_prt() does not actually have a dependency on the
>> > struct pci_bus, so I think its interface should be changed so it takes
>> > a segment number and a bus number instead of the "struct pci_bus *".
>> > The same applies to acpi_pci_irq_add_prt().
>> >
>> > This basically boils down to reverting 859a3f86ca8 and d9efae3688a.  I
>> > acked those changes at the time, but I think they were a mistake.  The
>> > reason is that passing in the struct pci_bus * ties them into the host
>> > bridge add/remove flow in a way that's not necessary.
>> >
>> > If we get rid of the struct pci_bus * dependency, then we can easily
>> > add the _PRT before doing PCI enumeration behind the bridge, and we
>> > can remove the _PRT after removing the PCI devices.  I think this is
>> > one small step toward getting rid of the add/start and stop/remove
>> > split.
>>
>> I'm going to work on doing this if nobody else is interested.
>
>   I'll do that. Maybee that is separeted from this patchset.

Great, thanks!  A separate patchset is definitely fine.

>> >> +       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-0903/include/linux/pci.h
>> >> ===================================================================
>> >> --- Bjorn-next-0903.orig/include/linux/pci.h
>> >> +++ Bjorn-next-0903/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-0903/drivers/acpi/pci_bind.c
>> >> ===================================================================
>> >> --- Bjorn-next-0903.orig/drivers/acpi/pci_bind.c
>> >> +++ Bjorn-next-0903/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-0903/include/acpi/acpi_drivers.h
>> >> ===================================================================
>> >> --- Bjorn-next-0903.orig/include/acpi/acpi_drivers.h
>> >> +++ Bjorn-next-0903/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 */
>> >>
>> >>
>>
>
>
> --
> Taku Izumi <izumi.taku@jp.fujitsu.com>
>
--
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 - Sept. 28, 2012, 2:16 p.m.
On Thu, Sep 27, 2012 at 6:23 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Sep 27, 2012 at 6:15 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> On Thu, 27 Sep 2012 10:48:09 -0600
>> Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> On Fri, Sep 21, 2012 at 2:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> > On Tue, Sep 18, 2012 at 12:25 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:

>>> >> @@ -661,6 +663,11 @@ 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_irq_del_prt(root->bus);
>>> >
>>> > acpi_pci_irq_del_prt() does not actually have a dependency on the
>>> > struct pci_bus, so I think its interface should be changed so it takes
>>> > a segment number and a bus number instead of the "struct pci_bus *".
>>> > The same applies to acpi_pci_irq_add_prt().
>>> >
>>> > This basically boils down to reverting 859a3f86ca8 and d9efae3688a.  I
>>> > acked those changes at the time, but I think they were a mistake.  The
>>> > reason is that passing in the struct pci_bus * ties them into the host
>>> > bridge add/remove flow in a way that's not necessary.
>>> >
>>> > If we get rid of the struct pci_bus * dependency, then we can easily
>>> > add the _PRT before doing PCI enumeration behind the bridge, and we
>>> > can remove the _PRT after removing the PCI devices.  I think this is
>>> > one small step toward getting rid of the add/start and stop/remove
>>> > split.
>>>
>>> I'm going to work on doing this if nobody else is interested.
>>
>>   I'll do that. Maybee that is separeted from this patchset.
>
> Great, thanks!  A separate patchset is definitely fine.

Whether this is a waste of time or not, I don't know.  But I did a
little bit of work on this before you volunteered, Taku.  Sorry, I
should have pointed you at the work-in-progress patches; maybe they
could have saved you a bit of time.  Anyway, here they are:

http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=shortlog;h=refs/heads/pci/bjorn-misc
--
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 - Sept. 28, 2012, 4:07 p.m.
On Thu, Sep 27, 2012 at 2:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Sep 27, 2012 at 11:44 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:

>> I explained the reasons why I think it's a good idea above, but just
>> to expand on it, we currently have to create the struct pci_bus before
>> we can add _PRT information.   But adding the _PRT info doesn't
>> actually depend on the struct pci_bus; it only requires the segment
>> number and the bus number.  We have that information before we scan
>> the bus .
>>
>> I think it's useful to disentangle _PRT interface from the specifics
>> of PCI (in this case, the struct pci_bus).  We're currently using the
>> struct pci_bus here just as a convenient way to pass around the
>> segment/bus number, but I don't think it's the appropriate abstraction
>> for that.
>>
>> Do you see a technical problem with it?  Even if it's not *necessary*
>> in order to make host bridge hotplug work, I think it's worth doing to
>> make the code more understandable.
>>
>> Do you see a problem with adding the _PRT info before scanning the bus
>> or with removing it after deleting the bus?
>
> If the bus is not there, do not need that prt.
>
> So if you find the prt and add it to the list before, later if
> scanning fail etc failing path
> you will need to clean that prt in the list.

That's true.  It will add a little bit to the failure paths in
acpi_pci_root_add(), but I don't think that's much of an issue.

>> I'd like the bus scan
>> code to be able to scan/add/bind drivers all at once in the PCI core.
>> Today I think we have scan/add _PRT/device_add, where we have to do
>> this _PRT stuff in the middle, so we have to use two PCI interfaces
>> rather than one.

This is the more important bit.  My longer-term goal is to separate
out the ACPI parts from the PCI parts.  Then we can use more generic
code for the PCI part, which will help unify the architectures.

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.

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 - Sept. 28, 2012, 4:19 p.m.
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.
--
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 - Sept. 28, 2012, 7:44 p.m.
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.
--
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

Patch

Index: Bjorn-next-0903/drivers/pci/remove.c
===================================================================
--- Bjorn-next-0903.orig/drivers/pci/remove.c
+++ Bjorn-next-0903/drivers/pci/remove.c
@@ -92,3 +92,30 @@  void pci_stop_and_remove_bus_device(stru
 	pci_destroy_dev(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) {
+		if (dev->subordinate)
+			pci_stop_bus_devices(dev->subordinate);
+		pci_stop_dev(dev);
+	}
+
+}
+EXPORT_SYMBOL(pci_stop_bus_devices);
+
+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_stop_and_remove_bus_device(dev);
+
+	pci_remove_bus(root);
+
+	device_unregister(&bridge->dev);
+}
+EXPORT_SYMBOL(pci_remove_host_bridge);
Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -652,8 +652,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);
@@ -661,6 +663,11 @@  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_irq_del_prt(root->bus);
+	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-0903/include/linux/pci.h
===================================================================
--- Bjorn-next-0903.orig/include/linux/pci.h
+++ Bjorn-next-0903/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-0903/drivers/acpi/pci_bind.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_bind.c
+++ Bjorn-next-0903/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-0903/include/acpi/acpi_drivers.h
===================================================================
--- Bjorn-next-0903.orig/include/acpi/acpi_drivers.h
+++ Bjorn-next-0903/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 */