Skip to contentSkip to conversation

USB: serial: xr: fix up remaining issues in new driver

PATCH In review Johan Hovold 1 participant 14 messages Jan 21, 2021 – Jan 26, 2021

Subsystems (2)
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

cover USB: serial: xr: fix up remaining issues in new driver

This series fixes the remaining issues in the new MaxLinear driver that were pointed out here:

https://lore.kernel.org/r/[email protected]

Johan

Johan Hovold (10):
USB: serial: xr: fix NULL-deref at probe
USB: serial: xr: fix interface leak at disconnect
USB: serial: xr: use subsystem usb_device at probe
USB: serial: xr: use termios flag helpers
USB: serial: xr: document vendor-request recipient
USB: serial: xr: clean up line-settings handling
USB: serial: xr: simplify line-speed logic
USB: serial: xr: fix gpio-mode handling
USB: serial: xr: fix pin configuration
USB: serial: xr: fix B0 handling

 drivers/usb/serial/xr_serial.c | 102 +++++++++++++++++++--------------
 1 file changed, 59 insertions(+), 43 deletions(-)
Signature · 1 line
2.26.2
author maintainer
#1 lore raw

PATCH 1/10 USB: serial: xr: fix NULL-deref at probe

Make sure that the probed device has an interface 0 to avoid dereferencing a NULL pointer in case of a malicious device or during USB-descriptor fuzzing.

Fixes a8f54b7bd132 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
---
 drivers/usb/serial/xr_serial.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/serial/xr_serial.c +3 -0
index bdb2df27b50b..7be6da6a5cf3 100644
@@ -552,6 +552,9 @@ static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
 
 	/* But claim the control interface during data interface probe */
 	control_interface = usb_ifnum_to_if(usb_dev, 0);
