Messages in this thread | | | Date | Tue, 18 Oct 2011 10:02:18 -0700 | From | Yinghai Lu <> | Subject | Re: [PATCH 1/8] pci: Make sriov work with hotplug removal |
| |
On 10/18/2011 09:49 AM, Bjorn Helgaas wrote: > On Mon, Oct 17, 2011 at 4:24 PM, Yinghai Lu<yinghai.lu@oracle.com> wrote: >> On 10/17/2011 03:12 PM, Bjorn Helgaas wrote: >>> >>> Maybe this is the best we can do, but it still doesn't seem ideal, and >>> it's certainly not obvious when reading the code. It doesn't seem >>> right for the driver ->remove() method to be calling >>> pci_destroy_dev(). Won't the core data structures be corrupted if a >>> defective driver doesn't call pci_disable_sriov()? Seems like we >>> could end up with a device that's been physically removed, but still >>> has pci_dev structs hanging around. >> >> i did add some print out in >> pci_stop_bus_device >> when stop PF, that function is called for those VFs. >> >> also driver have to call pci_disable_sriov() and that is current design. > > Yep. But I don't have to like the current design :) It doesn't seem > as robust as it could be. > > It took me a long time to puzzle out what was happening here. Here's > some possible changelog text that would have saved me a lot of time: > > The PCI hot-remove path calls pci_stop_bus_devices() via > pci_remove_bus_device(). > > pci_stop_bus_devices() traverses the bus->devices list (point A below), > stopping each device in turn, which calls the driver remove() method. When > the device is an SR-IOV PF, the driver calls pci_disable_sriov(), which > also uses pci_remove_bus_device() to remove the VF devices from the > bus->devices list (point B). > > pci_remove_bus_device > pci_stop_bus_device > pci_stop_bus_devices(subordinate) > list_for_each(bus->devices)<-- A > pci_stop_bus_device(PF) > ... > driver->remove > pci_disable_sriov > ... > pci_remove_bus_device(VF) > <remove from bus_list> <-- B > > At B, we're changing the same list we're iterating through at A, so when > the driver remove() method returns, the pci_stop_bus_devices() iterator has > a pointer to a list entry that has already been freed. > > This patch avoids the problem by building a separate list of all PFs on > the bus and traversing that at A instead of the bus->devices list.
yes.
| |