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
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(-)
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
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(+)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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: [email protected] # 5.11
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> 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
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: [email protected] # 5.11
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > 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