USB: serial: use wrappers for usb_control_msg()

PATCH In review Himadri Pandya 2 participants 9 messages Oct 1, 2021 – Oct 27, 2021

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Himadri Pandya

author

2021-10-01 06:59 UTC #0 lore raw

v3 cover USB: serial: use wrappers for usb_control_msg()

There are many usages of usb_control_msg() that can instead use the new wrapper functions usb_contro_msg_send() & usb_control_msg_recv() for better error checks on short reads. The wrappers also handle the allocation of dma buffers and callers don't need to manage them explicitly.

This is a follow up on v2 with 6 patches. 4 of them are already applied so v3 only includes the two remaining patches. The patches are compile-tested only.

Changes in v3:
- Rephrase commit messages
- Add notes on omitting the logs for short read sizes

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

Himadri Pandya (2):
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()

 drivers/usb/serial/ch341.c  |  90 ++++++++----------------------
 drivers/usb/serial/cp210x.c | 106 ++++++++++--------------------------
 2 files changed, 54 insertions(+), 142 deletions(-)

Signature · 1 line

2.17.1

Himadri Pandya

author

2021-10-01 07:00 UTC #1 lore raw

PATCH v3 1/2 USB: serial: ch314: use usb_control_msg_recv() and usb_control_msg_send()

usb_control_msg_send/recv are new wrapper functions for usb_control_msg() that have proper error checks for short reads. These functions can also accept data buffer on stack. Hence use these functions to simplify error handling for short reads. Short reads will now get reported as -EREMOTEIO with no indication of how short the transfer was.

Signed-off-by Himadri Pandya <[email protected]>

---
Changes in v3:
- Rephrase the commit message
- Include a note on not reporting size of the short reads in the commit
- Drop unnecessary changes in ch341_control_out()
- Drop a non-relevant style change
- Remove some more "out" labels
- Remove unnecessary return statement from a void function

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 | 90 ++++++++++----------------------------
 1 file changed, 24 insertions(+), 66 deletions(-)

drivers/usb/serial/ch341.c +24 ~~-66~~

index 2db917eab799..8aecc1f0dee4 100644
@@ -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,18 @@ 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)
-		goto out;
+	if (r)
+		return r;
 
 	spin_lock_irqsave(&priv->lock, flags);
 	priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-out:	kfree(buffer);
 	return r;
 }

@@ -312,30 +297,25 @@ 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)
-		goto out;
+	if (r)
+		return r;
 	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)
-		goto out;
+	if (r)
+		return r;
 
 	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
-		goto out;
+		return r;
 
 	r = ch341_set_handshake(dev, priv->mcr);
 
-out:	kfree(buffer);
 	return r;
 }

@@ -345,39 +325,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 +611,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) {
+	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 +641,9 @@ 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);
 }
 
 static int ch341_tiocmset(struct tty_struct *tty,

Signature · 1 line

2.17.1

Johan Hovold

maintainer

2021-10-27 13:04 UTC #2 lore raw

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

Quoted 5 lines · Himadri Pandya

usb_control_msg_send/recv are new wrapper functions for usb_control_msg() that have proper error checks for short reads. These functions can also accept data buffer on stack. Hence use these functions to simplify error handling for short reads. Short reads will now get reported as -EREMOTEIO with no indication of how short the transfer was.

You're no longer using usb_control_msg_send() so the commit message including Subject needs to be updated.

Quoted 34 lines

Signed-off-by Himadri Pandya <[email protected]>
---
Changes in v3:
- Rephrase the commit message
- Include a note on not reporting size of the short reads in the commit
- Drop unnecessary changes in ch341_control_out()
- Drop a non-relevant style change
- Remove some more "out" labels
- Remove unnecessary return statement from a void function
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 | 90 ++++++++++----------------------------
 1 file changed, 24 insertions(+), 66 deletions(-)
drivers/usb/serial/ch341.c +0 -8
index 2db917eab799..8aecc1f0dee4 100644
@@ -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);

drivers/usb/serial/ch341.c static int ch341_control_in(struct usb_device *dev, · line 136

Quoted 10 lines

drivers/usb/serial/ch341.c +3 -7

-			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);

Line is now over 80 chars for no good reason.

