Re: [Linux-kernel-mentees] [PATCH 04/15] usb: serial: cp210x: use usb_control_msg_recv() and usb_control_msg_send()
From: Johan Hovold <johan@kernel.org>
Date: 2020-12-04 09:34:41
Also in:
linux-usb, lkml
On Wed, Nov 04, 2020 at 12:16:52PM +0530, Himadri Pandya wrote:
The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps usb_control_msg() with proper error check. Hence use the wrappers instead of calling usb_control_msg() directly.
This too is a good example of when the new helpers can be used, but please mention the transfer buffers here are that is the primary reason.
quoted hunk ↗ jump to hunk
Signed-off-by: Himadri Pandya <redacted> --- drivers/usb/serial/cp210x.c | 148 ++++++++++-------------------------- 1 file changed, 42 insertions(+), 106 deletions(-)diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c index d0c05aa8a0d6..29436ab392e8 100644 --- a/drivers/usb/serial/cp210x.c +++ b/drivers/usb/serial/cp210x.c@@ -555,31 +555,15 @@ static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req, { struct usb_serial *serial = port->serial; struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); - void *dmabuf; int result; - dmabuf = kmalloc(bufsize, GFP_KERNEL); - if (!dmabuf) { - /* - * FIXME Some callers don't bother to check for error, - * at least give them consistent junk until they are fixed - */ - memset(buf, 0, bufsize); - return -ENOMEM; - } - - result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), - req, REQTYPE_INTERFACE_TO_HOST, 0, - port_priv->bInterfaceNumber, dmabuf, bufsize, - USB_CTRL_SET_TIMEOUT); - if (result == bufsize) { - memcpy(buf, dmabuf, bufsize); - result = 0; - } else { + result = usb_control_msg_recv(serial->dev, 0, req, + REQTYPE_INTERFACE_TO_HOST, 0, + port_priv->bInterfaceNumber, buf, bufsize, + USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
Please keep the existing indentation style. That way you don't need to change the middle-two argument lines just to align the arguments.
+ if (result) {
dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
- req, bufsize, result);
- if (result >= 0)
- result = -EIO;
+ req, bufsize, result);Nit: This is an unrelated indentation change.
quoted hunk ↗ jump to hunk
/* * FIXME Some callers don't bother to check for error,@@ -588,8 +572,6 @@ static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req, memset(buf, 0, bufsize); } - kfree(dmabuf); - return result; }@@ -648,29 +630,16 @@ static int cp210x_read_u8_reg(struct usb_serial_port *port, u8 req, u8 *val) static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val, void *buf, int bufsize) { - void *dmabuf; int result; - dmabuf = kmalloc(bufsize, GFP_KERNEL); - if (!dmabuf) - return -ENOMEM; - - result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), - CP210X_VENDOR_SPECIFIC, type, val, - cp210x_interface_num(serial), dmabuf, bufsize, - USB_CTRL_GET_TIMEOUT); - if (result == bufsize) { - memcpy(buf, dmabuf, bufsize); - result = 0; - } else { + result = usb_control_msg_recv(serial->dev, 0, CP210X_VENDOR_SPECIFIC, + type, val, cp210x_interface_num(serial), + buf, bufsize, USB_CTRL_GET_TIMEOUT, + GFP_KERNEL); + if (result) dev_err(&serial->interface->dev, "failed to get vendor val 0x%04x size %d: %d\n", val, bufsize, result); - if (result >= 0) - result = -EIO; - }
Nit: Please keep the brackets around multiline single statements since it improves readability. Similar comments apply to the rest of the patch. Johan _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees