[PATCH v2 0/6] USB: serial: use wrappers for usb_control_msg()

[PATCH v2 0/6] USB: serial: use wrappers for usb_control_msg()

[Himadri Pandya]

There are many usages of usb_control_msg() that can use the new wrapper
functions usb_contro_msg_send() & usb_control_msg_recv() for better
error checks on short reads and writes. They can also be used to avoid
allocating redundant dma buffers that are generally required to use
usb_control_msg(). Hence use them whenever possible and avoid using
usb_control_msg() directly.

Changes in v2:
 - Drop unnecessary use of wrappers
 - Drop unrelated style changes

Additional Info:
- This is a super late follow-up on v1 that was submitted a while
ago(my sincere apologies). (All patches are rebased and
compile tested.)
- v1 patches: https://lkml.org/lkml/2020/11/4/40
- Patches that introduced the wrapper functions:
  https://lkml.org/lkml/2020/9/14/859

Himadri Pandya (6):
  USB: serial: ch314: use usb_control_msg_recv() and
    usb_control_msg_send()
  USB: serial: cp210x: use usb_control_msg_recv() and
    usb_control_msg_send()
  USB: serial: f81232: use usb_control_msg_recv() and
    usb_control_msg_send()
  USB: serial: ftdi_sio: use usb_control_msg_recv()
  USB: serial: keyspan_pda: use usb_control_msg_recv()
  USB: serial: kl5kusb105: use usb_control_msg_recv() and
    usb_control_msg_send()

 drivers/usb/serial/ch341.c       |  97 +++++++++-------------------
 drivers/usb/serial/cp210x.c      | 107 +++++++++----------------------
 drivers/usb/serial/f81232.c      |  96 ++++++++++-----------------
 drivers/usb/serial/ftdi_sio.c    |  53 +++++----------
 drivers/usb/serial/keyspan_pda.c |  70 +++++++++-----------
 drivers/usb/serial/kl5kusb105.c  |  79 ++++++++++-------------
 6 files changed, 173 insertions(+), 329 deletions(-)

-- 
2.17.1

[PATCH v2 1/6] USB: serial: ch314: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

usb_control_msg_send/recv are new wrapper functions for usb_control_msg()
that have proper error checks for short read/writes. These functions
can also accept data buffer on stack. Hence use these functions to have
more robust error checks, and to reduce kernel memory usage for usb
messages.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
Changes in v2:
 - Fix callers of ch341_control_out() and ch341_control_in()
 - Remove label "out"
 - Remove an unnecessary assignment statement
---
 drivers/usb/serial/ch341.c | 97 ++++++++++++--------------------------
 1 file changed, 29 insertions(+), 68 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2db917eab799..c6f7ff9ca8ae 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -113,10 +113,10 @@ static int ch341_control_out(struct usb_device *dev, u8 request,
 	dev_dbg(&dev->dev, "%s - (%02x,%04x,%04x)\n", __func__,
 		request, value, index);
 
-	r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
-			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
-			    value, index, NULL, 0, DEFAULT_TIMEOUT);
-	if (r < 0)
+	r = usb_control_msg_send(dev, 0, request,
+				 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+				 value, index, NULL, 0, DEFAULT_TIMEOUT, GFP_KERNEL);
+	if (r)
 		dev_err(&dev->dev, "failed to send control message: %d\n", r);
 
 	return r;
@@ -131,23 +131,13 @@ static int ch341_control_in(struct usb_device *dev,
 	dev_dbg(&dev->dev, "%s - (%02x,%04x,%04x,%u)\n", __func__,
 		request, value, index, bufsize);
 
-	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
-			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			    value, index, buf, bufsize, DEFAULT_TIMEOUT);
-	if (r < (int)bufsize) {
-		if (r >= 0) {
-			dev_err(&dev->dev,
-				"short control message received (%d < %u)\n",
-				r, bufsize);
-			r = -EIO;
-		}
-
-		dev_err(&dev->dev, "failed to receive control message: %d\n",
-			r);
-		return r;
-	}
+	r = usb_control_msg_recv(dev, 0, request,
+				 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+				 value, index, buf, bufsize, DEFAULT_TIMEOUT, GFP_KERNEL);
+	if (r)
+		dev_err(&dev->dev, "failed to receive control message: %d\n", r);
 
-	return 0;
+	return r;
 }
 
 #define CH341_CLKRATE		48000000
@@ -287,23 +277,19 @@ static int ch341_set_handshake(struct usb_device *dev, u8 control)
 static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
 {
 	const unsigned int size = 2;
-	char *buffer;
+	u8 buffer[2];
 	int r;
 	unsigned long flags;
 
-	buffer = kmalloc(size, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
 	r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x0706, 0, buffer, size);
-	if (r < 0)
+	if (r)
 		goto out;
 
 	spin_lock_irqsave(&priv->lock, flags);
 	priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-out:	kfree(buffer);
+out:
 	return r;
 }
 
@@ -312,21 +298,17 @@ out:	kfree(buffer);
 static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 {
 	const unsigned int size = 2;
-	char *buffer;
+	u8 buffer[2];
 	int r;
 
-	buffer = kmalloc(size, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
 	/* expect two bytes 0x27 0x00 */
 	r = ch341_control_in(dev, CH341_REQ_READ_VERSION, 0, 0, buffer, size);
-	if (r < 0)
+	if (r)
 		goto out;
 	dev_dbg(&dev->dev, "Chip version: 0x%02x\n", buffer[0]);
 
 	r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0, 0);
-	if (r < 0)
+	if (r)
 		goto out;
 
 	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
@@ -335,7 +317,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 
 	r = ch341_set_handshake(dev, priv->mcr);
 
-out:	kfree(buffer);
+out:
 	return r;
 }
 