drivers/usb/serial/ch341.c static int ch341_control_in(struct usb_device *dev, · line 138

Quoted 2 lines

drivers/usb/serial/ch341.c +2 -0

+	if (r)
+		dev_err(&dev->dev, "failed to receive control message: %d\n", r);

Here too.

drivers/usb/serial/ch341.c static int ch341_control_in(struct usb_device *dev, · line 140

Quoted 3 lines

drivers/usb/serial/ch341.c +1 -1
-	return 0;
+	return r;

I'd prefer returning the errno in the error path above and keep an explicit zero here.

Quoted 18 lines

drivers/usb/serial/ch341.c +1 -6

 }
 
 #define CH341_CLKRATE		48000000

@@ -287,23 +277,18 @@ 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)

drivers/usb/serial/ch341.c static int ch341_set_handshake(struct usb_device *dev, u8 control) · line 292

Quoted 10 lines

drivers/usb/serial/ch341.c +2 -2

-		goto out;
+	if (r)
+		return r;
 
 	spin_lock_irqsave(&priv->lock, flags);
 	priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-out:	kfree(buffer);
 	return r;

This should now be

	return 0;

Quoted 17 lines

drivers/usb/serial/ch341.c +1 -6

@@ -312,30 +297,25 @@ 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)

drivers/usb/serial/ch341.c out: kfree(buffer); · line 311

Quoted 10 lines

drivers/usb/serial/ch341.c +4 -3

-		goto out;
+	if (r)
+		return r;
 	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)
-		goto out;
+	if (r)
+		return r;

Now an unrelated change.

drivers/usb/serial/ch341.c out: kfree(buffer); · line 319

Quoted 10 lines

drivers/usb/serial/ch341.c +1 -2

 	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
-		goto out;
+		return r;
 
 	r = ch341_set_handshake(dev, priv->mcr);
 
-out:	kfree(buffer);
 	return r;

This looks a bit inconsistent now so I'll make this an explicit return 0 too.

Quoted 16 lines

drivers/usb/serial/ch341.c +1 -5

@@ -345,39 +325,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

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_serial_port *port) · line 339

Quoted 10 lines

drivers/usb/serial/ch341.c +3 -3

 	 * 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);

Unnecessarily long line > 80 chars.

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_serial_port *port) · line 367

Quoted 4 lines

drivers/usb/serial/ch341.c +0 -1

 	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;

Oops, you just broke the driver. :(

This request is used to detect quirky devices and a return value of -EPIPE here should not abort probe by returning an error to the caller of this function.

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_serial_port *port) · line 343

Quoted 7 lines

drivers/usb/serial/ch341.c +1 -6

-		goto out;
-	}
-
-	if (r != size) {
-		if (r >= 0)
-			r = -EIO;
+	} else if (r)

And you still need brackets on the else branch.

Quoted 38 lines

drivers/usb/serial/ch341.c +3 ~~-13~~

 		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 +611,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) {
+	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]);

drivers/usb/serial/ch341.c static void ch341_break_ctl(struct tty_struct *tty, int break_state) · line 644

Quoted 5 lines

drivers/usb/serial/ch341.c +1 -1

@@ -681,11 +641,9 @@ 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)

Now also an unrelated change.

drivers/usb/serial/ch341.c static void ch341_break_ctl(struct tty_struct *tty, int break_state) · line 649

Quoted 7 lines

drivers/usb/serial/ch341.c +0 -2

 		dev_err(&port->dev, "%s - USB control write error (%d)\n",
 				__func__, r);
