lkml.org 
[lkml]   [2021]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RESEND PATCH v3 12/12] usb: misc: usbtest: update to use the usb_control_msg_{send|recv}() API
    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@gmail.com>
    > ---
    > 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

    \
     
     \ /
      Last update: 2021-01-27 18:05    [W:3.719 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site