On 07/28/2016 06:01 PM, Laine Stump wrote:
On 07/11/2016 02:23 PM, Jim Fehlig wrote:
Early in virPCIDeviceBindToStub, there is a check to see if the stub is already bound to the device, returning success with no further actions if that is the case.
The same condition is unnecessarily checked later in the function.
Looking at the original code, the condition (whether the device is bound to the stub driver) is checked after writing the PCI ID of the device to the stub driver's "new_id" node. If you look here:
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-pci
It says that when you write a PCI ID to a driver's "new_id", if there are any devices with the given PCI ID currently not bound to any driver, they will be immediately bound to the given driver. So I don't think the check is unnecessary - if the device wasn't bound to any driver to begin with (e.g. if the host driver for the device was blacklisted), it will be bound to the stub driver *immediately* after writing the PCI ID to new_id (ie before getting to the 2nd check).
So the condition isn't unnecessarily checked. It really can happen that we weren't bound to the stub in the first case, but were by the time we get to the 2nd.
On the other hand, this points out how utterly atrocious the new_id interface is - when I tested this by blacklisting the igbvf driver (so all the VFs of my 82576 card would have no driver bound to them), then started a domain that used a single VF, *all* of the VFs were suddenly bound to vfio-pci !!!
I also made a bunch of comments below before I noticed this part that I've put at the top, but in the end I think that especially since this whole bind/unbind/new_id interface is a dying thing, it would be better to leave it untouched (to avoid unexpected regressions) unless it's going to make a significant difference in how the driver_override stuff is added in.
Yes, I agree after reading your above observations. I've dropped this patch in V2 and with the exception of renaming functions, left the exiting code untouched. I think that approach will also make it easier to drop the new_id code once support for older kernels is no longer desired. Regards, Jim