-out:
-	kfree(break_reg);
 }
 
 static int ch341_tiocmset(struct tty_struct *tty,

I've fixed up the above so we don't have to spend any more time on this.

The result is here

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=74f266455062c158f343bc3aa35ef84b3eb7adf1

Johan

Himadri Pandya

author

2021-10-27 13:28 UTC #3 lore raw

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

Quoted 49 lines · Johan Hovold

Quoted 5 lines · Himadri Pandya
usb_control_msg_send/recv are new wrapper functions for usb_control_msg() that have proper error checks for short reads. These functions can also accept data buffer on stack. Hence use these functions to simplify error handling for short reads. Short reads will now get reported as -EREMOTEIO with no indication of how short the transfer was.
You're no longer using usb_control_msg_send() so the commit message including Subject needs to be updated.
Quoted 38 lines
Signed-off-by Himadri Pandya <[email protected]>
---
Changes in v3:
- Rephrase the commit message
- Include a note on not reporting size of the short reads in the commit
- Drop unnecessary changes in ch341_control_out()
- Drop a non-relevant style change
- Remove some more "out" labels
- Remove unnecessary return statement from a void function
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 | 90 ++++++++++----------------------------
 1 file changed, 24 insertions(+), 66 deletions(-)
drivers/usb/serial/ch341.c +0 ~~-12~~
index 2db917eab799..8aecc1f0dee4 100644
@@ -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",

Quoted 8 lines

drivers/usb/serial/ch341.c static int ch341_control_in(struct usb_device *dev, · line 136
Quoted 6 lines
drivers/usb/serial/ch341.c +3 -3
-                     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);
Line is now over 80 chars for no good reason.

Sorry, I changed machines and the checkpatch must not be configured properly in the new set-up.

Quoted 4 lines

drivers/usb/serial/ch341.c static int ch341_control_in(struct usb_device *dev, · line 138
Quoted 3 lines
drivers/usb/serial/ch341.c +2 -0
+     if (r)
+             dev_err(&dev->dev, "failed to receive control message: %d\n", r);

Quoted 9 lines

Here too.
drivers/usb/serial/ch341.c static int ch341_control_in(struct usb_device *dev, · line 140
Quoted 3 lines
drivers/usb/serial/ch341.c +1 -1
-     return 0;
+     return r;
I'd prefer returning the errno in the error path above and keep an explicit zero here.

Okay.

Quoted 26 lines

drivers/usb/serial/ch341.c +3 -7

 }
 
 #define CH341_CLKRATE                48000000

@@ -287,23 +277,18 @@ 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)
-             goto out;
+     if (r)
+             return r;
 
      spin_lock_irqsave(&priv->lock, flags);
      priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
      spin_unlock_irqrestore(&priv->lock, flags);

Quoted 7 lines

drivers/usb/serial/ch341.c static int ch341_set_handshake(struct usb_device *dev, u8 control) · line 292
Quoted 2 lines
drivers/usb/serial/ch341.c +0 -1
-out: kfree(buffer);
      return r;
This should now be
        return 0;

Yes. The function was returning the negative error value before the change. But now it doesn't need to as we are already taking care of it in the wrapper.

Quoted 22 lines

drivers/usb/serial/ch341.c +3 -7

@@ -312,30 +297,25 @@ 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)
-             goto out;
+     if (r)
+             return r;
      dev_dbg(&dev->dev, "Chip version: 0x%02x\n", buffer[0]);

Quoted 7 lines

drivers/usb/serial/ch341.c out: kfree(buffer); · line 311
Quoted 5 lines
drivers/usb/serial/ch341.c +2 -2
      r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0, 0);
-     if (r < 0)
-             goto out;
+     if (r)
+             return r;
Now an unrelated change.

I think it is a related change because we are removing the out label.

Quoted 7 lines

drivers/usb/serial/ch341.c +1 -1

      r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
      if (r < 0)
-             goto out;
+             return r;

Quoted 7 lines

drivers/usb/serial/ch341.c out: kfree(buffer); · line 319
Quoted 4 lines
drivers/usb/serial/ch341.c +0 -1
      r = ch341_set_handshake(dev, priv->mcr);
 
-out: kfree(buffer);
      return r;
This looks a bit inconsistent now so I'll make this an explicit return 0 too.

Okay.

Quoted 35 lines

Quoted 17 lines

drivers/usb/serial/ch341.c +1 -5

@@ -345,39 +325,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

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_serial_port *port) · line 339

Quoted 10 lines

drivers/usb/serial/ch341.c +3 -3

       * 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);

Unnecessarily long line > 80 chars.

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_serial_port *port) · line 367

Quoted 4 lines

drivers/usb/serial/ch341.c +0 -1

      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;

Quoted 5 lines

Oops, you just broke the driver. :(
This request is used to detect quirky devices and a return value of -EPIPE here should not abort probe by returning an error to the caller of this function.

Understood. Thanks for catching it.

Quoted 10 lines

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_serial_port *port) · line 343
Quoted 8 lines
drivers/usb/serial/ch341.c +1 -6
-             goto out;
-     }
-
-     if (r != size) {
-             if (r >= 0)
-                     r = -EIO;
+     } else if (r)
And you still need brackets on the else branch.

Yes. Okay.

Quoted 38 lines

drivers/usb/serial/ch341.c +3 ~~-13~~

              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 +611,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) {
+     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]);

Quoted 9 lines

drivers/usb/serial/ch341.c static void ch341_break_ctl(struct tty_struct *tty, int break_state) · line 644

Quoted 6 lines

