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

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

>>> On 3/3/2016 at 02:46 AM, in message <56D7350F.7010000@xxxxxxxxxx>, George
Dunlap <george.dunlap@xxxxxxxxxx> wrote: 
> On 02/03/16 18:32, George Dunlap 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. 
> >  
> >> +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." 
>  
> Sorry, just looked through the rest of the series, and there's one more 
> thing. 
>  
> Neither here nor in the man page do we explain what to do if something 
> goes wrong with the detach.  I think the best thing to do is probably to 
> make the logged error messages more helpful. 
>  
> What about something like this: 
>  
> * On failure to unbind: "Error removing device from guest.  Try running 
> usbdev-detach again." 
>  
> * On failure to rebind: "USB device removed from guest, but couldn't 
> re-bind to domain 0.  Try removing and re-inserting the USB device or 
> reloading the driver modules." 
Here, user could first try 'echo xxx > sysfs_driver_path/bind", so maybe:
"Try binding USB device to original driver by echoing the device to
[driver_path]/bind, or removing and re-inserting the USB device, or 
reloading the driver modules."

Chunyan

>  
> What do you think? 
>  
>  -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