Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb API

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 2/24/2016 at 01:10 AM, in message
<CAFLBxZYrdhLO-euMioWiitqh7+FDUKc7rxO3g=m=-3tPGFuRxw@xxxxxxxxxxxxxx>, George
Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote: 
> On Fri, Feb 19, 2016 at 10:39 AM, Chunyan Liu <cyliu@xxxxxxxx> wrote: 
> > Add pvusb APIs, including: 
> > - attach/detach (create/destroy) virtual usb controller. 
> > - attach/detach usb device 
> > - list usb controller and usb devices 
> > - some other helper functions 
> > 
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx> 
> > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx> 
> > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx> 
> > --- 
> > changes: 
> >   Address Olaf's comments: 
> >   * move DEFINE_DEVICE_REMOVE changes to a separate patch 
> >   Address Ian's comments: 
> >   * adjust order of removing xenstore and bind/unbind driver in usb_remove. 
> >   * reuse libxl_write_exactly in usbintf_bind/unbind 
> >   * several coding style fix 
>  
> [snip] 
>  
> > +/* Unbind USB device from "usbback" driver. 
> > + * 
> > + * If there are many interfaces under USB device, check each interface, 
> > + * unbind from "usbback" driver and rebind to its original driver. 
> > + */ 
> > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid) 
> > +{ 
> > +    char **intfs = NULL; 
> > +    char *usbdev_encode = NULL; 
> > +    char *path = NULL; 
> > +    int i, num = 0; 
> > +    int rc; 
> > + 
> > +    rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num); 
> > +    if (rc) goto out; 
> > + 
> > +    usbdev_encode = usb_interface_xenstore_encode(gc, busid); 
> > + 
> > +    for (i = 0; i < num; i++) { 
> > +        char *intf = intfs[i]; 
> > +        char *usbintf_encode = NULL; 
> > +        const char *drvpath; 
> > + 
> > +        /* check if the USB interface is already bound to "usbback" */ 
> > +        if (usbintf_is_assigned(gc, intf) > 0) { 
> > +            /* unbind interface from usbback driver */ 
> > +            rc = unbind_usbintf(gc, intf); 
> > +            if (rc) { 
> > +                LOGE(ERROR, "Couldn't unbind %s from usbback", intf); 
> > +                goto out; 
> > +            } 
> > +        } 
> > + 
> > +        /* rebind USB interface to its originial driver */ 
> > +        usbintf_encode = usb_interface_xenstore_encode(gc, intf); 
> > +        path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", 
> > +                         usbdev_encode, usbintf_encode); 
> > +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); 
> > +        if (rc) goto out; 
> > + 
> > +        if (drvpath) { 
> > +            rc = bind_usbintf(gc, intf, drvpath); 
> > +            if (rc) { 
> > +                LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); 
> > +                goto out; 
> > +            } 
> > +        } 
> > +    } 
>  
> So I see below that you're calling this before removing things from 
> xenstore, so that if any of these fail, the user can still call "xl 
> usb-remove" to retry. 
>  
> Unfortunately, since you reassign interfaces to the original driver 
> before all interfaces are de-assigned from usbback, you can end up in 
> a situation where the device is partially re-plugged into the original 
> drivers, partially still assigned to pciback. 
>  
> I think this whole process should be three steps: 
>  
> 1. Unassign all interfaces from usbback, stopping and returning an 
> error as soon as one attempt fails 
>  
> 2. Remove the pvusb xenstore nodes (stopping and returning an error if it  
> fails) 
>  
> 3. Attempt to re-assign all interfaces to the original drivers, 
> stopping and returning an error as soon as one attempt fails. 
>  
> And in the case of #3, the log message should give a suggestion as to 
> what might help; for instance, "Couldn't rebind USB device to driver 
> [driver name].  Reloading the module may help."  (I think you should 
> be able to get the driver name from the path, right?) 

Yes, that's right.

>  
> A couple of properties this gives us: 
>  
> * If the un-assign partially succeeds, the user can re-try the unassign 
>  
> * If one of the re-assignments fail, then the user will have to reload 
> the drivers, reboot, or mess around with sysfs to fix things. 
> *However*, this avoids a scenario where a user is completely unable to 
> remove a device from a domain because of a buggy driver. 

To guest or host, this situation that some interface is bound to pciback
but some is to original driver, is indeed a mess. But to toolstack, it can
still have chance to redo 'xl usbdev-remove' to retry the left work (unassign
interfaces those bound to pciback, and reassign to original interface).

>  
> What do you think? 

The 3 steps above are indeed more reasonable. Codes need to be
reorganized (previously doing unassingn_form_pciback and rebind to
original driver in a same cycle for each interface, now will be split into
two separate processes: walk through each interface to do
unassign_from_pciback, and walk through each interface to rebind to
original driver).

But now that if error happens in rebinding to original driver, user will
have to manually deal with driver things, user can do the same when
error happens in unassign_from_pciback. So, now I just wonder:
is it really better to switch the order of unassign_from_pciback and
remove_from_xenstore?  (A situation that "a USB device is assigned
to guest but some interface is bound to pciback and some is
unbound" is also a mess.)

-Chunyan
 
>  
> I realize this falls short of the "crash-only" design IanJ suggested 
> we try to do, but I think that in this case the work required to do 
> such a design would be a lot more work than the benefit it gives us. 
>  
> I haven't re-reviewed the whole patch, but all the other changes look 
> good to me. 
>  
>  -George 
>  
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel