Re: [PATCH] usb: storage: Add ums-cros-aoa driver

[Dan Williams]

On Wed, 2019-08-28 at 12:17 -0400, Alan Stern wrote:
> On Tue, 27 Aug 2019, Julius Werner wrote:
> 
> > This patch adds a new "unusual" USB mass storage device driver. This
> > driver will be used for a virtual USB storage device presented by an
> > Android phone running the 'Chrome OS Recovery'* Android app. This app
> > uses the Android Open Accessory (AOA) API to talk directly to a USB host
> > attached to the phone.
> > 
> > The AOA protocol requires the host to send a custom vendor command on
> > EP0 to "switch" the phone into "AOA mode" (making it drop off the bus
> > and reenumerate with different descriptors). The ums-cros-aoa driver is
> > just a small stub driver to send these vendor commands. It identifies
> > the device it should operate on by VID/PID passed in through a module
> > parameter (e.g. from the bootloader). After the phone is in AOA mode,
> > the normal USB mass storage stack will recognize it by its special
> > VID/PID like any other "unusual dev". An initializer function will
> > further double-check that the device is the device previously operated
> > on by ums-cros-aoa.

Isn't this scenario exactly what usb_modeswitch is supposed to do, in
userspace?  Various WWAN modems also require a vendor-specific command
(or a mass storage eject) to switch from driver CD mode into modem
mode, and all those are handled by usb_modeswitch.

In summary, does this really need to be in the kernel?

Dan

