[PATCH 0/6] USB: serial: cp210x: fixes and CP2105/CP2108 fw version

[PATCH 0/6] USB: serial: cp210x: fixes and CP2105/CP2108 fw version

[Johan Hovold]

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(-)

-- 
2.31.1

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

[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: stable@vger.kernel.org	# 5.11
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 09b845d0da41..b41e2c7649fb 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -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);
-- 
2.31.1

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

[Greg KH]

On Fri, Jul 02, 2021 at 03:42:22PM +0200, Johan Hovold wrote:
> 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: stable@vger.kernel.org	# 5.11
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/cp210x.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 09b845d0da41..b41e2c7649fb 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -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

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

[Johan Hovold]

On Fri, Jul 02, 2021 at 04:47:11PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 02, 2021 at 03:42:22PM +0200, Johan Hovold wrote:
> > 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: stable@vger.kernel.org	# 5.11
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/serial/cp210x.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> > index 09b845d0da41..b41e2c7649fb 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -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?

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

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

[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: stable@vger.kernel.org	# 5.11
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index b41e2c7649fb..eb3be4f65603 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -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);
 }
-- 
2.31.1

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

[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 <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eb3be4f65603..c7cea86c659c 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -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;
-- 
2.31.1

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

[Johan Hovold]

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

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index c7cea86c659c..4c51381cf9aa 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -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);
-- 
2.31.1

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

[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 <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 4c51381cf9aa..0f4cdba160d9 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -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);
-- 
2.31.1

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

[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 <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 0f4cdba160d9..7908e336f962 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -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:
-- 
2.31.1