@@ -345,39 +327,23 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
 	struct usb_device *udev = port->serial->dev;
 	const unsigned int size = 2;
 	unsigned long quirks = 0;
-	char *buffer;
+	u8 buffer[2];
 	int r;
 
-	buffer = kmalloc(size, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
 	/*
 	 * A subset of CH34x devices does not support all features. The
 	 * prescaler is limited and there is no support for sending a RS232
 	 * break condition. A read failure when trying to set up the latter is
 	 * used to detect these devices.
 	 */
-	r = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), CH341_REQ_READ_REG,
-			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
+	r = usb_control_msg_recv(udev, 0, CH341_REQ_READ_REG,
+				 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+				 CH341_REG_BREAK, 0, &buffer, size, DEFAULT_TIMEOUT, GFP_KERNEL);
 	if (r == -EPIPE) {
 		dev_info(&port->dev, "break control not supported, using simulated break\n");
 		quirks = CH341_QUIRK_LIMITED_PRESCALER | CH341_QUIRK_SIMULATE_BREAK;
-		r = 0;
-		goto out;
-	}
-
-	if (r != size) {
-		if (r >= 0)
-			r = -EIO;
+	} else if (r)
 		dev_err(&port->dev, "failed to read break control: %d\n", r);
-		goto out;
-	}
-
-	r = 0;
-out:
-	kfree(buffer);
 
 	if (quirks) {
 		dev_dbg(&port->dev, "enabling quirk flags: 0x%02lx\n", quirks);
@@ -647,23 +613,19 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 	struct ch341_private *priv = usb_get_serial_port_data(port);
 	int r;
 	uint16_t reg_contents;
-	uint8_t *break_reg;
+	uint8_t break_reg[2];
 
 	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
 		ch341_simulate_break(tty, break_state);
 		return;
 	}
 
-	break_reg = kmalloc(2, GFP_KERNEL);
-	if (!break_reg)
-		return;
-
-	r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG,
-			ch341_break_reg, 0, break_reg, 2);
-	if (r < 0) {
+	r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG, ch341_break_reg, 0,
+			     break_reg, 2);
+	if (r) {
 		dev_err(&port->dev, "%s - USB control read error (%d)\n",
 				__func__, r);
-		goto out;
+		return;
 	}
 	dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n",
 		__func__, break_reg[0], break_reg[1]);
@@ -681,11 +643,10 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 	reg_contents = get_unaligned_le16(break_reg);
 	r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
 			ch341_break_reg, reg_contents);
-	if (r < 0)
+	if (r)
 		dev_err(&port->dev, "%s - USB control write error (%d)\n",
 				__func__, r);
-out:
-	kfree(break_reg);
+	return;
 }
 
 static int ch341_tiocmset(struct tty_struct *tty,
-- 
2.17.1

Re: [PATCH v2 1/6] USB: serial: ch314: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Mon, Aug 02, 2021 at 02:01:17AM +0530, Himadri Pandya wrote:
> usb_control_msg_send/recv are new wrapper functions for usb_control_msg()
> that have proper error checks for short read/writes.

There no need for any special handling of short writes. Testing against
against negative errno is all that's needed.

> These functions
> can also accept data buffer on stack. Hence use these functions to have
> more robust error checks, and to reduce kernel memory usage for usb
> messages.

I'd rephrase this along the lines of "to simplify error handling for
short reads".

Also, you're not reducing kernel memory usage, you're just moving the
allocation to be handled by the wrapper instead.

> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
> Changes in v2:
>  - Fix callers of ch341_control_out() and ch341_control_in()
>  - Remove label "out"
>  - Remove an unnecessary assignment statement
> ---
>  drivers/usb/serial/ch341.c | 97 ++++++++++++--------------------------
>  1 file changed, 29 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 2db917eab799..c6f7ff9ca8ae 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -113,10 +113,10 @@ static int ch341_control_out(struct usb_device *dev, u8 request,
>  	dev_dbg(&dev->dev, "%s - (%02x,%04x,%04x)\n", __func__,
>  		request, value, index);
>  
> -	r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
> -			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> -			    value, index, NULL, 0, DEFAULT_TIMEOUT);
> -	if (r < 0)
> +	r = usb_control_msg_send(dev, 0, request,
> +				 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> +				 value, index, NULL, 0, DEFAULT_TIMEOUT, GFP_KERNEL);
> +	if (r)
>  		dev_err(&dev->dev, "failed to send control message: %d\n", r);
>  
>  	return r;

Since ch341_control_out() does not take a data buffer argument, there's
no need to use the new helper which only adds an extra function call.

Note that this function already return 0 on success or a negative errno
on errors.

> @@ -131,23 +131,13 @@ static int ch341_control_in(struct usb_device *dev,
>  	dev_dbg(&dev->dev, "%s - (%02x,%04x,%04x,%u)\n", __func__,
>  		request, value, index, bufsize);
>  
> -	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
> -			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> -			    value, index, buf, bufsize, DEFAULT_TIMEOUT);
> -	if (r < (int)bufsize) {
> -		if (r >= 0) {
> -			dev_err(&dev->dev,
> -				"short control message received (%d < %u)\n",
> -				r, bufsize);

You're also removing this error message. It should at least be
mentioned somewhere that short reads will now simply be reported as
-EREMOTEIO with no indication of how short the transfer was.

That's usually just fine, but I'm currently dealing with another driver
where a short read can be used to differentiate between device types as
an example.

> -			r = -EIO;
> -		}
> -
> -		dev_err(&dev->dev, "failed to receive control message: %d\n",
> -			r);
> -		return r;
> -	}
> +	r = usb_control_msg_recv(dev, 0, request,
> +				 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +				 value, index, buf, bufsize, DEFAULT_TIMEOUT, GFP_KERNEL);
> +	if (r)
> +		dev_err(&dev->dev, "failed to receive control message: %d\n", r);
>  
> -	return 0;
> +	return r;
>  }
>  
>  #define CH341_CLKRATE		48000000
> @@ -287,23 +277,19 @@ static int ch341_set_handshake(struct usb_device *dev, u8 control)
>  static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
>  {
>  	const unsigned int size = 2;
> -	char *buffer;
> +	u8 buffer[2];
>  	int r;
>  	unsigned long flags;
>  
> -	buffer = kmalloc(size, GFP_KERNEL);
> -	if (!buffer)
> -		return -ENOMEM;
> -
>  	r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x0706, 0, buffer, size);
> -	if (r < 0)
> +	if (r)
>  		goto out;
>  
>  	spin_lock_irqsave(&priv->lock, flags);
>  	priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
> -out:	kfree(buffer);
> +out:
>  	return r;

