* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-27 23:14 [PATCH] usb: storage: Add ums-cros-aoa driver Julius Werner
@ 2019-08-27 23:29 ` Matthew Dharm
[not found] ` <CAA6KcBAykS+VkhkcF42PhGyNu8KAEoaYPgA9-ru_HCxKrAEZzg@mail.gmail.com>
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Matthew Dharm @ 2019-08-27 23:29 UTC (permalink / raw)
To: Julius Werner
Cc: Alan Stern, Greg Kroah-Hartman, linux-kernel, linux-usb,
USB Mass Storage on Linux
(Sending this again, because I forgot to switch my mail client to
plain-text-only last time).
Why not do the mode switch from userspace? I thought we were trying
to move all the mode-switching stuff in that direction.....
Matt
On Tue, Aug 27, 2019 at 4:14 PM Julius Werner <jwerner@chromium.org> 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.
>
> *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@chromium.org>
> ---
> drivers/usb/storage/Kconfig | 12 +++
> drivers/usb/storage/Makefile | 2 +
> drivers/usb/storage/cros-aoa.c | 129 +++++++++++++++++++++++++++++
> drivers/usb/storage/initializers.c | 34 ++++++++
> drivers/usb/storage/initializers.h | 4 +
> drivers/usb/storage/unusual_devs.h | 18 ++++
> 6 files changed, 199 insertions(+)
> create mode 100644 drivers/usb/storage/cros-aoa.c
>
> diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
> index 59aad38b490a6..cc901ee2bb766 100644
> --- a/drivers/usb/storage/Kconfig
> +++ b/drivers/usb/storage/Kconfig
> @@ -184,6 +184,18 @@ config USB_STORAGE_ENE_UB6250
> To compile this driver as a module, choose M here: the
> module will be called ums-eneub6250.
>
> +config USB_STORAGE_CROS_AOA
> + tristate "Support for connecting to Chrome OS Recovery Android app"
> + default n
> + depends on USB_STORAGE
> + help
> + Say Y here if you want to connect an Android phone running the Chrome
> + OS Recovery app to this device and mount the image served by that app
> + as a virtual storage device. Unless you're building for Chrome OS, you
> + probably want to say N.
> +
> + If this driver is compiled as a module, it will be named ums-cros-aoa.
> +
> endif # USB_STORAGE
>
> config USB_UAS
> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
> index a67ddcbb4e249..f734741d4658b 100644
> --- a/drivers/usb/storage/Makefile
> +++ b/drivers/usb/storage/Makefile
> @@ -17,6 +17,7 @@ usb-storage-y += usual-tables.o
> usb-storage-$(CONFIG_USB_STORAGE_DEBUG) += debug.o
>
> obj-$(CONFIG_USB_STORAGE_ALAUDA) += ums-alauda.o
> +obj-$(CONFIG_USB_STORAGE_CROS_AOA) += ums-cros-aoa.o
> obj-$(CONFIG_USB_STORAGE_CYPRESS_ATACB) += ums-cypress.o
> obj-$(CONFIG_USB_STORAGE_DATAFAB) += ums-datafab.o
> obj-$(CONFIG_USB_STORAGE_ENE_UB6250) += ums-eneub6250.o
> @@ -31,6 +32,7 @@ obj-$(CONFIG_USB_STORAGE_SDDR55) += ums-sddr55.o
> obj-$(CONFIG_USB_STORAGE_USBAT) += ums-usbat.o
>
> ums-alauda-y := alauda.o
> +ums-cros-aoa-y := cros-aoa.o
> ums-cypress-y := cypress_atacb.o
> ums-datafab-y := datafab.o
> ums-eneub6250-y := ene_ub6250.o
> diff --git a/drivers/usb/storage/cros-aoa.c b/drivers/usb/storage/cros-aoa.c
> new file mode 100644
> index 0000000000000..269e9193209d9
> --- /dev/null
> +++ b/drivers/usb/storage/cros-aoa.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2 WITH Linux-syscall-note
> +/*
> + * Driver for Chrome OS Recovery via Android Open Accessory
> + *
> + * (c) 2019 Google LLC (Julius Werner <jwerner@chromium.org>)
> + *
> + * This driver connects to an Android device via the Android Open Accessory
> + * protocol to use it as a USB storage back-end. It is used for system recovery
> + * on Chrome OS. The descriptors sent are specific to the Chrome OS Recovery app
> + * for Android. The driver is inert unless activated by boot firmware with an
> + * explicit kernel command line parameter.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/usb.h>
> +
> +#include "initializers.h"
> +
> +#define DRV_NAME "ums-cros-aoa"
> +
> +MODULE_DESCRIPTION("Driver for Chrome OS Recovery via Android Open Accessory");
> +MODULE_AUTHOR("Julius Werner <jwerner@chromium.org>");
> +MODULE_LICENSE("GPL");
> +
> +#define AOA_GET_PROTOCOL 51
> +#define AOA_SET_STRING 52
> +#define AOA_START 53
> +
> +#define AOA_STR_MANUFACTURER 0
> +#define AOA_STR_MODEL 1
> +#define AOA_STR_DESCRIPTION 2
> +#define AOA_STR_VERSION 3
> +#define AOA_STR_URI 4
> +#define AOA_STR_SERIAL 5
> +
> +#define CROS_MANUF "Google"
> +#define CROS_MODEL "Chrome OS Recovery"
> +#define CROS_DESC "Chrome OS device in Recovery Mode"
> +#define CROS_VERSION "1.0"
> +#define CROS_URI "https://google.com/chromeos/recovery_android"
> +
> +static char *bind;
> +module_param(bind, charp, 0);
> +
> +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. */
> +
> +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);
> +}
> +
> +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);
> + 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;
> +
> + 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;
> + return 0;
> + }
> +
> +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);
> +}
> +
> +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;
> + }
> +
> + /*
> + * 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;
> +
> + 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) || ... */
> +
> /* Supplied with some Castlewood ORB removable drives */
> UNUSUAL_DEV( 0x2027, 0xa001, 0x0000, 0x9999,
> "Double-H Technology",
> --
> 2.20.1
>
--
Matthew Dharm
Former Maintainer, USB Mass Storage driver for Linux
^ permalink raw reply [flat|nested] 19+ messages in thread[parent not found: <CAA6KcBAykS+VkhkcF42PhGyNu8KAEoaYPgA9-ru_HCxKrAEZzg@mail.gmail.com>]
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
[not found] ` <CAA6KcBAykS+VkhkcF42PhGyNu8KAEoaYPgA9-ru_HCxKrAEZzg@mail.gmail.com>
@ 2019-08-27 23:59 ` Julius Werner
0 siblings, 0 replies; 19+ messages in thread
From: Julius Werner @ 2019-08-27 23:59 UTC (permalink / raw)
To: Matthew Dharm
Cc: Alan Stern, Greg Kroah-Hartman, LKML, linux-usb,
USB Mass Storage on Linux
> 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. 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).
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-27 23:14 [PATCH] usb: storage: Add ums-cros-aoa driver Julius Werner
2019-08-27 23:29 ` Matthew Dharm
[not found] ` <CAA6KcBAykS+VkhkcF42PhGyNu8KAEoaYPgA9-ru_HCxKrAEZzg@mail.gmail.com>
@ 2019-08-28 8:32 ` Greg Kroah-Hartman
2019-08-28 16:17 ` Alan Stern
3 siblings, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-28 8:32 UTC (permalink / raw)
To: Julius Werner; +Cc: Alan Stern, linux-kernel, linux-usb, usb-storage
On Tue, Aug 27, 2019 at 04:14:09PM -0700, 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.
>
> *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@chromium.org>
> ---
> drivers/usb/storage/Kconfig | 12 +++
> drivers/usb/storage/Makefile | 2 +
> drivers/usb/storage/cros-aoa.c | 129 +++++++++++++++++++++++++++++
> drivers/usb/storage/initializers.c | 34 ++++++++
> drivers/usb/storage/initializers.h | 4 +
> drivers/usb/storage/unusual_devs.h | 18 ++++
> 6 files changed, 199 insertions(+)
> create mode 100644 drivers/usb/storage/cros-aoa.c
Pure syntax issues noted below, nothing on the content, I'll wait until
after my coffee for that...
>
> diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
> index 59aad38b490a6..cc901ee2bb766 100644
> --- a/drivers/usb/storage/Kconfig
> +++ b/drivers/usb/storage/Kconfig
> @@ -184,6 +184,18 @@ config USB_STORAGE_ENE_UB6250
> To compile this driver as a module, choose M here: the
> module will be called ums-eneub6250.
>
> +config USB_STORAGE_CROS_AOA
> + tristate "Support for connecting to Chrome OS Recovery Android app"
> + default n
"default n" is the default, no need to list it again.
> + depends on USB_STORAGE
> + help
> + Say Y here if you want to connect an Android phone running the Chrome
> + OS Recovery app to this device and mount the image served by that app
> + as a virtual storage device. Unless you're building for Chrome OS, you
> + probably want to say N.
> +
> + If this driver is compiled as a module, it will be named ums-cros-aoa.
> +
> endif # USB_STORAGE
>
> config USB_UAS
> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
> index a67ddcbb4e249..f734741d4658b 100644
> --- a/drivers/usb/storage/Makefile
> +++ b/drivers/usb/storage/Makefile
> @@ -17,6 +17,7 @@ usb-storage-y += usual-tables.o
> usb-storage-$(CONFIG_USB_STORAGE_DEBUG) += debug.o
>
> obj-$(CONFIG_USB_STORAGE_ALAUDA) += ums-alauda.o
> +obj-$(CONFIG_USB_STORAGE_CROS_AOA) += ums-cros-aoa.o
> obj-$(CONFIG_USB_STORAGE_CYPRESS_ATACB) += ums-cypress.o
> obj-$(CONFIG_USB_STORAGE_DATAFAB) += ums-datafab.o
> obj-$(CONFIG_USB_STORAGE_ENE_UB6250) += ums-eneub6250.o
> @@ -31,6 +32,7 @@ obj-$(CONFIG_USB_STORAGE_SDDR55) += ums-sddr55.o
> obj-$(CONFIG_USB_STORAGE_USBAT) += ums-usbat.o
>
> ums-alauda-y := alauda.o
> +ums-cros-aoa-y := cros-aoa.o
> ums-cypress-y := cypress_atacb.o
> ums-datafab-y := datafab.o
> ums-eneub6250-y := ene_ub6250.o
> diff --git a/drivers/usb/storage/cros-aoa.c b/drivers/usb/storage/cros-aoa.c
> new file mode 100644
> index 0000000000000..269e9193209d9
> --- /dev/null
> +++ b/drivers/usb/storage/cros-aoa.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2 WITH Linux-syscall-note
.c files do not have that license, sorry (I have had _LONG_ discussions
with lawyers about that over the past few weeks...)
It should just be GPL-2
> +/*
> + * Driver for Chrome OS Recovery via Android Open Accessory
> + *
> + * (c) 2019 Google LLC (Julius Werner <jwerner@chromium.org>)
> + *
> + * This driver connects to an Android device via the Android Open Accessory
> + * protocol to use it as a USB storage back-end. It is used for system recovery
> + * on Chrome OS. The descriptors sent are specific to the Chrome OS Recovery app
> + * for Android. The driver is inert unless activated by boot firmware with an
> + * explicit kernel command line parameter.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/usb.h>
> +
> +#include "initializers.h"
> +
> +#define DRV_NAME "ums-cros-aoa"
KBUILD_MODNAME?
> +
> +MODULE_DESCRIPTION("Driver for Chrome OS Recovery via Android Open Accessory");
> +MODULE_AUTHOR("Julius Werner <jwerner@chromium.org>");
> +MODULE_LICENSE("GPL");
This usually goes at the end of the file, but ok...
> +
> +#define AOA_GET_PROTOCOL 51
> +#define AOA_SET_STRING 52
> +#define AOA_START 53
> +
> +#define AOA_STR_MANUFACTURER 0
> +#define AOA_STR_MODEL 1
> +#define AOA_STR_DESCRIPTION 2
> +#define AOA_STR_VERSION 3
> +#define AOA_STR_URI 4
> +#define AOA_STR_SERIAL 5
> +
> +#define CROS_MANUF "Google"
> +#define CROS_MODEL "Chrome OS Recovery"
> +#define CROS_DESC "Chrome OS device in Recovery Mode"
> +#define CROS_VERSION "1.0"
> +#define CROS_URI "https://google.com/chromeos/recovery_android"
> +
> +static char *bind;
> +module_param(bind, charp, 0);
No documentation for a module parameter?
And what is this, the 1990's? Please do not add these unless you have
no other option as they only work on a driver-wide basis, not a
per-device basis.
> +
> +static struct usb_device_id cros_aoa_ids[] = {
> + { USB_DEVICE(0, 0) }, /* to be filled out by cros_aoa_init() */
as-is, this driver is doing nothing, so it will never be auto-loaded :(
Please provide the real ids here for your device to start with. And if
you insist on adding new ids from userspace, then use the functionality
the driver core provides for you, do not invent a new one.
> + { }
> +};
> +/* No MODULE_DEVICE_TABLE(). Autoloading doesn't make sense for this module. */
I disagree, you need to bind to something...
> +
> +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);
> +}
> +
> +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);
> + 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;
Ok, I was going to stop and not talk about content, but what? You are
sending urls to a device???
> +
> + 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");
Do not be noisy for a normal functioning driver.
> + usb_stor_cros_aoa_bind_busnum = udev->bus->busnum;
> + usb_stor_cros_aoa_bind_route = udev->route;
> + return 0;
> + }
> +
> +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);
> +}
> +
> +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)
We have a macro to make that easier, please use it.
> +/*
> + * 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);
Ick.
And _GPL at the very least...
> +
> +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;
> + }
> +
> + /*
> + * 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;
> +
> + 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) || ... */
Hey look, device ids! Use them above please...
I'm with Matt, this should be trivial to do in userspace, why is this
needed in the kernel? Did you try it in userspace already? What does
the code for that look like?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-27 23:14 [PATCH] usb: storage: Add ums-cros-aoa driver Julius Werner
` (2 preceding siblings ...)
2019-08-28 8:32 ` Greg Kroah-Hartman
@ 2019-08-28 16:17 ` Alan Stern
2019-08-28 16:41 ` Dan Williams
3 siblings, 1 reply; 19+ messages in thread
From: Alan Stern @ 2019-08-28 16:17 UTC (permalink / raw)
To: Julius Werner
Cc: Greg Kroah-Hartman, Kernel development list, USB list, USB Storage list
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.
>
> *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@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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-28 16:17 ` Alan Stern
@ 2019-08-28 16:41 ` Dan Williams
2019-08-29 3:26 ` Julius Werner
0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2019-08-28 16:41 UTC (permalink / raw)
To: Alan Stern, Julius Werner
Cc: Greg Kroah-Hartman, Kernel development list, USB list, USB Storage list
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@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
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-28 16:41 ` Dan Williams
@ 2019-08-29 3:26 ` Julius Werner
2019-08-29 7:25 ` Greg Kroah-Hartman
2019-08-29 15:41 ` Alan Stern
0 siblings, 2 replies; 19+ messages in thread
From: Julius Werner @ 2019-08-29 3:26 UTC (permalink / raw)
To: Dan Williams
Cc: Alan Stern, Julius Werner, Greg Kroah-Hartman,
Kernel development list, USB list, USB Storage list
(Thanks for the reviews... I'll get back to the kernel code details
after double-checking if this can be done from userspace.)
> > 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?
It's not about devices that aren't switched into AOA mode... it's
about devices that are switched into AOA mode for other reasons
(connecting to other Android apps). I don't think a kernel driver like
that exists today, but it could be added, or people could use libusb
to talk to an AOA device. AOA is just a general mechanism to talk to
an Android app for whatever you want, and the descriptors sent during
mode switch clarify what app it's talking to (and thereby what
protocol it is using... it could be mass storage or it could be
something entirely different). But a device switched into AOA mode for
whatever app will always use that same well-known VID/PID (18d1:2d00).
So if I just add that VID/PID to the IDs bound by the usb-storage
driver, it would also grab a device that was mode-switched by
userspace to talk to a completely different app. I need some way to
make sure it only grabs the intended device, and there's no good
identifier for that other than comparing the dev path to what you
originally mode switched.
> > > + /*
> > > + * 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?
Yes, otherwise two separate usb-storage instances bind to the two
interfaces. The second interface is meant for a special ADB debugging
protocol and will not respond at all to USB mass storage packets, so
eventually the first request to it times out and
usb_stor_invoke_transport() will do a port reset to recover. That also
kills the first interface asynchronously even though it was working
fine.
> > IMO the userspace approach would be better, unless you can provide a
> > really compelling argument for why it won't suffice.
Well... okay, let me think through that again. I just found the new_id
sysfs API that I wasn't aware of before, maybe I could leverage that
to bind this from userspace after doing the mode switch. But it looks
like that only operates on whole devices... is there any way to force
it to only bind one particular interface?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-29 3:26 ` Julius Werner
@ 2019-08-29 7:25 ` Greg Kroah-Hartman
2019-08-29 15:41 ` Alan Stern
1 sibling, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-29 7:25 UTC (permalink / raw)
To: Julius Werner
Cc: Dan Williams, Alan Stern, Kernel development list, USB list,
USB Storage list
On Wed, Aug 28, 2019 at 08:26:15PM -0700, Julius Werner wrote:
> Well... okay, let me think through that again. I just found the new_id
> sysfs API that I wasn't aware of before, maybe I could leverage that
> to bind this from userspace after doing the mode switch. But it looks
> like that only operates on whole devices... is there any way to force
> it to only bind one particular interface?
USB drivers only bind to interfaces, are you saying that your device has
multiple interfaces on it?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-29 3:26 ` Julius Werner
2019-08-29 7:25 ` Greg Kroah-Hartman
@ 2019-08-29 15:41 ` Alan Stern
2019-08-30 0:31 ` Julius Werner
1 sibling, 1 reply; 19+ messages in thread
From: Alan Stern @ 2019-08-29 15:41 UTC (permalink / raw)
To: Julius Werner
Cc: Dan Williams, Greg Kroah-Hartman, Kernel development list,
USB list, USB Storage list
On Wed, 28 Aug 2019, Julius Werner wrote:
> (Thanks for the reviews... I'll get back to the kernel code details
> after double-checking if this can be done from userspace.)
>
> > > 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?
>
> It's not about devices that aren't switched into AOA mode... it's
> about devices that are switched into AOA mode for other reasons
> (connecting to other Android apps). I don't think a kernel driver like
> that exists today, but it could be added, or people could use libusb
> to talk to an AOA device. AOA is just a general mechanism to talk to
> an Android app for whatever you want, and the descriptors sent during
> mode switch clarify what app it's talking to (and thereby what
> protocol it is using... it could be mass storage or it could be
> something entirely different). But a device switched into AOA mode for
> whatever app will always use that same well-known VID/PID (18d1:2d00).
> So if I just add that VID/PID to the IDs bound by the usb-storage
> driver, it would also grab a device that was mode-switched by
> userspace to talk to a completely different app. I need some way to
> make sure it only grabs the intended device, and there's no good
> identifier for that other than comparing the dev path to what you
> originally mode switched.
Okay, I see. This sounds like a problem of communication between
userspace and the kernel driver; you want to tell the driver to bind
only to a specific device that is distinguishable only by its path.
In fact, there already is a way to do this in the kernel: write to the
sysfs "bind" file. The difficulty is that you can't force a driver to
bind to an interface if it doesn't believe it is compatible with the
interface. And if the driver believes it is compatible, it will
automatically attempt to bind with all such interfaces regardless of
their path.
Perhaps what you need is a usb_device_id flag to indicate that the
entry should never be used for automatic matches -- only for matches
made by the user via the "bind" file. Greg KH would probably be
willing to accept a new USB_DEVICE_ID_MATCH_NO_AUTO flag, which
could be included in your unusual_devs.h entries.
> > > > + /*
> > > > + * 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?
>
> Yes, otherwise two separate usb-storage instances bind to the two
> interfaces. The second interface is meant for a special ADB debugging
> protocol and will not respond at all to USB mass storage packets, so
> eventually the first request to it times out and
> usb_stor_invoke_transport() will do a port reset to recover. That also
> kills the first interface asynchronously even though it was working
> fine.
This isn't an issue if you rely on sysfs-directed binding. You write
the name of the specific interface to the "bind" file.
> > > IMO the userspace approach would be better, unless you can provide a
> > > really compelling argument for why it won't suffice.
>
> Well... okay, let me think through that again. I just found the new_id
> sysfs API that I wasn't aware of before, maybe I could leverage that
> to bind this from userspace after doing the mode switch. But it looks
> like that only operates on whole devices... is there any way to force
> it to only bind one particular interface?
No. But with the NO_AUTO flag in your match flags, this wouldn't
matter. Besides, the IDs you want to add aren't really dynamic -- they
are fixed and known in advance.
Try something like the patch below (completely untested). You'd
probably divide this up into two separate patches for submission: one
for the NO_AUTO flag and one to modify usb-storage.
Alan Stern
drivers/usb/core/driver.c | 4 ++++
drivers/usb/storage/unusual_devs.h | 13 +++++++++++++
drivers/usb/storage/usb.c | 2 ++
drivers/usb/storage/usual-tables.c | 6 ++++++
include/linux/mod_devicetable.h | 1 +
include/linux/usb.h | 20 ++++++++++++++++++++
6 files changed, 46 insertions(+)
Index: usb-devel/drivers/usb/core/driver.c
===================================================================
--- usb-devel.orig/drivers/usb/core/driver.c
+++ usb-devel/drivers/usb/core/driver.c
@@ -685,6 +685,10 @@ int usb_match_one_id(struct usb_interfac
if (id == NULL)
return 0;
+ /* Don't match entries intended only for manual binding */
+ if (id->match_flags & USB_DEVICE_ID_MATCH_NO_AUTO)
+ return 0;
+
intf = interface->cur_altsetting;
dev = interface_to_usbdev(interface);
Index: usb-devel/drivers/usb/storage/unusual_devs.h
===================================================================
--- usb-devel.orig/drivers/usb/storage/unusual_devs.h
+++ usb-devel/drivers/usb/storage/unusual_devs.h
@@ -2183,6 +2183,19 @@ UNUSUAL_DEV( 0x1822, 0x0001, 0x0000, 0x
USB_SC_DEVICE, USB_PR_DEVICE, usb_stor_euscsi_init,
US_FL_SCM_MULT_TARG ),
+/* Reported by Julius Werner <jwerner@chromium.org> */
+UNUSUAL_DEV_NO_AUTO( 0x18d1, 0x2d00, 0x0000, 0xffff,
+ "Google",
+ "Chrome OS Recovery via AOA",
+ USB_SC_SCSI, USB_PR_BULK, NULL,
+ US_FL_SINGLE_LUN | US_FL_CAPACITY_OK),
+
+UNUSUAL_DEV_NO_AUTO( 0x18d1, 0x2d01, 0x0000, 0xffff,
+ "Google",
+ "Chrome OS Recovery via AOA (with ADB)",
+ USB_SC_SCSI, USB_PR_BULK, NULL,
+ US_FL_SINGLE_LUN | US_FL_CAPACITY_OK),
+
/*
* Reported by Hans de Goede <hdegoede@redhat.com>
* These Appotech controllers are found in Picture Frames, they provide a
Index: usb-devel/drivers/usb/storage/usb.c
===================================================================
--- usb-devel.orig/drivers/usb/storage/usb.c
+++ usb-devel/drivers/usb/storage/usb.c
@@ -104,6 +104,8 @@ MODULE_PARM_DESC(quirks, "supplemental l
#define COMPLIANT_DEV UNUSUAL_DEV
+#define UNUSUAL_DEV_NO_AUTO UNUSUAL_DEV
+
#define USUAL_DEV(use_protocol, use_transport) \
{ \
.useProtocol = use_protocol, \
Index: usb-devel/drivers/usb/storage/usual-tables.c
===================================================================
--- usb-devel.orig/drivers/usb/storage/usual-tables.c
+++ usb-devel/drivers/usb/storage/usual-tables.c
@@ -23,6 +23,12 @@
#define COMPLIANT_DEV UNUSUAL_DEV
+#define UNUSUAL_DEV_NO_AUTO(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
+ vendorName, productName, useProtocol, useTransport, \
+ initFunction, flags) \
+{ USB_DEVICE_VER_NO_AUTO(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax), \
+ .driver_info = (flags) }
+
#define USUAL_DEV(useProto, useTrans) \
{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans) }
Index: usb-devel/include/linux/mod_devicetable.h
===================================================================
--- usb-devel.orig/include/linux/mod_devicetable.h
+++ usb-devel/include/linux/mod_devicetable.h
@@ -158,6 +158,7 @@ struct usb_device_id {
#define USB_DEVICE_ID_MATCH_INT_SUBCLASS 0x0100
#define USB_DEVICE_ID_MATCH_INT_PROTOCOL 0x0200
#define USB_DEVICE_ID_MATCH_INT_NUMBER 0x0400
+#define USB_DEVICE_ID_MATCH_NO_AUTO 0x0800
#define HID_ANY_ID (~0)
#define HID_BUS_ANY 0xffff
Index: usb-devel/include/linux/usb.h
===================================================================
--- usb-devel.orig/include/linux/usb.h
+++ usb-devel/include/linux/usb.h
@@ -925,6 +925,9 @@ static inline int usb_make_path(struct u
(USB_DEVICE_ID_MATCH_DEV_LO | USB_DEVICE_ID_MATCH_DEV_HI)
#define USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION \
(USB_DEVICE_ID_MATCH_DEVICE | USB_DEVICE_ID_MATCH_DEV_RANGE)
+#define USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION_NO_AUTO \
+ (USB_DEVICE_ID_MATCH_DEVICE | USB_DEVICE_ID_MATCH_DEV_RANGE | \
+ USB_DEVICE_ID_MATCH_NO_AUTO)
#define USB_DEVICE_ID_MATCH_DEV_INFO \
(USB_DEVICE_ID_MATCH_DEV_CLASS | \
USB_DEVICE_ID_MATCH_DEV_SUBCLASS | \
@@ -961,6 +964,23 @@ static inline int usb_make_path(struct u
.idVendor = (vend), \
.idProduct = (prod), \
.bcdDevice_lo = (lo), \
+ .bcdDevice_hi = (hi)
+/**
+ * USB_DEVICE_VER_NO_AUTO - describe a specific usb device with a version range
+ * @vend: the 16 bit USB Vendor ID
+ * @prod: the 16 bit USB Product ID
+ * @lo: the bcdDevice_lo value
+ * @hi: the bcdDevice_hi value
+ *
+ * This macro is used to create a struct usb_device_id that matches a
+ * specific device, with a version range, but it is meant for manual binding
+ * only, not automatic binding.
+ */
+#define USB_DEVICE_VER_NO_AUTO(vend, prod, lo, hi) \
+ .match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION_NO_AUTO, \
+ .idVendor = (vend), \
+ .idProduct = (prod), \
+ .bcdDevice_lo = (lo), \
.bcdDevice_hi = (hi)
/**
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-29 15:41 ` Alan Stern
@ 2019-08-30 0:31 ` Julius Werner
2019-08-30 17:43 ` Alan Stern
0 siblings, 1 reply; 19+ messages in thread
From: Julius Werner @ 2019-08-30 0:31 UTC (permalink / raw)
To: Alan Stern, Greg Kroah-Hartman
Cc: Julius Werner, Dan Williams, Kernel development list, USB list,
USB Storage list
> USB drivers only bind to interfaces, are you saying that your device has
> multiple interfaces on it?
Yes, I have a case where the device has two interfaces which both have
interface class 0xff (although they do differ in subclass and
protocol). I only want the usb-storage driver to bind to one of them
(if it binds to the other it will eventually cause a timeout error
that resets the whole device).
I tried doing a userspace mode switch and using
/sys/bus/usb/drivers/usb-storage/new_id to bind the device now, which
*almost* works, but I can't prevent it from binding to both
interfaces. Unfortunately new_id can only take an interface class, not
a subclass or protocol.
> In fact, there already is a way to do this in the kernel: write to the
> sysfs "bind" file. The difficulty is that you can't force a driver to
> bind to an interface if it doesn't believe it is compatible with the
> interface. And if the driver believes it is compatible, it will
> automatically attempt to bind with all such interfaces regardless of
> their path.
>
> Perhaps what you need is a usb_device_id flag to indicate that the
> entry should never be used for automatic matches -- only for matches
> made by the user via the "bind" file. Greg KH would probably be
> willing to accept a new USB_DEVICE_ID_MATCH_NO_AUTO flag, which
> could be included in your unusual_devs.h entries.
This is an interesting idea, but I don't quite see how it can work as
you described? When I write to 'bind', the driver core calls
driver_match_device(), which ends up calling usb_device_match()
(right?), which is the same path that it would call for automatic
matching. It still ends up in usb_match_one_id(), and if I check for
the NO_AUTO flag there it would abort just as if it was an auto-match
attempt. I see no way to pass the information that this is an
explicit, user-requested "bind" rather than an automatic match across
the bus->match() callback into the USB code. (I could change the
signature of the match() callback, but that would require changing
code for all device busses in Linux, which I assume is something we
wouldn't want to do? I could also add a flag to struct device to
communicate "this is currently trying to match for a user-initiated
bind", but that seems hacky and not really the right place to put
that.)
I could instead add a new sysfs node 'force_bind' to the driver core,
that would work like 'bind' except for skipping the
driver_match_device() call entirely and forcing a probe(). Do you
think that would be acceptable? Or is that too big of a hammer to make
available for all drivers in Linux? Maybe if I do the same thing but
only for usb drivers, or even only for the usb-storage driver
specifically, would that be acceptable?
If none of this works, I could also extend the new_id interface to
allow subclass/protocol matches instead. I don't like that as much
because it doesn't allow me to control the devpath of the device I'm
matching, but I think it would be enough for my use case (I can't make
the usb-storage driver bind all AOA devices at all times, but at the
times where I do want to use it for my one device, I don't expect any
other AOA devices to be connected). The problem with this is that the
order of arguments for new ID is already set in stone (vendor,
product, interface class, refVendor, refProduct), and I don't think I
can use the refVendor/refProduct for my case so I can't just append
more numbers behind that. I could maybe instead change it so that it
also accepts a key-value style line (like "idVendor=abcd
idProduct=efgh bInterfaceSubClass=ff"), while still being
backwards-compatible to the old format if you only give it numbers?
What do you think?
Thanks for your advice!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-30 0:31 ` Julius Werner
@ 2019-08-30 17:43 ` Alan Stern
2019-09-02 16:47 ` Greg KH
0 siblings, 1 reply; 19+ messages in thread
From: Alan Stern @ 2019-08-30 17:43 UTC (permalink / raw)
To: Julius Werner, Greg KH
Cc: Dan Williams, Kernel development list, USB list, USB Storage list
On Thu, 29 Aug 2019, Julius Werner wrote:
> > In fact, there already is a way to do this in the kernel: write to the
> > sysfs "bind" file. The difficulty is that you can't force a driver to
> > bind to an interface if it doesn't believe it is compatible with the
> > interface. And if the driver believes it is compatible, it will
> > automatically attempt to bind with all such interfaces regardless of
> > their path.
> >
> > Perhaps what you need is a usb_device_id flag to indicate that the
> > entry should never be used for automatic matches -- only for matches
> > made by the user via the "bind" file. Greg KH would probably be
> > willing to accept a new USB_DEVICE_ID_MATCH_NO_AUTO flag, which
> > could be included in your unusual_devs.h entries.
>
> This is an interesting idea, but I don't quite see how it can work as
> you described? When I write to 'bind', the driver core calls
> driver_match_device(), which ends up calling usb_device_match()
> (right?), which is the same path that it would call for automatic
> matching.
Oh, too bad. I had a vague memory that it did not call
driver_match_device().
> It still ends up in usb_match_one_id(), and if I check for
> the NO_AUTO flag there it would abort just as if it was an auto-match
> attempt. I see no way to pass the information that this is an
> explicit, user-requested "bind" rather than an automatic match across
> the bus->match() callback into the USB code. (I could change the
> signature of the match() callback, but that would require changing
> code for all device busses in Linux, which I assume is something we
> wouldn't want to do? I could also add a flag to struct device to
> communicate "this is currently trying to match for a user-initiated
> bind", but that seems hacky and not really the right place to put
> that.)
>
> I could instead add a new sysfs node 'force_bind' to the driver core,
> that would work like 'bind' except for skipping the
> driver_match_device() call entirely and forcing a probe(). Do you
> think that would be acceptable? Or is that too big of a hammer to make
> available for all drivers in Linux? Maybe if I do the same thing but
> only for usb drivers, or even only for the usb-storage driver
> specifically, would that be acceptable?
This is a question for Greg. The problem is that there may be drivers
which can't handle being probed for devices they don't match.
Still, we ought to have a mechanism for doing manual but not automatic
matches.
Greg, any thoughts?
> If none of this works, I could also extend the new_id interface to
> allow subclass/protocol matches instead. I don't like that as much
> because it doesn't allow me to control the devpath of the device I'm
> matching, but I think it would be enough for my use case (I can't make
> the usb-storage driver bind all AOA devices at all times, but at the
> times where I do want to use it for my one device, I don't expect any
> other AOA devices to be connected). The problem with this is that the
> order of arguments for new ID is already set in stone (vendor,
> product, interface class, refVendor, refProduct), and I don't think I
> can use the refVendor/refProduct for my case so I can't just append
> more numbers behind that. I could maybe instead change it so that it
> also accepts a key-value style line (like "idVendor=abcd
> idProduct=efgh bInterfaceSubClass=ff"), while still being
> backwards-compatible to the old format if you only give it numbers?
> What do you think?
I prefer the manual/automatic approach. It allows the user to control
exactly which device will be probed, which could be important.
Alan Stern
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-08-30 17:43 ` Alan Stern
@ 2019-09-02 16:47 ` Greg KH
2019-09-03 8:46 ` Oliver Neukum
0 siblings, 1 reply; 19+ messages in thread
From: Greg KH @ 2019-09-02 16:47 UTC (permalink / raw)
To: Alan Stern
Cc: Julius Werner, Dan Williams, Kernel development list, USB list,
USB Storage list
On Fri, Aug 30, 2019 at 01:43:36PM -0400, Alan Stern wrote:
> > I could instead add a new sysfs node 'force_bind' to the driver core,
> > that would work like 'bind' except for skipping the
> > driver_match_device() call entirely and forcing a probe(). Do you
> > think that would be acceptable? Or is that too big of a hammer to make
> > available for all drivers in Linux? Maybe if I do the same thing but
> > only for usb drivers, or even only for the usb-storage driver
> > specifically, would that be acceptable?
>
> This is a question for Greg. The problem is that there may be drivers
> which can't handle being probed for devices they don't match.
>
> Still, we ought to have a mechanism for doing manual but not automatic
> matches.
>
> Greg, any thoughts?
This should work just fine today. Add a new device id to the "new_id"
file and then tell the driver to bind. That's pretty much the same as a
"force_bind", right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-09-02 16:47 ` Greg KH
@ 2019-09-03 8:46 ` Oliver Neukum
2019-09-03 9:19 ` Greg KH
0 siblings, 1 reply; 19+ messages in thread
From: Oliver Neukum @ 2019-09-03 8:46 UTC (permalink / raw)
To: Greg KH, Alan Stern
Cc: Julius Werner, USB Storage list, Dan Williams,
Kernel development list, USB list
Am Montag, den 02.09.2019, 18:47 +0200 schrieb Greg KH:
>
> This should work just fine today. Add a new device id to the "new_id"
> file and then tell the driver to bind. That's pretty much the same as a
> "force_bind", right?
That looks like a race condition by design to me.
Regards
Oliver
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-09-03 8:46 ` Oliver Neukum
@ 2019-09-03 9:19 ` Greg KH
2019-09-03 10:04 ` Oliver Neukum
2019-09-03 14:14 ` Alan Stern
0 siblings, 2 replies; 19+ messages in thread
From: Greg KH @ 2019-09-03 9:19 UTC (permalink / raw)
To: Oliver Neukum
Cc: Alan Stern, Julius Werner, USB Storage list, Dan Williams,
Kernel development list, USB list
On Tue, Sep 03, 2019 at 10:46:14AM +0200, Oliver Neukum wrote:
> Am Montag, den 02.09.2019, 18:47 +0200 schrieb Greg KH:
> >
> > This should work just fine today. Add a new device id to the "new_id"
> > file and then tell the driver to bind. That's pretty much the same as a
> > "force_bind", right?
>
> That looks like a race condition by design to me.
How?
Anyway, this should all "just work" somehow, there's an old lwn.net
article I wrote about this over a decade ago when it was added. A
number of subsystems use this all the time (vfio?) and I haven't heard
any issues with it in a long time.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-09-03 9:19 ` Greg KH
@ 2019-09-03 10:04 ` Oliver Neukum
2019-09-03 12:45 ` Greg KH
2019-09-03 14:14 ` Alan Stern
1 sibling, 1 reply; 19+ messages in thread
From: Oliver Neukum @ 2019-09-03 10:04 UTC (permalink / raw)
To: Greg KH
Cc: Julius Werner, USB Storage list, Dan Williams, Alan Stern,
Kernel development list, USB list
Am Dienstag, den 03.09.2019, 11:19 +0200 schrieb Greg KH:
> On Tue, Sep 03, 2019 at 10:46:14AM +0200, Oliver Neukum wrote:
> > Am Montag, den 02.09.2019, 18:47 +0200 schrieb Greg KH:
> > >
> > > This should work just fine today. Add a new device id to the "new_id"
> > > file and then tell the driver to bind. That's pretty much the same as a
> > > "force_bind", right?
> >
> > That looks like a race condition by design to me.
>
> How?
You have one of these files and potentially multiple devices
to be bound. You need a locking scheme. As soon as the acts
of specifying and binding are distinct.
Regards
Oliver
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-09-03 10:04 ` Oliver Neukum
@ 2019-09-03 12:45 ` Greg KH
0 siblings, 0 replies; 19+ messages in thread
From: Greg KH @ 2019-09-03 12:45 UTC (permalink / raw)
To: Oliver Neukum
Cc: Julius Werner, USB Storage list, Dan Williams, Alan Stern,
Kernel development list, USB list
On Tue, Sep 03, 2019 at 12:04:03PM +0200, Oliver Neukum wrote:
> Am Dienstag, den 03.09.2019, 11:19 +0200 schrieb Greg KH:
> > On Tue, Sep 03, 2019 at 10:46:14AM +0200, Oliver Neukum wrote:
> > > Am Montag, den 02.09.2019, 18:47 +0200 schrieb Greg KH:
> > > >
> > > > This should work just fine today. Add a new device id to the "new_id"
> > > > file and then tell the driver to bind. That's pretty much the same as a
> > > > "force_bind", right?
> > >
> > > That looks like a race condition by design to me.
> >
> > How?
>
> You have one of these files and potentially multiple devices
> to be bound. You need a locking scheme. As soon as the acts
> of specifying and binding are distinct.
What needs to be locked here?
totally confused,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-09-03 9:19 ` Greg KH
2019-09-03 10:04 ` Oliver Neukum
@ 2019-09-03 14:14 ` Alan Stern
2019-09-06 21:02 ` Julius Werner
1 sibling, 1 reply; 19+ messages in thread
From: Alan Stern @ 2019-09-03 14:14 UTC (permalink / raw)
To: Greg KH
Cc: Oliver Neukum, Julius Werner, USB Storage list, Dan Williams,
Kernel development list, USB list
On Tue, 3 Sep 2019, Greg KH wrote:
> On Tue, Sep 03, 2019 at 10:46:14AM +0200, Oliver Neukum wrote:
> > Am Montag, den 02.09.2019, 18:47 +0200 schrieb Greg KH:
> > >
> > > This should work just fine today. Add a new device id to the "new_id"
> > > file and then tell the driver to bind. That's pretty much the same as a
> > > "force_bind", right?
> >
> > That looks like a race condition by design to me.
>
> How?
>
> Anyway, this should all "just work" somehow, there's an old lwn.net
> article I wrote about this over a decade ago when it was added. A
> number of subsystems use this all the time (vfio?) and I haven't heard
> any issues with it in a long time.
No, this won't "just work". The problem is that when you write to the
new_id file, usb_store_new_id() calls driver_attach(), which tries to
bind the driver to any matching device. There _will_ be other matching
devices, and trying to bind them will cause runtime errors.
That isn't what we want. We want to bind the driver to a _specific_
device and no others, even if the others match the new id.
Now, this is what writing to the sysfs "bind" file does -- except that
it won't let you bind a driver to a device it doesn't match!
So we have two problems:
Bind a driver to a particular device,
Even though the id's for the device don't match the driver.
The kernel already contains solutions for each of these problems, but
nothing that can handle both at once.
Alan Stern
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-09-03 14:14 ` Alan Stern
@ 2019-09-06 21:02 ` Julius Werner
2019-09-07 19:10 ` Alan Stern
0 siblings, 1 reply; 19+ messages in thread
From: Julius Werner @ 2019-09-06 21:02 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, Oliver Neukum, Julius Werner, USB Storage list,
Dan Williams, Kernel development list, USB list
FWIW, I found a suitable workaround now to get my use case working
with existing kernels: I can do the mode switch from userspace, then
after the device reenumerates I can manually disable any interfaces I
don't like by writing 0 to their 'authorized' node, and then I write
the VID/PID to usb-storage/new_id.
I still think it would be nice in general to be able to force a driver
to bind a specific device (rather than a VID/PID match), but it's not
a pressing need for me anymore.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: storage: Add ums-cros-aoa driver
2019-09-06 21:02 ` Julius Werner
@ 2019-09-07 19:10 ` Alan Stern
0 siblings, 0 replies; 19+ messages in thread
From: Alan Stern @ 2019-09-07 19:10 UTC (permalink / raw)
To: Julius Werner
Cc: Greg KH, Oliver Neukum, USB Storage list, Dan Williams,
Kernel development list, USB list
On Fri, 6 Sep 2019, Julius Werner wrote:
> FWIW, I found a suitable workaround now to get my use case working
> with existing kernels: I can do the mode switch from userspace, then
> after the device reenumerates I can manually disable any interfaces I
> don't like by writing 0 to their 'authorized' node, and then I write
> the VID/PID to usb-storage/new_id.
Okay, very good.
> I still think it would be nice in general to be able to force a driver
> to bind a specific device (rather than a VID/PID match), but it's not
> a pressing need for me anymore.
You _can_ do this currently, by writing the name of the interface to
/sys/bus/usb/drivers/usb-storage/bind. But as you know, this doesn't
work unless the VID & PID already match one of the driver's entries.
Alan Stern
^ permalink raw reply [flat|nested] 19+ messages in thread