diff mbox series

[15/15] usb: serial: kl5kusb105: use usb_control_msg_recv() and usb_control_msg_send()

Message ID 20201104064703.15123-16-himadrispandya@gmail.com
State New
Headers show
Series usb: serial: avoid using usb_control_msg() directly | expand

Commit Message

Himadri Pandya Nov. 4, 2020, 6:47 a.m. UTC 
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(-)

Comments

Johan Hovold Dec. 4, 2020, 10:37 a.m. UTC | #1 
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
diff mbox series

Patch

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 */