Skip to contentSkip to conversation

USB: serial: cp210x: fixes and CP2105/CP2108 fw version

PATCH Superseded Johan Hovold 2 participants 9 messages Jul 2, 2021 – Jul 5, 2021

Subsystems (3)
Stable branch USB USB serial

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

cover USB: serial: cp210x: fixes and CP2105/CP2108 fw version

Here are couple of minor fixes and some cleanups related to the recent regression which broke RTS control on some CP2102N devices with buggy firmware.

In case we run into another one of these, let's log the firmware version also for CP2105 and CP2108 for which it can be retrieved.

Johan

Johan Hovold (6):
USB: serial: cp210x: fix control-characters error handling
USB: serial: cp210x: fix flow-control error handling
USB: serial: cp210x: clean up control-request timeout
USB: serial: cp210x: clean up set-chars request
USB: serial: cp210x: clean up type detection
USB: serial: cp210x: determine fw version for CP2105 and CP2108

 drivers/usb/serial/cp210x.c | 73 ++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 45 deletions(-)
Signature · 1 line
2.31.1
author maintainer
#1 lore raw

PATCH 1/6 USB: serial: cp210x: fix control-characters error handling

In the unlikely event that setting the software flow-control characters fails the other flow-control settings should still be updated.

Fixes 7748feffcd80 ("USB: serial: cp210x: add support for software flow control")
Cc [email protected] # 5.11
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/usb/serial/cp210x.c +1 -3
index 09b845d0da41..b41e2c7649fb 100644
@@ -1217,9 +1217,7 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 		chars.bXonChar = START_CHAR(tty);
 		chars.bXoffChar = STOP_CHAR(tty);
 
-		ret = cp210x_set_chars(port, &chars);
-		if (ret)
-			return;
+		cp210x_set_chars(port, &chars);
 	}
 
 	mutex_lock(&port_priv->mutex);
Signature · 1 line
2.31.1
#2 lore raw

Re: [PATCH 1/6] USB: serial: cp210x: fix control-characters error handling

Quoted 15 lines · Johan Hovold

In the unlikely event that setting the software flow-control characters fails the other flow-control settings should still be updated.

Fixes 7748feffcd80 ("USB: serial: cp210x: add support for software flow control")
Cc [email protected] # 5.11
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/usb/serial/cp210x.c +0 -0
index 09b845d0da41..b41e2c7649fb 100644