+	if (!control_interface)
+		return -ENODEV;
+
 	ret = usb_driver_claim_interface(driver, control_interface, NULL);
 	if (ret) {
 		dev_err(&serial->interface->dev, "Failed to claim control interface\n");
Signature · 1 line
2.26.2
author maintainer
#2 lore raw

PATCH 1/1 USB: serial: cp210x: suppress modem-control error on open and close

In reply to Johan Hovold

The CP210X_SET_MHS request cannot be used to control DTR/RTS when hardware flow control is enabled and instead returns an error which is currently logged as:

	cp210x ttyUSB0: failed set request 0x7 status: -32

Add a crtscts flag to keep track of the hardware flow-control setting and use it to suppress the request in dtr_rts().

Note that both lines are still deasserted when disabling the UART as part of close().

Also note that TIOCMSET is left unchanged and will continue to return an error to user-space when flow control is enabled (i.e. instead of disabling and re-enabling auto-RTS when RTS is deasserted and re-asserted).

---
 drivers/usb/serial/cp210x.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/usb/serial/cp210x.c +15 -1
index d813a052738f..ac1e5cbe61dd 100644
@@ -264,7 +264,8 @@ struct cp210x_port_private {
 	u8			bInterfaceNumber;
 	bool			event_mode;
 	enum cp210x_event_state event_state;
-	u8 lsr;
+	u8			lsr;
+	bool			crtscts;
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -1117,6 +1118,7 @@ static bool cp210x_termios_change(const struct ktermios *a, const struct ktermio
 static void cp210x_set_flow_control(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
 	struct cp210x_special_chars chars;
 	struct cp210x_flow_ctl flow_ctl;
 	u32 flow_repl;
@@ -1161,10 +1163,12 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 		ctl_hs |= CP210X_SERIAL_CTS_HANDSHAKE;
 		flow_repl &= ~CP210X_SERIAL_RTS_MASK;
 		flow_repl |= CP210X_SERIAL_RTS_SHIFT(CP210X_SERIAL_RTS_FLOW_CTL);
+		port_priv->crtscts = true;
 	} else {
 		ctl_hs &= ~CP210X_SERIAL_CTS_HANDSHAKE;
 		flow_repl &= ~CP210X_SERIAL_RTS_MASK;
 		flow_repl |= CP210X_SERIAL_RTS_SHIFT(CP210X_SERIAL_RTS_ACTIVE);
+		port_priv->crtscts = false;
 	}
 
 	if (I_IXOFF(tty))
@@ -1298,6 +1302,16 @@ static int cp210x_tiocmset_port(struct usb_serial_port *port,
 
 static void cp210x_dtr_rts(struct usb_serial_port *port, int on)
 {
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+
+	/*
+	 * CP210X_SET_MHS cannot be used to control DTR/RTS when hardware flow
+	 * control is enabled. Note that both lines are still deasserted when
+	 * disabling the UART.
+	 */
+	if (port_priv->crtscts)
+		return;
+
 	if (on)
 		cp210x_tiocmset_port(port, TIOCM_DTR | TIOCM_RTS, 0);
 	else
Signature · 1 line
2.26.2
author maintainer
#3 lore raw

Re: [PATCH] USB: serial: cp210x: suppress modem-control error on open and close

Quoted 12 lines · Johan Hovold

The CP210X_SET_MHS request cannot be used to control DTR/RTS when hardware flow control is enabled and instead returns an error which is currently logged as:

	cp210x ttyUSB0: failed set request 0x7 status: -32

Add a crtscts flag to keep track of the hardware flow-control setting and use it to suppress the request in dtr_rts().

Note that both lines are still deasserted when disabling the UART as part of close().

Quoted 8 lines

Also note that TIOCMSET is left unchanged and will continue to return an error to user-space when flow control is enabled (i.e. instead of disabling and re-enabling auto-RTS when RTS is deasserted and re-asserted).

---

Oops, this one wasn't supposed to be submitted again. Please ignore.

Johan

author maintainer
#4 lore raw

PATCH 2/10 USB: serial: xr: fix interface leak at disconnect

In reply to Johan Hovold

Make sure to release the control interface at disconnect so that the driver can be unbound without leaking resources (and later rebound).

Fixes a8f54b7bd132 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
---
 drivers/usb/serial/xr_serial.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/usb/serial/xr_serial.c +10 -0
index 7be6da6a5cf3..5e110b0c8e71 100644
@@ -564,6 +564,15 @@ static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
 	return 0;
 }
 
+static void xr_disconnect(struct usb_serial *serial)
+{
+	struct usb_driver *driver = serial->type->usb_driver;
+	struct usb_interface *control_interface;
+
+	control_interface = usb_ifnum_to_if(serial->dev, 0);
+	usb_driver_release_interface(driver, control_interface);
+}
+
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
 	{ }
@@ -578,6 +587,7 @@ static struct usb_serial_driver xr_device = {
 	.id_table		= id_table,
 	.num_ports		= 1,
 	.probe			= xr_probe,
+	.disconnect		= xr_disconnect,
 	.open			= xr_open,
 	.close			= xr_close,
 	.break_ctl		= xr_break_ctl,
Signature · 1 line
2.26.2
author maintainer
#5 lore raw

PATCH 3/10 USB: serial: xr: use subsystem usb_device at probe

In reply to Johan Hovold

Use the subsystem struct usb_device pointer at probe instead of deriving it from the interface pointer.

---
 drivers/usb/serial/xr_serial.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/serial/xr_serial.c +1 -2
index 5e110b0c8e71..8f81f866d681 100644
@@ -541,7 +541,6 @@ static void xr_close(struct usb_serial_port *port)
 
 static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
 {
-	struct usb_device *usb_dev = interface_to_usbdev(serial->interface);
 	struct usb_driver *driver = serial->type->usb_driver;
 	struct usb_interface *control_interface;
 	int ret;
@@ -551,7 +550,7 @@ static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
 		return -ENODEV;
 
 	/* But claim the control interface during data interface probe */
-	control_interface = usb_ifnum_to_if(usb_dev, 0);
+	control_interface = usb_ifnum_to_if(serial->dev, 0);
 	if (!control_interface)
 		return -ENODEV;
 
Signature · 1 line
2.26.2
author maintainer
#6 lore raw

PATCH 4/10 USB: serial: xr: use termios flag helpers

In reply to Johan Hovold

Use the termios flag helpers consistently, including for CRTSCTS.

---
 drivers/usb/serial/xr_serial.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/serial/xr_serial.c +1 -2
index 8f81f866d681..52909dccb4dc 100644
@@ -401,7 +401,6 @@ static int xr_set_baudrate(struct tty_struct *tty,
 static void xr_set_flow_mode(struct tty_struct *tty,
 			     struct usb_serial_port *port)
 {
-	unsigned int cflag = tty->termios.c_cflag;
 	u8 flow, gpio_mode;
 	int ret;
 
@@ -409,7 +408,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 	if (ret)
 		return;
 
-	if (cflag & CRTSCTS) {
+	if (C_CRTSCTS(tty)) {
 		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
 
 		/*
Signature · 1 line
2.26.2
author maintainer
#7 lore raw

PATCH 5/10 USB: serial: xr: document vendor-request recipient

In reply to Johan Hovold

Add the missing device-recipient define to the vendor control requests for completeness.

---
 drivers/usb/serial/xr_serial.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/serial/xr_serial.c +4 -4
index 52909dccb4dc..202263211ba9 100644
@@ -116,8 +116,8 @@ static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg, u8 val)
 	ret = usb_control_msg(serial->dev,
 			      usb_sndctrlpipe(serial->dev, 0),
 			      XR21V141X_SET_REQ,
-			      USB_DIR_OUT | USB_TYPE_VENDOR, val,
-			      reg | (block << 8), NULL, 0,
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      val, reg | (block << 8), NULL, 0,
 			      USB_CTRL_SET_TIMEOUT);
 	if (ret < 0) {
 		dev_err(&port->dev, "Failed to set reg 0x%02x: %d\n", reg, ret);
@@ -140,8 +140,8 @@ static int xr_get_reg(struct usb_serial_port *port, u8 block, u8 reg, u8 *val)
 	ret = usb_control_msg(serial->dev,
 			      usb_rcvctrlpipe(serial->dev, 0),
 			      XR21V141X_GET_REQ,
-			      USB_DIR_IN | USB_TYPE_VENDOR, 0,
-			      reg | (block << 8), dmabuf, 1,
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, reg | (block << 8), dmabuf, 1,
 			      USB_CTRL_GET_TIMEOUT);
 	if (ret == 1) {
 		*val = *dmabuf;
Signature · 1 line
2.26.2
author maintainer
#8 lore raw

PATCH 6/10 USB: serial: xr: clean up line-settings handling

In reply to Johan Hovold

Shift the line-setting values when defining them rather than in set_termios() for consistency and improved readability.

---
 drivers/usb/serial/xr_serial.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)
drivers/usb/serial/xr_serial.c +15 -19
index 202263211ba9..2000277bacc1 100644
@@ -71,17 +71,17 @@ struct xr_txrx_clk_mask {
 #define XR21V141X_UART_DATA_8		0x8
 
 #define XR21V141X_UART_PARITY_MASK	GENMASK(6, 4)
-#define XR21V141X_UART_PARITY_SHIFT	0x4
-#define XR21V141X_UART_PARITY_NONE	0x0
-#define XR21V141X_UART_PARITY_ODD	0x1
-#define XR21V141X_UART_PARITY_EVEN	0x2
-#define XR21V141X_UART_PARITY_MARK	0x3
-#define XR21V141X_UART_PARITY_SPACE	0x4
+#define XR21V141X_UART_PARITY_SHIFT	4
+#define XR21V141X_UART_PARITY_NONE	(0x0 << XR21V141X_UART_PARITY_SHIFT)
+#define XR21V141X_UART_PARITY_ODD	(0x1 << XR21V141X_UART_PARITY_SHIFT)
+#define XR21V141X_UART_PARITY_EVEN	(0x2 << XR21V141X_UART_PARITY_SHIFT)
+#define XR21V141X_UART_PARITY_MARK	(0x3 << XR21V141X_UART_PARITY_SHIFT)
+#define XR21V141X_UART_PARITY_SPACE	(0x4 << XR21V141X_UART_PARITY_SHIFT)
 
 #define XR21V141X_UART_STOP_MASK	BIT(7)
-#define XR21V141X_UART_STOP_SHIFT	0x7
-#define XR21V141X_UART_STOP_1		0x0
-#define XR21V141X_UART_STOP_2		0x1
+#define XR21V141X_UART_STOP_SHIFT	7
+#define XR21V141X_UART_STOP_1		(0x0 << XR21V141X_UART_STOP_SHIFT)
+#define XR21V141X_UART_STOP_2		(0x1 << XR21V141X_UART_STOP_SHIFT)
 
 #define XR21V141X_UART_FLOW_MODE_NONE	0x0
 #define XR21V141X_UART_FLOW_MODE_HW	0x1
@@ -477,25 +477,21 @@ static void xr_set_termios(struct tty_struct *tty,
 	if (C_PARENB(tty)) {
 		if (C_CMSPAR(tty)) {
 			if (C_PARODD(tty))
-				bits |= XR21V141X_UART_PARITY_MARK <<
-					XR21V141X_UART_PARITY_SHIFT;
+				bits |= XR21V141X_UART_PARITY_MARK;
 			else
-				bits |= XR21V141X_UART_PARITY_SPACE <<
-					XR21V141X_UART_PARITY_SHIFT;
+				bits |= XR21V141X_UART_PARITY_SPACE;
 		} else {
 			if (C_PARODD(tty))
-				bits |= XR21V141X_UART_PARITY_ODD <<
-					XR21V141X_UART_PARITY_SHIFT;
+				bits |= XR21V141X_UART_PARITY_ODD;
 			else
-				bits |= XR21V141X_UART_PARITY_EVEN <<
-					XR21V141X_UART_PARITY_SHIFT;
+				bits |= XR21V141X_UART_PARITY_EVEN;
 		}
 	}
 
 	if (C_CSTOPB(tty))
-		bits |= XR21V141X_UART_STOP_2 << XR21V141X_UART_STOP_SHIFT;
+		bits |= XR21V141X_UART_STOP_2;
 	else
-		bits |= XR21V141X_UART_STOP_1 << XR21V141X_UART_STOP_SHIFT;
+		bits |= XR21V141X_UART_STOP_1;
 
 	ret = xr_set_reg_uart(port, XR21V141X_REG_FORMAT, bits);
 	if (ret)
Signature · 1 line
2.26.2
author maintainer
#9 lore raw

PATCH 7/10 USB: serial: xr: simplify line-speed logic

In reply to Johan Hovold

Simplify the changed-line-speed conditional expression.

---
 drivers/usb/serial/xr_serial.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/serial/xr_serial.c +1 -2
index 2000277bacc1..fc727f4283f2 100644
@@ -451,8 +451,7 @@ static void xr_set_termios(struct tty_struct *tty,
 	u8 bits = 0;
 	int ret;
 
-	if ((old_termios && tty->termios.c_ospeed != old_termios->c_ospeed) ||
-	    !old_termios)
+	if (!old_termios || (tty->termios.c_ospeed != old_termios->c_ospeed))
 		xr_set_baudrate(tty, port);
 
 	switch (C_CSIZE(tty)) {
Signature · 1 line
2.26.2
author maintainer
#10 lore raw

PATCH 8/10 USB: serial: xr: fix gpio-mode handling

In reply to Johan Hovold

Fix the gpio-mode handling so that all the pins are under driver control (i.e. in gpio mode) when hardware flow control is disabled.

This is specifically needed to be able to control RTS.

Fixes a8f54b7bd132 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
---
 drivers/usb/serial/xr_serial.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/usb/serial/xr_serial.c +3 -6
index fc727f4283f2..183731cd2ef7 100644
@@ -408,14 +408,11 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 	if (ret)
 		return;
 
+	/* Set GPIO mode for controlling the pins manually by default. */
+	gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK;
+
 	if (C_CRTSCTS(tty)) {
 		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
-
-		/*
-		 * RTS/CTS is the default flow control mode, so set GPIO mode
-		 * for controlling the pins manually by default.
-		 */
-		gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK;
 		gpio_mode |= XR21V141X_UART_MODE_RTS_CTS;
 		flow = XR21V141X_UART_FLOW_MODE_HW;
 	} else if (I_IXON(tty)) {
Signature · 1 line
2.26.2
author maintainer
#11 lore raw

PATCH 9/10 USB: serial: xr: fix pin configuration

In reply to Johan Hovold

Make sure that the modem pins are set up correctly when opening the port to avoid leaving, for example, DTR and RTS configured as inputs, which is the device default.

This is specifically needed to be able to control DTR and RTS when hardware flow control is disabled.

Fixes a8f54b7bd132 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
---
 drivers/usb/serial/xr_serial.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/usb/serial/xr_serial.c +8 -0
index 183731cd2ef7..f67e7dba9509 100644
@@ -502,6 +502,7 @@ static void xr_set_termios(struct tty_struct *tty,
 
 static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
+	u8 gpio_dir;
 	int ret;
 
 	ret = xr_uart_enable(port);
@@ -510,6 +511,13 @@ static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
 		return ret;
 	}
 
+	/*
+	 * Configure DTR and RTS as outputs and RI, CD, DSR and CTS as
+	 * inputs.
+	 */
+	gpio_dir = XR21V141X_UART_MODE_DTR | XR21V141X_UART_MODE_RTS;
+	xr_set_reg_uart(port, XR21V141X_REG_GPIO_DIR, gpio_dir);
+
 	/* Setup termios */
 	if (tty)
 		xr_set_termios(tty, port, NULL);
Signature · 1 line
2.26.2
author maintainer
#12 lore raw

PATCH 10/10 USB: serial: xr: fix B0 handling

In reply to Johan Hovold

Fix up B0 handling which should leave the baud rate unchanged and specifically not report back a non-B0 rate when B0 is requested; must temporarily disable hardware flow control so that RTS can be deasserted; and should reassert DTR/RTS when moving from B0.

Fixes a8f54b7bd132 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
---
 drivers/usb/serial/xr_serial.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
drivers/usb/serial/xr_serial.c +14 -9
index f67e7dba9509..483d07dee19d 100644
@@ -341,8 +341,11 @@ static int xr_set_baudrate(struct tty_struct *tty,
 	u16 tx_mask, rx_mask;
 	int ret;
 
-	baud = clamp(tty->termios.c_ospeed, XR21V141X_MIN_SPEED,
-		     XR21V141X_MAX_SPEED);
+	baud = tty->termios.c_ospeed;
+	if (!baud)
+		return 0;
+
+	baud = clamp(baud, XR21V141X_MIN_SPEED, XR21V141X_MAX_SPEED);
 	divisor = XR_INT_OSC_HZ / baud;
 	idx = ((32 * XR_INT_OSC_HZ) / baud) & 0x1f;
 	tx_mask = xr21v141x_txrx_clk_masks[idx].tx;
@@ -399,7 +402,8 @@ static int xr_set_baudrate(struct tty_struct *tty,
 }
 
 static void xr_set_flow_mode(struct tty_struct *tty,
-			     struct usb_serial_port *port)
+			     struct usb_serial_port *port,
+			     struct ktermios *old_termios)
 {
 	u8 flow, gpio_mode;
 	int ret;
@@ -411,7 +415,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 	/* Set GPIO mode for controlling the pins manually by default. */
 	gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK;
 
-	if (C_CRTSCTS(tty)) {
+	if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
 		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
 		gpio_mode |= XR21V141X_UART_MODE_RTS_CTS;
 		flow = XR21V141X_UART_FLOW_MODE_HW;
@@ -438,6 +442,11 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 	xr_uart_enable(port);
 
 	xr_set_reg_uart(port, XR21V141X_REG_GPIO_MODE, gpio_mode);
+
+	if (C_BAUD(tty) == B0)
+		xr_dtr_rts(port, 0);
+	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+		xr_dtr_rts(port, 1);
 }
 
 static void xr_set_termios(struct tty_struct *tty,
@@ -493,11 +502,7 @@ static void xr_set_termios(struct tty_struct *tty,
 	if (ret)
 		return;
 
-	/* If baud rate is B0, clear DTR and RTS */
-	if (C_BAUD(tty) == B0)
-		xr_dtr_rts(port, 0);
-
-	xr_set_flow_mode(tty, port);
+	xr_set_flow_mode(tty, port, old_termios);
 }
 
 static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
Signature · 1 line
2.26.2
author maintainer
#13 lore raw
In reply to Johan Hovold
Quoted 17 lines · Johan Hovold

This series fixes the remaining issues in the new MaxLinear driver that were pointed out here:

https://lore.kernel.org/r/[email protected]

Johan Hovold (10):
USB: serial: xr: fix NULL-deref at probe
USB: serial: xr: fix interface leak at disconnect
USB: serial: xr: use subsystem usb_device at probe
USB: serial: xr: use termios flag helpers
USB: serial: xr: document vendor-request recipient
USB: serial: xr: clean up line-settings handling
USB: serial: xr: simplify line-speed logic
USB: serial: xr: fix gpio-mode handling
USB: serial: xr: fix pin configuration
USB: serial: xr: fix B0 handling

Quoted 2 lines
 drivers/usb/serial/xr_serial.c | 102 +++++++++++++++++++--------------
 1 file changed, 59 insertions(+), 43 deletions(-)

I have applied these now.

Johan