[Linux-kernel-mentees] [PATCH 00/15] usb: serial: avoid using usb_control_msg() directly

[Linux-kernel-mentees] [PATCH 00/15] usb: serial: avoid using usb_control_msg() directly

[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. Hence use them whenever possible
and avoid using usb_control_msg() directly.

Himadri Pandya (15):
  usb: serial: ark3116: use usb_control_msg_recv() and
    usb_control_msg_send()
  usb: serial: belkin_sa: use usb_control_msg_send()
  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: cypress_m8: 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: f81534: use usb_control_msg_recv() and
    usb_control_msg_send()
  usb: serial: ftdi_sio: use usb_control_msg_recv() and
    usb_control_msg_send()
  usb: serial: io_edgeport: use usb_control_msg_recv() and
    usb_control_msg_send()
  usb: serial: io_ti: use usb_control_msg_recv() and
    usb_control_msg_send()
  usb: serial: ipaq: use usb_control_msg_send()
  usb: serial: ipw: use usb_control_msg_send()
  usb: serial: iuu_phoenix: use usb_control_msg_send()
  usb: serial: keyspan_pda: use usb_control_msg_recv() and
    usb_control_msg_send()
  usb: serial: kl5kusb105: use usb_control_msg_recv() and
    usb_control_msg_send()

 drivers/usb/serial/ark3116.c     |  29 +----
 drivers/usb/serial/belkin_sa.c   |  35 +++---
 drivers/usb/serial/ch341.c       |  45 +++-----
 drivers/usb/serial/cp210x.c      | 148 +++++++------------------
 drivers/usb/serial/cypress_m8.c  |  38 ++++---
 drivers/usb/serial/f81232.c      |  88 +++------------
 drivers/usb/serial/f81534.c      |  63 +++--------
 drivers/usb/serial/ftdi_sio.c    | 182 +++++++++++++------------------
 drivers/usb/serial/io_edgeport.c |  73 +++++--------
 drivers/usb/serial/io_ti.c       |  28 ++---
 drivers/usb/serial/ipaq.c        |   9 +-
 drivers/usb/serial/ipw.c         | 107 ++++++------------
 drivers/usb/serial/iuu_phoenix.c |   5 +-
 drivers/usb/serial/keyspan_pda.c | 172 ++++++++++++-----------------
 drivers/usb/serial/kl5kusb105.c  |  94 ++++++++--------
 15 files changed, 406 insertions(+), 710 deletions(-)

-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 01/15] usb: serial: ark3116: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/ark3116.c | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
index 71a9206ea1e2..51302892c779 100644
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -77,38 +77,17 @@ struct ark3116_private {
 static int ark3116_write_reg(struct usb_serial *serial,
 			     unsigned reg, __u8 val)
 {
-	int result;
 	 /* 0xfe 0x40 are magic values taken from original driver */
-	result = usb_control_msg(serial->dev,
-				 usb_sndctrlpipe(serial->dev, 0),
-				 0xfe, 0x40, val, reg,
-				 NULL, 0, ARK_TIMEOUT);
-	if (result)
-		return result;
-
-	return 0;
+	return usb_control_msg_send(serial->dev, 0, 0xfe, 0x40, val, reg, NULL, 0,
+				    ARK_TIMEOUT, GFP_KERNEL);
 }
 
 static int ark3116_read_reg(struct usb_serial *serial,
 			    unsigned reg, unsigned char *buf)
 {
-	int result;
 	/* 0xfe 0xc0 are magic values taken from original driver */
-	result = usb_control_msg(serial->dev,
-				 usb_rcvctrlpipe(serial->dev, 0),
-				 0xfe, 0xc0, 0, reg,
-				 buf, 1, ARK_TIMEOUT);
-	if (result < 1) {
-		dev_err(&serial->interface->dev,
-				"failed to read register %u: %d\n",
-				reg, result);
-		if (result >= 0)
-			result = -EIO;
-
-		return result;
-	}
-
-	return 0;
+	return usb_control_msg_recv(serial->dev, 0, 0xfe, 0xc0, 0, reg, buf, 1,
+				    ARK_TIMEOUT, GFP_KERNEL);
 }
 
 static inline int calc_divisor(int bps)
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 01/15] usb: serial: ark3116: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:49PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/ark3116.c | 29 ++++-------------------------
>  1 file changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
> index 71a9206ea1e2..51302892c779 100644
> --- a/drivers/usb/serial/ark3116.c
> +++ b/drivers/usb/serial/ark3116.c
> @@ -77,38 +77,17 @@ struct ark3116_private {
>  static int ark3116_write_reg(struct usb_serial *serial,
>  			     unsigned reg, __u8 val)
>  {
> -	int result;
>  	 /* 0xfe 0x40 are magic values taken from original driver */
> -	result = usb_control_msg(serial->dev,
> -				 usb_sndctrlpipe(serial->dev, 0),
> -				 0xfe, 0x40, val, reg,
> -				 NULL, 0, ARK_TIMEOUT);
> -	if (result)
> -		return result;
> -
> -	return 0;
> +	return usb_control_msg_send(serial->dev, 0, 0xfe, 0x40, val, reg, NULL, 0,
> +				    ARK_TIMEOUT, GFP_KERNEL);

For control transfers without a data stage there's no point in using
usb_control_msg_send() as it already returns a negative errno on error
or 0 on success.

>  }
>  
>  static int ark3116_read_reg(struct usb_serial *serial,
>  			    unsigned reg, unsigned char *buf)
>  {
> -	int result;
>  	/* 0xfe 0xc0 are magic values taken from original driver */
> -	result = usb_control_msg(serial->dev,
> -				 usb_rcvctrlpipe(serial->dev, 0),
> -				 0xfe, 0xc0, 0, reg,
> -				 buf, 1, ARK_TIMEOUT);
> -	if (result < 1) {
> -		dev_err(&serial->interface->dev,
> -				"failed to read register %u: %d\n",
> -				reg, result);
> -		if (result >= 0)
> -			result = -EIO;
> -
> -		return result;
> -	}
> -
> -	return 0;
> +	return usb_control_msg_recv(serial->dev, 0, 0xfe, 0xc0, 0, reg, buf, 1,
> +				    ARK_TIMEOUT, GFP_KERNEL);

This driver already use a DMA-able transfer buffer which is allocated
once and then passed to this helper repeatedly. This change would
introduce additional redandant memdup + memcpy for every call and for no
real gain.

You also have an unrelated change here as you simply remove an existing
error message.

Please drop this patch.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 02/15] usb: serial: belkin_sa: use usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_send() nicely wraps usb_control_msg() with
proper error check. Hence use the wrapper instead of calling
usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/belkin_sa.c | 35 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/serial/belkin_sa.c b/drivers/usb/serial/belkin_sa.c
index 9bb123ab9bc9..a5ff55f48303 100644
--- a/drivers/usb/serial/belkin_sa.c
+++ b/drivers/usb/serial/belkin_sa.c
@@ -105,9 +105,10 @@ struct belkin_sa_private {
 #define WDR_TIMEOUT 5000 /* default urb timeout */
 
 /* assumes that struct usb_serial *serial is available */
-#define BSA_USB_CMD(c, v) usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0), \
-					    (c), BELKIN_SA_SET_REQUEST_TYPE, \
-					    (v), 0, NULL, 0, WDR_TIMEOUT)
+#define BSA_USB_CMD(c, v) usb_control_msg_send(serial->dev, 0, (c), \
+					       BELKIN_SA_SET_REQUEST_TYPE, \
+					       (v), 0, NULL, 0, WDR_TIMEOUT, \
+					       GFP_KERNEL)
 
 static int belkin_sa_port_probe(struct usb_serial_port *port)
 {
@@ -309,12 +310,11 @@ static void belkin_sa_set_termios(struct tty_struct *tty,
 		/* reassert DTR and (maybe) RTS on transition from B0 */
 		if ((old_cflag & CBAUD) == B0) {
 			control_state |= (TIOCM_DTR|TIOCM_RTS);
-			if (BSA_USB_CMD(BELKIN_SA_SET_DTR_REQUEST, 1) < 0)
+			if (BSA_USB_CMD(BELKIN_SA_SET_DTR_REQUEST, 1))
 				dev_err(&port->dev, "Set DTR error\n");
 			/* don't set RTS if using hardware flow control */
 			if (!(old_cflag & CRTSCTS))
-				if (BSA_USB_CMD(BELKIN_SA_SET_RTS_REQUEST
-								, 1) < 0)
+				if (BSA_USB_CMD(BELKIN_SA_SET_RTS_REQUEST, 1))
 					dev_err(&port->dev, "Set RTS error\n");
 		}
 	}
@@ -330,18 +330,18 @@ static void belkin_sa_set_termios(struct tty_struct *tty,
 
 		/* Report the actual baud rate back to the caller */
 		tty_encode_baud_rate(tty, baud, baud);
-		if (BSA_USB_CMD(BELKIN_SA_SET_BAUDRATE_REQUEST, urb_value) < 0)
+		if (BSA_USB_CMD(BELKIN_SA_SET_BAUDRATE_REQUEST, urb_value))
 			dev_err(&port->dev, "Set baudrate error\n");
 	} else {
 		/* Disable flow control */
 		if (BSA_USB_CMD(BELKIN_SA_SET_FLOW_CTRL_REQUEST,
-						BELKIN_SA_FLOW_NONE) < 0)
+				BELKIN_SA_FLOW_NONE))
 			dev_err(&port->dev, "Disable flowcontrol error\n");
 		/* Drop RTS and DTR */
 		control_state &= ~(TIOCM_DTR | TIOCM_RTS);
-		if (BSA_USB_CMD(BELKIN_SA_SET_DTR_REQUEST, 0) < 0)
+		if (BSA_USB_CMD(BELKIN_SA_SET_DTR_REQUEST, 0))
 			dev_err(&port->dev, "DTR LOW error\n");
-		if (BSA_USB_CMD(BELKIN_SA_SET_RTS_REQUEST, 0) < 0)
+		if (BSA_USB_CMD(BELKIN_SA_SET_RTS_REQUEST, 0))
 			dev_err(&port->dev, "RTS LOW error\n");
 	}
 
@@ -352,7 +352,7 @@ static void belkin_sa_set_termios(struct tty_struct *tty,
 						: BELKIN_SA_PARITY_EVEN;
 		else
 			urb_value = BELKIN_SA_PARITY_NONE;
-		if (BSA_USB_CMD(BELKIN_SA_SET_PARITY_REQUEST, urb_value) < 0)
+		if (BSA_USB_CMD(BELKIN_SA_SET_PARITY_REQUEST, urb_value))
 			dev_err(&port->dev, "Set parity error\n");
 	}
 
@@ -377,7 +377,7 @@ static void belkin_sa_set_termios(struct tty_struct *tty,
 			urb_value = BELKIN_SA_DATA_BITS(8);
 			break;
 		}
-		if (BSA_USB_CMD(BELKIN_SA_SET_DATA_BITS_REQUEST, urb_value) < 0)
+		if (BSA_USB_CMD(BELKIN_SA_SET_DATA_BITS_REQUEST, urb_value))
 			dev_err(&port->dev, "Set data bits error\n");
 	}
 
@@ -385,8 +385,7 @@ static void belkin_sa_set_termios(struct tty_struct *tty,
 	if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
 		urb_value = (cflag & CSTOPB) ? BELKIN_SA_STOP_BITS(2)
 						: BELKIN_SA_STOP_BITS(1);
-		if (BSA_USB_CMD(BELKIN_SA_SET_STOP_BITS_REQUEST,
-							urb_value) < 0)
+		if (BSA_USB_CMD(BELKIN_SA_SET_STOP_BITS_REQUEST, urb_value))
 			dev_err(&port->dev, "Set stop bits error\n");
 	}
 
@@ -407,7 +406,7 @@ static void belkin_sa_set_termios(struct tty_struct *tty,
 		if (bad_flow_control)
 			urb_value &= ~(BELKIN_SA_FLOW_IRTS);
 
-		if (BSA_USB_CMD(BELKIN_SA_SET_FLOW_CTRL_REQUEST, urb_value) < 0)
+		if (BSA_USB_CMD(BELKIN_SA_SET_FLOW_CTRL_REQUEST, urb_value))
 			dev_err(&port->dev, "Set flow control error\n");
 	}
 
@@ -422,7 +421,7 @@ static void belkin_sa_break_ctl(struct tty_struct *tty, int break_state)
 	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = port->serial;
 
-	if (BSA_USB_CMD(BELKIN_SA_SET_BREAK_REQUEST, break_state ? 1 : 0) < 0)
+	if (BSA_USB_CMD(BELKIN_SA_SET_BREAK_REQUEST, break_state ? 1 : 0))
 		dev_err(&port->dev, "Set break_ctl %d\n", break_state);
 }
 