Just return r in the error path above and drop the label.

>  }
>  
> @@ -312,21 +298,17 @@ out:	kfree(buffer);
>  static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
>  {
>  	const unsigned int size = 2;
> -	char *buffer;
> +	u8 buffer[2];
>  	int r;
>  
> -	buffer = kmalloc(size, GFP_KERNEL);
> -	if (!buffer)
> -		return -ENOMEM;
> -
>  	/* expect two bytes 0x27 0x00 */
>  	r = ch341_control_in(dev, CH341_REQ_READ_VERSION, 0, 0, buffer, size);
> -	if (r < 0)
> +	if (r)
>  		goto out;
>  	dev_dbg(&dev->dev, "Chip version: 0x%02x\n", buffer[0]);
>  
>  	r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0, 0);
> -	if (r < 0)
> +	if (r)
>  		goto out;
>  
>  	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
> @@ -335,7 +317,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
>  
>  	r = ch341_set_handshake(dev, priv->mcr);
>  
> -out:	kfree(buffer);
> +out:
>  	return r;
>  }

Same here (even if you need to touch the other error paths).

> @@ -647,23 +613,19 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
>  	struct ch341_private *priv = usb_get_serial_port_data(port);
>  	int r;
>  	uint16_t reg_contents;
> -	uint8_t *break_reg;
> +	uint8_t break_reg[2];
>  
>  	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
>  		ch341_simulate_break(tty, break_state);
>  		return;
>  	}
>  
> -	break_reg = kmalloc(2, GFP_KERNEL);
> -	if (!break_reg)
> -		return;
> -
> -	r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG,
> -			ch341_break_reg, 0, break_reg, 2);
> -	if (r < 0) {
> +	r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG, ch341_break_reg, 0,
> +			     break_reg, 2);

Drop this unrelated style change (which creates a line > 80 chars too).

> +	if (r) {
>  		dev_err(&port->dev, "%s - USB control read error (%d)\n",
>  				__func__, r);
> -		goto out;
> +		return;
>  	}
>  	dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n",
>  		__func__, break_reg[0], break_reg[1]);
> @@ -681,11 +643,10 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
>  	reg_contents = get_unaligned_le16(break_reg);
>  	r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
>  			ch341_break_reg, reg_contents);
> -	if (r < 0)
> +	if (r)
>  		dev_err(&port->dev, "%s - USB control write error (%d)\n",
>  				__func__, r);
> -out:
> -	kfree(break_reg);
> +	return;

No need for return at the end of a void function.

>  }
>  
>  static int ch341_tiocmset(struct tty_struct *tty,

Johan

[PATCH v2 2/6] USB: serial: cp210x: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new wrapper functions for usb_control_msg() can accept data from
stack with robust error checks. Hence use the wrappers with stack
variables for usb transfer buffers to save kernel memory.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
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);
-		if (result >= 0)
-			result = -EIO;
 	}
 
-	kfree(dmabuf);
-
 	return result;
 }
 
@@ -669,30 +658,17 @@ 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;
 	}
-
-	kfree(dmabuf);
-
 	return result;
 }
 