drivers/usb/serial/cp210x.c static void cp210x_set_flow_control(struct tty_struct *tty, · line 1220

Quoted 7 lines
drivers/usb/serial/cp210x.c +1 -3
@@ -1217,9 +1217,7 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 		chars.bXonChar = START_CHAR(tty);
 		chars.bXoffChar = STOP_CHAR(tty);
 
-		ret = cp210x_set_chars(port, &chars);
-		if (ret)
-			return;
+		cp210x_set_chars(port, &chars);

What's the odds that someone tries to add the error checking back in here, in a few years? Can you put a comment here saying why you are not checking it?

thanks,

greg k-h

author maintainer
#3 lore raw

Re: [PATCH 1/6] USB: serial: cp210x: fix control-characters error handling

Quoted 19 lines · Greg Kroah-Hartman
Quoted 18 lines · Johan Hovold

In the unlikely event that setting the software flow-control characters fails the other flow-control settings should still be updated.

Fixes 7748feffcd80 ("USB: serial: cp210x: add support for software flow control")
Cc [email protected] # 5.11
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/usb/serial/cp210x.c +0 -0
index 09b845d0da41..b41e2c7649fb 100644
@@ -1217,9 +1217,7 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 		chars.bXonChar = START_CHAR(tty);
 		chars.bXoffChar = STOP_CHAR(tty);
Quoted 8 lines

drivers/usb/serial/cp210x.c static void cp210x_set_flow_control(struct tty_struct *tty, · line 1220

Quoted 4 lines
drivers/usb/serial/cp210x.c +1 -3
-		ret = cp210x_set_chars(port, &chars);
-		if (ret)
-			return;
+		cp210x_set_chars(port, &chars);

What's the odds that someone tries to add the error checking back in here, in a few years? Can you put a comment here saying why you are not checking it?

This is just how set_termios() works and how the other requests are handled by the driver. I can add an explicit error message here though just like when setting the line-control register so that it doesn't look like an oversight. The error message is currently printed by the set_chars() helper, but I can move that out when removing the helper later in the series.

Johan

author maintainer
#4 lore raw

PATCH 2/6 USB: serial: cp210x: fix flow-control error handling

In reply to Johan Hovold

Make sure that the driver crtscts state is not updated in the unlikely event that the flow-control request fails. Not doing so could break RTS control.

Fixes 5951b8508855 ("USB: serial: cp210x: suppress modem-control errors")
Cc [email protected] # 5.11
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/usb/serial/cp210x.c +8 -3
index b41e2c7649fb..eb3be4f65603 100644
@@ -1191,6 +1191,7 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 	struct cp210x_flow_ctl flow_ctl;
 	u32 flow_repl;
 	u32 ctl_hs;
+	bool crtscts;
 	int ret;
 
 	/*

@@ -1246,14 +1247,14 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 			flow_repl |= CP210X_SERIAL_RTS_FLOW_CTL;
 		else
 			flow_repl |= CP210X_SERIAL_RTS_INACTIVE;
-		port_priv->crtscts = true;
+		crtscts = true;
 	} else {
 		ctl_hs &= ~CP210X_SERIAL_CTS_HANDSHAKE;
 		if (port_priv->rts)
 			flow_repl |= CP210X_SERIAL_RTS_ACTIVE;
 		else
 			flow_repl |= CP210X_SERIAL_RTS_INACTIVE;
-		port_priv->crtscts = false;
+		crtscts = false;
 	}
 
 	if (I_IXOFF(tty)) {

@@ -1276,8 +1277,12 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 	flow_ctl.ulControlHandshake = cpu_to_le32(ctl_hs);
 	flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
 
-	cp210x_write_reg_block(port, CP210X_SET_FLOW, &flow_ctl,
+	ret = cp210x_write_reg_block(port, CP210X_SET_FLOW, &flow_ctl,
 			sizeof(flow_ctl));
+	if (ret)
+		goto out_unlock;
+
+	port_priv->crtscts = crtscts;
 out_unlock:
 	mutex_unlock(&port_priv->mutex);
 }
Signature · 1 line
2.31.1
author maintainer
#5 lore raw

PATCH 3/6 USB: serial: cp210x: clean up control-request timeout

In reply to Johan Hovold

For consistency use the USB_CTRL_GET_TIMEOUT define for the read-register request timeout (same value as USB_CTRL_SET_TIMEOUT).

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/serial/cp210x.c +1 -1
index eb3be4f65603..c7cea86c659c 100644
@@ -637,7 +637,7 @@ static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
 	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);
+			USB_CTRL_GET_TIMEOUT);
 	if (result == bufsize) {
 		memcpy(buf, dmabuf, bufsize);
 		result = 0;
Signature · 1 line
2.31.1
author maintainer
#6 lore raw

PATCH 4/6 USB: serial: cp210x: clean up set-chars request

In reply to Johan Hovold

Use the generic control request helper to implement the SET_CHARS request.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)
drivers/usb/serial/cp210x.c +2 -28
index c7cea86c659c..4c51381cf9aa 100644
@@ -1144,33 +1144,6 @@ static void cp210x_disable_event_mode(struct usb_serial_port *port)
 	port_priv->event_mode = false;
 }
 
-static int cp210x_set_chars(struct usb_serial_port *port,
-		struct cp210x_special_chars *chars)
-{
-	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-	struct usb_serial *serial = port->serial;
-	void *dmabuf;
-	int result;
-
-	dmabuf = kmemdup(chars, sizeof(*chars), GFP_KERNEL);
-	if (!dmabuf)
-		return -ENOMEM;
-
-	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-				CP210X_SET_CHARS, REQTYPE_HOST_TO_INTERFACE, 0,
-				port_priv->bInterfaceNumber,
-				dmabuf, sizeof(*chars), USB_CTRL_SET_TIMEOUT);
-
-	kfree(dmabuf);
-
-	if (result < 0) {
-		dev_err(&port->dev, "failed to set special chars: %d\n", result);
-		return result;
-	}
-
-	return 0;
-}
-
 static bool cp210x_termios_change(const struct ktermios *a, const struct ktermios *b)
 {
 	bool iflag_change, cc_change;

@@ -1218,7 +1191,8 @@ static void cp210x_set_flow_control(struct tty_struct *tty,
 		chars.bXonChar = START_CHAR(tty);
 		chars.bXoffChar = STOP_CHAR(tty);
 
-		cp210x_set_chars(port, &chars);
+		cp210x_write_reg_block(port, CP210X_SET_CHARS, &chars,
+				sizeof(chars));
 	}
 
 	mutex_lock(&port_priv->mutex);
Signature · 1 line
2.31.1
author maintainer
#7 lore raw

PATCH 5/6 USB: serial: cp210x: clean up type detection

In reply to Johan Hovold

Clean up attach somewhat by moving type detection into the quirk helper and giving it a more generic name.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
drivers/usb/serial/cp210x.c +12 -11
index 4c51381cf9aa..0f4cdba160d9 100644
@@ -2087,11 +2087,21 @@ static int cp210x_get_fw_version(struct usb_serial *serial, u16 value)
 	return 0;
 }
 
-static void cp210x_determine_quirks(struct usb_serial *serial)
+static void cp210x_determine_type(struct usb_serial *serial)
 {
 	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
 	int ret;
 
+	ret = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
+			CP210X_GET_PARTNUM, &priv->partnum,
+			sizeof(priv->partnum));
+	if (ret < 0) {
+		dev_warn(&serial->interface->dev,
+				"querying part number failed\n");
+		priv->partnum = CP210X_PARTNUM_UNKNOWN;
+		return;
+	}
+
 	switch (priv->partnum) {
 	case CP210X_PARTNUM_CP2102N_QFN28:
 	case CP210X_PARTNUM_CP2102N_QFN24:

@@ -2116,18 +2126,9 @@ static int cp210x_attach(struct usb_serial *serial)
 	if (!priv)
 		return -ENOMEM;
 
-	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
-					  CP210X_GET_PARTNUM, &priv->partnum,
-					  sizeof(priv->partnum));
-	if (result < 0) {
-		dev_warn(&serial->interface->dev,
-			 "querying part number failed\n");
-		priv->partnum = CP210X_PARTNUM_UNKNOWN;
-	}
-
 	usb_set_serial_data(serial, priv);
 
-	cp210x_determine_quirks(serial);
+	cp210x_determine_type(serial);
 	cp210x_init_max_speed(serial);
 
 	result = cp210x_gpio_init(serial);
Signature · 1 line
2.31.1
author maintainer
#8 lore raw

PATCH 6/6 USB: serial: cp210x: determine fw version for CP2105 and CP2108

In reply to Johan Hovold

CP2105, CP2108 and CP2102N have vendor requests that can be used to retrieve the firmware version. Having this information available is essential when trying to work around buggy firmware as a recent CP2102N regression showed.

Determine and log the firmware version also for CP2105 and CP2108 during type detection at probe.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/usb/serial/cp210x.c +5 -0
index 0f4cdba160d9..7908e336f962 100644
@@ -399,6 +399,7 @@ struct cp210x_special_chars {
 };
 
 /* CP210X_VENDOR_SPECIFIC values */
+#define CP210X_GET_FW_VER	0x000E
 #define CP210X_READ_2NCONFIG	0x000E
 #define CP210X_GET_FW_VER_2N	0x0010
 #define CP210X_READ_LATCH	0x00C2

@@ -2103,6 +2104,10 @@ static void cp210x_determine_type(struct usb_serial *serial)
 	}
 
 	switch (priv->partnum) {
+	case CP210X_PARTNUM_CP2105:
+	case CP210X_PARTNUM_CP2108:
+		cp210x_get_fw_version(serial, CP210X_GET_FW_VER);
+		break;
 	case CP210X_PARTNUM_CP2102N_QFN28:
 	case CP210X_PARTNUM_CP2102N_QFN24:
 	case CP210X_PARTNUM_CP2102N_QFN20:
Signature · 1 line
2.31.1