@@ -476,13 +475,13 @@ static int belkin_sa_tiocmset(struct tty_struct *tty,
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	retval = BSA_USB_CMD(BELKIN_SA_SET_RTS_REQUEST, rts);
-	if (retval < 0) {
+	if (retval) {
 		dev_err(&port->dev, "Set RTS error %d\n", retval);
 		goto exit;
 	}
 
 	retval = BSA_USB_CMD(BELKIN_SA_SET_DTR_REQUEST, dtr);
-	if (retval < 0) {
+	if (retval) {
 		dev_err(&port->dev, "Set DTR error %d\n", retval);
 		goto exit;
 	}
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 02/15] usb: serial: belkin_sa: use usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:50PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_send() nicely wraps usb_control_msg() with
> proper error check. Hence use the wrapper instead of calling
> usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/belkin_sa.c | 35 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/serial/belkin_sa.c b/drivers/usb/serial/belkin_sa.c
> index 9bb123ab9bc9..a5ff55f48303 100644
> --- a/drivers/usb/serial/belkin_sa.c
> +++ b/drivers/usb/serial/belkin_sa.c
> @@ -105,9 +105,10 @@ struct belkin_sa_private {
>  #define WDR_TIMEOUT 5000 /* default urb timeout */
>  
>  /* assumes that struct usb_serial *serial is available */
> -#define BSA_USB_CMD(c, v) usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0), \
> -					    (c), BELKIN_SA_SET_REQUEST_TYPE, \
> -					    (v), 0, NULL, 0, WDR_TIMEOUT)
> +#define BSA_USB_CMD(c, v) usb_control_msg_send(serial->dev, 0, (c), \
> +					       BELKIN_SA_SET_REQUEST_TYPE, \
> +					       (v), 0, NULL, 0, WDR_TIMEOUT, \
> +					       GFP_KERNEL)

Also here there's no data stage so there no point in using the new
helpers.

Please drop this one as well.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 03/15] usb: serial: ch314: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/ch341.c | 45 ++++++++++++--------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index a2e2f56c88cd..58c5d3ce4f75 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -111,10 +111,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;
@@ -129,23 +129,14 @@ 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;
-		}
-
+	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 r;
-	}
 
-	return 0;
+	return r;
 }
 
 #define CH341_CLKRATE		48000000
@@ -343,22 +334,18 @@ 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;
@@ -366,17 +353,13 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
 		goto out;
 	}
 
-	if (r != size) {
-		if (r >= 0)
-			r = -EIO;
+	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);
 		priv->quirks |= quirks;
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 03/15] usb: serial: ch314: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:51PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/ch341.c | 45 ++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index a2e2f56c88cd..58c5d3ce4f75 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -111,10 +111,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;
> @@ -129,23 +129,14 @@ 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;
> -		}
> -
> +	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 r;
> -	}
>  
> -	return 0;
> +	return r;
>  }

The callers of these functions are allocating temporary transfers buffer
for each request so this is a case where the new helpers may be of worth
if you fix up the callers as well (otherwise, you're adding redundant
memdup + memcpy for each call here).

>  
>  #define CH341_CLKRATE		48000000
> @@ -343,22 +334,18 @@ 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;
> @@ -366,17 +353,13 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
>  		goto out;
>  	}
>  
> -	if (r != size) {
> -		if (r >= 0)
> -			r = -EIO;
> +	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);
>  		priv->quirks |= quirks;

This is a good example of when to use the helpers. But it seems you
should remove the "r = 0" and "out" label as well, right?

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 04/15] usb: serial: cp210x: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/cp210x.c | 148 ++++++++++--------------------------
 1 file changed, 42 insertions(+), 106 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index d0c05aa8a0d6..29436ab392e8 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -555,31 +555,15 @@ static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
 {
 	struct usb_serial *serial = port->serial;
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-	void *dmabuf;
 	int result;
 
-	dmabuf = kmalloc(bufsize, GFP_KERNEL);
-	if (!dmabuf) {
-		/*
-		 * FIXME Some callers don't bother to check for error,
-		 * at least give them consistent junk until they are fixed
-		 */
-		memset(buf, 0, bufsize);
-		return -ENOMEM;
-	}
-
-	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-			req, REQTYPE_INTERFACE_TO_HOST, 0,
-			port_priv->bInterfaceNumber, dmabuf, bufsize,
-			USB_CTRL_SET_TIMEOUT);
-	if (result == bufsize) {
-		memcpy(buf, dmabuf, bufsize);
-		result = 0;
-	} else {
+	result = usb_control_msg_recv(serial->dev, 0, req,
+				      REQTYPE_INTERFACE_TO_HOST, 0,
+				      port_priv->bInterfaceNumber, buf, bufsize,
+				      USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
+	if (result) {
 		dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
-				req, bufsize, result);
-		if (result >= 0)
-			result = -EIO;
+			req, bufsize, result);
 
 		/*
 		 * FIXME Some callers don't bother to check for error,
@@ -588,8 +572,6 @@ static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
 		memset(buf, 0, bufsize);
 	}
 
-	kfree(dmabuf);
-
 	return result;
 }
 
@@ -648,29 +630,16 @@ static int cp210x_read_u8_reg(struct usb_serial_port *port, u8 req, u8 *val)
 static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
 				    void *buf, int bufsize)
 {
-	void *dmabuf;
 	int result;
 
-	dmabuf = kmalloc(bufsize, GFP_KERNEL);
-	if (!dmabuf)
-		return -ENOMEM;
-
-	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-				 CP210X_VENDOR_SPECIFIC, type, val,
-				 cp210x_interface_num(serial), dmabuf, bufsize,
-				 USB_CTRL_GET_TIMEOUT);
-	if (result == bufsize) {
-		memcpy(buf, dmabuf, bufsize);
-		result = 0;
-	} else {
+	result = usb_control_msg_recv(serial->dev, 0, CP210X_VENDOR_SPECIFIC,
+				      type, val, cp210x_interface_num(serial),
+				      buf, bufsize, USB_CTRL_GET_TIMEOUT,
+				      GFP_KERNEL);
+	if (result)
 		dev_err(&serial->interface->dev,
 			"failed to get vendor val 0x%04x size %d: %d\n", val,
 			bufsize, result);
-		if (result >= 0)
-			result = -EIO;
-	}
-
-	kfree(dmabuf);
 
 	return result;
 }
@@ -685,14 +654,13 @@ static int cp210x_write_u16_reg(struct usb_serial_port *port, u8 req, u16 val)
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
 	int result;
 
-	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			req, REQTYPE_HOST_TO_INTERFACE, val,
-			port_priv->bInterfaceNumber, NULL, 0,
-			USB_CTRL_SET_TIMEOUT);
-	if (result < 0) {
+	result = usb_control_msg_send(serial->dev, 0, req,
+				      REQTYPE_HOST_TO_INTERFACE, val,
+				      port_priv->bInterfaceNumber, NULL, 0,
+				      USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
+	if (result)
 		dev_err(&port->dev, "failed set request 0x%x status: %d\n",
 				req, result);
-	}
 
 	return result;
 }
@@ -706,28 +674,17 @@ 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 == bufsize) {
-		result = 0;
-	} else {
+	if (result)
 		dev_err(&port->dev, "failed set req 0x%x size %d status: %d\n",
 				req, bufsize, result);
-		if (result >= 0)
-			result = -EIO;
-	}
 
 	return result;
 }
@@ -752,29 +709,17 @@ 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);
-
-	kfree(dmabuf);
+	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);
 
-	if (result == bufsize) {
-		result = 0;
-	} else {
+	if (result)
 		dev_err(&serial->interface->dev,
 			"failed to set vendor val 0x%04x size %d: %d\n", val,
 			bufsize, result);
-		if (result >= 0)
-			result = -EIO;
-	}
 
 	return result;
 }
@@ -995,27 +940,19 @@ 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;
 }
@@ -1624,13 +1561,12 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
 	} else {
 		u16 wIndex = buf.state << 8 | buf.mask;
 
-		result = usb_control_msg(serial->dev,
-					 usb_sndctrlpipe(serial->dev, 0),
-					 CP210X_VENDOR_SPECIFIC,
-					 REQTYPE_HOST_TO_DEVICE,
-					 CP210X_WRITE_LATCH,
-					 wIndex,
-					 NULL, 0, USB_CTRL_SET_TIMEOUT);
+		result = usb_control_msg_send(serial->dev, 0,
+					      CP210X_VENDOR_SPECIFIC,
+					      REQTYPE_HOST_TO_DEVICE,
+					      CP210X_WRITE_LATCH,
+					      wIndex, NULL, 0,
+					      USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
 	}
 
 	usb_autopm_put_interface(serial->interface);
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 04/15] usb: serial: cp210x: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:52PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.

This too is a good example of when the new helpers can be used, but
please mention the transfer buffers here are that is the primary reason.

> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 148 ++++++++++--------------------------
>  1 file changed, 42 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index d0c05aa8a0d6..29436ab392e8 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -555,31 +555,15 @@ static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
>  {
>  	struct usb_serial *serial = port->serial;
>  	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> -	void *dmabuf;
>  	int result;
>  
> -	dmabuf = kmalloc(bufsize, GFP_KERNEL);
> -	if (!dmabuf) {
> -		/*
> -		 * FIXME Some callers don't bother to check for error,
> -		 * at least give them consistent junk until they are fixed
> -		 */
> -		memset(buf, 0, bufsize);
> -		return -ENOMEM;
> -	}
> -
> -	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> -			req, REQTYPE_INTERFACE_TO_HOST, 0,
> -			port_priv->bInterfaceNumber, dmabuf, bufsize,
> -			USB_CTRL_SET_TIMEOUT);
> -	if (result == bufsize) {
> -		memcpy(buf, dmabuf, bufsize);
> -		result = 0;
> -	} else {
> +	result = usb_control_msg_recv(serial->dev, 0, req,
> +				      REQTYPE_INTERFACE_TO_HOST, 0,
> +				      port_priv->bInterfaceNumber, buf, bufsize,
> +				      USB_CTRL_SET_TIMEOUT, GFP_KERNEL);

Please keep the existing indentation style. That way you don't need to
change the middle-two argument lines just to align the arguments.

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

Nit: This is an unrelated indentation change.

>  
>  		/*
>  		 * FIXME Some callers don't bother to check for error,
> @@ -588,8 +572,6 @@ static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
>  		memset(buf, 0, bufsize);
>  	}
>  
> -	kfree(dmabuf);
> -
>  	return result;
>  }
>  
> @@ -648,29 +630,16 @@ static int cp210x_read_u8_reg(struct usb_serial_port *port, u8 req, u8 *val)
>  static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
>  				    void *buf, int bufsize)
>  {
> -	void *dmabuf;
>  	int result;
>  
> -	dmabuf = kmalloc(bufsize, GFP_KERNEL);
> -	if (!dmabuf)
> -		return -ENOMEM;
> -
> -	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> -				 CP210X_VENDOR_SPECIFIC, type, val,
> -				 cp210x_interface_num(serial), dmabuf, bufsize,
> -				 USB_CTRL_GET_TIMEOUT);
> -	if (result == bufsize) {
> -		memcpy(buf, dmabuf, bufsize);
> -		result = 0;
> -	} else {
> +	result = usb_control_msg_recv(serial->dev, 0, CP210X_VENDOR_SPECIFIC,
> +				      type, val, cp210x_interface_num(serial),
> +				      buf, bufsize, USB_CTRL_GET_TIMEOUT,
> +				      GFP_KERNEL);
> +	if (result)
>  		dev_err(&serial->interface->dev,
>  			"failed to get vendor val 0x%04x size %d: %d\n", val,
>  			bufsize, result);
> -		if (result >= 0)
> -			result = -EIO;
> -	}

Nit: Please keep the brackets around multiline single statements since
it improves readability.

Similar comments apply to the rest of the patch.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 05/15] usb: serial: cypress_m8: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/cypress_m8.c | 38 +++++++++++++++++----------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
index cc028601c388..4d66cab8eece 100644
--- a/drivers/usb/serial/cypress_m8.c
+++ b/drivers/usb/serial/cypress_m8.c
@@ -341,20 +341,21 @@ static int cypress_serial_control(struct tty_struct *tty,
 			feature_buffer[4]);
 
 		do {
-			retval = usb_control_msg(port->serial->dev,
-					usb_sndctrlpipe(port->serial->dev, 0),
-					HID_REQ_SET_REPORT,
-					USB_DIR_OUT | USB_RECIP_INTERFACE | USB_TYPE_CLASS,
-					0x0300, 0, feature_buffer,
-					feature_len, 500);
+			retval = usb_control_msg_send(port->serial->dev, 0,
+						      HID_REQ_SET_REPORT,
+						      USB_DIR_OUT |
+						      USB_RECIP_INTERFACE |
+						      USB_TYPE_CLASS, 0x0300,
+						      0, feature_buffer,
+						      feature_len, 500,
+						      GFP_KERNEL);
 
 			if (tries++ >= 3)
 				break;
 
-		} while (retval != feature_len &&
-			 retval != -ENODEV);
+		} while (retval != -ENODEV);
 
-		if (retval != feature_len) {
+		if (retval) {
 			dev_err(dev, "%s - failed sending serial line settings - %d\n",
 				__func__, retval);
 			cypress_set_dead(port);
@@ -379,19 +380,20 @@ static int cypress_serial_control(struct tty_struct *tty,
 		}
 		dev_dbg(dev, "%s - retrieving serial line settings\n", __func__);
 		do {
-			retval = usb_control_msg(port->serial->dev,
-					usb_rcvctrlpipe(port->serial->dev, 0),
-					HID_REQ_GET_REPORT,
-					USB_DIR_IN | USB_RECIP_INTERFACE | USB_TYPE_CLASS,
-					0x0300, 0, feature_buffer,
-					feature_len, 500);
+			retval = usb_control_msg_recv(port->serial->dev, 0,
+						      HID_REQ_GET_REPORT,
+						      USB_DIR_IN |
+						      USB_RECIP_INTERFACE |
+						      USB_TYPE_CLASS, 0x0300,
+						      0, feature_buffer,
+						      feature_len, 500,
+						      GFP_KERNEL);
 
 			if (tries++ >= 3)
 				break;
-		} while (retval != feature_len
-						&& retval != -ENODEV);
+		} while (retval != -ENODEV);
 
-		if (retval != feature_len) {
+		if (retval) {
 			dev_err(dev, "%s - failed to retrieve serial line settings - %d\n",
 				__func__, retval);
 			cypress_set_dead(port);
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 05/15] usb: serial: cypress_m8: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:53PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/cypress_m8.c | 38 +++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
> index cc028601c388..4d66cab8eece 100644
> --- a/drivers/usb/serial/cypress_m8.c
> +++ b/drivers/usb/serial/cypress_m8.c
> @@ -341,20 +341,21 @@ static int cypress_serial_control(struct tty_struct *tty,
>  			feature_buffer[4]);
>  
>  		do {
> -			retval = usb_control_msg(port->serial->dev,
> -					usb_sndctrlpipe(port->serial->dev, 0),
> -					HID_REQ_SET_REPORT,
> -					USB_DIR_OUT | USB_RECIP_INTERFACE | USB_TYPE_CLASS,
> -					0x0300, 0, feature_buffer,
> -					feature_len, 500);
> +			retval = usb_control_msg_send(port->serial->dev, 0,
> +						      HID_REQ_SET_REPORT,
> +						      USB_DIR_OUT |
> +						      USB_RECIP_INTERFACE |
> +						      USB_TYPE_CLASS, 0x0300,
> +						      0, feature_buffer,
> +						      feature_len, 500,
> +						      GFP_KERNEL);
>  
>  			if (tries++ >= 3)
>  				break;
>  
> -		} while (retval != feature_len &&
> -			 retval != -ENODEV);
> +		} while (retval != -ENODEV);

Here you actually break the logic; after this change the driver will
always retry as the test for success is removed.

>  
> -		if (retval != feature_len) {
> +		if (retval) {
>  			dev_err(dev, "%s - failed sending serial line settings - %d\n",
>  				__func__, retval);
>  			cypress_set_dead(port);
> @@ -379,19 +380,20 @@ static int cypress_serial_control(struct tty_struct *tty,
>  		}
>  		dev_dbg(dev, "%s - retrieving serial line settings\n", __func__);
>  		do {
> -			retval = usb_control_msg(port->serial->dev,
> -					usb_rcvctrlpipe(port->serial->dev, 0),
> -					HID_REQ_GET_REPORT,
> -					USB_DIR_IN | USB_RECIP_INTERFACE | USB_TYPE_CLASS,
> -					0x0300, 0, feature_buffer,
> -					feature_len, 500);
> +			retval = usb_control_msg_recv(port->serial->dev, 0,
> +						      HID_REQ_GET_REPORT,
> +						      USB_DIR_IN |
> +						      USB_RECIP_INTERFACE |
> +						      USB_TYPE_CLASS, 0x0300,
> +						      0, feature_buffer,
> +						      feature_len, 500,
> +						      GFP_KERNEL);
>  
>  			if (tries++ >= 3)
>  				break;
> -		} while (retval != feature_len
> -						&& retval != -ENODEV);
> +		} while (retval != -ENODEV);

Same here.

>  
> -		if (retval != feature_len) {
> +		if (retval) {
>  			dev_err(dev, "%s - failed to retrieve serial line settings - %d\n",
>  				__func__, retval);
>  			cypress_set_dead(port);

As the driver is already using a DMA-able buffer and is reusing it for
every retry as well, this change is now introducing redundant memdup +
memcpy for each call.

I suggest just dropping this one.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 06/15] usb: serial: f81232: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/f81232.c | 88 ++++++++-----------------------------
 1 file changed, 18 insertions(+), 70 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 0c7eacc630e0..78107feef8a8 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -139,71 +139,36 @@ 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 != sizeof(val)) {
+	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);
 
-		if (status < 0)
-			status = usb_translate_errors(status);
-		else
-			status = -EIO;
-	} else {
-		status = 0;
+		status = usb_translate_errors(status);
 	}
-
-	kfree(tmp);
 	return status;
 }
 
@@ -866,32 +831,16 @@ 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) {
+			/* Retry on error or short transfers */
 			status = usb_translate_errors(status);
 			if (status == -EIO)
 				continue;
-		} else if (status != size) {
-			/* Retry on short transfers */
-			status = -EIO;
-			continue;
-		} else {
-			status = 0;
 		}
 
 		break;