drivers/usb/serial/ch341.c +1 -1

@@ -681,11 +641,9 @@ 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)

Now also an unrelated change.

Maybe I misunderstood your comments on v2. I thought you asked to get rid of the out labels in callers.

Quoted 10 lines

drivers/usb/serial/ch341.c static void ch341_break_ctl(struct tty_struct *tty, int break_state) · line 649
Quoted 7 lines
drivers/usb/serial/ch341.c +0 -2
              dev_err(&port->dev, "%s - USB control write error (%d)\n",
                              __func__, r);
-out:
-     kfree(break_reg);
 }
 
 static int ch341_tiocmset(struct tty_struct *tty,
I've fixed up the above so we don't have to spend any more time on this.

Thank you.

Quoted 5 lines

The result is here

        https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=74f266455062c158f343bc3aa35ef84b3eb7adf1

Johan

Looks good. Thanks for taking the patch.

Regards,
Himadri

Johan Hovold

maintainer

2021-10-27 13:38 UTC #4 lore raw

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

Quoted 31 lines · Himadri Pandya

Quoted 30 lines · Johan Hovold

Quoted 15 lines · Himadri Pandya

drivers/usb/serial/ch341.c +1 -6

@@ -287,23 +277,18 @@ 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)

drivers/usb/serial/ch341.c static int ch341_set_handshake(struct usb_device *dev, u8 control) · line 292

Quoted 10 lines

drivers/usb/serial/ch341.c +2 -2

-             goto out;
+     if (r)
+             return r;
 
      spin_lock_irqsave(&priv->lock, flags);
      priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
      spin_unlock_irqrestore(&priv->lock, flags);
 
-out: kfree(buffer);
      return r;

This should now be

Quoted 6 lines

Quoted 2 lines
        return 0;
Yes. The function was returning the negative error value before the change. But now it doesn't need to as we are already taking care of it in the wrapper.

It has more to do with the fact that we now return early on errors so r will always be zero here. It's better to be explicit about that.

Quoted 30 lines

Quoted 22 lines

drivers/usb/serial/ch341.c +3 -7

@@ -312,30 +297,25 @@ 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)
-             goto out;
+     if (r)
+             return r;
      dev_dbg(&dev->dev, "Chip version: 0x%02x\n", buffer[0]);

Quoted 7 lines

drivers/usb/serial/ch341.c out: kfree(buffer); · line 311
Quoted 5 lines
drivers/usb/serial/ch341.c +2 -2
      r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0, 0);
-     if (r < 0)
-             goto out;
+     if (r)
+             return r;
Now an unrelated change.

Quoted 1 line

I think it is a related change because we are removing the out label.

Sorry, I meant that the (r < 0) change was unrelated since you're no longer touching ch341_control_out(). The return is indeed still needed.

Quoted 37 lines

Quoted 27 lines

drivers/usb/serial/ch341.c +3 -7

@@ -647,23 +611,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) {
+     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]);

Quoted 9 lines

drivers/usb/serial/ch341.c static void ch341_break_ctl(struct tty_struct *tty, int break_state) · line 644

Quoted 6 lines

drivers/usb/serial/ch341.c +1 -1

@@ -681,11 +641,9 @@ 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)

Now also an unrelated change.

Quoted 2 lines

Maybe I misunderstood your comments on v2. I thought you asked to get rid of the out labels in callers.

Yes, but as above I'm referring to the (r < 0) change for ch341_control_out() which is now unrelated to the rest of the patch.

Johan

Himadri Pandya

author

2021-10-27 13:47 UTC #5 lore raw

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

Quoted 36 lines · Johan Hovold

Quoted 34 lines · Himadri Pandya

Quoted 32 lines · Johan Hovold

Quoted 15 lines · Himadri Pandya

drivers/usb/serial/ch341.c +1 -6

@@ -287,23 +277,18 @@ 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)

drivers/usb/serial/ch341.c static int ch341_set_handshake(struct usb_device *dev, u8 control) · line 292

Quoted 10 lines

drivers/usb/serial/ch341.c +2 -2

-             goto out;
+     if (r)
+             return r;
 
      spin_lock_irqsave(&priv->lock, flags);
      priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
      spin_unlock_irqrestore(&priv->lock, flags);
 
-out: kfree(buffer);
      return r;

This should now be

        return 0;

Quoted 7 lines

