[PATCH v2 00/11] USB: new USB control message helper functions
In a recent discussion about a USB networking bug found by syzbot, and fixed by Himadri Pandya, the design of the existing usb_control_msg() call was brought up as not being the "nicest" thing to use by Dmitry Vyukov: https://lore.kernel.org/r/CACT4Y+YbDODLRFn8M5QcY4CazhpeCaunJnP_udXtAs0rYoASS...
The function makes it hard to get right, in that it will return the number of bytes sent/received, but almost no one checks to see if a short read/write happens. With a malicious, or broken, USB device, this can cause drivers to act on data that they did not anticipate, and sometimes copy internal kernel data out to userspace.
So let's fix this up by creating two new functions, usb_control_msg_send() and usb_control_msg_recv(). These functions either complete the full transation, or they return an error, a short send/recv is now an error.
They also accept data off of the stack, saving individual drivers the pain of having to constantly allocate memory on their own for tiny messages, thereby saving overall kernel code space.
The api also does not require a raw USB "pipe" to be sent to the function, as we know the direction, so just pass in the endpoint number instead, again making it easier on the USB driver author to use.
This series first takes a helper function out of the sound core for verifying USB endpoints to be able to use internally, and then adds the new functions, converts over some internal USB code to use them, and then starts to clean up some drivers using these new functions, as an example of the savings that can happen by using these functions.
Thanks to Dmitry and Himadri for the idea on how to do all of this.
greg k-h
Changes from v1: - added acks from Takashi Iwai - dropped changes to one function in patch 04 thanks to review from Alan Stern - typo fix in comment in patch 01 - added new patch 11 to remove some unneeded checks in the sound drivers for endpoint statuses that would always be true.
*** BLURB HERE ***
Greg Kroah-Hartman (11): USB: move snd_usb_pipe_sanity_check into the USB core USB: add usb_control_msg_send() and usb_control_msg_recv() USB: core: message.c: use usb_control_msg_send() in a few places USB: core: hub.c: use usb_control_msg_send() in a few places USB: legousbtower: use usb_control_msg_recv() sound: usx2y: move to use usb_control_msg_send() sound: 6fire: move to use usb_control_msg_send() and usb_control_msg_recv() sound: line6: move to use usb_control_msg_send() and usb_control_msg_recv() sound: hiface: move to use usb_control_msg_send() Bluetooth: ath3k: use usb_control_msg_send() and usb_control_msg_recv() ALSA: remove calls to usb_pipe_type_check for control endpoints
drivers/bluetooth/ath3k.c | 90 +++++------------ drivers/usb/core/hub.c | 99 ++++++++---------- drivers/usb/core/message.c | 171 ++++++++++++++++++++++++++++---- drivers/usb/core/urb.c | 31 ++++-- drivers/usb/misc/legousbtower.c | 60 ++++------- include/linux/usb.h | 7 ++ sound/usb/6fire/firmware.c | 38 +++---- sound/usb/helper.c | 16 +-- sound/usb/helper.h | 1 - sound/usb/hiface/pcm.c | 14 ++- sound/usb/line6/driver.c | 69 +++++-------- sound/usb/line6/podhd.c | 17 ++-- sound/usb/line6/toneport.c | 8 +- sound/usb/mixer_scarlett_gen2.c | 2 +- sound/usb/quirks.c | 12 +-- sound/usb/usx2y/us122l.c | 42 ++------ 16 files changed, 335 insertions(+), 342 deletions(-)
snd_usb_pipe_sanity_check() is a great function, so let's move it into the USB core so that other parts of the kernel, including the USB core, can call it.
Name it usb_pipe_type_check() to match the existing usb_urb_ep_type_check() call, which now uses this function.
Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: "Gustavo A. R. Silva" gustavoars@kernel.org Cc: Eli Billauer eli.billauer@gmail.com Cc: Emiliano Ingrassia ingrassia@epigenesys.com Cc: Alan Stern stern@rowland.harvard.edu Cc: Alexander Tsoy alexander@tsoy.me Cc: "Geoffrey D. Bennett" g@b4.vu Cc: Jussi Laako jussi@sonarnerd.net Cc: Nick Kossifidis mickflemm@gmail.com Cc: Dmitry Panchenko dmitry@d-systems.ee Cc: Chris Wulff crwulff@gmail.com Cc: Jesus Ramos jesus-ramos@live.com Cc: linux-usb@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: alsa-devel@alsa-project.org Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - use usb_pipe_type_check() instead of usb_urb_ep_type_check in urb.c - fix typo in function description - both changes thanks to Alan Stern's review comments - added Takashi Iwai's reviewed-by
drivers/usb/core/urb.c | 31 +++++++++++++++++++++++-------- include/linux/usb.h | 1 + sound/usb/helper.c | 16 +--------------- sound/usb/helper.h | 1 - sound/usb/mixer_scarlett_gen2.c | 2 +- sound/usb/quirks.c | 12 ++++++------ 6 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 27e83e55a590..357b149b20d3 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -192,24 +192,39 @@ static const int pipetypes[4] = { };
/** - * usb_urb_ep_type_check - sanity check of endpoint in the given urb - * @urb: urb to be checked + * usb_pipe_type_check - sanity check of a specific pipe for a usb device + * @dev: struct usb_device to be checked + * @pipe: pipe to check * * This performs a light-weight sanity check for the endpoint in the - * given urb. It returns 0 if the urb contains a valid endpoint, otherwise - * a negative error code. + * given usb device. It returns 0 if the pipe is valid for the specific usb + * device, otherwise a negative error code. */ -int usb_urb_ep_type_check(const struct urb *urb) +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe) { const struct usb_host_endpoint *ep;
- ep = usb_pipe_endpoint(urb->dev, urb->pipe); + ep = usb_pipe_endpoint(dev, pipe); if (!ep) return -EINVAL; - if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) + if (usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) return -EINVAL; return 0; } +EXPORT_SYMBOL_GPL(usb_pipe_type_check); + +/** + * usb_urb_ep_type_check - sanity check of endpoint in the given urb + * @urb: urb to be checked + * + * This performs a light-weight sanity check for the endpoint in the + * given urb. It returns 0 if the urb contains a valid endpoint, otherwise + * a negative error code. + */ +int usb_urb_ep_type_check(const struct urb *urb) +{ + return usb_pipe_type_check(urb->dev, urb->pipe); +} EXPORT_SYMBOL_GPL(usb_urb_ep_type_check);
/** @@ -474,7 +489,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) */
/* Check that the pipe's type matches the endpoint's type */ - if (usb_urb_ep_type_check(urb)) + if (usb_pipe_type_check(urb->dev, urb->pipe)) dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n", usb_pipetype(urb->pipe), pipetypes[xfertype]);
diff --git a/include/linux/usb.h b/include/linux/usb.h index 20c555db4621..0b3963d7ec38 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1764,6 +1764,7 @@ static inline int usb_urb_dir_out(struct urb *urb) return (urb->transfer_flags & URB_DIR_MASK) == URB_DIR_OUT; }
+int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe); int usb_urb_ep_type_check(const struct urb *urb);
void *usb_alloc_coherent(struct usb_device *dev, size_t size, diff --git a/sound/usb/helper.c b/sound/usb/helper.c index 4c12cc5b53fd..cf92d7110773 100644 --- a/sound/usb/helper.c +++ b/sound/usb/helper.c @@ -63,20 +63,6 @@ void *snd_usb_find_csint_desc(void *buffer, int buflen, void *after, u8 dsubtype return NULL; }
-/* check the validity of pipe and EP types */ -int snd_usb_pipe_sanity_check(struct usb_device *dev, unsigned int pipe) -{ - static const int pipetypes[4] = { - PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT - }; - struct usb_host_endpoint *ep; - - ep = usb_pipe_endpoint(dev, pipe); - if (!ep || usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) - return -EINVAL; - return 0; -} - /* * Wrapper for usb_control_msg(). * Allocates a temp buffer to prevent dmaing from/to the stack. @@ -89,7 +75,7 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request, void *buf = NULL; int timeout;
- if (snd_usb_pipe_sanity_check(dev, pipe)) + if (usb_pipe_type_check(dev, pipe)) return -EINVAL;
if (size > 0) { diff --git a/sound/usb/helper.h b/sound/usb/helper.h index 5e8a18b4e7b9..f5b4c6647e4d 100644 --- a/sound/usb/helper.h +++ b/sound/usb/helper.h @@ -7,7 +7,6 @@ unsigned int snd_usb_combine_bytes(unsigned char *bytes, int size); void *snd_usb_find_desc(void *descstart, int desclen, void *after, u8 dtype); void *snd_usb_find_csint_desc(void *descstart, int desclen, void *after, u8 dsubtype);
-int snd_usb_pipe_sanity_check(struct usb_device *dev, unsigned int pipe); int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request, __u8 requesttype, __u16 value, __u16 index, void *data, __u16 size); diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index 0ffff7640892..9609c6d9655c 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1978,7 +1978,7 @@ static int scarlett2_mixer_status_create(struct usb_mixer_interface *mixer) return 0; }
- if (snd_usb_pipe_sanity_check(dev, pipe)) + if (usb_pipe_type_check(dev, pipe)) return -EINVAL;
mixer->urb = usb_alloc_urb(0, GFP_KERNEL); diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index abf99b814a0f..fc3aab04a0bc 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -846,7 +846,7 @@ static int snd_usb_accessmusic_boot_quirk(struct usb_device *dev) static const u8 seq[] = { 0x4e, 0x73, 0x52, 0x01 }; void *buf;
- if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x05))) + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x05))) return -EINVAL; buf = kmemdup(seq, ARRAY_SIZE(seq), GFP_KERNEL); if (!buf) @@ -875,7 +875,7 @@ static int snd_usb_nativeinstruments_boot_quirk(struct usb_device *dev) { int ret;
- if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) return -EINVAL; ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), 0xaf, USB_TYPE_VENDOR | USB_RECIP_DEVICE, @@ -984,7 +984,7 @@ static int snd_usb_axefx3_boot_quirk(struct usb_device *dev)
dev_dbg(&dev->dev, "Waiting for Axe-Fx III to boot up...\n");
- if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) return -EINVAL; /* If the Axe-Fx III has not fully booted, it will timeout when trying * to enable the audio streaming interface. A more generous timeout is @@ -1018,7 +1018,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf, { int err, actual_length;
- if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x01))) + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x01))) return -EINVAL; err = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x01), buf, *length, &actual_length, 1000); @@ -1030,7 +1030,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf,
memset(buf, 0, buf_size);
- if (snd_usb_pipe_sanity_check(dev, usb_rcvintpipe(dev, 0x82))) + if (usb_pipe_type_check(dev, usb_rcvintpipe(dev, 0x82))) return -EINVAL; err = usb_interrupt_msg(dev, usb_rcvintpipe(dev, 0x82), buf, buf_size, &actual_length, 1000); @@ -1117,7 +1117,7 @@ static int snd_usb_motu_m_series_boot_quirk(struct usb_device *dev) { int ret;
- if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) return -EINVAL; ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), 1, USB_TYPE_VENDOR | USB_RECIP_DEVICE,
New core functions to make sending/receiving USB control messages easier and saner.
In discussions, it turns out that the large majority of users of usb_control_msg() do so in potentially incorrect ways. The most common issue is where a "short" message is received, yet never detected properly due to "incorrect" error handling.
Handle all of this in the USB core with two new functions to try to make working with USB control messages simpler.
No more need for dynamic data, messages can be on the stack, and only "complete" send/receive will work without causing an error.
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - no change from v1
drivers/usb/core/message.c | 133 +++++++++++++++++++++++++++++++++++++ include/linux/usb.h | 6 ++ 2 files changed, 139 insertions(+)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 6197938dcc2d..6aa49b237717 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -162,6 +162,139 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, __u8 request, } EXPORT_SYMBOL_GPL(usb_control_msg);
+/** + * usb_control_msg_send - Builds a control "send" message, sends it off and waits for completion + * @dev: pointer to the usb device to send the message to + * @endpoint: endpoint to send the message to + * @request: USB message request value + * @requesttype: USB message request type value + * @value: USB message value + * @index: USB message index value + * @driver_data: pointer to the data to send + * @size: length in bytes of the data to send + * @timeout: time in msecs to wait for the message to complete before timing + * out (if 0 the wait is forever) + * + * Context: !in_interrupt () + * + * This function sends a control message to a specified endpoint that is not + * expected to fill in a response (i.e. a "send message") and waits for the + * message to complete, or timeout. + * + * Do not use this function from within an interrupt context. If you need + * an asynchronous message, or need to send a message from within interrupt + * context, use usb_submit_urb(). If a thread in your driver uses this call, + * make sure your disconnect() method can wait for it to complete. Since you + * don't have a handle on the URB used, you can't cancel the request. + * + * The data pointer can be made to a reference on the stack, or anywhere else, + * as it will not be modified at all. This does not have the restriction that + * usb_control_msg() has where the data pointer must be to dynamically allocated + * memory (i.e. memory that can be successfully DMAed to a device). + * + * Return: If successful, 0 is returned, Otherwise, a negative error number. + */ +int usb_control_msg_send(struct usb_device *dev, __u8 endpoint, __u8 request, + __u8 requesttype, __u16 value, __u16 index, + const void *driver_data, __u16 size, int timeout) +{ + unsigned int pipe = usb_sndctrlpipe(dev, endpoint); + int ret; + u8 *data = NULL; + + if (usb_pipe_type_check(dev, pipe)) + return -EINVAL; + + if (size) { + data = kmemdup(driver_data, size, GFP_KERNEL); + if (!data) + return -ENOMEM; + } + + ret = usb_control_msg(dev, pipe, request, requesttype, value, index, + data, size, timeout); + kfree(data); + + if (ret < 0) + return ret; + if (ret == size) + return 0; + return -EINVAL; +} +EXPORT_SYMBOL_GPL(usb_control_msg_send); + +/** + * usb_control_msg_recv - Builds a control "receive" message, sends it off and waits for completion + * @dev: pointer to the usb device to send the message to + * @endpoint: endpoint to send the message to + * @request: USB message request value + * @requesttype: USB message request type value + * @value: USB message value + * @index: USB message index value + * @driver_data: pointer to the data to be filled in by the message + * @size: length in bytes of the data to be received + * @timeout: time in msecs to wait for the message to complete before timing + * out (if 0 the wait is forever) + * + * Context: !in_interrupt () + * + * This function sends a control message to a specified endpoint that is + * expected to fill in a response (i.e. a "receive message") and waits for the + * message to complete, or timeout. + * + * Do not use this function from within an interrupt context. If you need + * an asynchronous message, or need to send a message from within interrupt + * context, use usb_submit_urb(). If a thread in your driver uses this call, + * make sure your disconnect() method can wait for it to complete. Since you + * don't have a handle on the URB used, you can't cancel the request. + * + * The data pointer can be made to a reference on the stack, or anywhere else + * that can be successfully written to. This function does not have the + * restriction that usb_control_msg() has where the data pointer must be to + * dynamically allocated memory (i.e. memory that can be successfully DMAed to a + * device). + * + * The "whole" message must be properly received from the device in order for + * this function to be successful. If a device returns less than the expected + * amount of data, then the function will fail. Do not use this for messages + * where a variable amount of data might be returned. + * + * Return: If successful, 0 is returned, Otherwise, a negative error number. + */ +int usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, __u8 request, + __u8 requesttype, __u16 value, __u16 index, + void *driver_data, __u16 size, int timeout) +{ + unsigned int pipe = usb_rcvctrlpipe(dev, endpoint); + int ret; + u8 *data; + + if (!size || !driver_data || usb_pipe_type_check(dev, pipe)) + return -EINVAL; + + data = kmalloc(size, GFP_KERNEL); + if (!data) + return -ENOMEM; + + ret = usb_control_msg(dev, pipe, request, requesttype, value, index, + data, size, timeout); + + if (ret < 0) + goto exit; + + if (ret == size) { + memcpy(driver_data, data, size); + ret = 0; + } else { + ret = -EINVAL; + } + +exit: + kfree(data); + return ret; +} +EXPORT_SYMBOL_GPL(usb_control_msg_recv); + /** * usb_interrupt_msg - Builds an interrupt urb, sends it off and waits for completion * @usb_dev: pointer to the usb device to send the message to diff --git a/include/linux/usb.h b/include/linux/usb.h index 0b3963d7ec38..a5460f08126e 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1802,6 +1802,12 @@ extern int usb_bulk_msg(struct usb_device *usb_dev, unsigned int pipe, int timeout);
/* wrappers around usb_control_msg() for the most common standard requests */ +int usb_control_msg_send(struct usb_device *dev, __u8 endpoint, __u8 request, + __u8 requesttype, __u16 value, __u16 index, + const void *data, __u16 size, int timeout); +int usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, __u8 request, + __u8 requesttype, __u16 value, __u16 index, + void *data, __u16 size, int timeout); extern int usb_get_descriptor(struct usb_device *dev, unsigned char desctype, unsigned char descindex, void *buf, int size); extern int usb_get_status(struct usb_device *dev,
There are a few calls to usb_control_msg() that can be converted to use usb_control_msg_send() instead, so do that in order to make the error checking a bit simpler.
Cc: Alan Stern stern@rowland.harvard.edu Cc: "Rafael J. Wysocki" rafael.j.wysocki@intel.com Cc: Andy Shevchenko andriy.shevchenko@linux.intel.com Cc: linux-usb@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - no change from v1
drivers/usb/core/message.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 6aa49b237717..dfd079485c76 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -1081,7 +1081,7 @@ int usb_set_isoch_delay(struct usb_device *dev) if (dev->speed < USB_SPEED_SUPER) return 0;
- return usb_control_msg(dev, usb_sndctrlpipe(dev, 0), + return usb_control_msg_send(dev, 0, USB_REQ_SET_ISOCH_DELAY, USB_DIR_OUT | USB_TYPE_STANDARD | USB_RECIP_DEVICE, dev->hub_delay, 0, NULL, 0, @@ -1203,13 +1203,13 @@ int usb_clear_halt(struct usb_device *dev, int pipe) * (like some ibmcam model 1 units) seem to expect hosts to make * this request for iso endpoints, which can't halt! */ - result = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), - USB_REQ_CLEAR_FEATURE, USB_RECIP_ENDPOINT, - USB_ENDPOINT_HALT, endp, NULL, 0, - USB_CTRL_SET_TIMEOUT); + result = usb_control_msg_send(dev, 0, + USB_REQ_CLEAR_FEATURE, USB_RECIP_ENDPOINT, + USB_ENDPOINT_HALT, endp, NULL, 0, + USB_CTRL_SET_TIMEOUT);
/* don't un-halt or force to DATA0 except on success */ - if (result < 0) + if (result) return result;
/* NOTE: seems like Microsoft and Apple don't bother verifying @@ -1558,9 +1558,10 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) if (dev->quirks & USB_QUIRK_NO_SET_INTF) ret = -EPIPE; else - ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), - USB_REQ_SET_INTERFACE, USB_RECIP_INTERFACE, - alternate, interface, NULL, 0, 5000); + ret = usb_control_msg_send(dev, 0, + USB_REQ_SET_INTERFACE, + USB_RECIP_INTERFACE, alternate, + interface, NULL, 0, 5000);
/* 9.4.10 says devices don't need this and are free to STALL the * request if the interface only has one alternate setting. @@ -1570,7 +1571,7 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) "manual set_interface for iface %d, alt %d\n", interface, alternate); manual = 1; - } else if (ret < 0) { + } else if (ret) { /* Re-instate the old alt setting */ usb_hcd_alloc_bandwidth(dev, NULL, alt, iface->cur_altsetting); usb_enable_lpm(dev); @@ -1718,11 +1719,10 @@ int usb_reset_configuration(struct usb_device *dev) mutex_unlock(hcd->bandwidth_mutex); return retval; } - retval = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), - USB_REQ_SET_CONFIGURATION, 0, - config->desc.bConfigurationValue, 0, - NULL, 0, USB_CTRL_SET_TIMEOUT); - if (retval < 0) + retval = usb_control_msg_send(dev, 0, USB_REQ_SET_CONFIGURATION, 0, + config->desc.bConfigurationValue, 0, + NULL, 0, USB_CTRL_SET_TIMEOUT); + if (retval) goto reset_old_alts; mutex_unlock(hcd->bandwidth_mutex);
@@ -2103,10 +2103,10 @@ int usb_set_configuration(struct usb_device *dev, int configuration) } kfree(new_interfaces);
- ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), - USB_REQ_SET_CONFIGURATION, 0, configuration, 0, - NULL, 0, USB_CTRL_SET_TIMEOUT); - if (ret < 0 && cp) { + ret = usb_control_msg_send(dev, 0, USB_REQ_SET_CONFIGURATION, 0, + configuration, 0, NULL, 0, + USB_CTRL_SET_TIMEOUT); + if (ret && cp) { /* * All the old state is gone, so what else can we do? * The device is probably useless now anyway.
On Mon, Sep 07, 2020 at 04:51:00PM +0200, Greg Kroah-Hartman wrote:
There are a few calls to usb_control_msg() that can be converted to use usb_control_msg_send() instead, so do that in order to make the error checking a bit simpler.
Cc: Alan Stern stern@rowland.harvard.edu Cc: "Rafael J. Wysocki" rafael.j.wysocki@intel.com Cc: Andy Shevchenko andriy.shevchenko@linux.intel.com Cc: linux-usb@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Oops, Andy, sorry, you gave me a Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com on the previous version of this, I'll add it next round, or when it's queued up.
thanks,
greg k-h
On Mon, Sep 07, 2020 at 04:56:44PM +0200, Greg Kroah-Hartman wrote:
On Mon, Sep 07, 2020 at 04:51:00PM +0200, Greg Kroah-Hartman wrote:
There are a few calls to usb_control_msg() that can be converted to use usb_control_msg_send() instead, so do that in order to make the error checking a bit simpler.
Cc: Alan Stern stern@rowland.harvard.edu Cc: "Rafael J. Wysocki" rafael.j.wysocki@intel.com Cc: Andy Shevchenko andriy.shevchenko@linux.intel.com Cc: linux-usb@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Oops, Andy, sorry, you gave me a Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com on the previous version of this, I'll add it next round, or when it's queued up.
NP! Whatever works better for you.
There are a few calls to usb_control_msg() that can be converted to use usb_control_msg_send() instead, so do that in order to make the error checking a bit simpler and the code smaller.
Cc: Alan Stern stern@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - dropped changes to usb_req_set_sel() thanks to review from Alan
drivers/usb/core/hub.c | 99 +++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 60 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 5b768b80d1ee..bbb41993d4ba 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -410,8 +410,8 @@ static int get_hub_descriptor(struct usb_device *hdev, */ static int clear_hub_feature(struct usb_device *hdev, int feature) { - return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), - USB_REQ_CLEAR_FEATURE, USB_RT_HUB, feature, 0, NULL, 0, 1000); + return usb_control_msg_send(hdev, 0, USB_REQ_CLEAR_FEATURE, USB_RT_HUB, + feature, 0, NULL, 0, 1000); }
/* @@ -419,9 +419,8 @@ static int clear_hub_feature(struct usb_device *hdev, int feature) */ int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature) { - return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), - USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1, - NULL, 0, 1000); + return usb_control_msg_send(hdev, 0, USB_REQ_CLEAR_FEATURE, USB_RT_PORT, + feature, port1, NULL, 0, 1000); }
/* @@ -429,9 +428,8 @@ int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature) */ static int set_port_feature(struct usb_device *hdev, int port1, int feature) { - return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), - USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1, - NULL, 0, 1000); + return usb_control_msg_send(hdev, 0, USB_REQ_SET_FEATURE, USB_RT_PORT, + feature, port1, NULL, 0, 1000); }
static char *to_led_name(int selector) @@ -755,15 +753,14 @@ hub_clear_tt_buffer(struct usb_device *hdev, u16 devinfo, u16 tt) /* Need to clear both directions for control ep */ if (((devinfo >> 11) & USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_CONTROL) { - int status = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), - HUB_CLEAR_TT_BUFFER, USB_RT_PORT, - devinfo ^ 0x8000, tt, NULL, 0, 1000); + int status = usb_control_msg_send(hdev, 0, + HUB_CLEAR_TT_BUFFER, USB_RT_PORT, + devinfo ^ 0x8000, tt, NULL, 0, 1000); if (status) return status; } - return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), - HUB_CLEAR_TT_BUFFER, USB_RT_PORT, devinfo, - tt, NULL, 0, 1000); + return usb_control_msg_send(hdev, 0, HUB_CLEAR_TT_BUFFER, USB_RT_PORT, + devinfo, tt, NULL, 0, 1000); }
/* @@ -1049,11 +1046,10 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) */ if (type != HUB_RESUME) { if (hdev->parent && hub_is_superspeed(hdev)) { - ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), - HUB_SET_DEPTH, USB_RT_HUB, - hdev->level - 1, 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); - if (ret < 0) + ret = usb_control_msg_send(hdev, 0, HUB_SET_DEPTH, USB_RT_HUB, + hdev->level - 1, 0, NULL, 0, + USB_CTRL_SET_TIMEOUT); + if (ret) dev_err(hub->intfdev, "set hub depth failed\n"); } @@ -2329,13 +2325,10 @@ static int usb_enumerate_device_otg(struct usb_device *udev) /* enable HNP before suspend, it's simpler */ if (port1 == bus->otg_port) { bus->b_hnp_enable = 1; - err = usb_control_msg(udev, - usb_sndctrlpipe(udev, 0), - USB_REQ_SET_FEATURE, 0, - USB_DEVICE_B_HNP_ENABLE, - 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); - if (err < 0) { + err = usb_control_msg_send(udev, 0, USB_REQ_SET_FEATURE, 0, + USB_DEVICE_B_HNP_ENABLE, 0, + NULL, 0, USB_CTRL_SET_TIMEOUT); + if (err) { /* * OTG MESSAGE: report errors here, * customize to match your product. @@ -2347,13 +2340,10 @@ static int usb_enumerate_device_otg(struct usb_device *udev) } else if (desc->bLength == sizeof (struct usb_otg_descriptor)) { /* Set a_alt_hnp_support for legacy otg device */ - err = usb_control_msg(udev, - usb_sndctrlpipe(udev, 0), - USB_REQ_SET_FEATURE, 0, - USB_DEVICE_A_ALT_HNP_SUPPORT, - 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); - if (err < 0) + err = usb_control_msg_send(udev, 0, USB_REQ_SET_FEATURE, 0, + USB_DEVICE_A_ALT_HNP_SUPPORT, 0, + NULL, 0, USB_CTRL_SET_TIMEOUT); + if (err) dev_err(&udev->dev, "set a_alt_hnp_support failed: %d\n", err); @@ -3121,10 +3111,8 @@ int usb_disable_ltm(struct usb_device *udev) if (!udev->actconfig) return 0;
- return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), - USB_REQ_CLEAR_FEATURE, USB_RECIP_DEVICE, - USB_DEVICE_LTM_ENABLE, 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); + return usb_control_msg_send(udev, 0, USB_REQ_CLEAR_FEATURE, USB_RECIP_DEVICE, + USB_DEVICE_LTM_ENABLE, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); } EXPORT_SYMBOL_GPL(usb_disable_ltm);
@@ -3143,10 +3131,8 @@ void usb_enable_ltm(struct usb_device *udev) if (!udev->actconfig) return;
- usb_control_msg(udev, usb_sndctrlpipe(udev, 0), - USB_REQ_SET_FEATURE, USB_RECIP_DEVICE, - USB_DEVICE_LTM_ENABLE, 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); + usb_control_msg_send(udev, 0, USB_REQ_SET_FEATURE, USB_RECIP_DEVICE, + USB_DEVICE_LTM_ENABLE, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); } EXPORT_SYMBOL_GPL(usb_enable_ltm);
@@ -3163,17 +3149,14 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm); static int usb_enable_remote_wakeup(struct usb_device *udev) { if (udev->speed < USB_SPEED_SUPER) - return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), - USB_REQ_SET_FEATURE, USB_RECIP_DEVICE, - USB_DEVICE_REMOTE_WAKEUP, 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); + return usb_control_msg_send(udev, 0, USB_REQ_SET_FEATURE, USB_RECIP_DEVICE, + USB_DEVICE_REMOTE_WAKEUP, 0, + NULL, 0, USB_CTRL_SET_TIMEOUT); else - return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), - USB_REQ_SET_FEATURE, USB_RECIP_INTERFACE, - USB_INTRF_FUNC_SUSPEND, - USB_INTRF_FUNC_SUSPEND_RW | - USB_INTRF_FUNC_SUSPEND_LP, - NULL, 0, USB_CTRL_SET_TIMEOUT); + return usb_control_msg_send(udev, 0, USB_REQ_SET_FEATURE, USB_RECIP_INTERFACE, + USB_INTRF_FUNC_SUSPEND, + USB_INTRF_FUNC_SUSPEND_RW | USB_INTRF_FUNC_SUSPEND_LP, + NULL, 0, USB_CTRL_SET_TIMEOUT); }
/* @@ -3189,15 +3172,11 @@ static int usb_enable_remote_wakeup(struct usb_device *udev) static int usb_disable_remote_wakeup(struct usb_device *udev) { if (udev->speed < USB_SPEED_SUPER) - return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), - USB_REQ_CLEAR_FEATURE, USB_RECIP_DEVICE, - USB_DEVICE_REMOTE_WAKEUP, 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); + return usb_control_msg_send(udev, 0, USB_REQ_CLEAR_FEATURE, USB_RECIP_DEVICE, + USB_DEVICE_REMOTE_WAKEUP, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); else - return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), - USB_REQ_SET_FEATURE, USB_RECIP_INTERFACE, - USB_INTRF_FUNC_SUSPEND, 0, NULL, 0, - USB_CTRL_SET_TIMEOUT); + return usb_control_msg_send(udev, 0, USB_REQ_SET_FEATURE, USB_RECIP_INTERFACE, + USB_INTRF_FUNC_SUSPEND, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); }
/* Count of wakeup-enabled devices at or below udev */ @@ -4056,7 +4035,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev, * associated with the link state we're about to enable. */ ret = usb_req_set_sel(udev, state); - if (ret < 0) { + if (ret) { dev_warn(&udev->dev, "Set SEL for device-initiated %s failed.\n", usb3_lpm_names[state]); return;
On Mon, Sep 07, 2020 at 04:51:01PM +0200, Greg Kroah-Hartman wrote:
There are a few calls to usb_control_msg() that can be converted to use usb_control_msg_send() instead, so do that in order to make the error checking a bit simpler and the code smaller.
Cc: Alan Stern stern@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
v2:
- dropped changes to usb_req_set_sel() thanks to review from Alan
@@ -4056,7 +4035,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev, * associated with the link state we're about to enable. */ ret = usb_req_set_sel(udev, state);
- if (ret < 0) {
- if (ret) { dev_warn(&udev->dev, "Set SEL for device-initiated %s failed.\n", usb3_lpm_names[state]); return;
Did this change survive by mistake?
Actually, it looks like usb_req_set_sel needs to check the value returned by usb_control_msg -- a perfect example of the sort of thing you were trying to fix in the first place!
Alan Stern
On Mon, Sep 07, 2020 at 11:08:58AM -0400, Alan Stern wrote:
On Mon, Sep 07, 2020 at 04:51:01PM +0200, Greg Kroah-Hartman wrote:
There are a few calls to usb_control_msg() that can be converted to use usb_control_msg_send() instead, so do that in order to make the error checking a bit simpler and the code smaller.
Cc: Alan Stern stern@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
v2:
- dropped changes to usb_req_set_sel() thanks to review from Alan
@@ -4056,7 +4035,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev, * associated with the link state we're about to enable. */ ret = usb_req_set_sel(udev, state);
- if (ret < 0) {
- if (ret) { dev_warn(&udev->dev, "Set SEL for device-initiated %s failed.\n", usb3_lpm_names[state]); return;
Did this change survive by mistake?
Actually, it looks like usb_req_set_sel needs to check the value returned by usb_control_msg -- a perfect example of the sort of thing you were trying to fix in the first place!
Ugh, good catch, and yes, the original code is buggy :)
thanks,
greg k-h
The usb_control_msg_recv() function can handle data on the stack, as well as properly detecting short reads, so move to use that function instead of the older usb_control_msg() call. This ends up removing a lot of extra lines in the driver.
Cc: Juergen Stuber starblue@users.sourceforge.net Cc: legousb-devel@lists.sourceforge.net Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - no change from v1
drivers/usb/misc/legousbtower.c | 60 +++++++++++---------------------- 1 file changed, 19 insertions(+), 41 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c index f922544056de..c3583df4c324 100644 --- a/drivers/usb/misc/legousbtower.c +++ b/drivers/usb/misc/legousbtower.c @@ -308,15 +308,9 @@ static int tower_open(struct inode *inode, struct file *file) int subminor; int retval = 0; struct usb_interface *interface; - struct tower_reset_reply *reset_reply; + struct tower_reset_reply reset_reply; int result;
- reset_reply = kmalloc(sizeof(*reset_reply), GFP_KERNEL); - if (!reset_reply) { - retval = -ENOMEM; - goto exit; - } - nonseekable_open(inode, file); subminor = iminor(inode);
@@ -347,15 +341,11 @@ static int tower_open(struct inode *inode, struct file *file) }
/* reset the tower */ - result = usb_control_msg(dev->udev, - usb_rcvctrlpipe(dev->udev, 0), - LEGO_USB_TOWER_REQUEST_RESET, - USB_TYPE_VENDOR | USB_DIR_IN | USB_RECIP_DEVICE, - 0, - 0, - reset_reply, - sizeof(*reset_reply), - 1000); + result = usb_control_msg_recv(dev->udev, 0, + LEGO_USB_TOWER_REQUEST_RESET, + USB_TYPE_VENDOR | USB_DIR_IN | USB_RECIP_DEVICE, + 0, 0, + &reset_reply, sizeof(reset_reply), 1000); if (result < 0) { dev_err(&dev->udev->dev, "LEGO USB Tower reset control request failed\n"); @@ -394,7 +384,6 @@ static int tower_open(struct inode *inode, struct file *file) mutex_unlock(&dev->lock);
exit: - kfree(reset_reply); return retval; }
@@ -753,7 +742,7 @@ static int tower_probe(struct usb_interface *interface, const struct usb_device_ struct device *idev = &interface->dev; struct usb_device *udev = interface_to_usbdev(interface); struct lego_usb_tower *dev; - struct tower_get_version_reply *get_version_reply = NULL; + struct tower_get_version_reply get_version_reply; int retval = -ENOMEM; int result;
@@ -798,34 +787,25 @@ static int tower_probe(struct usb_interface *interface, const struct usb_device_ dev->interrupt_in_interval = interrupt_in_interval ? interrupt_in_interval : dev->interrupt_in_endpoint->bInterval; dev->interrupt_out_interval = interrupt_out_interval ? interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
- get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL); - if (!get_version_reply) { - retval = -ENOMEM; - goto error; - } - /* get the firmware version and log it */ - result = usb_control_msg(udev, - usb_rcvctrlpipe(udev, 0), - LEGO_USB_TOWER_REQUEST_GET_VERSION, - USB_TYPE_VENDOR | USB_DIR_IN | USB_RECIP_DEVICE, - 0, - 0, - get_version_reply, - sizeof(*get_version_reply), - 1000); - if (result != sizeof(*get_version_reply)) { - if (result >= 0) - result = -EIO; + result = usb_control_msg_recv(udev, 0, + LEGO_USB_TOWER_REQUEST_GET_VERSION, + USB_TYPE_VENDOR | USB_DIR_IN | USB_RECIP_DEVICE, + 0, + 0, + &get_version_reply, + sizeof(get_version_reply), + 1000); + if (!result) { dev_err(idev, "get version request failed: %d\n", result); retval = result; goto error; } dev_info(&interface->dev, "LEGO USB Tower firmware version is %d.%d build %d\n", - get_version_reply->major, - get_version_reply->minor, - le16_to_cpu(get_version_reply->build_no)); + get_version_reply.major, + get_version_reply.minor, + le16_to_cpu(get_version_reply.build_no));
/* we can register the device now, as it is ready */ usb_set_intfdata(interface, dev); @@ -844,11 +824,9 @@ static int tower_probe(struct usb_interface *interface, const struct usb_device_ USB_MAJOR, dev->minor);
exit: - kfree(get_version_reply); return retval;
error: - kfree(get_version_reply); tower_delete(dev); return retval; }
The usb_control_msg_send() call can handle data on the stack, as well as returning an error if a "short" write happens, so move the driver over to using that call instead. This ends up removing a helper function that is no longer needed.
Cc: Jaroslav Kysela perex@perex.cz Cc: alsa-devel@alsa-project.org Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - Added reviewed-by from Takashi
sound/usb/usx2y/us122l.c | 42 ++++++++-------------------------------- 1 file changed, 8 insertions(+), 34 deletions(-)
diff --git a/sound/usb/usx2y/us122l.c b/sound/usb/usx2y/us122l.c index f86f7a61fb36..e5c5a0d03d8a 100644 --- a/sound/usb/usx2y/us122l.c +++ b/sound/usb/usx2y/us122l.c @@ -82,40 +82,13 @@ static int us144_create_usbmidi(struct snd_card *card) &US122L(card)->midi_list, &quirk); }
-/* - * Wrapper for usb_control_msg(). - * Allocates a temp buffer to prevent dmaing from/to the stack. - */ -static int us122l_ctl_msg(struct usb_device *dev, unsigned int pipe, - __u8 request, __u8 requesttype, - __u16 value, __u16 index, void *data, - __u16 size, int timeout) -{ - int err; - void *buf = NULL; - - if (size > 0) { - buf = kmemdup(data, size, GFP_KERNEL); - if (!buf) - return -ENOMEM; - } - err = usb_control_msg(dev, pipe, request, requesttype, - value, index, buf, size, timeout); - if (size > 0) { - memcpy(data, buf, size); - kfree(buf); - } - return err; -} - static void pt_info_set(struct usb_device *dev, u8 v) { int ret;
- ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), - 'I', - USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, - v, 0, NULL, 0, 1000); + ret = usb_control_msg_send(dev, 0, 'I', + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + v, 0, NULL, 0, 1000); snd_printdd(KERN_DEBUG "%i\n", ret); }
@@ -305,10 +278,11 @@ static int us122l_set_sample_rate(struct usb_device *dev, int rate) data[0] = rate; data[1] = rate >> 8; data[2] = rate >> 16; - err = us122l_ctl_msg(dev, usb_sndctrlpipe(dev, 0), UAC_SET_CUR, - USB_TYPE_CLASS|USB_RECIP_ENDPOINT|USB_DIR_OUT, - UAC_EP_CS_ATTR_SAMPLE_RATE << 8, ep, data, 3, 1000); - if (err < 0) + err = usb_control_msg_send(dev, 0, UAC_SET_CUR, + USB_TYPE_CLASS|USB_RECIP_ENDPOINT|USB_DIR_OUT, + UAC_EP_CS_ATTR_SAMPLE_RATE << 8, ep, data, 3, + 1000); + if (err) snd_printk(KERN_ERR "%d: cannot set freq %d to ep 0x%x\n", dev->devnum, rate, ep); return err;
The usb_control_msg_send() and usb_control_msg_recv() calls can return an error if a "short" write/read happens, so move the driver over to using those calls instead, saving some logic in the wrapper functions that were being used in this driver.
This also resolves a long-staging bug where data on the stack was being sent in a USB control message, which was not allowed.
Cc: Jaroslav Kysela perex@perex.cz Cc: alsa-devel@alsa-project.org Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - Added reviewed-by from Takashi
sound/usb/6fire/firmware.c | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-)
diff --git a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c index 69137c14d0dc..5b8994070c96 100644 --- a/sound/usb/6fire/firmware.c +++ b/sound/usb/6fire/firmware.c @@ -158,29 +158,17 @@ static int usb6fire_fw_ihex_init(const struct firmware *fw, static int usb6fire_fw_ezusb_write(struct usb_device *device, int type, int value, char *data, int len) { - int ret; - - ret = usb_control_msg(device, usb_sndctrlpipe(device, 0), type, - USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, - value, 0, data, len, HZ); - if (ret < 0) - return ret; - else if (ret != len) - return -EIO; - return 0; + return usb_control_msg_send(device, 0, type, + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + value, 0, data, len, HZ); }
static int usb6fire_fw_ezusb_read(struct usb_device *device, int type, int value, char *data, int len) { - int ret = usb_control_msg(device, usb_rcvctrlpipe(device, 0), type, - USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, value, - 0, data, len, HZ); - if (ret < 0) - return ret; - else if (ret != len) - return -EIO; - return 0; + return usb_control_msg_recv(device, 0, type, + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + value, 0, data, len, HZ); }
static int usb6fire_fw_fpga_write(struct usb_device *device, @@ -230,7 +218,7 @@ static int usb6fire_fw_ezusb_upload( /* upload firmware image */ data = 0x01; /* stop ezusb cpu */ ret = usb6fire_fw_ezusb_write(device, 0xa0, 0xe600, &data, 1); - if (ret < 0) { + if (ret) { kfree(rec); release_firmware(fw); dev_err(&intf->dev, @@ -242,7 +230,7 @@ static int usb6fire_fw_ezusb_upload( while (usb6fire_fw_ihex_next_record(rec)) { /* write firmware */ ret = usb6fire_fw_ezusb_write(device, 0xa0, rec->address, rec->data, rec->len); - if (ret < 0) { + if (ret) { kfree(rec); release_firmware(fw); dev_err(&intf->dev, @@ -257,7 +245,7 @@ static int usb6fire_fw_ezusb_upload( if (postdata) { /* write data after firmware has been uploaded */ ret = usb6fire_fw_ezusb_write(device, 0xa0, postaddr, postdata, postlen); - if (ret < 0) { + if (ret) { dev_err(&intf->dev, "unable to upload ezusb firmware %s: post urb.\n", fwname); @@ -267,7 +255,7 @@ static int usb6fire_fw_ezusb_upload(
data = 0x00; /* resume ezusb cpu */ ret = usb6fire_fw_ezusb_write(device, 0xa0, 0xe600, &data, 1); - if (ret < 0) { + if (ret) { dev_err(&intf->dev, "unable to upload ezusb firmware %s: end message.\n", fwname); @@ -302,7 +290,7 @@ static int usb6fire_fw_fpga_upload( end = fw->data + fw->size;
ret = usb6fire_fw_ezusb_write(device, 8, 0, NULL, 0); - if (ret < 0) { + if (ret) { kfree(buffer); release_firmware(fw); dev_err(&intf->dev, @@ -327,7 +315,7 @@ static int usb6fire_fw_fpga_upload( kfree(buffer);
ret = usb6fire_fw_ezusb_write(device, 9, 0, NULL, 0); - if (ret < 0) { + if (ret) { dev_err(&intf->dev, "unable to upload fpga firmware: end urb.\n"); return ret; @@ -363,7 +351,7 @@ int usb6fire_fw_init(struct usb_interface *intf) u8 buffer[12];
ret = usb6fire_fw_ezusb_read(device, 1, 0, buffer, 8); - if (ret < 0) { + if (ret) { dev_err(&intf->dev, "unable to receive device firmware state.\n"); return ret;
The usb_control_msg_send() and usb_control_msg_recv() calls can return an error if a "short" write/read happens, and they can handle data off of the stack, so move the driver over to using those calls instead, saving some logic when dynamically allocating memory.
Cc: Jaroslav Kysela perex@perex.cz Cc: Vasily Khoruzhick anarsoul@gmail.com Cc: alsa-devel@alsa-project.org Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - Added reviewed-by from Takashi
sound/usb/line6/driver.c | 69 +++++++++++++++----------------------- sound/usb/line6/podhd.c | 17 ++++------ sound/usb/line6/toneport.c | 8 ++--- 3 files changed, 37 insertions(+), 57 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 60674ce4879b..601292c51491 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -337,23 +337,18 @@ int line6_read_data(struct usb_line6 *line6, unsigned address, void *data, { struct usb_device *usbdev = line6->usbdev; int ret; - unsigned char *len; + u8 len; unsigned count;
if (address > 0xffff || datalen > 0xff) return -EINVAL;
- len = kmalloc(1, GFP_KERNEL); - if (!len) - return -ENOMEM; - /* query the serial number: */ - ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67, - USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT, - (datalen << 8) | 0x21, address, - NULL, 0, LINE6_TIMEOUT * HZ); - - if (ret < 0) { + ret = usb_control_msg_send(usbdev, 0, 0x67, + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT, + (datalen << 8) | 0x21, address, NULL, 0, + LINE6_TIMEOUT * HZ); + if (ret) { dev_err(line6->ifcdev, "read request failed (error %d)\n", ret); goto exit; } @@ -362,45 +357,41 @@ int line6_read_data(struct usb_line6 *line6, unsigned address, void *data, for (count = 0; count < LINE6_READ_WRITE_MAX_RETRIES; count++) { mdelay(LINE6_READ_WRITE_STATUS_DELAY);
- ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), 0x67, - USB_TYPE_VENDOR | USB_RECIP_DEVICE | - USB_DIR_IN, - 0x0012, 0x0000, len, 1, - LINE6_TIMEOUT * HZ); - if (ret < 0) { + ret = usb_control_msg_recv(usbdev, 0, 0x67, + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, + 0x0012, 0x0000, &len, 1, + LINE6_TIMEOUT * HZ); + if (ret) { dev_err(line6->ifcdev, "receive length failed (error %d)\n", ret); goto exit; }
- if (*len != 0xff) + if (len != 0xff) break; }
ret = -EIO; - if (*len == 0xff) { + if (len == 0xff) { dev_err(line6->ifcdev, "read failed after %d retries\n", count); goto exit; - } else if (*len != datalen) { + } else if (len != datalen) { /* should be equal or something went wrong */ dev_err(line6->ifcdev, "length mismatch (expected %d, got %d)\n", - (int)datalen, (int)*len); + (int)datalen, len); goto exit; }
/* receive the result: */ - ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), 0x67, - USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, - 0x0013, 0x0000, data, datalen, - LINE6_TIMEOUT * HZ); - - if (ret < 0) + ret = usb_control_msg_recv(usbdev, 0, 0x67, + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, + 0x0013, 0x0000, data, datalen, LINE6_TIMEOUT * HZ); + if (ret) dev_err(line6->ifcdev, "read failed (error %d)\n", ret);
exit: - kfree(len); return ret; } EXPORT_SYMBOL_GPL(line6_read_data); @@ -423,12 +414,10 @@ int line6_write_data(struct usb_line6 *line6, unsigned address, void *data, if (!status) return -ENOMEM;
- ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67, - USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT, - 0x0022, address, data, datalen, - LINE6_TIMEOUT * HZ); - - if (ret < 0) { + ret = usb_control_msg_send(usbdev, 0, 0x67, + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT, + 0x0022, address, data, datalen, LINE6_TIMEOUT * HZ); + if (ret) { dev_err(line6->ifcdev, "write request failed (error %d)\n", ret); goto exit; @@ -437,14 +426,10 @@ int line6_write_data(struct usb_line6 *line6, unsigned address, void *data, for (count = 0; count < LINE6_READ_WRITE_MAX_RETRIES; count++) { mdelay(LINE6_READ_WRITE_STATUS_DELAY);
- ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), - 0x67, - USB_TYPE_VENDOR | USB_RECIP_DEVICE | - USB_DIR_IN, - 0x0012, 0x0000, - status, 1, LINE6_TIMEOUT * HZ); - - if (ret < 0) { + ret = usb_control_msg_recv(usbdev, 0, 0x67, + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, + 0x0012, 0x0000, status, 1, LINE6_TIMEOUT * HZ); + if (ret) { dev_err(line6->ifcdev, "receiving status failed (error %d)\n", ret); goto exit; diff --git a/sound/usb/line6/podhd.c b/sound/usb/line6/podhd.c index eef45f7fef0d..a1261f55d62b 100644 --- a/sound/usb/line6/podhd.c +++ b/sound/usb/line6/podhd.c @@ -183,29 +183,25 @@ static const struct attribute_group podhd_dev_attr_group = { static int podhd_dev_start(struct usb_line6_podhd *pod) { int ret; - u8 *init_bytes; + u8 init_bytes[8]; int i; struct usb_device *usbdev = pod->line6.usbdev;
- init_bytes = kmalloc(8, GFP_KERNEL); - if (!init_bytes) - return -ENOMEM; - - ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), + ret = usb_control_msg_send(usbdev, 0, 0x67, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT, 0x11, 0, NULL, 0, LINE6_TIMEOUT * HZ); - if (ret < 0) { + if (ret) { dev_err(pod->line6.ifcdev, "read request failed (error %d)\n", ret); goto exit; }
/* NOTE: looks like some kind of ping message */ - ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), 0x67, + ret = usb_control_msg_recv(usbdev, 0, 0x67, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, 0x11, 0x0, init_bytes, 3, LINE6_TIMEOUT * HZ); - if (ret < 0) { + if (ret) { dev_err(pod->line6.ifcdev, "receive length failed (error %d)\n", ret); goto exit; @@ -220,13 +216,12 @@ static int podhd_dev_start(struct usb_line6_podhd *pod) goto exit; }
- ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), + ret = usb_control_msg_send(usbdev, 0, USB_REQ_SET_FEATURE, USB_TYPE_STANDARD | USB_RECIP_DEVICE | USB_DIR_OUT, 1, 0, NULL, 0, LINE6_TIMEOUT * HZ); exit: - kfree(init_bytes); return ret; }
diff --git a/sound/usb/line6/toneport.c b/sound/usb/line6/toneport.c index 94dd5e7ab2e6..a9b56085b76a 100644 --- a/sound/usb/line6/toneport.c +++ b/sound/usb/line6/toneport.c @@ -126,11 +126,11 @@ static int toneport_send_cmd(struct usb_device *usbdev, int cmd1, int cmd2) { int ret;
- ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67, - USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT, - cmd1, cmd2, NULL, 0, LINE6_TIMEOUT * HZ); + ret = usb_control_msg_send(usbdev, 0, 0x67, + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT, + cmd1, cmd2, NULL, 0, LINE6_TIMEOUT * HZ);
- if (ret < 0) { + if (ret) { dev_err(&usbdev->dev, "send failed (error %d)\n", ret); return ret; }
The usb_control_msg_send() call can return an error if a "short" write happens, so move the driver over to using that call instead.
Cc: Jaroslav Kysela perex@perex.cz Cc: alsa-devel@alsa-project.org Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - Added reviewed-by from Takashi
sound/usb/hiface/pcm.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c index a148caa5f48e..f9c924e3964e 100644 --- a/sound/usb/hiface/pcm.c +++ b/sound/usb/hiface/pcm.c @@ -156,16 +156,14 @@ static int hiface_pcm_set_rate(struct pcm_runtime *rt, unsigned int rate) * This control message doesn't have any ack from the * other side */ - ret = usb_control_msg(device, usb_sndctrlpipe(device, 0), - HIFACE_SET_RATE_REQUEST, - USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_OTHER, - rate_value, 0, NULL, 0, 100); - if (ret < 0) { + ret = usb_control_msg_send(device, 0, + HIFACE_SET_RATE_REQUEST, + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_OTHER, + rate_value, 0, NULL, 0, 100); + if (ret) dev_err(&device->dev, "Error setting samplerate %d.\n", rate); - return ret; - }
- return 0; + return ret; }
static struct pcm_substream *hiface_pcm_get_substream(struct snd_pcm_substream
The usb_control_msg_send() and usb_control_msg_recv() calls can return an error if a "short" write/read happens, and they can handle data off of the stack, so move the driver over to using those calls instead, saving some logic when dynamically allocating memory.
Cc: Marcel Holtmann marcel@holtmann.org Cc: Johan Hedberg johan.hedberg@gmail.com Cc: linux-bluetooth@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - no change from v1
drivers/bluetooth/ath3k.c | 90 +++++++++++---------------------------- 1 file changed, 26 insertions(+), 64 deletions(-)
diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c index 4ce270513695..1472cccfd0b3 100644 --- a/drivers/bluetooth/ath3k.c +++ b/drivers/bluetooth/ath3k.c @@ -212,19 +212,16 @@ static int ath3k_load_firmware(struct usb_device *udev,
BT_DBG("udev %p", udev);
- pipe = usb_sndctrlpipe(udev, 0); - send_buf = kmalloc(BULK_SIZE, GFP_KERNEL); if (!send_buf) { BT_ERR("Can't allocate memory chunk for firmware"); return -ENOMEM; }
- memcpy(send_buf, firmware->data, FW_HDR_SIZE); - err = usb_control_msg(udev, pipe, USB_REQ_DFU_DNLOAD, USB_TYPE_VENDOR, - 0, 0, send_buf, FW_HDR_SIZE, - USB_CTRL_SET_TIMEOUT); - if (err < 0) { + err = usb_control_msg_send(udev, 0, USB_REQ_DFU_DNLOAD, USB_TYPE_VENDOR, + 0, 0, firmware->data, FW_HDR_SIZE, + USB_CTRL_SET_TIMEOUT); + if (err) { BT_ERR("Can't change to loading configuration err"); goto error; } @@ -259,44 +256,17 @@ static int ath3k_load_firmware(struct usb_device *udev,
static int ath3k_get_state(struct usb_device *udev, unsigned char *state) { - int ret, pipe = 0; - char *buf; - - buf = kmalloc(sizeof(*buf), GFP_KERNEL); - if (!buf) - return -ENOMEM; - - pipe = usb_rcvctrlpipe(udev, 0); - ret = usb_control_msg(udev, pipe, ATH3K_GETSTATE, - USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, - buf, sizeof(*buf), USB_CTRL_SET_TIMEOUT); - - *state = *buf; - kfree(buf); - - return ret; + return usb_control_msg_recv(udev, 0, ATH3K_GETSTATE, + USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, + state, 1, USB_CTRL_SET_TIMEOUT); }
static int ath3k_get_version(struct usb_device *udev, struct ath3k_version *version) { - int ret, pipe = 0; - struct ath3k_version *buf; - const int size = sizeof(*buf); - - buf = kmalloc(size, GFP_KERNEL); - if (!buf) - return -ENOMEM; - - pipe = usb_rcvctrlpipe(udev, 0); - ret = usb_control_msg(udev, pipe, ATH3K_GETVERSION, - USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, - buf, size, USB_CTRL_SET_TIMEOUT); - - memcpy(version, buf, size); - kfree(buf); - - return ret; + return usb_control_msg_recv(udev, 0, ATH3K_GETVERSION, + USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, + version, sizeof(*version), USB_CTRL_SET_TIMEOUT); }
static int ath3k_load_fwfile(struct usb_device *udev, @@ -316,13 +286,10 @@ static int ath3k_load_fwfile(struct usb_device *udev, }
size = min_t(uint, count, FW_HDR_SIZE); - memcpy(send_buf, firmware->data, size);
- pipe = usb_sndctrlpipe(udev, 0); - ret = usb_control_msg(udev, pipe, ATH3K_DNLOAD, - USB_TYPE_VENDOR, 0, 0, send_buf, - size, USB_CTRL_SET_TIMEOUT); - if (ret < 0) { + ret = usb_control_msg_send(udev, 0, ATH3K_DNLOAD, USB_TYPE_VENDOR, 0, 0, + firmware->data, size, USB_CTRL_SET_TIMEOUT); + if (ret) { BT_ERR("Can't change to loading configuration err"); kfree(send_buf); return ret; @@ -355,23 +322,19 @@ static int ath3k_load_fwfile(struct usb_device *udev, return 0; }
-static int ath3k_switch_pid(struct usb_device *udev) +static void ath3k_switch_pid(struct usb_device *udev) { - int pipe = 0; - - pipe = usb_sndctrlpipe(udev, 0); - return usb_control_msg(udev, pipe, USB_REG_SWITCH_VID_PID, - USB_TYPE_VENDOR, 0, 0, - NULL, 0, USB_CTRL_SET_TIMEOUT); + usb_control_msg_send(udev, 0, USB_REG_SWITCH_VID_PID, USB_TYPE_VENDOR, + 0, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); }
static int ath3k_set_normal_mode(struct usb_device *udev) { unsigned char fw_state; - int pipe = 0, ret; + int ret;
ret = ath3k_get_state(udev, &fw_state); - if (ret < 0) { + if (ret) { BT_ERR("Can't get state to change to normal mode err"); return ret; } @@ -381,10 +344,9 @@ static int ath3k_set_normal_mode(struct usb_device *udev) return 0; }
- pipe = usb_sndctrlpipe(udev, 0); - return usb_control_msg(udev, pipe, ATH3K_SET_NORMAL_MODE, - USB_TYPE_VENDOR, 0, 0, - NULL, 0, USB_CTRL_SET_TIMEOUT); + return usb_control_msg_send(udev, 0, ATH3K_SET_NORMAL_MODE, + USB_TYPE_VENDOR, 0, 0, NULL, 0, + USB_CTRL_SET_TIMEOUT); }
static int ath3k_load_patch(struct usb_device *udev) @@ -397,7 +359,7 @@ static int ath3k_load_patch(struct usb_device *udev) int ret;
ret = ath3k_get_state(udev, &fw_state); - if (ret < 0) { + if (ret) { BT_ERR("Can't get state to change to load ram patch err"); return ret; } @@ -408,7 +370,7 @@ static int ath3k_load_patch(struct usb_device *udev) }
ret = ath3k_get_version(udev, &fw_version); - if (ret < 0) { + if (ret) { BT_ERR("Can't get version to change to load ram patch err"); return ret; } @@ -449,13 +411,13 @@ static int ath3k_load_syscfg(struct usb_device *udev) int clk_value, ret;
ret = ath3k_get_state(udev, &fw_state); - if (ret < 0) { + if (ret) { BT_ERR("Can't get state to change to load configuration err"); return -EBUSY; }
ret = ath3k_get_version(udev, &fw_version); - if (ret < 0) { + if (ret) { BT_ERR("Can't get version to change to load ram patch err"); return ret; } @@ -529,7 +491,7 @@ static int ath3k_probe(struct usb_interface *intf, return ret; } ret = ath3k_set_normal_mode(udev); - if (ret < 0) { + if (ret) { BT_ERR("Set normal mode failed"); return ret; }
A USB device will always haev a bi-directional endpoint 0, that's just how the devices work, so no need to check for that in a few quirk tests as it will always pass.
Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Alexander Tsoy alexander@tsoy.me Cc: alsa-devel@alsa-project.org Reported-by: Alan Stern stern@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: - new patch, was not in v1 series, suggested by Alan.
sound/usb/quirks.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index fc3aab04a0bc..35da4aa918c7 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -875,8 +875,6 @@ static int snd_usb_nativeinstruments_boot_quirk(struct usb_device *dev) { int ret;
- if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) - return -EINVAL; ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), 0xaf, USB_TYPE_VENDOR | USB_RECIP_DEVICE, 1, 0, NULL, 0, 1000); @@ -984,8 +982,6 @@ static int snd_usb_axefx3_boot_quirk(struct usb_device *dev)
dev_dbg(&dev->dev, "Waiting for Axe-Fx III to boot up...\n");
- if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) - return -EINVAL; /* If the Axe-Fx III has not fully booted, it will timeout when trying * to enable the audio streaming interface. A more generous timeout is * used here to detect when the Axe-Fx III has finished booting as the @@ -1117,8 +1113,6 @@ static int snd_usb_motu_m_series_boot_quirk(struct usb_device *dev) { int ret;
- if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) - return -EINVAL; ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), 1, USB_TYPE_VENDOR | USB_RECIP_DEVICE, 0x0, 0, NULL, 0, 1000);
participants (3)
-
Alan Stern -
Andy Shevchenko -
Greg Kroah-Hartman