@@ -902,7 +851,6 @@ static int f81534a_ctrl_set_register(struct usb_interface *intf, u16 reg,
 				reg, status);
 	}
 
-	kfree(tmp);
 	return status;
 }
 
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 06/15] usb: serial: f81232: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:54PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 88 ++++++++-----------------------------
>  1 file changed, 18 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 0c7eacc630e0..78107feef8a8 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -139,71 +139,36 @@ 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);

This driver use tabs for indentation consistently so please stick to
that style. That should also amount to a smaller patch (and less noise
in the logs, e.g. for git blame).

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

> @@ -866,32 +831,16 @@ 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) {
> +			/* Retry on error or short transfers */
>  			status = usb_translate_errors(status);
>  			if (status == -EIO)
>  				continue;

So this now depends on the new helper returning an errno on short
transfers which gets mapped to -EIO by usb_translate_errors(). This
works currently (and with my suggested change to use -EREMOTEIO) but
could possibly lead to subtle regressions later.

I don't think we need to worry about it, but just wanted to highlight
the kind of impact these changes can have.

Note that this change also replaces a single memdup with a memdup+memcpy
for each iteration. But as this helper is used rarely (e.g. probe,
resume and disconnect), and hopefully the retries are exceptions, the
overhead could be acceptable.

> -		} else if (status != size) {
> -			/* Retry on short transfers */
> -			status = -EIO;
> -			continue;
> -		} else {
> -			status = 0;
>  		}
>  
>  		break;
> @@ -902,7 +851,6 @@ static int f81534a_ctrl_set_register(struct usb_interface *intf, u16 reg,
>  				reg, status);
>  	}
>  
> -	kfree(tmp);
>  	return status;
>  }

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 07/15] usb: serial: f81534: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/f81534.c | 63 +++++++++++--------------------------
 1 file changed, 18 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 5661fd03e545..23eb17a2c052 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -217,38 +217,26 @@ static int f81534_set_register(struct usb_serial *serial, u16 reg, u8 data)
 	struct usb_device *dev = serial->dev;
 	size_t count = F81534_USB_MAX_RETRY;
 	int status;
-	u8 *tmp;
-
-	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
-
-	*tmp = data;
 
 	/*
 	 * Our device maybe not reply when heavily loading, We'll retry for
 	 * F81534_USB_MAX_RETRY times.
 	 */
 	while (count--) {
-		status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-					 F81534_SET_GET_REGISTER,
-					 USB_TYPE_VENDOR | USB_DIR_OUT,
-					 reg, 0, tmp, sizeof(u8),
-					 F81534_USB_TIMEOUT);
-		if (status > 0) {
-			status = 0;
-			break;
-		} else if (status == 0) {
-			status = -EIO;
+		status = usb_control_msg_send(dev, 0, F81534_SET_GET_REGISTER,
+					      USB_TYPE_VENDOR | USB_DIR_OUT,
+					      reg, 0, &data, sizeof(u8),
+					      F81534_USB_TIMEOUT, GFP_KERNEL);
+		if (status) {
+			/* Try again */
+			continue;
 		}
 	}
 
-	if (status < 0) {
+	if (status)
 		dev_err(&interface->dev, "%s: reg: %x data: %x failed: %d\n",
-				__func__, reg, data, status);
-	}
+			__func__, reg, data, status);
 
-	kfree(tmp);
 	return status;
 }
 
@@ -258,40 +246,25 @@ static int f81534_get_register(struct usb_serial *serial, u16 reg, u8 *data)
 	struct usb_device *dev = serial->dev;
 	size_t count = F81534_USB_MAX_RETRY;
 	int status;
-	u8 *tmp;
-
-	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
 
 	/*
 	 * Our device maybe not reply when heavily loading, We'll retry for
 	 * F81534_USB_MAX_RETRY times.
 	 */
 	while (count--) {
-		status = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-					 F81534_SET_GET_REGISTER,
-					 USB_TYPE_VENDOR | USB_DIR_IN,
-					 reg, 0, tmp, sizeof(u8),
-					 F81534_USB_TIMEOUT);
-		if (status > 0) {
-			status = 0;
-			break;
-		} else if (status == 0) {
-			status = -EIO;
+		status = usb_control_msg_recv(dev, 0, F81534_SET_GET_REGISTER,
+					      USB_TYPE_VENDOR | USB_DIR_IN, reg,
+					      0, data, sizeof(u8),
+					      F81534_USB_TIMEOUT, GFP_KERNEL);
+		if (status) {
+			/* Try again */
+			continue;
 		}
 	}
 
-	if (status < 0) {
+	if (status)
 		dev_err(&interface->dev, "%s: reg: %x failed: %d\n", __func__,
-				reg, status);
-		goto end;
-	}
-
-	*data = *tmp;
-
-end:
-	kfree(tmp);
+			reg, status);
 	return status;
 }
 
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 07/15] usb: serial: f81534: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:55PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/f81534.c | 63 +++++++++++--------------------------
>  1 file changed, 18 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 5661fd03e545..23eb17a2c052 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -217,38 +217,26 @@ static int f81534_set_register(struct usb_serial *serial, u16 reg, u8 data)
>  	struct usb_device *dev = serial->dev;
>  	size_t count = F81534_USB_MAX_RETRY;
>  	int status;
> -	u8 *tmp;
> -
> -	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> -	if (!tmp)
> -		return -ENOMEM;
> -
> -	*tmp = data;
>  
>  	/*
>  	 * Our device maybe not reply when heavily loading, We'll retry for
>  	 * F81534_USB_MAX_RETRY times.
>  	 */
>  	while (count--) {
> -		status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> -					 F81534_SET_GET_REGISTER,
> -					 USB_TYPE_VENDOR | USB_DIR_OUT,
> -					 reg, 0, tmp, sizeof(u8),
> -					 F81534_USB_TIMEOUT);
> -		if (status > 0) {
> -			status = 0;
> -			break;
> -		} else if (status == 0) {
> -			status = -EIO;
> +		status = usb_control_msg_send(dev, 0, F81534_SET_GET_REGISTER,
> +					      USB_TYPE_VENDOR | USB_DIR_OUT,
> +					      reg, 0, &data, sizeof(u8),
> +					      F81534_USB_TIMEOUT, GFP_KERNEL);
> +		if (status) {
> +			/* Try again */
> +			continue;
>  		}
>  	}

Here too this change breaks the logic and the control transfer is now
repeated also after successful transfer (ten times!).

This change would also introduce an additional malloc + memcpy for every
retry.