Quoted 3 lines
Yes. The function was returning the negative error value before the change. But now it doesn't need to as we are already taking care of it in the wrapper.
It has more to do with the fact that we now return early on errors so r will always be zero here. It's better to be explicit about that.

Okay. Right.

Quoted 30 lines

Quoted 29 lines

Quoted 17 lines

drivers/usb/serial/ch341.c +1 -6

@@ -312,30 +297,25 @@ 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)

drivers/usb/serial/ch341.c out: kfree(buffer); · line 311

Quoted 10 lines

drivers/usb/serial/ch341.c +4 -3

-             goto out;
+     if (r)
+             return r;
      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)
-             goto out;
+     if (r)
+             return r;

Now an unrelated change.

Quoted 5 lines

Quoted 1 line
I think it is a related change because we are removing the out label.
Sorry, I meant that the (r < 0) change was unrelated since you're no longer touching ch341_control_out(). The return is indeed still needed.

Oh, okay. My bad.

Quoted 37 lines

Quoted 36 lines

Quoted 28 lines

drivers/usb/serial/ch341.c +3 -7

@@ -647,23 +611,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) {
+     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]);

drivers/usb/serial/ch341.c static void ch341_break_ctl(struct tty_struct *tty, int break_state) · line 644

Quoted 5 lines

drivers/usb/serial/ch341.c +1 -1

@@ -681,11 +641,9 @@ 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)

Now also an unrelated change.

Quoted 7 lines

Quoted 2 lines
Maybe I misunderstood your comments on v2. I thought you asked to get rid of the out labels in callers.
Yes, but as above I'm referring to the (r < 0) change for ch341_control_out() which is now unrelated to the rest of the patch.
Johan

Yes, got it.

Thanks,
Himadri

Himadri Pandya

author

2021-10-01 07:00 UTC #6 lore raw

PATCH v3 2/2 USB: serial: cp210x: use usb_control_msg_recv() and usb_control_msg_send()

In reply to Himadri Pandya

The new wrapper functions for usb_control_msg() can accept data from stack and treat short reads as error. Hence use the wrappers functions. Please note that because of this change, cp210x_read_reg_block() will no longer log the length of short reads.

Signed-off-by Himadri Pandya <[email protected]>

---
Changes in v3:
- Rephrase the commit message
- Explicitly mention that short reads don't log length now

Changes in v2:
- Drop unrelated style fixes

---
 drivers/usb/serial/cp210x.c | 106 ++++++++++--------------------------
 1 file changed, 30 insertions(+), 76 deletions(-)

drivers/usb/serial/cp210x.c +30 ~~-76~~

index 189279869a8b..3c3ca46b0b82 100644
@@ -631,29 +631,19 @@ 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_GET_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;
 }

@@ -672,30 +662,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;
 }

@@ -730,21 +707,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(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			req, REQTYPE_HOST_TO_INTERFACE, 0,
-			port_priv->bInterfaceNumber, dmabuf, bufsize,
-			USB_CTRL_SET_TIMEOUT);
-
-	kfree(dmabuf);
+	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);
 
-	if (result < 0) {
+	if (result) {
 		dev_err(&port->dev, "failed set req 0x%x size %d status: %d\n",
 				req, bufsize, result);
 		return result;

@@ -773,21 +743,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_send(serial->dev, 0, CP210X_VENDOR_SPECIFIC,
+				      type, val, cp210x_interface_num(serial),
+				      buf, bufsize, USB_CTRL_SET_TIMEOUT,
+				      GFP_KERNEL);
 
-	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);
-
-	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);

@@ -952,27 +915,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;
 }

Signature · 1 line

2.17.1

Johan Hovold

maintainer

2021-10-27 13:18 UTC #7 lore raw

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

Quoted 5 lines · Himadri Pandya

The new wrapper functions for usb_control_msg() can accept data from stack and treat short reads as error. Hence use the wrappers functions. Please note that because of this change, cp210x_read_reg_block() will no longer log the length of short reads.

Quoted 8 lines

Signed-off-by Himadri Pandya <[email protected]>
---
Changes in v3:
- Rephrase the commit message
- Explicitly mention that short reads don't log length now
Changes in v2:
- Drop unrelated style fixes

This looks good now, but I did do some minor style changes described below.

Quoted 22 lines

---
 drivers/usb/serial/cp210x.c | 106 ++++++++++--------------------------
 1 file changed, 30 insertions(+), 76 deletions(-)

drivers/usb/serial/cp210x.c +0 -7

