On Fri, 13 Feb 2009 20:23:03 -0800 ebiederm@xxxxxxxxxxxx (Eric W. Biederman) wrote: > > pcie_port_device_remove currently calls the remove method > of port drivers twice. Ouch! > > We don't get the correct interrupt mode unless there is a port device > present. > > We are calling device_for_each_child multiple times for no apparent > reason. > > So make it simple. Use pcie_port_driver_ext so we always properly > know the interrupt mode the we placed the pci device in. Place > put_device and device_unregister into remove_iter, and throw out the > rest. Only call device_for_each_child once. > > The code is simpler and actually works! > What's happening with this? > > --- > drivers/pci/pcie/portdrv_core.c | 31 +++++++------------------------ > 1 files changed, 7 insertions(+), 24 deletions(-) > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index 8b3f8c1..c642828 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -326,16 +326,9 @@ int pcie_port_device_resume(struct pci_dev *dev) > > static int remove_iter(struct device *dev, void *data) > { > - struct pcie_port_service_driver *service_driver; > - > if (dev->bus == &pcie_port_bus_type) { > - if (dev->driver) { > - service_driver = to_service_driver(dev->driver); > - if (service_driver->remove) > - service_driver->remove(to_pcie_device(dev)); > - } > - *(unsigned long*)data = (unsigned long)dev; > - return 1; > + put_device(dev); > + device_unregister(dev); > } > return 0; > } > @@ -349,24 +342,14 @@ static int remove_iter(struct device *dev, void *data) > */ > void pcie_port_device_remove(struct pci_dev *dev) > { > - struct device *device; > - unsigned long device_addr; > - int interrupt_mode = PCIE_PORT_INTx_MODE; > - int status; > + struct pcie_port_device_ext *p_ext = pci_get_drvdata(dev); > + > + device_for_each_child(&dev->dev, NULL, remove_iter); > > - do { > - status = device_for_each_child(&dev->dev, &device_addr, remove_iter); > - if (status) { > - device = (struct device*)device_addr; > - interrupt_mode = (to_pcie_device(device))->interrupt_mode; > - put_device(device); > - device_unregister(device); > - } > - } while (status); > /* Switch to INTx by default if MSI enabled */ > - if (interrupt_mode == PCIE_PORT_MSIX_MODE) > + if (p_ext->interrupt_mode == PCIE_PORT_MSIX_MODE) > pci_disable_msix(dev); > - else if (interrupt_mode == PCIE_PORT_MSI_MODE) > + else if (p_ext->interrupt_mode == PCIE_PORT_MSI_MODE) > pci_disable_msi(dev); > } > There are large-scale and conflicting changes to this file in linux-next. If we want to jam this fix into 2.6.29 (and it looks like something we want) then this will trash the linux-next changes. It will cause me grief, and will cause Stephen grief unless the pci tree is suitably changed, which will cause Jesse grief. Either way: grief. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html