Messages in this thread | | | Date | Thu, 19 Mar 2009 21:53:14 -0600 | From | Alex Chiang <> | Subject | Re: [PATCH v4 08/12] PCI: Introduce /sys/bus/pci/devices/.../remove |
| |
* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>: > Alex Chiang wrote: >> * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>: >>> Alex Chiang wrote: >>>> + >>>> +static void remove_callback(struct device *dev) >>>> +{ >>>> + int bridge = 0; >>>> + struct pci_dev *pdev = to_pci_dev(dev); >>>> + >>>> + mutex_lock(&pci_remove_rescan_mutex); >>>> + >>>> + if (pdev->subordinate) >>>> + bridge = 1; >>>> + >>>> + pci_remove_bus_device(pdev); >>>> + if (bridge && list_empty(&pdev->bus->devices)) >>>> + pci_remove_bus(pdev->bus); >>> I cannot understand the above two lines. Could you explain >>> what it intend? >> >> If the user says: >> >> echo 1 > /sys/bus/pci/devices/.../remove >> >> And that device is a bridge, then we need to specifically call >> pci_remove_bus as well, to actually remove the bus itself. >> Without it, pci_bus_remove_device() will remove all of its >> children (and subordinate buses) in a depth-first manner, but we >> will never actually remove the bus that the user specified. >> > > Do you mean user removes bridge device to remove its *primary* > bus? It is very strange. I think the bus should be removed > when its parent bridge is removed.
You are correct.
>> In other words, without it, we will still see the bus in: >> >> /sys/class/pci_bus/... >> > > What is the problem? > >> We only want to remove the bus if it has no children left. I >> think the check for list_empty(&pdev->bus->devices) might be >> overkill... I can try taking that bit out and testing again. >> > > I think we don't need the two lines. But if you do that, you > need list_empty(&pdev->bus->devices), doesn't it? On the other > hand, we must not check 'bridge' in the if statement. Or bus > will never be removed when non-bridge device is removed last > on the bus. > > Again, I think we don't need the two lines. But am I > misunderstanding something?
No, you are correct.
I think what was happening was that I inserted that code before I discovered the double-free in the PCIe port driver, and that extra call to pci_remove_bus() helped mask the double-free.
I re-tested again tonight with the port driver fix, and also removing the two lines you mention, and it is behaving correctly.
As always, thanks for your review.
/ac
| |