As this is a function that is used often and the comment suggest that
having to retry isn't that rare, I suggest dropping this patch as well.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 08/15] usb: serial: ftdi_sio: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/ftdi_sio.c | 182 +++++++++++++++-------------------
 1 file changed, 78 insertions(+), 104 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index e0f4c3d9649c..37e9e75b90d0 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1245,13 +1245,12 @@ static int update_mctrl(struct usb_serial_port *port, unsigned int set,
 		value |= FTDI_SIO_SET_DTR_HIGH;
 	if (set & TIOCM_RTS)
 		value |= FTDI_SIO_SET_RTS_HIGH;
-	rv = usb_control_msg(port->serial->dev,
-			       usb_sndctrlpipe(port->serial->dev, 0),
-			       FTDI_SIO_SET_MODEM_CTRL_REQUEST,
-			       FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
-			       value, priv->interface,
-			       NULL, 0, WDR_TIMEOUT);
-	if (rv < 0) {
+	rv = usb_control_msg_send(port->serial->dev, 0,
+				  FTDI_SIO_SET_MODEM_CTRL_REQUEST,
+				  FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
+				  value, priv->interface,
+				  NULL, 0, WDR_TIMEOUT, GFP_KERNEL);
+	if (rv) {
 		dev_dbg(dev, "%s Error from MODEM_CTRL urb: DTR %s, RTS %s\n",
 			__func__,
 			(set & TIOCM_DTR) ? "HIGH" : (clear & TIOCM_DTR) ? "LOW" : "unchanged",
@@ -1393,12 +1392,11 @@ static int change_speed(struct tty_struct *tty, struct usb_serial_port *port)
 		index = (u16)((index << 8) | priv->interface);
 	}
 
-	rv = usb_control_msg(port->serial->dev,
-			    usb_sndctrlpipe(port->serial->dev, 0),
-			    FTDI_SIO_SET_BAUDRATE_REQUEST,
-			    FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
-			    value, index,
-			    NULL, 0, WDR_SHORT_TIMEOUT);
+	rv = usb_control_msg_send(port->serial->dev, 0,
+				  FTDI_SIO_SET_BAUDRATE_REQUEST,
+				  FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
+				  value, index,
+				  NULL, 0, WDR_SHORT_TIMEOUT, GFP_KERNEL);
 	return rv;
 }
 
@@ -1417,13 +1415,12 @@ static int write_latency_timer(struct usb_serial_port *port)
 
 	dev_dbg(&port->dev, "%s: setting latency timer = %i\n", __func__, l);
 
-	rv = usb_control_msg(udev,
-			     usb_sndctrlpipe(udev, 0),
-			     FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
-			     FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
-			     l, priv->interface,
-			     NULL, 0, WDR_TIMEOUT);
-	if (rv < 0)
+	rv = usb_control_msg_send(udev, 0,
+				  FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
+				  FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
+				  l, priv->interface,
+				  NULL, 0, WDR_TIMEOUT, GFP_KERNEL);
+	if (rv)
 		dev_err(&port->dev, "Unable to write latency timer: %i\n", rv);
 	return rv;
 }
@@ -1432,27 +1429,16 @@ 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;
 }