index 189279869a8b..3c3ca46b0b82 100644
@@ -631,29 +631,19 @@ 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,

drivers/usb/serial/cp210x.c static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req, · line 641

Quoted 10 lines

drivers/usb/serial/cp210x.c +5 -5

-			USB_CTRL_GET_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);

The indentation style of this driver is a bit inconsistent but there's no need to change to the open-parenthesis alignment style when you can avoid it (it's mostly just "checkpacth.pl --subjective" that insists on it).

Indenting continuation lines two tabs is just fine and avoids excessive indentation and having to realign arguments when symbol names change.

drivers/usb/serial/cp210x.c static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req, · line 647

Quoted 10 lines

drivers/usb/serial/cp210x.c +1 -4

+	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;

I changed this to explicit zero and return an error above instead.

Quoted 24 lines

drivers/usb/serial/cp210x.c +5 ~~-13~~

@@ -952,27 +915,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,

drivers/usb/serial/cp210x.c static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port, · line 975

Quoted 10 lines

drivers/usb/serial/cp210x.c +4 -5

+				      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);

The above is now also better handled with an explicit error check and early return and the doing the *count assignment in the success path.

drivers/usb/serial/cp210x.c static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port, · line 931

Quoted 3 lines

drivers/usb/serial/cp210x.c +0 -0
 	return result;
 }

Now applied with the above changes. The result is here:

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=f5cfbecb0a162319464c9408420282d22ed69721

Johan

Himadri Pandya

author

2021-10-27 13:34 UTC #8 lore raw

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

Quoted 57 lines · Johan Hovold

Quoted 5 lines · Himadri Pandya
The new wrapper functions for usb_control_msg() can accept data from stack and treat short reads as error. Hence use the wrappers functions. Please note that because of this change, cp210x_read_reg_block() will no longer log the length of short reads.
Quoted 8 lines
Signed-off-by Himadri Pandya <[email protected]>
---
Changes in v3:
- Rephrase the commit message
- Explicitly mention that short reads don't log length now
Changes in v2:
- Drop unrelated style fixes
This looks good now, but I did do some minor style changes described below.
Quoted 22 lines
---
 drivers/usb/serial/cp210x.c | 106 ++++++++++--------------------------
 1 file changed, 30 insertions(+), 76 deletions(-)
drivers/usb/serial/cp210x.c +0 -7
index 189279869a8b..3c3ca46b0b82 100644
@@ -631,29 +631,19 @@ 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,
drivers/usb/serial/cp210x.c static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req, · line 641
Quoted 10 lines
drivers/usb/serial/cp210x.c +5 -5
-                     USB_CTRL_GET_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);
The indentation style of this driver is a bit inconsistent but there's no need to change to the open-parenthesis alignment style when you can avoid it (it's mostly just "checkpacth.pl --subjective" that insists on it).

Quoted 3 lines

Indenting continuation lines two tabs is just fine and avoids excessive indentation and having to realign arguments when symbol names change.

Thanks for clearing this up. I followed the checkpatch suggestion in v2 and then came back to the original style of the driver in v3. Feeling the extra spaces to match the indentation wasn't really fun.

Quoted 7 lines

drivers/usb/serial/cp210x.c +1 -2

+     if (result) {
              dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
                              req, bufsize, result);
-             if (result >= 0)
-                     result = -EIO;
      }

Quoted 5 lines

drivers/usb/serial/cp210x.c static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req, · line 647
Quoted 3 lines
drivers/usb/serial/cp210x.c +0 -2
-     kfree(dmabuf);
-
      return result;
I changed this to explicit zero and return an error above instead.

Okay.

Quoted 30 lines

drivers/usb/serial/cp210x.c +9 ~~-13~~

@@ -952,27 +915,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);

Quoted 9 lines

drivers/usb/serial/cp210x.c static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port, · line 975
Quoted 5 lines
drivers/usb/serial/cp210x.c +0 -5
-             if (result >= 0)
-                     result = -EIO;
-     }
-
-     kfree(sts);
The above is now also better handled with an explicit error check and early return and the doing the *count assignment in the success path.

Yes.

Quoted 9 lines

drivers/usb/serial/cp210x.c static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port, · line 931
Quoted 3 lines
drivers/usb/serial/cp210x.c +0 -0
      return result;
 }
Now applied with the above changes. The result is here:
        https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=f5cfbecb0a162319464c9408420282d22ed69721
Johan

Thank you.

Regards,
Himadri