On Mon, Aug 02, 2021 at 02:01:18AM +0530, Himadri Pandya wrote:
> The new wrapper functions for usb_control_msg() can accept data from
> stack with robust error checks.
Please rephrase the "robust error checks" along the lines of "treats
short reads as an error".
> Hence use the wrappers with stack
> variables for usb transfer buffers to save kernel memory.
Again, you're not saving memory here, just moving the allocation.
> Signed-off-by: Himadri Pandya <himadrispandya@xxxxxxxxx>
> ---
> Changes in v2:
> - Drop unrelated style fixes
> ---
> drivers/usb/serial/cp210x.c | 107 ++++++++++--------------------------
> 1 file changed, 30 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 3c80bfbf3bec..b73581fc1768 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -628,29 +628,18 @@ 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)
> - 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);
> + if (result) {
> dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
> req, bufsize, result);
This will also no longer log the length of short reads. Please at least
mention it in the commit message.
> - if (result >= 0)
> - result = -EIO;
> }
>
> - kfree(dmabuf);
> -
> return result;
> }
Johan
