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

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

>>> On 3/3/2016 at 02:32 AM, in message <56D731B1.60009@xxxxxxxxxx>, George 
>>> Dunlap
<george.dunlap@xxxxxxxxxx> wrote: 
> On 01/03/16 08:09, Chunyan Liu 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: 
> >   reorder usbdev_remove to following 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. 
>  
> Thanks, Chunyan!  One minor comment about these changes... 
>  
> > +static int usbdev_rebind(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; 
> > + 
> > +        /* 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; 
> > +            } 
> > +        } 
> > +    } 
> > + 
> > +    path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); 
> > +    rc = libxl__xs_rm_checked(gc, XBT_NULL, path); 
> > + 
> > +out: 
>  
> So it looks like if one of the re-binds fails, then it stops where it is 
> and leaves the USBBACK re-bind info in xenstore.  In that case it's not 
> clear to me how that information would ever be removed. 
>  
> I think until such time as we have a command to re-attempt the re-bind, 
>  if there's an error in the actual rebind, it should just break out of 
> the for loop, and remove the re-bind nodes, and document a way to let 
> the user try to clean things up. 

Just according to last time discussion about how to handle the rebind
failure, seems Ian preferred to add a xl command to deal with rebind
in future, based on that need, I think driver_path info should be kept
in xenstore then. Without that need, I agree it's best to remove
xenstore nodes. So, keep or remove?

[Post last time Ian's idea]
[start]
The only wrinkle is that the obvious implementation reads the old
bindings from xenstore between 1 and 2, deletes the information from
xenstore in 2, and uses that information in step 3, which is cheating
(and leads to the sysfs-wrangling-required state).  But that is quite
easy to avoid:
  - record the old driver bindings somewhere in xenstore (keyed by
    the physical host device, not the guest domain)
  - provide a libxl call and corresponding xl command which attempts
    to rebind

But, FAOD, I do not want to block this patch until such a thing is
implemented.  I think it is sufficient to document the existence of
the awkward state, and the likely workarounds.
[end]

>  
> > +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid, 
> > +                            libxl_device_usbdev *usbdev) 
> > +{ 
> > +    int rc; 
> > +    char *busid; 
> > +    libxl_device_usbctrl usbctrl; 
> > +    libxl_usbctrlinfo usbctrlinfo; 
> > + 
> > +    libxl_device_usbctrl_init(&usbctrl); 
> > +    libxl_usbctrlinfo_init(&usbctrlinfo); 
> > +    usbctrl.devid = usbdev->ctrl; 
> > + 
> > +    rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo); 
> > +    if (rc) goto out; 
> > + 
> > +    switch (usbctrlinfo.type) { 
> > +    case LIBXL_USBCTRL_TYPE_PV: 
> > +        busid = usbdev_busid_from_ctrlport(gc, domid, usbdev); 
> > +        if (!busid) { 
> > +            rc = ERROR_FAIL; 
> > +            goto out; 
> > +        } 
> > + 
> > +        rc = usbback_dev_unassign(gc, busid); 
> > +        if (rc) goto out; 
> > + 
> > +        rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev); 
> > +        if (rc) goto out; 
> > + 
> > +        rc = usbdev_rebind(gc, busid); 
> > +        if (rc) goto out; 
>  
> I think we need a comment here saying why we're doing things in this 
> order.  Maybe: 
>  
> "Things are done in this order to balance simplicity with robustness in 
> the case of failure: 
> * We unbind all interfaces before rebinding any interfaces, so that we 
> never get into a situation where some interfaces are assigned to usbback 
> and some are assigned to the original drivers. 
> * We also unbind the interfaces before removing the pvusb xenstore 
> nodes, so that if the unbind fails in the middle, the device still shows 
> up in xl usb-list, and the user can re-try removing it." 

Make sense. I'll add this in comment.

Chunyan

>  
> Other than that, I gave this patche a moderately thorough review again 
> today, and I think everything else looks good to me. 
>  
>  -George 
>  
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 
>  


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