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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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?)

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.

What do you think?

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