Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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.
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
> 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!
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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:
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
(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?
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
> > ---
>
> > +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, _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(>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(>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(>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
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
> ---
> +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, _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(>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(>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(>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", _aoa_ids[0].idVendor,
> + _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(_aoa_stub_driver);
As Greg pointed
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
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
> ---
> 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 0..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 )
> + *
> + * 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
> +#include
> +#include
> +
> +#include "initializers.h"
> +
> +#define DRV_NAME "ums-cros-aoa"
KBUILD_MODNAME?
> +
> +MODULE_DESCRIPTION("Driver for Chrome OS Recovery via Android Open
> Accessory");
>
