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

[Michal Privoznik]

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 at redhat.com>
>>> ---
>>>  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