Re: [PATCH V13 3/5] libxl: add pvusb API

[George Dunlap <george.dunlap@citrix.com>]

On 03/02/16 07:34, Chun Yan Liu wrote:
> 
> 
>>>> On 2/3/2016 at 02:11 AM, in message
> <22192.61775.427189.268007@mariner.uk.xensource.com>, Ian Jackson
> <Ian.Jackson@eu.citrix.com> wrote: 
>> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"): 
>>> There are effectively four states a device can be in, from the 
>>> 'assignment' point of view: 
>>>  
>>> 1. Assigned to the normal Linux device driver(s) for the USB device 
>>>  
>>> 2. Assigned to no driver 
>>>  
>>> 3. Assigned to usbback, but not yet assigned to any guest 
>>>  
>>> 4. Assigned to a guest 
>>  
>> Thanks for your clear explanation (of which I will snip much). 
>>  
>>> Additionally, each USB "device" has one or more "interfaces".  To 
>>> assign a "device" to usbback in the sysfs case means assigning all of 
>>> the "interfaces".  The code seems to assume that different interfaces 
>>> from the same device can be assigned to different drivers. 
>>  
>> It is indeed the case that in principle a single USB device with 
>> multiple interfaces can be assigned to multiple different drivers. 
>>  
>>> Regarding Ian's comments: 
>>>  
>>> Since "assigned to the guest" and "listed under the pvusb xenstore 
>>> node" are the same thing, is it even *possible* to (safely) unassign 
>>> the device from usbback before removing the xenstore nodes? 
>>  
>> It might be possible to remove some of the xenstore nodes but leave 
>> others present, so that usbback detaches, but enough information 
>> remains for libxl to know that Xen still `owns' the device. 
> 
> Indeed "unassign from usbback, but listed under pvusb xenstore" is
> a confusing state. usb-list can list it but guest can not see it. 
> What user can do under that state is: reattempt usbdev_remove, if it 
> succeeds, everything is cleaned up, that's the best result; but 
> possibly it still fails (for example, in my testing, always cannot 
> rebind to original driver), in this case, the confusing state will 
> be lasting, and the device could not be removed, this is worse.

As I said in my other mail, I think removing the pvusb nodes should be
done once it's successfully un-bound from usbback, *even if* the re-bind
to the original driver fails.  (That is, once it reaches state 2,
usb-list should no longer list it.)

>>> Perhaps the best approach code-wise is to change the "goto out" on 
>>> failure of unbind_usbintf() into a "continue".  That way: 
>>>  
>>> 1. All interfaces which can be re-assigned are re-assigned (and work 
>>> as much as possible) 
>>>  
>>> 2. All interfaces which can be unbound but not re-assigned are at 
>>> least unbound (so that reloading the original driver might pick them 
>>> up) 
>>  
>> I certainly don't mind the software trying to do as much of its task 
>> as possible.
> 
> Could I understand that this way is acceptable? That means: removing 
> xenstore, and as much as we could (on failure of "unbind from usbback"
> or "bind to original driver", don't "goto out", just "continue").

I think part of it depends on what is *possible*.  If it's possible to
safely unbind the device from usbback while retaining its place in the
pvusb-related xenstore nodes, then I think we should (so that the user
can re-try removing it).  If it's not possible, then of course we have
to remove the pvusb xenstore nodes first, and then we'll just have to
deal as gracefully as possible with failure unbinding from usbback.

 -George