diff mbox series

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

Message ID 20201104064703.15123-9-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:46 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/ftdi_sio.c | 182 +++++++++++++++-------------------
 1 file changed, 78 insertions(+), 104 deletions(-)

Comments

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

Patch

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