> > *NOTE: The Android app is still under development and will be released
> > at a later date. I'm submitting this patch now so that the driver name
> > and module parameters can be set in stone already, because I have to
> > bake them into bootloader code that is not field-updatable.
> > 
> > Signed-off-by: Julius Werner <jwerner@xxxxxxxxxxxx>
> > ---
> 
> > +static struct usb_device_id cros_aoa_ids[] = {
> > +     { USB_DEVICE(0, 0) },   /* to be filled out by cros_aoa_init() */
> > +     { }
> > +};
> > +/* No MODULE_DEVICE_TABLE(). Autoloading doesn't make sense for this module. */
> 
> Aren't you going to get in trouble if there are two attached devices
> that need to be put into AOA mode?
> 
> > +static int set_string(struct usb_device *udev, u16 type, const char *string)
> > +{
> > +     return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), AOA_SET_STRING,
> > +                            USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > +                            0, type, (char *)string, strlen(string) + 1,
> > +                            USB_CTRL_SET_TIMEOUT);
> 
> Although this is technically invalid, it's probably okay...
> 
> > +}
> > +
> > +static int cros_aoa_probe(struct usb_interface *intf,
> > +                       const struct usb_device_id *id)
> > +{
> > +     int rv;
> > +     u16 aoa_protocol;
> > +     struct usb_device *udev = interface_to_usbdev(intf);
> > +
> > +     rv = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), AOA_GET_PROTOCOL,
> > +                          USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > +                          0, 0, &aoa_protocol, sizeof(aoa_protocol),
> > +                          USB_CTRL_GET_TIMEOUT);
> 
> but this most certainly is not okay.  Buffers passed to
> usb_control_msg() (or used with URBs in general) should be allocated
> with kmalloc.
> 
> > +     if (rv < 0 && rv != -EPROTO)
> > +             goto fail;
> > +     if (rv != sizeof(aoa_protocol) || aoa_protocol < 1) {
> > +             dev_err(&intf->dev, "bound device does not support AOA?\n");
> > +             rv = -ENODEV;
> > +             goto fail;
> > +     }
> > +
> > +     if ((rv = set_string(udev, AOA_STR_MANUFACTURER, CROS_MANUF)) < 0 ||
> > +         (rv = set_string(udev, AOA_STR_MODEL, CROS_MODEL)) < 0 ||
> > +         (rv = set_string(udev, AOA_STR_DESCRIPTION, CROS_DESC)) < 0 ||
> > +         (rv = set_string(udev, AOA_STR_VERSION, CROS_VERSION)) < 0 ||
> > +         (rv = set_string(udev, AOA_STR_URI, CROS_URI)) < 0)
> > +             goto fail;
> 
> Bad programming style (assignment within "if" expression).
> 
> > +
> > +     rv = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), AOA_START,
> > +                          USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > +                          0, 0, NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +
> > +     if (!rv) {
> > +             dev_info(&intf->dev, "switching to AOA mode\n");
> > +             usb_stor_cros_aoa_bind_busnum = udev->bus->busnum;
> > +             usb_stor_cros_aoa_bind_route = udev->route;
> 
> Bear in mind that udev->route is supposed to be reliable only if the 
> device is running at SuperSpeed.
> 
> > +             return 0;
> 
> Why return 0?  You're going to unbind immediately anyway.
> 
> > +     }
> > +
> > +fail:        dev_err(&intf->dev, "probe error %d\n", rv);
> > +     return rv;
> > +}
> > +
> > +static void cros_aoa_disconnect(struct usb_interface *intf)
> > +{
> > +     /* nothing to do -- we expect this to happen right after probe() */
> > +}
> > +
> > +static struct usb_driver cros_aoa_stub_driver = {
> > +     .name =         DRV_NAME,
> > +     .probe =        cros_aoa_probe,
> > +     .disconnect =   cros_aoa_disconnect,
> > +     .id_table =     cros_aoa_ids,
> > +};
> > +
> > +static int __init cros_aoa_init(void)
> > +{
> > +     if (!bind || sscanf(bind, "%hx:%hx", &cros_aoa_ids[0].idVendor,
> > +                                          &cros_aoa_ids[0].idProduct) != 2)
> > +             return -ENODEV;
> > +     pr_info(DRV_NAME ": bound to USB device %4x:%4x\n",
> > +             cros_aoa_ids[0].idVendor, cros_aoa_ids[0].idProduct);
> > +     return usb_register(&cros_aoa_stub_driver);
> 
> As Greg pointed out, there are better ways to do this.
> 
> > +}
> > +
> > +static void __exit cros_aoa_exit(void)
> > +{
> > +     usb_deregister(&cros_aoa_stub_driver);
> > +}
> > +
> > +module_init(cros_aoa_init);
> > +module_exit(cros_aoa_exit);
> > diff --git a/drivers/usb/storage/initializers.c b/drivers/usb/storage/initializers.c
> > index f8f9ce8dc7102..3056db79cd1d9 100644
> > --- a/drivers/usb/storage/initializers.c
> > +++ b/drivers/usb/storage/initializers.c
> > @@ -92,3 +92,37 @@ int usb_stor_huawei_e220_init(struct us_data *us)
> >       usb_stor_dbg(us, "Huawei mode set result is %d\n", result);
> >       return 0;
> >  }
> > +
> > +#if defined(CONFIG_USB_STORAGE_CROS_AOA) || \
> > +             defined(CONFIG_USB_STORAGE_CROS_AOA_MODULE)
> > +/*
> > + * Our VID/PID match grabs any Android device that was switched into Android
> > + * Open Accessory mode. We only want to bind to the one that was switched by the
> > + * ums-cros-aoa driver. There's no 100% way to identify the same device again
> > + * (because it changes all descriptors), but checking that it is on the same bus
> > + * with the same topology route should be a pretty good heuristic.
> > + */
> > +int usb_stor_cros_aoa_bind_busnum = -1;
> > +EXPORT_SYMBOL(usb_stor_cros_aoa_bind_busnum);
> > +u32 usb_stor_cros_aoa_bind_route;
> > +EXPORT_SYMBOL(usb_stor_cros_aoa_bind_route);
> > +
> > +int usb_stor_cros_aoa_validate(struct us_data *us)
> > +{
> > +     if (us->pusb_dev->bus->busnum != usb_stor_cros_aoa_bind_busnum ||
> > +         us->pusb_dev->route != usb_stor_cros_aoa_bind_route) {
> > +             dev_info(&us->pusb_intf->dev,
> > +                      "ums-cros-aoa ignoring unknown AOA device\n");
> > +             return -ENODEV;
> > +     }
> 
> What happens if two devices switch modes concurrently?  You have room 
> to store only one topology route.
> 
> Besides, what's wrong with binding to devices that weren't switched 
> into AOA mode?  Would that just provoke a bunch of unnecessary error 
> messages?
> 
> > +
> > +     /*
> > +      * Only interface 0 connects to the AOA app. Android devices that have
> > +      * ADB enabled also export an interface 1. We don't want it.
> > +      */
> > +     if (us->pusb_intf->cur_altsetting->desc.bInterfaceNumber != 0)
> > +             return -ENODEV;
> 
> Do you really need this test?  What would go wrong if you don't do it?
> 
> > +
> > +     return 0;
> > +}
> > +#endif /* defined(CONFIG_USB_STORAGE_CROS_AOA) || ... */
> > diff --git a/drivers/usb/storage/initializers.h b/drivers/usb/storage/initializers.h
> > index 2dbf9c7d97492..35fe9ef3247d6 100644
> > --- a/drivers/usb/storage/initializers.h
> > +++ b/drivers/usb/storage/initializers.h
> > @@ -37,3 +37,7 @@ int usb_stor_ucr61s2b_init(struct us_data *us);
> >  
> >  /* This places the HUAWEI E220 devices in multi-port mode */
> >  int usb_stor_huawei_e220_init(struct us_data *us);
> > +
> > +extern int usb_stor_cros_aoa_bind_busnum;
> > +extern u32 usb_stor_cros_aoa_bind_route;
> > +int usb_stor_cros_aoa_validate(struct us_data *us);
> > diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
> > index ea0d27a94afe0..45fe9bbc6da18 100644
> > --- a/drivers/usb/storage/unusual_devs.h
> > +++ b/drivers/usb/storage/unusual_devs.h
> > @@ -2259,6 +2259,24 @@ UNUSUAL_DEV( 0x1e74, 0x4621, 0x0000, 0x0000,
> >               USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> >               US_FL_BULK_IGNORE_TAG | US_FL_MAX_SECTORS_64 ),
> >  
> > +/*
> > + * Using an Android phone as USB storage back-end for Chrome OS recovery. See
> > + * usb/storage/cros-aoa.c for details.
> > + */
> > +#if defined(CONFIG_USB_STORAGE_CROS_AOA) || \
> > +             defined(CONFIG_USB_STORAGE_CROS_AOA_MODULE)
> > +UNUSUAL_DEV(  0x18d1, 0x2d00, 0x0000, 0xffff,
> > +             "Google",
> > +             "Chrome OS Recovery via AOA",
> > +             USB_SC_SCSI, USB_PR_BULK, usb_stor_cros_aoa_validate,
> > +             US_FL_SINGLE_LUN | US_FL_CAPACITY_OK),
> > +UNUSUAL_DEV(  0x18d1, 0x2d01, 0x0000, 0xffff,
> > +             "Google",
> > +             "Chrome OS Recovery via AOA (with ADB)",
> > +             USB_SC_SCSI, USB_PR_BULK, usb_stor_cros_aoa_validate,
> > +             US_FL_SINGLE_LUN | US_FL_CAPACITY_OK),
> > +#endif /* defined(CONFIG_USB_STORAGE_CROS_AOA) || ... */
> 
> Subdrivers (the ums_* modules) are supposed to have their own 
> individual unusual_devs files.  They don't use the main file.
> 
> > > Why not do the mode switch from userspace?  I thought we were trying to move all the mode-switching stuff in that direction.....
> > 
> > I need to tie in to the main USB mass storage driver in a way that I
> > think would make it too complicated to move the mode switching part to
> > userspace. See the part I'm adding to initializers.c... that one has
> > to be in the kernel since it operates on the device after the mode
> > switch when it is claimed by the normal USB storage driver.
> 
> I'm not convinced that you really need to do this.
> 
> >  But the
> > mode switch part has to communicate information to it to make sure it
> > picks up the right device (just relying on the normal USB device
> > matching isn't enough in this case, because all Android devices in AOA
> > mode identify themselves with that well-known VID/PID... I don't know
> > if there's any other kernel driver using this protocol today, but
> > there may be at some point and then it becomes important to make sure
> > you really grab the device you meant to grab). Some of that
> > information (the 'route' field) isn't even directly available from
> > userspace (I could use the device name string instead and that would
> > roughly come out to the same thing, but seems less clean to me).
> 
> The full, reliable routing information (not just the data in
> udev->route) is indeed available to userspace.  See the definition of
> the USBDEVFS_CONNINFO_EX usbfs ioctl.
> 
> > So I could either do the mode switch in userspace and add a big custom
> > sysfs interface to the usb-storage driver to allow userspace to
> > configure all this, or I can add a small kernel shim driver like in
> > this patch. Considering how little code the shim driver actually needs
> > I expect it would come out to roughly the same amount of kernel code
> > in both cases, and I feel like this version is much simpler to follow
> > and fits cleaner in the existing "unusual device" driver
> > infrastructure.
> 
> IMO the userspace approach would be better, unless you can provide a
> really compelling argument for why it won't suffice.
> 
> Alan Stern
>