From: Johan Hovold <johan@kernel.org>
To: Anant Thazhemadam <anant.thazhemadam@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Alan Stern <stern@rowland.harvard.edu>,
Bixuan Cui <cuibixuan@huawei.com>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Zqiang <qiang.zhang@windriver.com>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RESEND PATCH v3 12/12] usb: misc: usbtest: update to use the usb_control_msg_{send|recv}() API
Date: Wed, 27 Jan 2021 18:00:21 +0100 [thread overview]
Message-ID: <YBGcJb5YtCtGjPtW@hovoldconsulting.com> (raw)
In-Reply-To: <20210127121247.9938-1-anant.thazhemadam@gmail.com>
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
prev parent reply other threads:[~2021-01-27 17:03 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-26 18:33 [PATCH v3 00/12] drivers: usb: misc: update to use usb_control_msg_{send|recv}() API Anant Thazhemadam
2021-01-26 18:33 ` [PATCH v3 01/12] usb: misc: appledisplay: update to use the " Anant Thazhemadam
2021-01-27 13:58 ` Johan Hovold
2021-01-27 14:42 ` Anant Thazhemadam
2021-01-27 17:20 ` Johan Hovold
2021-01-26 18:33 ` [PATCH v3 02/12] usb: misc: cypress_cy7c63: update to use usb_control_msg_recv() Anant Thazhemadam
2021-01-27 14:09 ` Johan Hovold
2021-01-27 14:40 ` Anant Thazhemadam
2021-01-26 18:33 ` [PATCH v3 03/12] usb: misc: cytherm: " Anant Thazhemadam
2021-01-27 14:21 ` Johan Hovold
2021-01-26 18:33 ` [PATCH v3 04/12] usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API Anant Thazhemadam
2021-01-27 14:49 ` Johan Hovold
2021-01-26 18:33 ` [PATCH v3 05/12] usb: misc: ezusb: update to use usb_control_msg_send() Anant Thazhemadam
2021-01-27 14:52 ` Johan Hovold
2021-01-26 18:33 ` [PATCH v3 06/12] usb: misc: iowarrior: update to use the usb_control_msg_{send|recv}() API Anant Thazhemadam
2021-01-27 14:59 ` Johan Hovold
2021-01-26 18:33 ` [PATCH v3 07/12] usb: misc: isight_firmware: update to use usb_control_msg_send() Anant Thazhemadam
2021-01-27 15:04 ` Johan Hovold
2021-01-26 18:33 ` [PATCH v3 08/12] usb: misc: ldusb: " Anant Thazhemadam
2021-01-27 15:06 ` Johan Hovold
2021-01-26 18:39 ` [PATCH v3 09/12] usb: misc: lvstest: update to use the usb_control_msg_{send|recv}() API Anant Thazhemadam
2021-01-27 15:15 ` Johan Hovold
2021-01-26 18:40 ` [PATCH v3 10/12] usb: misc: trancevibrator: update to use usb_control_msg_send() Anant Thazhemadam
2021-01-27 15:17 ` Johan Hovold
2021-01-26 18:40 ` [PATCH v3 11/12] usb: misc: usbsevseg: " Anant Thazhemadam
2021-01-27 16:46 ` Johan Hovold
[not found] ` <20210126184043.915235-1-anant.thazhemadam@gmail.com>
2021-01-26 18:47 ` [PATCH v3 12/12] [PATCH v3 12/12] usb: misc: usbtest: update to use the, usb_control_msg_{send|recv}() API Anant Thazhemadam
2021-01-27 11:31 ` Greg Kroah-Hartman
2021-01-27 12:12 ` [RESEND PATCH v3 12/12] usb: misc: usbtest: update to use the " Anant Thazhemadam
2021-01-27 16:26 ` Alan Stern
2021-01-27 17:00 ` Johan Hovold [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YBGcJb5YtCtGjPtW@hovoldconsulting.com \
--to=johan@kernel.org \
--cc=anant.thazhemadam@gmail.com \
--cc=cuibixuan@huawei.com \
--cc=gregkh@linuxfoundation.org \
--cc=gustavoars@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=qiang.zhang@windriver.com \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).