At 2011-7-1 18:24, Guannan Ren write:

the "virsh nodedev-reattch" command could return successfully, but the pci device is still bound to pci-stub driver. The reason is noddev-reattach trys to use the variables set by nodedev-dettach commands. Becuase these variables is not persistent, this is not we expected. the patch try to fix it.

I do not agree with this patch. You should read this mail: https://www.redhat.com/archives/libvir-list/2011-April/msg00315.html This patch will cause regression about hotpluging host pci device. I think the problem is that: the init value of these variables is wrong. We can fix this bug by modifing pciGetDevice() or its caller.

--- src/util/pci.c | 74 +++++++++++++++++++------------------------------------ 1 files changed, 26 insertions(+), 48 deletions(-)

diff --git a/src/util/pci.c b/src/util/pci.c index 46a3a83..d345c3e 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -67,9 +67,6 @@ struct _pciDevice { unsigned managed : 1;

/* used by reattach function */ - unsigned unbind_from_stub : 1; - unsigned remove_slot : 1; - unsigned reprobe : 1; };

struct _pciDeviceList { @@ -882,14 +879,23 @@ pciUnbindDeviceFromStub(pciDevice *dev, const char *driver) if (pciDriverDir(&drvdir, driver)< 0) goto cleanup;

- if (!dev->unbind_from_stub) - goto remove_slot; - - /* If the device is bound to stub, unbind it. - */ if (pciDeviceFile(&path, dev->name, "driver")< 0) goto cleanup;

+ /* If the device is bound to a driver instead of pci-stub, do nothing. + */ + if (virFileExists(path)&& ! virFileLinkPointsTo(path, drvdir)){ + result = 0; + goto cleanup; + } + + /* If the device is not bound to any driver, reprobe it. + */ + if (!virFileExists(path)) + goto reprobe; + + /* If the device is bound to stub, unbind it. + */ if (virFileExists(drvdir)&& virFileLinkPointsTo(path, drvdir)) { if (pciDriverFile(&path, driver, "unbind")< 0) { goto cleanup; @@ -902,11 +908,6 @@ pciUnbindDeviceFromStub(pciDevice *dev, const char *driver) goto cleanup; } } - dev->unbind_from_stub = 0; - -remove_slot: - if (!dev->remove_slot) - goto reprobe;

/* Xen's pciback.ko wants you to use remove_slot on the specific device */ if (pciDriverFile(&path, driver, "remove_slot")< 0) { @@ -919,14 +920,8 @@ remove_slot: dev->name, driver); goto cleanup; } - dev->remove_slot = 0;

reprobe: - if (!dev->reprobe) { - result = 0; - goto cleanup; - } - /* Trigger a re-probe of the device is not in the stub's dynamic * ID table. If the stub is available, but 'remove_id' isn't * available, then re-probing would just cause the device to be @@ -935,6 +930,12 @@ reprobe: if (pciDriverFile(&path, driver, "remove_id")< 0) { goto cleanup; } + + /* If the device is still in stub's dynamic ID table,remove it, + * otherwise, ignore the error. + */ + if (virFileExists(path)&& virFileWriteStr(path, dev->name, 0)< 0){ + }

if (!virFileExists(drvdir) || virFileExists(path)) { if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0)< 0) { @@ -948,11 +949,6 @@ reprobe: result = 0;

cleanup: - /* do not do it again */ - dev->unbind_from_stub = 0; - dev->remove_slot = 0; - dev->reprobe = 0; - VIR_FREE(drvdir); VIR_FREE(path);

@@ -966,7 +962,6 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) int result = -1; char *drvdir = NULL; char *path = NULL; - int reprobe = 0;

/* check whether the device is already bound to a driver */ if (pciDriverDir(&drvdir, driver)< 0 || @@ -980,7 +975,6 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) result = 0; goto cleanup; } - reprobe = 1; }

/* Add the PCI device ID to the stub's dynamic ID table; @@ -1011,8 +1005,7 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) }

if (virFileLinkPointsTo(path, drvdir)) { - dev->unbind_from_stub = 1; - dev->remove_slot = 1; + result = 0; goto remove_id; }

@@ -1022,7 +1015,7 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) * your root filesystem. */ if (pciDeviceFile(&path, dev->name, "driver/unbind")< 0) { - goto cleanup; + goto remove_id; }

if (virFileExists(path)) { @@ -1030,9 +1023,8 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) virReportSystemError(errno, _("Failed to unbind PCI device '%s'"), dev->name); - goto cleanup; + goto remove_id; } - dev->reprobe = reprobe; }

/* If the device isn't already bound to pci-stub, try binding it now. @@ -1054,7 +1046,6 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) dev->name, driver); goto remove_id; } - dev->remove_slot = 1;

if (pciDriverFile(&path, driver, "bind")< 0) { goto remove_id; @@ -1066,20 +1057,15 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) dev->name, driver); goto remove_id; } - dev->unbind_from_stub = 1; }

+ result = 0; remove_id: /* If 'remove_id' exists, remove the device id from pci-stub's dynamic * ID table so that 'drivers_probe' works below. */ if (pciDriverFile(&path, driver, "remove_id")< 0) { - /* We do not remove PCI ID from pci-stub, and we cannot reprobe it */ - if (dev->reprobe) { - VIR_WARN("Could not remove PCI ID '%s' from %s, and the device " - "cannot be probed again.", dev->id, driver); - } - dev->reprobe = 0; + result = -1; goto cleanup; }

@@ -1087,18 +1073,10 @@ remove_id: virReportSystemError(errno, _("Failed to remove PCI ID '%s' from %s"), dev->id, driver); - - /* remove PCI ID from pci-stub failed, and we cannot reprobe it */ - if (dev->reprobe) { - VIR_WARN("Failed to remove PCI ID '%s' from %s, and the device " - "cannot be probed again.", dev->id, driver); - } - dev->reprobe = 0; + result = -1; goto cleanup; }

- result = 0; - cleanup: VIR_FREE(drvdir); VIR_FREE(path);