@@ -1731,13 +1717,12 @@ static ssize_t event_char_store(struct device *dev,
 
 	dev_dbg(&port->dev, "%s: setting event char = 0x%03x\n", __func__, v);
 
-	rv = usb_control_msg(udev,
-			     usb_sndctrlpipe(udev, 0),
-			     FTDI_SIO_SET_EVENT_CHAR_REQUEST,
-			     FTDI_SIO_SET_EVENT_CHAR_REQUEST_TYPE,
-			     v, priv->interface,
-			     NULL, 0, WDR_TIMEOUT);
-	if (rv < 0) {
+	rv = usb_control_msg_send(udev, 0,
+				  FTDI_SIO_SET_EVENT_CHAR_REQUEST,
+				  FTDI_SIO_SET_EVENT_CHAR_REQUEST_TYPE,
+				  v, priv->interface,
+				  NULL, 0, WDR_TIMEOUT, GFP_KERNEL);
+	if (rv) {
 		dev_dbg(&port->dev, "Unable to write event character: %i\n", rv);
 		return -EIO;
 	}
@@ -1805,12 +1790,12 @@ static int ftdi_set_bitmode(struct usb_serial_port *port, u8 mode)
 		return result;
 
 	val = (mode << 8) | (priv->gpio_output << 4) | priv->gpio_value;
-	result = usb_control_msg(serial->dev,
-				 usb_sndctrlpipe(serial->dev, 0),
-				 FTDI_SIO_SET_BITMODE_REQUEST,
-				 FTDI_SIO_SET_BITMODE_REQUEST_TYPE, val,
-				 priv->interface, NULL, 0, WDR_TIMEOUT);
-	if (result < 0) {
+	result = usb_control_msg_send(serial->dev, 0,
+				      FTDI_SIO_SET_BITMODE_REQUEST,
+				      FTDI_SIO_SET_BITMODE_REQUEST_TYPE, val,
+				      priv->interface, NULL, 0, WDR_TIMEOUT,
+				      GFP_KERNEL);
+	if (result) {
 		dev_err(&serial->interface->dev,
 			"bitmode request failed for value 0x%04x: %d\n",
 			val, result);
@@ -1866,32 +1851,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;
@@ -2311,6 +2285,7 @@ static int ftdi_NDI_device_setup(struct usb_serial *serial)
 {
 	struct usb_device *udev = serial->dev;
 	int latency = ndi_latency_timer;
+	int ret;
 
 	if (latency == 0)
 		latency = 1;
@@ -2320,12 +2295,12 @@ static int ftdi_NDI_device_setup(struct usb_serial *serial)
 	dev_dbg(&udev->dev, "%s setting NDI device latency to %d\n", __func__, latency);
 	dev_info(&udev->dev, "NDI device with a latency value of %d\n", latency);
 
-	/* FIXME: errors are not returned */
-	usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-				FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
-				FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
-				latency, 0, NULL, 0, WDR_TIMEOUT);
-	return 0;
+	ret = usb_control_msg_send(udev, 0,
+				   FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
+				   FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
+				   latency, 0, NULL, 0, WDR_TIMEOUT,
+				   GFP_KERNEL);
+	return ret;
 }
 
 /*
@@ -2424,12 +2399,11 @@ static void ftdi_dtr_rts(struct usb_serial_port *port, int on)
 
 	/* Disable flow control */
 	if (!on) {
-		if (usb_control_msg(port->serial->dev,
-			    usb_sndctrlpipe(port->serial->dev, 0),
-			    FTDI_SIO_SET_FLOW_CTRL_REQUEST,
-			    FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
-			    0, priv->interface, NULL, 0,
-			    WDR_TIMEOUT) < 0) {
+		if (usb_control_msg_send(port->serial->dev, 0,
+					 FTDI_SIO_SET_FLOW_CTRL_REQUEST,
+					 FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
+					 0, priv->interface, NULL, 0,
+					 WDR_TIMEOUT, GFP_KERNEL)) {
 			dev_err(&port->dev, "error from flowcontrol urb\n");
 		}
 	}
@@ -2617,12 +2591,11 @@ static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
 	else
 		value = priv->last_set_data_value;
 
-	if (usb_control_msg(port->serial->dev,
-			usb_sndctrlpipe(port->serial->dev, 0),
-			FTDI_SIO_SET_DATA_REQUEST,
-			FTDI_SIO_SET_DATA_REQUEST_TYPE,
-			value , priv->interface,
-			NULL, 0, WDR_TIMEOUT) < 0) {
+	if (usb_control_msg_send(port->serial->dev, 0,
+				 FTDI_SIO_SET_DATA_REQUEST,
+				 FTDI_SIO_SET_DATA_REQUEST_TYPE,
+				 value, priv->interface,
+				 NULL, 0, WDR_TIMEOUT, GFP_KERNEL)) {
 		dev_err(&port->dev, "%s FAILED to enable/disable break state (state was %d)\n",
 			__func__, break_state);
 	}
@@ -2754,11 +2727,11 @@ static void ftdi_set_termios(struct tty_struct *tty,
 	   - but is or'ed with this value  */
 	priv->last_set_data_value = value;
 
-	if (usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-			    FTDI_SIO_SET_DATA_REQUEST,
-			    FTDI_SIO_SET_DATA_REQUEST_TYPE,
-			    value , priv->interface,
-			    NULL, 0, WDR_SHORT_TIMEOUT) < 0) {
+	if (usb_control_msg_send(dev, 0,
+				 FTDI_SIO_SET_DATA_REQUEST,
+				 FTDI_SIO_SET_DATA_REQUEST_TYPE,
+				 value, priv->interface,
+				 NULL, 0, WDR_SHORT_TIMEOUT, GFP_KERNEL)) {
 		dev_err(ddev, "%s FAILED to set databits/stopbits/parity\n",
 			__func__);
 	}
@@ -2767,11 +2740,11 @@ static void ftdi_set_termios(struct tty_struct *tty,
 no_data_parity_stop_changes:
 	if ((cflag & CBAUD) == B0) {
 		/* Disable flow control */
-		if (usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-				    FTDI_SIO_SET_FLOW_CTRL_REQUEST,
-				    FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
-				    0, priv->interface,
-				    NULL, 0, WDR_TIMEOUT) < 0) {
+		if (usb_control_msg_send(dev, 0,
+					 FTDI_SIO_SET_FLOW_CTRL_REQUEST,
+					 FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
+					 0, priv->interface,
+					 NULL, 0, WDR_TIMEOUT, GFP_KERNEL)) {
 			dev_err(ddev, "%s error from disable flowcontrol urb\n",
 				__func__);
 		}
@@ -2806,11 +2779,12 @@ static void ftdi_set_termios(struct tty_struct *tty,
 
 	index |= priv->interface;
 
-	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-			FTDI_SIO_SET_FLOW_CTRL_REQUEST,
-			FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
-			value, index, NULL, 0, WDR_TIMEOUT);
-	if (ret < 0)
+	ret = usb_control_msg_send(dev, 0,
+				   FTDI_SIO_SET_FLOW_CTRL_REQUEST,
+				   FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
+				   value, index, NULL, 0, WDR_TIMEOUT,
+				   GFP_KERNEL);
+	if (ret)
 		dev_err(&port->dev, "failed to set flow control: %d\n", ret);
 }
 
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 08/15] usb: serial: ftdi_sio: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:56PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/ftdi_sio.c | 182 +++++++++++++++-------------------
>  1 file changed, 78 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index e0f4c3d9649c..37e9e75b90d0 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1245,13 +1245,12 @@ static int update_mctrl(struct usb_serial_port *port, unsigned int set,
>  		value |= FTDI_SIO_SET_DTR_HIGH;
>  	if (set & TIOCM_RTS)
>  		value |= FTDI_SIO_SET_RTS_HIGH;
> -	rv = usb_control_msg(port->serial->dev,
> -			       usb_sndctrlpipe(port->serial->dev, 0),
> -			       FTDI_SIO_SET_MODEM_CTRL_REQUEST,
> -			       FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
> -			       value, priv->interface,
> -			       NULL, 0, WDR_TIMEOUT);
> -	if (rv < 0) {
> +	rv = usb_control_msg_send(port->serial->dev, 0,
> +				  FTDI_SIO_SET_MODEM_CTRL_REQUEST,
> +				  FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
> +				  value, priv->interface,
> +				  NULL, 0, WDR_TIMEOUT, GFP_KERNEL);
> +	if (rv) {
>  		dev_dbg(dev, "%s Error from MODEM_CTRL urb: DTR %s, RTS %s\n",
>  			__func__,
>  			(set & TIOCM_DTR) ? "HIGH" : (clear & TIOCM_DTR) ? "LOW" : "unchanged",

As I mentioned earlier there's no point in using these helper for
control transfers without a data stage so please drop those conversions
and only update _read_latency_timer() ftdi_read_cbus_pins().

> @@ -2311,6 +2285,7 @@ static int ftdi_NDI_device_setup(struct usb_serial *serial)
>  {
>  	struct usb_device *udev = serial->dev;
>  	int latency = ndi_latency_timer;
> +	int ret;
>  
>  	if (latency == 0)
>  		latency = 1;
> @@ -2320,12 +2295,12 @@ static int ftdi_NDI_device_setup(struct usb_serial *serial)
>  	dev_dbg(&udev->dev, "%s setting NDI device latency to %d\n", __func__, latency);
>  	dev_info(&udev->dev, "NDI device with a latency value of %d\n", latency);
>  
> -	/* FIXME: errors are not returned */
> -	usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> -				FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> -				FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> -				latency, 0, NULL, 0, WDR_TIMEOUT);
> -	return 0;
> +	ret = usb_control_msg_send(udev, 0,
> +				   FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> +				   FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> +				   latency, 0, NULL, 0, WDR_TIMEOUT,
> +				   GFP_KERNEL);
> +	return ret;

Also note that returning ret here is an unrelated change which could
potentially break probing in case there are devices which do not support
this request (and would need to be done in a separate patch if at all).

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 09/15] usb: serial: io_edgeport: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/io_edgeport.c | 73 ++++++++++++--------------------
 1 file changed, 27 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index ba5d8df69518..8479d5684af7 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -568,34 +568,29 @@ static int get_epic_descriptor(struct edgeport_serial *ep)
 	int result;
 	struct usb_serial *serial = ep->serial;
 	struct edgeport_product_info *product_info = &ep->product_info;
-	struct edge_compatibility_descriptor *epic;
+	struct edge_compatibility_descriptor epic;
 	struct edge_compatibility_bits *bits;
 	struct device *dev = &serial->dev->dev;
 
 	ep->is_epic = 0;
 
-	epic = kmalloc(sizeof(*epic), GFP_KERNEL);
-	if (!epic)
-		return -ENOMEM;
-
-	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-				 USB_REQUEST_ION_GET_EPIC_DESC,
-				 0xC0, 0x00, 0x00,
-				 epic, sizeof(*epic),
-				 300);
-	if (result == sizeof(*epic)) {
+	result = usb_control_msg_recv(serial->dev, 0,
+				      USB_REQUEST_ION_GET_EPIC_DESC, 0xC0,
+				      0x00, 0x00, &epic, sizeof(epic), 300,
+				      GFP_KERNEL);
+	if (result) {
 		ep->is_epic = 1;
-		memcpy(&ep->epic_descriptor, epic, sizeof(*epic));
+		memcpy(&ep->epic_descriptor, &epic, sizeof(epic));
 		memset(product_info, 0, sizeof(struct edgeport_product_info));
 
-		product_info->NumPorts = epic->NumPorts;
+		product_info->NumPorts = epic.NumPorts;
 		product_info->ProdInfoVer = 0;
-		product_info->FirmwareMajorVersion = epic->MajorVersion;
-		product_info->FirmwareMinorVersion = epic->MinorVersion;
-		product_info->FirmwareBuildNumber = epic->BuildNumber;
-		product_info->iDownloadFile = epic->iDownloadFile;
-		product_info->EpicVer = epic->EpicVer;
-		product_info->Epic = epic->Supports;
+		product_info->FirmwareMajorVersion = epic.MajorVersion;
+		product_info->FirmwareMinorVersion = epic.MinorVersion;
+		product_info->FirmwareBuildNumber = epic.BuildNumber;
+		product_info->iDownloadFile = epic.iDownloadFile;
+		product_info->EpicVer = epic.EpicVer;
+		product_info->Epic = epic.Supports;
 		product_info->ProductId = ION_DEVICE_ID_EDGEPORT_COMPATIBLE;
 		dump_product_info(ep, product_info);
 
@@ -614,16 +609,12 @@ static int get_epic_descriptor(struct edgeport_serial *ep)
 		dev_dbg(dev, "  IOSPWriteLCR     : %s\n", bits->IOSPWriteLCR	? "TRUE": "FALSE");
 		dev_dbg(dev, "  IOSPSetBaudRate  : %s\n", bits->IOSPSetBaudRate	? "TRUE": "FALSE");
 		dev_dbg(dev, "  TrueEdgeport     : %s\n", bits->TrueEdgeport	? "TRUE": "FALSE");
-
-		result = 0;
-	} else if (result >= 0) {
+	} else {
 		dev_warn(&serial->interface->dev, "short epic descriptor received: %d\n",
 			 result);
 		result = -EIO;
 	}
 
-	kfree(epic);
-
 	return result;
 }
 
@@ -2168,11 +2159,6 @@ static int rom_read(struct usb_serial *serial, __u16 extAddr,
 {
 	int result;
 	__u16 current_length;
-	unsigned char *transfer_buffer;
-
-	transfer_buffer =  kmalloc(64, GFP_KERNEL);
-	if (!transfer_buffer)
-		return -ENOMEM;
 
 	/* need to split these reads up into 64 byte chunks */
 	result = 0;
@@ -2181,25 +2167,18 @@ static int rom_read(struct usb_serial *serial, __u16 extAddr,
 			current_length = 64;
 		else
 			current_length = length;
-		result = usb_control_msg(serial->dev,
-					usb_rcvctrlpipe(serial->dev, 0),
-					USB_REQUEST_ION_READ_ROM,
-					0xC0, addr, extAddr, transfer_buffer,
-					current_length, 300);
-		if (result < current_length) {
-			if (result >= 0)
-				result = -EIO;
+		result = usb_control_msg_recv(serial->dev, 0,
+					      USB_REQUEST_ION_READ_ROM, 0xC0,
+					      addr, extAddr, data,
+					      current_length, 300, GFP_KERNEL);
+		if (result)
 			break;
-		}
-		memcpy(data, transfer_buffer, current_length);
+
 		length -= current_length;
 		addr += current_length;
 		data += current_length;
 
-		result = 0;
 	}
-
-	kfree(transfer_buffer);
 	return result;
 }
 
@@ -2801,10 +2780,12 @@ static void load_application_firmware(struct edgeport_serial *edge_serial)
 	}
 
 	dev_dbg(dev, "sending exec_dl_code\n");
-	response = usb_control_msg (edge_serial->serial->dev,
-				    usb_sndctrlpipe(edge_serial->serial->dev, 0),
-				    USB_REQUEST_ION_EXEC_DL_CODE,
-				    0x40, 0x4000, 0x0001, NULL, 0, 3000);
+	response = usb_control_msg_send(edge_serial->serial->dev, 0,
+					USB_REQUEST_ION_EXEC_DL_CODE,
+					0x40, 0x4000, 0x0001, NULL, 0, 3000,
+					GFP_KERNEL);
+	if (response)
+		dev_err(dev, "failed sending exec_dl_decode %d\n", response);
 
 	release_firmware(fw);
 }
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 09/15] usb: serial: io_edgeport: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:57PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/io_edgeport.c | 73 ++++++++++++--------------------
>  1 file changed, 27 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> index ba5d8df69518..8479d5684af7 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
> @@ -568,34 +568,29 @@ static int get_epic_descriptor(struct edgeport_serial *ep)
>  	int result;
>  	struct usb_serial *serial = ep->serial;
>  	struct edgeport_product_info *product_info = &ep->product_info;
> -	struct edge_compatibility_descriptor *epic;
> +	struct edge_compatibility_descriptor epic;
>  	struct edge_compatibility_bits *bits;
>  	struct device *dev = &serial->dev->dev;
>  
>  	ep->is_epic = 0;
>  
> -	epic = kmalloc(sizeof(*epic), GFP_KERNEL);
> -	if (!epic)
> -		return -ENOMEM;
> -
> -	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> -				 USB_REQUEST_ION_GET_EPIC_DESC,
> -				 0xC0, 0x00, 0x00,
> -				 epic, sizeof(*epic),
> -				 300);
> -	if (result == sizeof(*epic)) {
> +	result = usb_control_msg_recv(serial->dev, 0,
> +				      USB_REQUEST_ION_GET_EPIC_DESC, 0xC0,
> +				      0x00, 0x00, &epic, sizeof(epic), 300,
> +				      GFP_KERNEL);
> +	if (result) {

Here's another logical error due to the test being inverted, which
results in the uninitialised stack-allocated buffer to be used for debug
printks (potentially leaking stack data) in case of errors.

>  		ep->is_epic = 1;
> -		memcpy(&ep->epic_descriptor, epic, sizeof(*epic));
> +		memcpy(&ep->epic_descriptor, &epic, sizeof(epic));
>  		memset(product_info, 0, sizeof(struct edgeport_product_info));
>  
> -		product_info->NumPorts = epic->NumPorts;
> +		product_info->NumPorts = epic.NumPorts;
>  		product_info->ProdInfoVer = 0;
> -		product_info->FirmwareMajorVersion = epic->MajorVersion;
> -		product_info->FirmwareMinorVersion = epic->MinorVersion;
> -		product_info->FirmwareBuildNumber = epic->BuildNumber;
> -		product_info->iDownloadFile = epic->iDownloadFile;
> -		product_info->EpicVer = epic->EpicVer;
> -		product_info->Epic = epic->Supports;
> +		product_info->FirmwareMajorVersion = epic.MajorVersion;
> +		product_info->FirmwareMinorVersion = epic.MinorVersion;
> +		product_info->FirmwareBuildNumber = epic.BuildNumber;
> +		product_info->iDownloadFile = epic.iDownloadFile;
> +		product_info->EpicVer = epic.EpicVer;
> +		product_info->Epic = epic.Supports;
>  		product_info->ProductId = ION_DEVICE_ID_EDGEPORT_COMPATIBLE;
>  		dump_product_info(ep, product_info);
>  
> @@ -614,16 +609,12 @@ static int get_epic_descriptor(struct edgeport_serial *ep)
>  		dev_dbg(dev, "  IOSPWriteLCR     : %s\n", bits->IOSPWriteLCR	? "TRUE": "FALSE");
>  		dev_dbg(dev, "  IOSPSetBaudRate  : %s\n", bits->IOSPSetBaudRate	? "TRUE": "FALSE");
>  		dev_dbg(dev, "  TrueEdgeport     : %s\n", bits->TrueEdgeport	? "TRUE": "FALSE");
> -
> -		result = 0;
> -	} else if (result >= 0) {
> +	} else {
>  		dev_warn(&serial->interface->dev, "short epic descriptor received: %d\n",
>  			 result);
>  		result = -EIO;

...and the driver now always fails to probe with -EIO.

>  	}
>  
> -	kfree(epic);
> -
>  	return result;
>  }
>  
> @@ -2168,11 +2159,6 @@ static int rom_read(struct usb_serial *serial, __u16 extAddr,
>  {
>  	int result;
>  	__u16 current_length;
> -	unsigned char *transfer_buffer;
> -
> -	transfer_buffer =  kmalloc(64, GFP_KERNEL);
> -	if (!transfer_buffer)
> -		return -ENOMEM;
>  
>  	/* need to split these reads up into 64 byte chunks */
>  	result = 0;
> @@ -2181,25 +2167,18 @@ static int rom_read(struct usb_serial *serial, __u16 extAddr,
>  			current_length = 64;
>  		else
>  			current_length = length;
> -		result = usb_control_msg(serial->dev,
> -					usb_rcvctrlpipe(serial->dev, 0),
> -					USB_REQUEST_ION_READ_ROM,
> -					0xC0, addr, extAddr, transfer_buffer,
> -					current_length, 300);
> -		if (result < current_length) {
> -			if (result >= 0)
> -				result = -EIO;
> +		result = usb_control_msg_recv(serial->dev, 0,
> +					      USB_REQUEST_ION_READ_ROM, 0xC0,
> +					      addr, extAddr, data,
> +					      current_length, 300, GFP_KERNEL);
> +		if (result)
>  			break;
> -		}
> -		memcpy(data, transfer_buffer, current_length);
> +
>  		length -= current_length;
>  		addr += current_length;
>  		data += current_length;
>  
> -		result = 0;
>  	}
> -
> -	kfree(transfer_buffer);
>  	return result;
>  }

Here a single allocation of a buffer that get's used repeatedly is
replaced with one allocation per iteration. I suggest leaving this as
is.

Please just drop this patch.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 10/15] usb: serial: io_ti: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/io_ti.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index c327d4cf7928..ab2370b80b3c 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -260,16 +260,12 @@ static int ti_vread_sync(struct usb_device *dev, __u8 request,
 {
 	int status;
 
-	status = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
-			(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN),
-			value, index, data, size, 1000);
-	if (status < 0)
+	status = usb_control_msg_recv(dev, 0, request, USB_TYPE_VENDOR |
+				      USB_RECIP_DEVICE | USB_DIR_IN, value,
+				      index, data, size, 1000, GFP_KERNEL);
+	if (status)
 		return status;
-	if (status != size) {
-		dev_dbg(&dev->dev, "%s - wanted to write %d, but only wrote %d\n",
-			__func__, size, status);
-		return -ECOMM;
-	}
+
 	return 0;
 }
 
@@ -278,16 +274,12 @@ static int ti_vsend_sync(struct usb_device *dev, u8 request, u16 value,
 {
 	int status;
 
-	status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
-			(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT),
-			value, index, data, size, timeout);
-	if (status < 0)
+	status = usb_control_msg_send(dev, 0, request, USB_TYPE_VENDOR |
+				      USB_RECIP_DEVICE | USB_DIR_OUT, value,
+				      index, data, size, timeout, GFP_KERNEL);
+	if (status)
 		return status;
-	if (status != size) {
-		dev_dbg(&dev->dev, "%s - wanted to write %d, but only wrote %d\n",
-			__func__, size, status);
-		return -ECOMM;
-	}
+
 	return 0;
 }
 
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 10/15] usb: serial: io_ti: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:58PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/io_ti.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> index c327d4cf7928..ab2370b80b3c 100644
> --- a/drivers/usb/serial/io_ti.c
> +++ b/drivers/usb/serial/io_ti.c
> @@ -260,16 +260,12 @@ static int ti_vread_sync(struct usb_device *dev, __u8 request,
>  {
>  	int status;
>  
> -	status = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
> -			(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN),
> -			value, index, data, size, 1000);
> -	if (status < 0)
> +	status = usb_control_msg_recv(dev, 0, request, USB_TYPE_VENDOR |
> +				      USB_RECIP_DEVICE | USB_DIR_IN, value,
> +				      index, data, size, 1000, GFP_KERNEL);
> +	if (status)
>  		return status;
> -	if (status != size) {
> -		dev_dbg(&dev->dev, "%s - wanted to write %d, but only wrote %d\n",
> -			__func__, size, status);
> -		return -ECOMM;
> -	}
> +
>  	return 0;
>  }
>  
> @@ -278,16 +274,12 @@ static int ti_vsend_sync(struct usb_device *dev, u8 request, u16 value,
>  {
>  	int status;
>  
> -	status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
> -			(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT),
> -			value, index, data, size, timeout);
> -	if (status < 0)
> +	status = usb_control_msg_send(dev, 0, request, USB_TYPE_VENDOR |
> +				      USB_RECIP_DEVICE | USB_DIR_OUT, value,
> +				      index, data, size, timeout, GFP_KERNEL);
> +	if (status)
>  		return status;
> -	if (status != size) {
> -		dev_dbg(&dev->dev, "%s - wanted to write %d, but only wrote %d\n",
> -			__func__, size, status);
> -		return -ECOMM;
> -	}
> +
>  	return 0;
>  }

These helper are only used with DMA-able buffers so this change
introduces an additional redundant allocation and memcpy for every call
for no real gain.

Please drop this one as well.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 11/15] usb: serial: ipaq: use usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_send() nicely wraps usb_control_msg() with proper
error check. Hence use the wrapper instead of calling usb_control_msg()
directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/ipaq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
index f81746c3c26c..99505a76035d 100644
--- a/drivers/usb/serial/ipaq.c
+++ b/drivers/usb/serial/ipaq.c
@@ -530,15 +530,14 @@ static int ipaq_open(struct tty_struct *tty,
 	 */
 	while (retries) {
 		retries--;
-		result = usb_control_msg(serial->dev,
-				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
-				0x1, 0, NULL, 0, 100);
-		if (!result)
+		result = usb_control_msg_send(serial->dev, 0, 0x22, 0x21, 0x1,
+					      0, NULL, 0, 100, GFP_KERNEL);
+		if (result == 0)
 			break;
 
 		msleep(1000);
 	}
-	if (!retries && result) {
+	if (result) {
 		dev_err(&port->dev, "%s - failed doing control urb, error %d\n",
 							__func__, result);
 		return result;
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 11/15] usb: serial: ipaq: use usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:16:59PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_send() nicely wraps usb_control_msg() with proper
> error check. Hence use the wrapper instead of calling usb_control_msg()
> directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/ipaq.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
> index f81746c3c26c..99505a76035d 100644
> --- a/drivers/usb/serial/ipaq.c
> +++ b/drivers/usb/serial/ipaq.c
> @@ -530,15 +530,14 @@ static int ipaq_open(struct tty_struct *tty,
>  	 */
>  	while (retries) {
>  		retries--;
> -		result = usb_control_msg(serial->dev,
> -				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
> -				0x1, 0, NULL, 0, 100);
> -		if (!result)
> +		result = usb_control_msg_send(serial->dev, 0, 0x22, 0x21, 0x1,
> +					      0, NULL, 0, 100, GFP_KERNEL);
> +		if (result == 0)
>  			break;

There's not point in using the new helper since there's no data stage
and usb_control_msg already returns negative errno or 0.

>  		msleep(1000);
>  	}
> -	if (!retries && result) {
> +	if (result) {
>  		dev_err(&port->dev, "%s - failed doing control urb, error %d\n",
>  							__func__, result);
>  		return result;

This looks like just an unrelated simplification of the logic; there was
never any need to check !retries here. You can send that as a clean up
patch of its own if you want.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 12/15] usb: serial: ipw: use usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_send() nicely wraps usb_control_msg() with proper
error check. Hence use the wrapper instead of calling usb_control_msg()
directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/ipw.c | 107 +++++++++++++--------------------------
 1 file changed, 36 insertions(+), 71 deletions(-)

diff --git a/drivers/usb/serial/ipw.c b/drivers/usb/serial/ipw.c
index d04c7cc5c1c2..3c3895b4dff8 100644
--- a/drivers/usb/serial/ipw.c
+++ b/drivers/usb/serial/ipw.c
@@ -134,26 +134,16 @@ static int ipw_open(struct tty_struct *tty, struct usb_serial_port *port)
 	struct usb_device *udev = port->serial->dev;
 	struct device *dev = &port->dev;
 	u8 buf_flow_static[16] = IPW_BYTES_FLOWINIT;
-	u8 *buf_flow_init;
 	int result;
 
-	buf_flow_init = kmemdup(buf_flow_static, 16, GFP_KERNEL);
-	if (!buf_flow_init)
-		return -ENOMEM;
-
 	/* --1: Tell the modem to initialize (we think) From sniffs this is
 	 *	always the first thing that gets sent to the modem during
 	 *	opening of the device */
 	dev_dbg(dev, "%s: Sending SIO_INIT (we guess)\n", __func__);
-	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			 IPW_SIO_INIT,
-			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
-			 0,
-			 0, /* index */
-			 NULL,
-			 0,
-			 100000);
-	if (result < 0)
+	result = usb_control_msg_send(udev, 0, IPW_SIO_INIT, USB_TYPE_VENDOR |
+				      USB_RECIP_INTERFACE | USB_DIR_OUT, 0,
+				      0,  NULL, 0, 100000, GFP_KERNEL);
+	if (result)
 		dev_err(dev, "Init of modem failed (error = %d)\n", result);
 
 	/* reset the bulk pipes */
@@ -166,31 +156,22 @@ static int ipw_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 	/*--3: Tell the modem to open the floodgates on the rx bulk channel */
 	dev_dbg(dev, "%s:asking modem for RxRead (RXBULK_ON)\n", __func__);
-	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			 IPW_SIO_RXCTL,
-			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
-			 IPW_RXBULK_ON,
-			 0, /* index */
-			 NULL,
-			 0,
-			 100000);
-	if (result < 0)
+	result = usb_control_msg_send(udev, 0, IPW_SIO_RXCTL, USB_TYPE_VENDOR |
+				      USB_RECIP_INTERFACE | USB_DIR_OUT,
+				      IPW_RXBULK_ON, 0, NULL, 0, 100000,
+				      GFP_KERNEL);
+	if (result)
 		dev_err(dev, "Enabling bulk RxRead failed (error = %d)\n", result);
 
 	/*--4: setup the initial flowcontrol */
-	dev_dbg(dev, "%s:setting init flowcontrol (%s)\n", __func__, buf_flow_init);
-	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			 IPW_SIO_HANDFLOW,
-			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
-			 0,
-			 0,
-			 buf_flow_init,
-			 0x10,
-			 200000);
-	if (result < 0)
+	dev_dbg(dev, "%s:setting init flowcontrol (%s)\n", __func__, buf_flow_static);
+	result = usb_control_msg_send(udev, 0, IPW_SIO_HANDFLOW,
+				      USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
+				      USB_DIR_OUT, 0, 0, &buf_flow_static, 0x10,
+				      200000, GFP_KERNEL);
+	if (result)
 		dev_err(dev, "initial flowcontrol failed (error = %d)\n", result);
 
-	kfree(buf_flow_init);
 	return 0;
 }
 
@@ -223,26 +204,20 @@ static void ipw_dtr_rts(struct usb_serial_port *port, int on)
 
 	dev_dbg(dev, "%s: on = %d\n", __func__, on);
 
-	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			 IPW_SIO_SET_PIN,
-			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
-			 on ? IPW_PIN_SETDTR : IPW_PIN_CLRDTR,
-			 0,
-			 NULL,
-			 0,
-			 200000);
-	if (result < 0)
+	result = usb_control_msg_send(udev, 0, IPW_SIO_SET_PIN,
+				      USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
+				      USB_DIR_OUT,
+				      on ? IPW_PIN_SETDTR : IPW_PIN_CLRDTR, 0,
+				      NULL, 0, 200000, GFP_KERNEL);
+	if (result)
 		dev_err(dev, "setting dtr failed (error = %d)\n", result);
 
-	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			 IPW_SIO_SET_PIN, USB_TYPE_VENDOR |
-					USB_RECIP_INTERFACE | USB_DIR_OUT,
-			 on ? IPW_PIN_SETRTS : IPW_PIN_CLRRTS,
-			 0,
-			 NULL,
-			 0,
-			 200000);
-	if (result < 0)
+	result = usb_control_msg_send(udev, 0, IPW_SIO_SET_PIN,
+				      USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
+				      USB_DIR_OUT,
+				      on ? IPW_PIN_SETRTS : IPW_PIN_CLRRTS,
+				      0, NULL, 0, 200000, GFP_KERNEL);
+	if (result)
 		dev_err(dev, "setting rts failed (error = %d)\n", result);
 }
 