@@ -727,21 +703,14 @@ static int cp210x_write_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 = kmemdup(buf, bufsize, GFP_KERNEL);
-	if (!dmabuf)
-		return -ENOMEM;
+	result = usb_control_msg_send(serial->dev, 0, req,
+				      REQTYPE_HOST_TO_INTERFACE, 0,
+				      port_priv->bInterfaceNumber, buf, bufsize,
+				      USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
 
-	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			req, REQTYPE_HOST_TO_INTERFACE, 0,
-			port_priv->bInterfaceNumber, dmabuf, bufsize,
-			USB_CTRL_SET_TIMEOUT);
-
-	kfree(dmabuf);
-
-	if (result < 0) {
+	if (result) {
 		dev_err(&port->dev, "failed set req 0x%x size %d status: %d\n",
 				req, bufsize, result);
 		return result;
@@ -770,21 +739,14 @@ static int cp210x_write_u32_reg(struct usb_serial_port *port, u8 req, u32 val)
 static int cp210x_write_vendor_block(struct usb_serial *serial, u8 type,
 				     u16 val, void *buf, int bufsize)
 {
-	void *dmabuf;
 	int result;
 
-	dmabuf = kmemdup(buf, bufsize, GFP_KERNEL);
-	if (!dmabuf)
-		return -ENOMEM;
-
-	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-				 CP210X_VENDOR_SPECIFIC, type, val,
-				 cp210x_interface_num(serial), dmabuf, bufsize,
-				 USB_CTRL_SET_TIMEOUT);
+	result = usb_control_msg_send(serial->dev, 0, CP210X_VENDOR_SPECIFIC,
+				      type, val, cp210x_interface_num(serial),
+				      buf, bufsize, USB_CTRL_SET_TIMEOUT,
+				      GFP_KERNEL);
 
-	kfree(dmabuf);
-
-	if (result < 0) {
+	if (result) {
 		dev_err(&serial->interface->dev,
 			"failed to set vendor val 0x%04x size %d: %d\n", val,
 			bufsize, result);
@@ -949,27 +911,18 @@ static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port,
 {
 	struct usb_serial *serial = port->serial;
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-	struct cp210x_comm_status *sts;
+	struct cp210x_comm_status sts;
 	int result;
 
-	sts = kmalloc(sizeof(*sts), GFP_KERNEL);
-	if (!sts)
-		return -ENOMEM;
-
-	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-			CP210X_GET_COMM_STATUS, REQTYPE_INTERFACE_TO_HOST,
-			0, port_priv->bInterfaceNumber, sts, sizeof(*sts),
-			USB_CTRL_GET_TIMEOUT);
-	if (result == sizeof(*sts)) {
-		*count = le32_to_cpu(sts->ulAmountInOutQueue);
-		result = 0;
-	} else {
+	result = usb_control_msg_recv(serial->dev, 0, CP210X_GET_COMM_STATUS,
+				      REQTYPE_INTERFACE_TO_HOST, 0,
+				      port_priv->bInterfaceNumber, &sts,
+				      sizeof(sts), USB_CTRL_GET_TIMEOUT,
+				      GFP_KERNEL);
+	if (result == 0)
+		*count = le32_to_cpu(sts.ulAmountInOutQueue);
+	else
 		dev_err(&port->dev, "failed to get comm status: %d\n", result);
-		if (result >= 0)
-			result = -EIO;
-	}
-
-	kfree(sts);
 
 	return result;
 }
-- 
2.17.1

Re: [PATCH v2 2/6] USB: serial: cp210x: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

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@gmail.com>
> ---
> 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

[PATCH v2 3/6] USB: serial: f81232: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

New wrapper functions usb_control_msg_send/recv accept stack variables
for usb message buffer and eliminate the need of creating temporary dma
buffers. The wrappers also have proper error checks for short
read/writes. Hence use the wrappers instead of using usb_control_msg()
directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
Changes in v2:
 - Drop unrelated style changes
---
 drivers/usb/serial/f81232.c | 96 +++++++++++++------------------------
 1 file changed, 34 insertions(+), 62 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index a7a7af8d05bf..3ad1f515fb68 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -139,67 +139,46 @@ static int calc_baud_divisor(speed_t baudrate, speed_t clockrate)
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
 {
 	int status;
-	u8 *tmp;
 	struct usb_device *dev = port->serial->dev;
 
-	tmp = kmalloc(sizeof(*val), GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
-
-	status = usb_control_msg(dev,
-				usb_rcvctrlpipe(dev, 0),
-				F81232_REGISTER_REQUEST,
-				F81232_GET_REGISTER,
-				reg,
-				0,
-				tmp,
-				sizeof(*val),
-				USB_CTRL_GET_TIMEOUT);
-	if (status != sizeof(*val)) {
+	status = usb_control_msg_recv(dev,
+				      0,
+				      F81232_REGISTER_REQUEST,
+				      F81232_GET_REGISTER,
+				      reg,
+				      0,
+				      val,
+				      sizeof(*val),
+				      USB_CTRL_GET_TIMEOUT,
+				      GFP_KERNEL);
+	if (status) {
 		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
-
-		if (status < 0)
-			status = usb_translate_errors(status);
-		else
-			status = -EIO;
-	} else {
-		status = 0;
-		*val = *tmp;
+		status = usb_translate_errors(status);
 	}
 
-	kfree(tmp);
 	return status;
 }
 
 static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 val)
 {
 	int status;
-	u8 *tmp;
 	struct usb_device *dev = port->serial->dev;
 
-	tmp = kmalloc(sizeof(val), GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
-
-	*tmp = val;
-
-	status = usb_control_msg(dev,
-				usb_sndctrlpipe(dev, 0),
-				F81232_REGISTER_REQUEST,
-				F81232_SET_REGISTER,
-				reg,
-				0,
-				tmp,
-				sizeof(val),
-				USB_CTRL_SET_TIMEOUT);
-	if (status < 0) {
+	status = usb_control_msg_send(dev,
+				      0,
+				      F81232_REGISTER_REQUEST,
+				      F81232_SET_REGISTER,
+				      reg,
+				      0,
+				      &val,
+				      sizeof(val),
+				      USB_CTRL_SET_TIMEOUT,
+				      GFP_KERNEL);
+	if (status) {
 		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
 		status = usb_translate_errors(status);
-	} else {
-		status = 0;
 	}
 
-	kfree(tmp);
 	return status;
 }
 
@@ -857,28 +836,22 @@ static int f81534a_ctrl_set_register(struct usb_interface *intf, u16 reg,
 	struct usb_device *dev = interface_to_usbdev(intf);
 	int retry = F81534A_ACCESS_REG_RETRY;
 	int status;
-	u8 *tmp;
-
-	tmp = kmemdup(val, size, GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
 
 	while (retry--) {
-		status = usb_control_msg(dev,
-					usb_sndctrlpipe(dev, 0),
-					F81232_REGISTER_REQUEST,
-					F81232_SET_REGISTER,
-					reg,
-					0,
-					tmp,
-					size,
-					USB_CTRL_SET_TIMEOUT);
-		if (status < 0) {
+		status = usb_control_msg_send(dev,
+					      0,
+					      F81232_REGISTER_REQUEST,
+					      F81232_SET_REGISTER,
+					      reg,
+					      0,
+					      val,
+					      size,
+					      USB_CTRL_SET_TIMEOUT,
+					      GFP_KERNEL);
+		if (status) {
 			status = usb_translate_errors(status);
 			if (status == -EIO)
 				continue;
-		} else {
-			status = 0;
 		}
 
 		break;
@@ -889,7 +862,6 @@ static int f81534a_ctrl_set_register(struct usb_interface *intf, u16 reg,
 				reg, status);
 	}
 
-	kfree(tmp);
 	return status;
 }
 
-- 
2.17.1

Re: [PATCH v2 3/6] USB: serial: f81232: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Mon, Aug 02, 2021 at 02:01:19AM +0530, Himadri Pandya wrote:
> New wrapper functions usb_control_msg_send/recv accept stack variables
> for usb message buffer and eliminate the need of creating temporary dma
> buffers. The wrappers also have proper error checks for short
> read/writes. Hence use the wrappers instead of using usb_control_msg()
> directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>

I amended the commit message as follows when applying this one:

 USB: serial: f81232: use usb_control_msg_recv() and usb_control_msg_send()
    
    The new wrapper functions usb_control_msg_send/recv accept stack
    variables for USB message buffers and eliminate the need of manually
    allocating temporary DMA buffers. The read wrapper also treats short
    reads as errors. Hence use the wrappers instead of using
    usb_control_msg() directly.
    
    Note that the conversion of f81534a_ctrl_set_register() adds an extra an
    extra allocation and memcpy for every retry. Since this function is
    called rarely and retries are hopefully rare, the overhead should be
    acceptable.
    
    Also note that short reads are now logged as -EREMOTEIO instead of
    indicating the amount of data read.

Johan

[PATCH v2 4/6] USB: serial: ftdi_sio: use usb_control_msg_recv()

[Himadri Pandya]

usb_control_msg_recv() nicely wraps usb_control_msg() and removes the
compulsion of using dma buffers for usb messages. It also includes proper
error check for possible short read. So use the wrapper and remove dma
buffers from the callers.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
Changes in v2:
 - Drop unnecessary use of wrappers
---
 drivers/usb/serial/ftdi_sio.c | 53 ++++++++++-------------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 4a1f3a95d017..d4c61568b549 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1436,27 +1436,15 @@ static int _read_latency_timer(struct usb_serial_port *port)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct usb_device *udev = port->serial->dev;
-	unsigned char *buf;
+	u8 buf;
 	int rv;
 
-	buf = kmalloc(1, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	rv = usb_control_msg(udev,
-			     usb_rcvctrlpipe(udev, 0),
-			     FTDI_SIO_GET_LATENCY_TIMER_REQUEST,
-			     FTDI_SIO_GET_LATENCY_TIMER_REQUEST_TYPE,
-			     0, priv->interface,
-			     buf, 1, WDR_TIMEOUT);
-	if (rv < 1) {
-		if (rv >= 0)
-			rv = -EIO;
-	} else {
-		rv = buf[0];
-	}
-
-	kfree(buf);
+	rv = usb_control_msg_recv(udev, 0, FTDI_SIO_GET_LATENCY_TIMER_REQUEST,
+				  FTDI_SIO_GET_LATENCY_TIMER_REQUEST_TYPE, 0,
+				  priv->interface, &buf, 1, WDR_TIMEOUT,
+				  GFP_KERNEL);
+	if (rv == 0)
+		rv = buf;
 
 	return rv;
 }
@@ -1851,32 +1839,21 @@ static int ftdi_read_cbus_pins(struct usb_serial_port *port)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct usb_serial *serial = port->serial;
-	unsigned char *buf;
+	u8 buf;
 	int result;
 
 	result = usb_autopm_get_interface(serial->interface);
 	if (result)
 		return result;
 
-	buf = kmalloc(1, GFP_KERNEL);
-	if (!buf) {
-		usb_autopm_put_interface(serial->interface);
-		return -ENOMEM;
-	}
-
-	result = usb_control_msg(serial->dev,
-				 usb_rcvctrlpipe(serial->dev, 0),
-				 FTDI_SIO_READ_PINS_REQUEST,
-				 FTDI_SIO_READ_PINS_REQUEST_TYPE, 0,
-				 priv->interface, buf, 1, WDR_TIMEOUT);
-	if (result < 1) {
-		if (result >= 0)
-			result = -EIO;
-	} else {
-		result = buf[0];
-	}
+	result = usb_control_msg_recv(serial->dev, 0,
+				      FTDI_SIO_READ_PINS_REQUEST,
+				      FTDI_SIO_READ_PINS_REQUEST_TYPE, 0,
+				      priv->interface, &buf, 1, WDR_TIMEOUT,
+				      GFP_KERNEL);
+	if (result == 0)
+		result = buf;
 
-	kfree(buf);
 	usb_autopm_put_interface(serial->interface);
 
 	return result;
-- 
2.17.1

Re: [PATCH v2 4/6] USB: serial: ftdi_sio: use usb_control_msg_recv()

[Johan Hovold]

On Mon, Aug 02, 2021 at 02:01:20AM +0530, Himadri Pandya wrote:
> usb_control_msg_recv() nicely wraps usb_control_msg() and removes the
> compulsion of using dma buffers for usb messages. It also includes proper
> error check for possible short read. So use the wrapper and remove dma
> buffers from the callers.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
> Changes in v2:
>  - Drop unnecessary use of wrappers

Now applied, with a slightly amended commit message:

    USB: serial: ftdi_sio: use usb_control_msg_recv()
    
    usb_control_msg_recv() nicely wraps usb_control_msg() and removes the
    compulsion of using DMA buffers for USB messages. It also includes proper
    error check for possible short read. So use the wrapper where
    appropriate and remove DMA buffers from the callers.

Johan

[PATCH v2 5/6] USB: serial: keyspan_pda: use usb_control_msg_recv()

[Himadri Pandya]

Use the wrapper function usb_control_msg_recv() that accepts stack
variables and remove dma buffers from callers of usb_control_msg().

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
Changes in v2:
 - Rebase the patch on top of recent changes
 - Drop unrelated style changes
---
 drivers/usb/serial/keyspan_pda.c | 70 ++++++++++++++------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
index 39b0f5f344c2..831dc5f42dea 100644
--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -77,35 +77,28 @@ static int keyspan_pda_get_write_room(struct keyspan_pda_private *priv)
 {
 	struct usb_serial_port *port = priv->port;
 	struct usb_serial *serial = port->serial;
-	u8 *room;
+	u8 room;
 	int rc;
 
-	room = kmalloc(1, GFP_KERNEL);
-	if (!room)
-		return -ENOMEM;
-
-	rc = usb_control_msg(serial->dev,
-			     usb_rcvctrlpipe(serial->dev, 0),
-			     6, /* write_room */
-			     USB_TYPE_VENDOR | USB_RECIP_INTERFACE
-			     | USB_DIR_IN,
-			     0, /* value: 0 means "remaining room" */
-			     0, /* index */
-			     room,
-			     1,
-			     2000);
-	if (rc != 1) {
-		if (rc >= 0)
-			rc = -EIO;
+	rc = usb_control_msg_recv(serial->dev,
+				  0,
+				  6, /* write_room */
+				  USB_TYPE_VENDOR | USB_RECIP_INTERFACE
+				  | USB_DIR_IN,
+				  0, /* value: 0 means "remaining room" */
+				  0, /* index */
+				  &room,
+				  1,
+				  2000,
+				  GFP_KERNEL);
+	if (rc) {
 		dev_dbg(&port->dev, "roomquery failed: %d\n", rc);
-		goto out_free;
+		goto out;
 	}
 
-	dev_dbg(&port->dev, "roomquery says %d\n", *room);
-	rc = *room;
-out_free:
-	kfree(room);
-
+	dev_dbg(&port->dev, "roomquery says %d\n", room);
+	rc = room;
+out:
 	return rc;
 }
 
@@ -381,22 +374,21 @@ static int keyspan_pda_get_modem_info(struct usb_serial *serial,
 				      unsigned char *value)
 {
 	int rc;
-	u8 *data;
-
-	data = kmalloc(1, GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	rc = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-			     3, /* get pins */
-			     USB_TYPE_VENDOR|USB_RECIP_INTERFACE|USB_DIR_IN,
-			     0, 0, data, 1, 2000);
-	if (rc == 1)
-		*value = *data;
-	else if (rc >= 0)
-		rc = -EIO;
+	u8 data;
+
+	rc = usb_control_msg_recv(serial->dev, 0,
+				  3, /* get pins */
+				  USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
+				  USB_DIR_IN,
+				  0,
+				  0,
+				  &data,
+				  1,
+				  2000,
+				  GFP_KERNEL);
+	if (rc == 0)
+		*value = data;
 
-	kfree(data);
 	return rc;
 }
 
-- 
2.17.1

Re: [PATCH v2 5/6] USB: serial: keyspan_pda: use usb_control_msg_recv()

[Johan Hovold]

On Mon, Aug 02, 2021 at 02:01:21AM +0530, Himadri Pandya wrote:
> Use the wrapper function usb_control_msg_recv() that accepts stack
> variables and remove dma buffers from callers of usb_control_msg().
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
> Changes in v2:
>  - Rebase the patch on top of recent changes
>  - Drop unrelated style changes
> ---
>  drivers/usb/serial/keyspan_pda.c | 70 ++++++++++++++------------------
>  1 file changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
> index 39b0f5f344c2..831dc5f42dea 100644
> --- a/drivers/usb/serial/keyspan_pda.c
> +++ b/drivers/usb/serial/keyspan_pda.c
> @@ -77,35 +77,28 @@ static int keyspan_pda_get_write_room(struct keyspan_pda_private *priv)
>  {
>  	struct usb_serial_port *port = priv->port;
>  	struct usb_serial *serial = port->serial;
> -	u8 *room;
> +	u8 room;
>  	int rc;
>  
> -	room = kmalloc(1, GFP_KERNEL);
> -	if (!room)
> -		return -ENOMEM;
> -
> -	rc = usb_control_msg(serial->dev,
> -			     usb_rcvctrlpipe(serial->dev, 0),
> -			     6, /* write_room */
> -			     USB_TYPE_VENDOR | USB_RECIP_INTERFACE
> -			     | USB_DIR_IN,
> -			     0, /* value: 0 means "remaining room" */
> -			     0, /* index */
> -			     room,
> -			     1,
> -			     2000);
> -	if (rc != 1) {
> -		if (rc >= 0)
> -			rc = -EIO;
> +	rc = usb_control_msg_recv(serial->dev,
> +				  0,
> +				  6, /* write_room */
> +				  USB_TYPE_VENDOR | USB_RECIP_INTERFACE
> +				  | USB_DIR_IN,
> +				  0, /* value: 0 means "remaining room" */
> +				  0, /* index */
> +				  &room,
> +				  1,
> +				  2000,
> +				  GFP_KERNEL);
> +	if (rc) {
>  		dev_dbg(&port->dev, "roomquery failed: %d\n", rc);
> -		goto out_free;
> +		goto out;
>  	}
>  
> -	dev_dbg(&port->dev, "roomquery says %d\n", *room);
> -	rc = *room;
> -out_free:
> -	kfree(room);
> -
> +	dev_dbg(&port->dev, "roomquery says %d\n", room);
> +	rc = room;
> +out:

I changed this to just return rc in the error path above directly
instead.

>  	return rc;
>  }

Now applied, thanks.

Johan

[PATCH v2 6/6] USB: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The wrappers usb_control_msg_send/recv eliminate the need of allocating
dma buffers for usb message. They also impose proper error checks on the
return value of usb_control_msg() to handle short read/write. Hence use
the wrappers and remove dma allocations.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
Changes in v2:
 - Fix the caller of klsi_105_chg_port_settings()
 - Drop unnecessary use of the wrappers
---
 drivers/usb/serial/kl5kusb105.c | 79 ++++++++++++++-------------------
 1 file changed, 34 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/serial/kl5kusb105.c b/drivers/usb/serial/kl5kusb105.c
index f1e9628a9907..72d3920c9c48 100644
--- a/drivers/usb/serial/kl5kusb105.c
+++ b/drivers/usb/serial/kl5kusb105.c
@@ -124,16 +124,18 @@ static int klsi_105_chg_port_settings(struct usb_serial_port *port,
 {
 	int rc;
 
-	rc = usb_control_msg(port->serial->dev,
-			usb_sndctrlpipe(port->serial->dev, 0),
-			KL5KUSB105A_SIO_SET_DATA,
-			USB_TYPE_VENDOR | USB_DIR_OUT | USB_RECIP_INTERFACE,
-			0, /* value */
-			0, /* index */
-			settings,
-			sizeof(struct klsi_105_port_settings),
-			KLSI_TIMEOUT);
-	if (rc < 0)
+	rc = usb_control_msg_send(port->serial->dev,
+				  0,
+				  KL5KUSB105A_SIO_SET_DATA,
+				  USB_TYPE_VENDOR | USB_DIR_OUT |
+				  USB_RECIP_INTERFACE,
+				  0, /* value */
+				  0, /* index */
+				  settings,
+				  sizeof(struct klsi_105_port_settings),
+				  KLSI_TIMEOUT,
+				  GFP_KERNEL);
+	if (rc)
 		dev_err(&port->dev,
 			"Change port settings failed (error = %d)\n", rc);
 
@@ -167,28 +169,21 @@ static int klsi_105_get_line_state(struct usb_serial_port *port,
 				   unsigned long *line_state_p)
 {
 	int rc;
-	u8 *status_buf;
+	u8 status_buf[KLSI_STATUSBUF_LEN];
 	__u16 status;
 
-	status_buf = kmalloc(KLSI_STATUSBUF_LEN, GFP_KERNEL);
-	if (!status_buf)
-		return -ENOMEM;
-
 	status_buf[0] = 0xff;
 	status_buf[1] = 0xff;
-	rc = usb_control_msg(port->serial->dev,
-			     usb_rcvctrlpipe(port->serial->dev, 0),
-			     KL5KUSB105A_SIO_POLL,
-			     USB_TYPE_VENDOR | USB_DIR_IN,
-			     0, /* value */
-			     0, /* index */
-			     status_buf, KLSI_STATUSBUF_LEN,
-			     10000
-			     );
-	if (rc != KLSI_STATUSBUF_LEN) {
+	rc = usb_control_msg_recv(port->serial->dev, 0,
+				  KL5KUSB105A_SIO_POLL,
+				  USB_TYPE_VENDOR | USB_DIR_IN,
+				  0, /* value */
+				  0, /* index */
+				  status_buf, KLSI_STATUSBUF_LEN,
+				  10000,
+				  GFP_KERNEL);
+	if (rc) {
 		dev_err(&port->dev, "reading line status failed: %d\n", rc);
-		if (rc >= 0)
-			rc = -EIO;
 	} else {
 		status = get_unaligned_le16(status_buf);
 
@@ -198,7 +193,6 @@ static int klsi_105_get_line_state(struct usb_serial_port *port,
 		*line_state_p = klsi_105_status2linestate(status);
 	}
 
-	kfree(status_buf);
 	return rc;
 }
 
@@ -245,7 +239,7 @@ static int  klsi_105_open(struct tty_struct *tty, struct usb_serial_port *port)
 	int retval = 0;
 	int rc;
 	unsigned long line_state;
-	struct klsi_105_port_settings *cfg;
+	struct klsi_105_port_settings cfg;
 	unsigned long flags;
 
 	/* Do a defined restart:
@@ -255,27 +249,22 @@ static int  klsi_105_open(struct tty_struct *tty, struct usb_serial_port *port)
 	 * Then read the modem line control and store values in
 	 * priv->line_state.
 	 */
-	cfg = kmalloc(sizeof(*cfg), GFP_KERNEL);
-	if (!cfg)
-		return -ENOMEM;
 
-	cfg->pktlen   = 5;
-	cfg->baudrate = kl5kusb105a_sio_b9600;
-	cfg->databits = kl5kusb105a_dtb_8;
-	cfg->unknown1 = 0;
-	cfg->unknown2 = 1;
-	klsi_105_chg_port_settings(port, cfg);
+	cfg.pktlen   = 5;
+	cfg.baudrate = kl5kusb105a_sio_b9600;
+	cfg.databits = kl5kusb105a_dtb_8;
+	cfg.unknown1 = 0;
+	cfg.unknown2 = 1;
+	klsi_105_chg_port_settings(port, &cfg);
 
 	spin_lock_irqsave(&priv->lock, flags);
-	priv->cfg.pktlen   = cfg->pktlen;
-	priv->cfg.baudrate = cfg->baudrate;
-	priv->cfg.databits = cfg->databits;
-	priv->cfg.unknown1 = cfg->unknown1;
-	priv->cfg.unknown2 = cfg->unknown2;
+	priv->cfg.pktlen   = cfg.pktlen;
+	priv->cfg.baudrate = cfg.baudrate;
+	priv->cfg.databits = cfg.databits;
+	priv->cfg.unknown1 = cfg.unknown1;
+	priv->cfg.unknown2 = cfg.unknown2;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	kfree(cfg);
-
 	/* READ_ON and urb submission */
 	rc = usb_serial_generic_open(tty, port);
 	if (rc)
-- 
2.17.1

Re: [PATCH v2 6/6] USB: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Mon, Aug 02, 2021 at 02:01:22AM +0530, Himadri Pandya wrote:
> The wrappers usb_control_msg_send/recv eliminate the need of allocating
> dma buffers for usb message. They also impose proper error checks on the
> return value of usb_control_msg() to handle short read/write. Hence use
> the wrappers and remove dma allocations.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
> Changes in v2:
>  - Fix the caller of klsi_105_chg_port_settings()
>  - Drop unnecessary use of the wrappers

Now applied with an amended commit message:

    USB: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()
    
    The wrappers usb_control_msg_send/recv eliminate the need of
    manually allocating DMA buffers for USB messages. They also treat
    short reads as an error. Hence use the wrappers and remove DMA
    allocations.
    
    Note that short reads are now logged as -EREMOTEIO instead of the amount
    of data read.

I've applied all but the first two patches in the series now. Would you
mind respinning those two?

Thanks,
Johan

Re: [PATCH v2 6/6] USB: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

On Tue, Sep 21, 2021 at 2:41 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Aug 02, 2021 at 02:01:22AM +0530, Himadri Pandya wrote:
> > The wrappers usb_control_msg_send/recv eliminate the need of allocating
> > dma buffers for usb message. They also impose proper error checks on the
> > return value of usb_control_msg() to handle short read/write. Hence use
> > the wrappers and remove dma allocations.
> >
> > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> > ---
> > Changes in v2:
> >  - Fix the caller of klsi_105_chg_port_settings()
> >  - Drop unnecessary use of the wrappers
>
> Now applied with an amended commit message:
>
>     USB: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()
>
>     The wrappers usb_control_msg_send/recv eliminate the need of
>     manually allocating DMA buffers for USB messages. They also treat
>     short reads as an error. Hence use the wrappers and remove DMA
>     allocations.
>
>     Note that short reads are now logged as -EREMOTEIO instead of the amount
>     of data read.
>
> I've applied all but the first two patches in the series now. Would you
> mind respinning those two?
>
> Thanks,
> Johan

Thanks for reviewing and applying these patches. And yes, I'm sending
the revised version for the first two.

Regards,
Himadri

Re: [PATCH v2 0/6] USB: serial: use wrappers for usb_control_msg()

[Johan Hovold]

On Mon, Aug 02, 2021 at 02:01:16AM +0530, Himadri Pandya wrote:
> There are many usages of usb_control_msg() that can use the new wrapper
> functions usb_contro_msg_send() & usb_control_msg_recv() for better
> error checks on short reads and writes.

As I said before, there no need to worry about short writes as that will
always return an error. So this description (and some of the commit
messages) needs a bit of work.

> They can also be used to avoid
> allocating redundant dma buffers that are generally required to use
> usb_control_msg(). 

The DMA buffers are anything but redundant; they are required. But the
wrappers can be used to not have to manage the dma buffers explicitly.

> Hence use them whenever possible and avoid using
> usb_control_msg() directly.
> 
> Changes in v2:
>  - Drop unnecessary use of wrappers
>  - Drop unrelated style changes
> 
> Additional Info:
> - This is a super late follow-up on v1 that was submitted a while
> ago(my sincere apologies). (All patches are rebased and
> compile tested.)
> - v1 patches: https://lkml.org/lkml/2020/11/4/40
> - Patches that introduced the wrapper functions:
>   https://lkml.org/lkml/2020/9/14/859

No worries, I'll go review the patches now.

Johan