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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list