@@ -254,30 +229,20 @@ static void ipw_close(struct usb_serial_port *port)
 
 	/*--3: purge */
 	dev_dbg(dev, "%s:sending purge\n", __func__);
-	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			 IPW_SIO_PURGE, USB_TYPE_VENDOR |
-			 		USB_RECIP_INTERFACE | USB_DIR_OUT,
-			 0x03,
-			 0,
-			 NULL,
-			 0,
-			 200000);
-	if (result < 0)
+	result = usb_control_msg_send(udev, 0, IPW_SIO_PURGE, USB_TYPE_VENDOR |
+				      USB_RECIP_INTERFACE | USB_DIR_OUT, 0x03,
+				      0, NULL, 0, 200000, GFP_KERNEL);
+	if (result)
 		dev_err(dev, "purge failed (error = %d)\n", result);
 
 
 	/* send RXBULK_off (tell modem to stop transmitting bulk data on
 	   rx chan) */
-	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			 IPW_SIO_RXCTL,
-			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
-			 IPW_RXBULK_OFF,
-			 0, /* index */
-			 NULL,
-			 0,
-			 100000);
-
-	if (result < 0)
+	result = usb_control_msg_send(udev, 0, IPW_SIO_RXCTL, USB_TYPE_VENDOR |
+				      USB_RECIP_INTERFACE | USB_DIR_OUT,
+				      IPW_RXBULK_OFF, 0, NULL, 0, 100000, GFP_KERNEL);
+
+	if (result)
 		dev_err(dev, "Disabling bulk RxRead failed (error = %d)\n", result);
 
 	usb_wwan_close(port);
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 12/15] usb: serial: ipw: use usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:17:00PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_send() nicely wraps usb_control_msg() with proper
> error check. Hence use the wrapper instead of calling usb_control_msg()
> directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/ipw.c | 107 +++++++++++++--------------------------
>  1 file changed, 36 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/usb/serial/ipw.c b/drivers/usb/serial/ipw.c
> index d04c7cc5c1c2..3c3895b4dff8 100644
> --- a/drivers/usb/serial/ipw.c
> +++ b/drivers/usb/serial/ipw.c
> @@ -134,26 +134,16 @@ static int ipw_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	struct usb_device *udev = port->serial->dev;
>  	struct device *dev = &port->dev;
>  	u8 buf_flow_static[16] = IPW_BYTES_FLOWINIT;
> -	u8 *buf_flow_init;
>  	int result;
>  
> -	buf_flow_init = kmemdup(buf_flow_static, 16, GFP_KERNEL);
> -	if (!buf_flow_init)
> -		return -ENOMEM;
> -
>  	/* --1: Tell the modem to initialize (we think) From sniffs this is
>  	 *	always the first thing that gets sent to the modem during
>  	 *	opening of the device */
>  	dev_dbg(dev, "%s: Sending SIO_INIT (we guess)\n", __func__);
> -	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> -			 IPW_SIO_INIT,
> -			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
> -			 0,
> -			 0, /* index */
> -			 NULL,
> -			 0,
> -			 100000);
> -	if (result < 0)
> +	result = usb_control_msg_send(udev, 0, IPW_SIO_INIT, USB_TYPE_VENDOR |
> +				      USB_RECIP_INTERFACE | USB_DIR_OUT, 0,
> +				      0,  NULL, 0, 100000, GFP_KERNEL);

