On 17.01.2014 16:34, Jiri Denemark wrote: > 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@xxxxxxxxxx> >>> --- >>> 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 > Yeah, fixing it deserves own patch. ACK to this as-is then, but if you have some spare time, please look at it while we are at this. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list