On Wed, Jan 27, 2021 at 05:42:47PM +0530, Anant Thazhemadam wrote:
> The newer usb_control_msg_{send|recv}() API are an improvement on the
> existing usb_control_msg() as it ensures that a short read/write is treated
> as an error, data can be used off the stack, and raw usb pipes need not be
> created in the calling functions.
> For this reason, instances of usb_control_msg() have been replaced with
> usb_control_msg_{recv|send}() and the return value checking conditions have
> also been modified appropriately.
>
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@xxxxxxxxx>
> ---
> Resending this patch since the subject line for the initial submission
> (sent as a part of the patch series) wasn't set correctly.
>
> drivers/usb/misc/usbtest.c | 69 ++++++++++++++++----------------------
> 1 file changed, 29 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index 150090ee4ec1..4337eff2a749 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -672,19 +672,15 @@ static int get_altsetting(struct usbtest_dev *dev)
> struct usb_device *udev = interface_to_usbdev(iface);
> int retval;
>
> - retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> - USB_REQ_GET_INTERFACE, USB_DIR_IN|USB_RECIP_INTERFACE,
> - 0, iface->altsetting[0].desc.bInterfaceNumber,
> - dev->buf, 1, USB_CTRL_GET_TIMEOUT);
> - switch (retval) {
> - case 1:
> - return dev->buf[0];
> - case 0:
> - retval = -ERANGE;
> - fallthrough;
> - default:
> + retval = usb_control_msg_recv(udev, 0, USB_REQ_GET_INTERFACE,
> + USB_DIR_IN|USB_RECIP_INTERFACE,
> + 0, iface->altsetting[0].desc.bInterfaceNumber,
> + dev->buf, 1, USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
> +
> + if (retval < 0)
> return retval;
This changes the return value for short reads. Maybe not an issue, but
since this a test driver and the value is propagated to user space it is
not clear cut.
> - }
> +
> + return dev->buf[0];
> }
>
> static int set_altsetting(struct usbtest_dev *dev, int alternate)
> @@ -872,14 +868,15 @@ static int ch9_postconfig(struct usbtest_dev *dev)
> * ... although some cheap devices (like one TI Hub I've got)
> * won't return config descriptors except before set_config.
> */
> - retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> - USB_REQ_GET_CONFIGURATION,
> - USB_DIR_IN | USB_RECIP_DEVICE,
> - 0, 0, dev->buf, 1, USB_CTRL_GET_TIMEOUT);
> - if (retval != 1 || dev->buf[0] != expected) {
> + retval = usb_control_msg_recv(udev, 0, USB_REQ_GET_CONFIGURATION,
> + USB_DIR_IN | USB_RECIP_DEVICE, 0,
> + 0, dev->buf, 1, USB_CTRL_GET_TIMEOUT,
> + GFP_KERNEL);
> +
> + if (retval != 0 || dev->buf[0] != expected) {
> dev_err(&iface->dev, "get config --> %d %d (1 %d)\n",
> retval, dev->buf[0], expected);
> - return (retval < 0) ? retval : -EDOM;
> + return retval;
Same here (but different error, so probably not an issue in either
place).
> @@ -1845,30 +1842,22 @@ static int ctrl_out(struct usbtest_dev *dev,
> /* write patterned data */
> for (j = 0; j < len; j++)
> buf[j] = (u8)(i + j);
> - retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> - 0x5b, USB_DIR_OUT|USB_TYPE_VENDOR,
> - 0, 0, buf, len, USB_CTRL_SET_TIMEOUT);
> - if (retval != len) {
> + retval = usb_control_msg_send(udev, 0, 0x5b,
> + USB_DIR_OUT | USB_TYPE_VENDOR, 0,
> + 0, buf, len, USB_CTRL_SET_TIMEOUT,
> + GFP_KERNEL);
This introduces a redundant allocation and memcpy() for every iteration
for the buffer which is already DMA-able. So this looks like a bad fit
for the new helper.
> + if (retval < 0) {
> what = "write";
> - if (retval >= 0) {
> - ERROR(dev, "ctrl_out, wlen %d (expected %d)\n",
> - retval, len);
> - retval = -EBADMSG;
> - }
You could drop this redundant short write test though if you want.
> break;
> }
>
> /* read it back -- assuming nothing intervened!! */
> - retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> - 0x5c, USB_DIR_IN|USB_TYPE_VENDOR,
> - 0, 0, buf, len, USB_CTRL_GET_TIMEOUT);
> - if (retval != len) {
> + retval = usb_control_msg_recv(udev, 0,
> + 0x5c, USB_DIR_IN|USB_TYPE_VENDOR,
> + 0, 0, buf, len, USB_CTRL_GET_TIMEOUT,
> + GFP_KERNEL);
> + if (retval < 0) {
> what = "read";
> - if (retval >= 0) {
> - ERROR(dev, "ctrl_out, rlen %d (expected %d)\n",
> - retval, len);
> - retval = -EBADMSG;
> - }
Same here; the buffer is already DMA-able and you remove the error
message, which someone using a test driver like this may want to see.
Probably better left as is.
> break;
> }
Johan