Try to keep the existing indentation style and also avoid breaking the
request argument (USB_TYPE_VENDOR | ...) over multiple lines.

> +	if (result)
>  		dev_err(dev, "Init of modem failed (error = %d)\n", result);
>  
>  	/* reset the bulk pipes */
> @@ -166,31 +156,22 @@ static int ipw_open(struct tty_struct *tty, struct usb_serial_port *port)
>  
>  	/*--3: Tell the modem to open the floodgates on the rx bulk channel */
>  	dev_dbg(dev, "%s:asking modem for RxRead (RXBULK_ON)\n", __func__);
> -	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> -			 IPW_SIO_RXCTL,
> -			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
> -			 IPW_RXBULK_ON,
> -			 0, /* index */
> -			 NULL,
> -			 0,
> -			 100000);
> -	if (result < 0)
> +	result = usb_control_msg_send(udev, 0, IPW_SIO_RXCTL, USB_TYPE_VENDOR |
> +				      USB_RECIP_INTERFACE | USB_DIR_OUT,
> +				      IPW_RXBULK_ON, 0, NULL, 0, 100000,
> +				      GFP_KERNEL);
> +	if (result)
>  		dev_err(dev, "Enabling bulk RxRead failed (error = %d)\n", result);
>  
>  	/*--4: setup the initial flowcontrol */
> -	dev_dbg(dev, "%s:setting init flowcontrol (%s)\n", __func__, buf_flow_init);
> -	result = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> -			 IPW_SIO_HANDFLOW,
> -			 USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
> -			 0,
> -			 0,
> -			 buf_flow_init,
> -			 0x10,
> -			 200000);
> -	if (result < 0)
> +	dev_dbg(dev, "%s:setting init flowcontrol (%s)\n", __func__, buf_flow_static);
> +	result = usb_control_msg_send(udev, 0, IPW_SIO_HANDFLOW,
> +				      USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
> +				      USB_DIR_OUT, 0, 0, &buf_flow_static, 0x10,
> +				      200000, GFP_KERNEL);
> +	if (result)
>  		dev_err(dev, "initial flowcontrol failed (error = %d)\n", result);

Note that sending a short control message is always an error so there's
nothing wrong with the current errors checks.

In fact, since all but this control request lacks a data stage, I'd just
leave this one as is as well.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 13/15] usb: serial: iuu_phoenix: use usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_send() nicely wraps usb_control_msg() with proper
error check. Hence use the wrapper instead of calling usb_control_msg()
directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/iuu_phoenix.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index b4ba79123d9d..dfbcdcec94e7 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -968,9 +968,8 @@ static int iuu_open(struct tty_struct *tty, struct usb_serial_port *port)
 	priv->poll = 0;
 
 #define SOUP(a, b, c, d)  do { \
-	result = usb_control_msg(port->serial->dev,	\
-				usb_sndctrlpipe(port->serial->dev, 0),	\
-				b, a, c, d, NULL, 0, 1000); \
+	result = usb_control_msg_send(port->serial->dev, 0, b, a, c, d, NULL,\
+				      0, 1000, GFP_KERNEL);		     \
 	dev_dbg(dev, "0x%x:0x%x:0x%x:0x%x  %d\n", a, b, c, d, result); } while (0)
 
 	/*  This is not UART related but IUU USB driver related or something */
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 13/15] usb: serial: iuu_phoenix: use usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:17:01PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_send() nicely wraps usb_control_msg() with proper
> error check. Hence use the wrapper instead of calling usb_control_msg()
> directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/iuu_phoenix.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
> index b4ba79123d9d..dfbcdcec94e7 100644
> --- a/drivers/usb/serial/iuu_phoenix.c
> +++ b/drivers/usb/serial/iuu_phoenix.c
> @@ -968,9 +968,8 @@ static int iuu_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	priv->poll = 0;
>  
>  #define SOUP(a, b, c, d)  do { \
> -	result = usb_control_msg(port->serial->dev,	\
> -				usb_sndctrlpipe(port->serial->dev, 0),	\
> -				b, a, c, d, NULL, 0, 1000); \
> +	result = usb_control_msg_send(port->serial->dev, 0, b, a, c, d, NULL,\
> +				      0, 1000, GFP_KERNEL);		     \

No need to for this either even if the result was used for anything else
but logging (since there's no data stage).

>  	dev_dbg(dev, "0x%x:0x%x:0x%x:0x%x  %d\n", a, b, c, d, result); } while (0)
>  
>  	/*  This is not UART related but IUU USB driver related or something */

Please drop.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 14/15] usb: serial: keyspan_pda: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/keyspan_pda.c | 172 +++++++++++++------------------
 1 file changed, 72 insertions(+), 100 deletions(-)

diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
index c1333919716b..44e1c4490fa9 100644
--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -115,17 +115,17 @@ static void keyspan_pda_request_unthrottle(struct work_struct *work)
 
 	/* ask the device to tell us when the tx buffer becomes
 	   sufficiently empty */
-	result = usb_control_msg(serial->dev,
-				 usb_sndctrlpipe(serial->dev, 0),
-				 7, /* request_unthrottle */
-				 USB_TYPE_VENDOR | USB_RECIP_INTERFACE
-				 | USB_DIR_OUT,
-				 16, /* value: threshold */
-				 0, /* index */
-				 NULL,
-				 0,
-				 2000);
-	if (result < 0)
+	result = usb_control_msg_send(serial->dev, 0,
+				      7, /* request_unthrottle */
+				      USB_TYPE_VENDOR | USB_RECIP_INTERFACE
+				      | USB_DIR_OUT,
+				      16, /* value: threshold */
+				      0, /* index */
+				      NULL,
+				      0,
+				      2000,
+				      GFP_KERNEL);
+	if (result)
 		dev_dbg(&serial->dev->dev, "%s - error %d from usb_control_msg\n",
 			__func__, result);
 }
@@ -269,17 +269,18 @@ static speed_t keyspan_pda_setbaud(struct usb_serial *serial, speed_t baud)
 
 	/* rather than figure out how to sleep while waiting for this
 	   to complete, I just use the "legacy" API. */
-	rc = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			     0, /* set baud */
-			     USB_TYPE_VENDOR
-			     | USB_RECIP_INTERFACE
-			     | USB_DIR_OUT, /* type */
-			     bindex, /* value */
-			     0, /* index */
-			     NULL, /* &data */
-			     0, /* size */
-			     2000); /* timeout */
-	if (rc < 0)
+	rc = usb_control_msg_send(serial->dev, 0,
+				  0, /* set baud */
+				  USB_TYPE_VENDOR
+				  | USB_RECIP_INTERFACE
+				  | USB_DIR_OUT, /* type */
+				  bindex, /* value */
+				  0, /* index */
+				  NULL, /* &data */
+				  0, /* size */
+				  2000, /* timeout */
+				  GFP_KERNEL);
+	if (rc)
 		return 0;
 	return baud;
 }
@@ -296,11 +297,12 @@ static void keyspan_pda_break_ctl(struct tty_struct *tty, int break_state)
 		value = 1; /* start break */
 	else
 		value = 0; /* clear break */
-	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			4, /* set break */
-			USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
-			value, 0, NULL, 0, 2000);
-	if (result < 0)
+	result = usb_control_msg_send(serial->dev, 0,
+				      4, /* set break */
+				      USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
+				      USB_DIR_OUT,
+				      value, 0, NULL, 0, 2000, GFP_KERNEL);
+	if (result)
 		dev_dbg(&port->dev, "%s - error %d from usb_control_msg\n",
 			__func__, result);
 	/* there is something funky about this.. the TCSBRK that 'cu' performs
@@ -359,22 +361,11 @@ 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;
-
-	kfree(data);
+	rc = usb_control_msg_recv(serial->dev, 0,
+				  3, /* get pins */
+				  USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
+				  USB_DIR_IN, 0, 0, value, 1, 2000,
+				  GFP_KERNEL);
 	return rc;
 }
 
@@ -383,10 +374,11 @@ static int keyspan_pda_set_modem_info(struct usb_serial *serial,
 				      unsigned char value)
 {
 	int rc;
-	rc = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			     3, /* set pins */
-			     USB_TYPE_VENDOR|USB_RECIP_INTERFACE|USB_DIR_OUT,
-			     value, 0, NULL, 0, 2000);
+	rc = usb_control_msg_send(serial->dev, 0,
+				  3, /* set pins */
+				  USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
+				  USB_DIR_OUT, value, 0, NULL, 0, 2000,
+				  GFP_KERNEL);
 	return rc;
 }
 
@@ -481,36 +473,25 @@ static int keyspan_pda_write(struct tty_struct *tty,
 	   device how much room it really has.  This is done only on
 	   scheduler time, since usb_control_msg() sleeps. */
 	if (count > priv->tx_room && !in_interrupt()) {
-		u8 *room;
-
-		room = kmalloc(1, GFP_KERNEL);
-		if (!room) {
-			rc = -ENOMEM;
-			goto exit;
+		u8 room;
+
+		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 == 0) {
+			dev_dbg(&port->dev, "roomquery says %d\n", room);
+			priv->tx_room = room;
 		}
 
-		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 > 0) {
-			dev_dbg(&port->dev, "roomquery says %d\n", *room);
-			priv->tx_room = *room;
-		}
-		kfree(room);
-		if (rc < 0) {
-			dev_dbg(&port->dev, "roomquery failed\n");
-			goto exit;
-		}
-		if (rc == 0) {
-			dev_dbg(&port->dev, "roomquery returned 0 bytes\n");
-			rc = -EIO; /* device didn't return any data */
+		if (rc) {
+			dev_dbg(&port->dev, "roomquery failed %d\n", rc);
 			goto exit;
 		}
 	}
@@ -613,36 +594,28 @@ static int keyspan_pda_open(struct tty_struct *tty,
 					struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
-	u8 *room;
+	u8 room;
 	int rc = 0;
 	struct keyspan_pda_private *priv;
 
-	/* find out how much room is in the Tx ring */
-	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, /* index */
-			     room,
-			     1,
-			     2000);
-	if (rc < 0) {
-		dev_dbg(&port->dev, "%s - roomquery failed\n", __func__);
-		goto error;
-	}
-	if (rc == 0) {
-		dev_dbg(&port->dev, "%s - roomquery returned 0 bytes\n", __func__);
-		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, /* index */
+				  &room,
+				  1,
+				  2000,
+				  GFP_KERNEL);
+	if (rc) {
+		dev_dbg(&port->dev, "%s - roomquery failed %d\n", __func__, rc);
 		goto error;
 	}
+
 	priv = usb_get_serial_port_data(port);
-	priv->tx_room = *room;
-	priv->tx_throttled = *room ? 0 : 1;
+	priv->tx_room = room;
+	priv->tx_throttled = room ? 0 : 1;
 
 	/*Start reading from the device*/
 	rc = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
@@ -651,7 +624,6 @@ static int keyspan_pda_open(struct tty_struct *tty,
 		goto error;
 	}
 error:
-	kfree(room);
 	return rc;
 }
 static void keyspan_pda_close(struct usb_serial_port *port)
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 14/15] usb: serial: keyspan_pda: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:17:02PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/keyspan_pda.c | 172 +++++++++++++------------------
>  1 file changed, 72 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
> index c1333919716b..44e1c4490fa9 100644
> --- a/drivers/usb/serial/keyspan_pda.c
> +++ b/drivers/usb/serial/keyspan_pda.c

There were some changes done to this driver around the same time that
you submitted these so this one needs to be rebased.

> @@ -359,22 +361,11 @@ 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;
> -
> -	kfree(data);
> +	rc = usb_control_msg_recv(serial->dev, 0,
> +				  3, /* get pins */
> +				  USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
> +				  USB_DIR_IN, 0, 0, value, 1, 2000,
> +				  GFP_KERNEL);
>  	return rc;
>  }

Please only change the requests with a data stage like this one, but
avoid breaking the request argument over multiple lines.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 15/15] usb: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/kl5kusb105.c | 94 +++++++++++++++------------------
 1 file changed, 44 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/serial/kl5kusb105.c b/drivers/usb/serial/kl5kusb105.c
index 5ee48b0650c4..75cfd1c907f3 100644
--- a/drivers/usb/serial/kl5kusb105.c
+++ b/drivers/usb/serial/kl5kusb105.c
@@ -124,16 +124,17 @@ 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 +168,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[2];
 	__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 +192,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;
 }
 
