Re: [libvirt] [PATCH 1/9] pci: Make reattach work for unbound devices

On Fri, Jan 17, 2014 at 15:29:27 +0100, Michal Privoznik wrote: > On 17.01.2014 11:39, Jiri Denemark wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1046919 >> >> When a PCI device is not bound to any driver, reattach should just >> trigger driver probe rather than failing with >> >> Invalid device 0000:00:19.0 driver file >> /sys/bus/pci/devices/0000:00:19.0/driver is not a symlink >> >> While virPCIDeviceGetDriverPathAndName was documented to return success >> and NULL driver and path when a device is not attached to any driver but >> didn't do so. Thus callers could not distinguish unbound devices from >> failures. >> >> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; >> --- >> src/util/virpci.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/src/util/virpci.c b/src/util/virpci.c >> index 51dbee6..8577fd4 100644 >> --- a/src/util/virpci.c >> +++ b/src/util/virpci.c >> @@ -236,6 +236,11 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) >> if (virPCIFile(&drvlink, dev->name, "driver") < 0) >> goto cleanup; >> >> + if (!virFileExists(drvlink)) { >> + ret = 0; >> + goto cleanup; >> + } >> + >> if (virFileIsLink(drvlink) != 1) { >> virReportError(VIR_ERR_INTERNAL_ERROR, >> _("Invalid device %s driver file %s is not a symlink"), >> @@ -1023,6 +1028,11 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) >> if (virPCIDeviceGetDriverPathAndName(dev, &drvdir, &driver) < 0) >> goto cleanup; >> >> + if (!driver) { >> + /* The device is not bound to any driver and we are almost done. */ >> + goto reprobe; >> + } >> + >> if (!dev->unbind_from_stub) >> goto remove_slot; >> >> @@ -1079,11 +1089,10 @@ reprobe: >> * available, then re-probing would just cause the device to be >> * re-bound to the stub. >> */ >> - if (virPCIDriverFile(&path, driver, "remove_id") < 0) { >> + if (driver && virPCIDriverFile(&path, driver, "remove_id") < 0) >> goto cleanup; >> - } >> > > Up to here I understand the patch. > >> - if (!virFileExists(drvdir) || virFileExists(path)) { >> + if (!driver || !virFileExists(drvdir) || virFileExists(path)) { >> if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) { >> virReportSystemError(errno, >> _("Failed to trigger a re-probe for PCI device '%s'"), >> > > But this bit doesn't make much sense to me. If a device is bound to a > stub driver, say pci-stub, then the > @path="/sys/bus/pci/drivers/pci-stub/remove_id"; I don't see a reason > how/why this should have any affect on decision whether to write into > "/sys/bus/pci/drivers_probe" or not. > > Even though it's pre-existing, now that you're touching the line it > makes sense to do it right. Yeah, this whole code is quite strange... if there is remove_id file in the driver's directory, then the device's ID has already been written there as part of attaching a device to a stub driver at the time the device was detached from host system (see virPCIDeviceBindToStub). So this code is just checking if detach could have done that and if so, triggers driver probe. Note that when we get this, the device is already unbound from stub. No idea why it is done this way but I'd rather keep it as is at least in this series. Jirka