| Message ID | 20120918152553.8c8390d8.izumi.taku@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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
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
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
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
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
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 */ > >> > >> >
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
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
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
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
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
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 */
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