@@ -283,16 +276,17 @@ static int  klsi_105_open(struct tty_struct *tty, struct usb_serial_port *port)
 		goto err_free_cfg;
 	}
 
-	rc = usb_control_msg(port->serial->dev,
-			     usb_sndctrlpipe(port->serial->dev, 0),
-			     KL5KUSB105A_SIO_CONFIGURE,
-			     USB_TYPE_VENDOR|USB_DIR_OUT|USB_RECIP_INTERFACE,
-			     KL5KUSB105A_SIO_CONFIGURE_READ_ON,
-			     0, /* index */
-			     NULL,
-			     0,
-			     KLSI_TIMEOUT);
-	if (rc < 0) {
+	rc  = usb_control_msg_send(port->serial->dev, 0,
+				   KL5KUSB105A_SIO_CONFIGURE,
+				   USB_TYPE_VENDOR | USB_DIR_OUT |
+				   USB_RECIP_INTERFACE,
+				   KL5KUSB105A_SIO_CONFIGURE_READ_ON,
+				   0, /* index */
+				   NULL,
+				   0,
+				   KLSI_TIMEOUT,
+				   GFP_KERNEL);
+	if (rc) {
 		dev_err(&port->dev, "Enabling read failed (error = %d)\n", rc);
 		retval = rc;
 		goto err_generic_close;
@@ -314,14 +308,14 @@ static int  klsi_105_open(struct tty_struct *tty, struct usb_serial_port *port)
 	return 0;
 
 err_disable_read:
-	usb_control_msg(port->serial->dev,
-			     usb_sndctrlpipe(port->serial->dev, 0),
+	usb_control_msg_send(port->serial->dev, 0,
 			     KL5KUSB105A_SIO_CONFIGURE,
 			     USB_TYPE_VENDOR | USB_DIR_OUT,
 			     KL5KUSB105A_SIO_CONFIGURE_READ_OFF,
 			     0, /* index */
 			     NULL, 0,
-			     KLSI_TIMEOUT);
+			     KLSI_TIMEOUT,
+			     GFP_KERNEL);
 err_generic_close:
 	usb_serial_generic_close(port);
 err_free_cfg:
@@ -335,15 +329,15 @@ static void klsi_105_close(struct usb_serial_port *port)
 	int rc;
 
 	/* send READ_OFF */
-	rc = usb_control_msg(port->serial->dev,
-			     usb_sndctrlpipe(port->serial->dev, 0),
-			     KL5KUSB105A_SIO_CONFIGURE,
-			     USB_TYPE_VENDOR | USB_DIR_OUT,
-			     KL5KUSB105A_SIO_CONFIGURE_READ_OFF,
-			     0, /* index */
-			     NULL, 0,
-			     KLSI_TIMEOUT);
-	if (rc < 0)
+	rc = usb_control_msg_send(port->serial->dev, 0,
+				  KL5KUSB105A_SIO_CONFIGURE,
+				  USB_TYPE_VENDOR | USB_DIR_OUT,
+				  KL5KUSB105A_SIO_CONFIGURE_READ_OFF,
+				  0, /* index */
+				  NULL, 0,
+				  KLSI_TIMEOUT,
+				  GFP_KERNEL);
+	if (rc)
 		dev_err(&port->dev, "failed to disable read: %d\n", rc);
 
 	/* shutdown our bulk reads and writes */
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 15/15] usb: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()

[Johan Hovold]

On Wed, Nov 04, 2020 at 12:17:03PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/usb/serial/kl5kusb105.c | 94 +++++++++++++++------------------
>  1 file changed, 44 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/serial/kl5kusb105.c b/drivers/usb/serial/kl5kusb105.c
> index 5ee48b0650c4..75cfd1c907f3 100644
> --- a/drivers/usb/serial/kl5kusb105.c
> +++ b/drivers/usb/serial/kl5kusb105.c
> @@ -124,16 +124,17 @@ 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);

The callers of this function already allocates a transfer-buffer which
you should now remove to avoid introducing an additional memdup() per
call.

Note that there was a related bug in open() which failed to release it's
transfer buffer on successful open. I CCed you on the fix which we
should get merged and backported before converting to using the new
helper.

> @@ -283,16 +276,17 @@ static int  klsi_105_open(struct tty_struct *tty, struct usb_serial_port *port)
>  		goto err_free_cfg;
>  	}
>  
> -	rc = usb_control_msg(port->serial->dev,
> -			     usb_sndctrlpipe(port->serial->dev, 0),
> -			     KL5KUSB105A_SIO_CONFIGURE,
> -			     USB_TYPE_VENDOR|USB_DIR_OUT|USB_RECIP_INTERFACE,
> -			     KL5KUSB105A_SIO_CONFIGURE_READ_ON,
> -			     0, /* index */
> -			     NULL,
> -			     0,
> -			     KLSI_TIMEOUT);
> -	if (rc < 0) {
> +	rc  = usb_control_msg_send(port->serial->dev, 0,
> +				   KL5KUSB105A_SIO_CONFIGURE,
> +				   USB_TYPE_VENDOR | USB_DIR_OUT |
> +				   USB_RECIP_INTERFACE,
> +				   KL5KUSB105A_SIO_CONFIGURE_READ_ON,
> +				   0, /* index */
> +				   NULL,
> +				   0,
> +				   KLSI_TIMEOUT,
> +				   GFP_KERNEL);
> +	if (rc) {

And again there's no need to change the control transfers without a data
stage.

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 00/15] usb: serial: avoid using usb_control_msg() directly

[Greg KH]

On Wed, Nov 04, 2020 at 12:16:48PM +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. Hence use them whenever possible
> and avoid using usb_control_msg() directly.
> 
> Himadri Pandya (15):
>   usb: serial: ark3116: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: belkin_sa: use usb_control_msg_send()
>   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: cypress_m8: 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: f81534: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ftdi_sio: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_edgeport: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_ti: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ipaq: use usb_control_msg_send()
>   usb: serial: ipw: use usb_control_msg_send()
>   usb: serial: iuu_phoenix: use usb_control_msg_send()
>   usb: serial: keyspan_pda: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: kl5kusb105: use usb_control_msg_recv() and
>     usb_control_msg_send()

For the whole series:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 00/15] usb: serial: avoid using usb_control_msg() directly

[Johan Hovold]

Hi Himadri,

and sorry about the late feedback on this one. I'm still trying to dig
myself out of some backlog.

On Wed, Nov 04, 2020 at 12:16:48PM +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. Hence use them whenever possible
> and avoid using usb_control_msg() directly.

Replacing working code with shiny new helpers unfortunately often ends
up introducing new bugs and I'm afraid there are a few examples of that
in this series as well.

I'll comment on the patches individually, but my impression is that we
should primarily use these helpers to replace code which allocates a
temporary buffer for each transfer as otherwise there's no clear gain
from using them.

Some of your patches contains unrelated changes which needs to go in
separate patches if they're to be included at all.

Please also try to write dedicated commit messages rater than reusing
more or less the same stock message since not everything in these
messages apply to each patch. You never mention that these helpers
nicely hides the allocation of temporary transfer buffers in some cases
for examples. In other places they instead introduce additional
allocations which at least should have been highlighted.

> Himadri Pandya (15):
>   usb: serial: ark3116: use usb_control_msg_recv() and
>     usb_control_msg_send()

Nit: please also use an uppercase "USB" prefix.

>   usb: serial: belkin_sa: use usb_control_msg_send()
>   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: cypress_m8: 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: f81534: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ftdi_sio: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_edgeport: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_ti: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ipaq: use usb_control_msg_send()
>   usb: serial: ipw: use usb_control_msg_send()
>   usb: serial: iuu_phoenix: use usb_control_msg_send()
>   usb: serial: keyspan_pda: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: kl5kusb105: use usb_control_msg_recv() and
>     usb_control_msg_send()
> 
>  drivers/usb/serial/ark3116.c     |  29 +----
>  drivers/usb/serial/belkin_sa.c   |  35 +++---
>  drivers/usb/serial/ch341.c       |  45 +++-----
>  drivers/usb/serial/cp210x.c      | 148 +++++++------------------
>  drivers/usb/serial/cypress_m8.c  |  38 ++++---
>  drivers/usb/serial/f81232.c      |  88 +++------------
>  drivers/usb/serial/f81534.c      |  63 +++--------
>  drivers/usb/serial/ftdi_sio.c    | 182 +++++++++++++------------------
>  drivers/usb/serial/io_edgeport.c |  73 +++++--------
>  drivers/usb/serial/io_ti.c       |  28 ++---
>  drivers/usb/serial/ipaq.c        |   9 +-
>  drivers/usb/serial/ipw.c         | 107 ++++++------------
>  drivers/usb/serial/iuu_phoenix.c |   5 +-
>  drivers/usb/serial/keyspan_pda.c | 172 ++++++++++++-----------------
>  drivers/usb/serial/kl5kusb105.c  |  94 ++++++++--------
>  15 files changed, 406 insertions(+), 710 deletions(-)

Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 00/15] usb: serial: avoid using usb_control_msg() directly

[Himadri Pandya]

On Fri, Dec 4, 2020 at 2:38 PM Johan Hovold <johan@kernel.org> wrote:
>
> Hi Himadri,
>
> and sorry about the late feedback on this one. I'm still trying to dig
> myself out of some backlog.
>
> On Wed, Nov 04, 2020 at 12:16:48PM +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. Hence use them whenever possible
> > and avoid using usb_control_msg() directly.
>
> Replacing working code with shiny new helpers unfortunately often ends
> up introducing new bugs and I'm afraid there are a few examples of that
> in this series as well.
>
> I'll comment on the patches individually, but my impression is that we
> should primarily use these helpers to replace code which allocates a
> temporary buffer for each transfer as otherwise there's no clear gain
> from using them.
>
> Some of your patches contains unrelated changes which needs to go in
> separate patches if they're to be included at all.
>
> Please also try to write dedicated commit messages rater than reusing
> more or less the same stock message since not everything in these
> messages apply to each patch. You never mention that these helpers
> nicely hides the allocation of temporary transfer buffers in some cases
> for examples. In other places they instead introduce additional
> allocations which at least should have been highlighted.
>
> > Himadri Pandya (15):
> >   usb: serial: ark3116: use usb_control_msg_recv() and
> >     usb_control_msg_send()
>
> Nit: please also use an uppercase "USB" prefix.

Hi Johan,

Thanks for reviewing this series and sorry for the late reply. I'll
soon send a v2 according to your comments.

Best regards,
Himadri

>
> >   usb: serial: belkin_sa: use usb_control_msg_send()
> >   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: cypress_m8: 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: f81534: use usb_control_msg_recv() and
> >     usb_control_msg_send()
> >   usb: serial: ftdi_sio: use usb_control_msg_recv() and
> >     usb_control_msg_send()
> >   usb: serial: io_edgeport: use usb_control_msg_recv() and
> >     usb_control_msg_send()
> >   usb: serial: io_ti: use usb_control_msg_recv() and
> >     usb_control_msg_send()
> >   usb: serial: ipaq: use usb_control_msg_send()
> >   usb: serial: ipw: use usb_control_msg_send()
> >   usb: serial: iuu_phoenix: use usb_control_msg_send()
> >   usb: serial: keyspan_pda: use usb_control_msg_recv() and
> >     usb_control_msg_send()
> >   usb: serial: kl5kusb105: use usb_control_msg_recv() and
> >     usb_control_msg_send()
> >
> >  drivers/usb/serial/ark3116.c     |  29 +----
> >  drivers/usb/serial/belkin_sa.c   |  35 +++---
> >  drivers/usb/serial/ch341.c       |  45 +++-----
> >  drivers/usb/serial/cp210x.c      | 148 +++++++------------------
> >  drivers/usb/serial/cypress_m8.c  |  38 ++++---
> >  drivers/usb/serial/f81232.c      |  88 +++------------
> >  drivers/usb/serial/f81534.c      |  63 +++--------
> >  drivers/usb/serial/ftdi_sio.c    | 182 +++++++++++++------------------
> >  drivers/usb/serial/io_edgeport.c |  73 +++++--------
> >  drivers/usb/serial/io_ti.c       |  28 ++---
> >  drivers/usb/serial/ipaq.c        |   9 +-
> >  drivers/usb/serial/ipw.c         | 107 ++++++------------
> >  drivers/usb/serial/iuu_phoenix.c |   5 +-
> >  drivers/usb/serial/keyspan_pda.c | 172 ++++++++++++-----------------
> >  drivers/usb/serial/kl5kusb105.c  |  94 ++++++++--------
> >  15 files changed, 406 insertions(+), 710 deletions(-)
>
> Johan
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees