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 <jwer...@chromium.org>
> > ---
>
> > +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
>
