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. > Signed-off-by: Himadri Pandya <[email protected]> > --- > 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. > > /* > * 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
