Re: [PATCH 3/3] USB: serial: cp210x: Workaround cp2108 GET_LINE_CTL bug

[Johan Hovold]

On Thu, Oct 22, 2015 at 04:12:00PM -0500, Konstantin Shkolnyy wrote:
> cp2108 GET_LINE_CTL returns the 16-bit value with the 2 bytes swapped.
> However, SET_LINE_CTL functions properly. When the driver tries to modify
> the register, it reads it, modifies some bits and writes back. Because the
> read bytes were swapped, this often results in an invalid value to be
> written. In turn, this causes cp2108 respond with a stall. The stall
> sometimes doesn't clear properly and cp2108 starts responding to following
> valid commands also with stalls, effectively failing.
> 
> Signed-off-by: Konstantin Shkolnyy 
> ---
>  drivers/usb/serial/cp210x.c | 73 
> ++---
>  1 file changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 352fe63..eb02184 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -199,6 +199,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  
>  struct cp210x_port_private {
>   __u8bInterfaceNumber;
> + bool  cp2108_with_get_line_ctl_swap;

Odd indentation. Either match the previous line or use a single space.

Please also drop the cp2108 prefix and rename the flag something like
has_swapped_line_ctl.

>  };
>  
>  static struct usb_serial_driver cp210x_device = {
> @@ -419,6 +420,63 @@ static inline int cp210x_set_config_single(struct 
> usb_serial_port *port,
>  }
>  
>  /*
> + * Detect CP2108 GET_LINE_CTL bug and activate workaround.
> + * Write a known good value 0x800, read it back.
> + * If it comes back swapped the bug is detected.
> + * Preserve the original register value.
> + */
> +static int cp210x_activate_workarounds(struct usb_serial_port *port)

Rename this to something more specific like
cp210x_detect_swapped_line_ctl.

> +{
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + unsigned int line_ctl_save;
> + unsigned int line_ctl_test;
> + int err;
> +
> + port_priv->cp2108_with_get_line_ctl_swap = false;

You can drop this and rely on the private data being kzalloced.

> +
> + err = cp210x_get_config(port, CP210X_GET_LINE_CTL, _ctl_save, 2);
> + if (err)
> + return err;
> +
> + line_ctl_test = 0x800;
> + err = cp210x_set_config(port, CP210X_SET_LINE_CTL, _ctl_test, 2);
> + if (err)
> + return err;
> +
> + err = cp210x_get_config(port, CP210X_GET_LINE_CTL, _ctl_test, 2);
> + if (err)
> + return err;
> +
> + if (line_ctl_test == 8) {
> + port_priv->cp2108_with_get_line_ctl_swap = true;
> + swab16s((u16 *)(_ctl_save));

This will not work on a big-endian machine. Just use swab16 here.

> + }
> +
> + return cp210x_set_config(port, CP210X_SET_LINE_CTL, _ctl_save, 2);
> +}
> +
> +/*
> + * Must always be called instead of cp210x_get_config(CP210X_GET_LINE_CTL)
> + * to workaround cp2108 bug and get correct value.
> + */
> +static int cp210x_get_line_ctl(struct usb_serial_port *port,
> + unsigned int *pctrl)

Drop the pointer prefix (p) from the parameter name.

> +{
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + int err;
> +
> + err = cp210x_get_config(port, CP210X_GET_LINE_CTL, pctrl, 2);
> + if (err)
> + return err;
> +
> + /* Workaround swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
> + if (port_priv->cp2108_with_get_line_ctl_swap)
> + swab16s((u16 *) pctrl);

Same as above.

> +
> + return 0;
> +}
> +
> +/*
>   * cp210x_quantise_baudrate
>   * Quantises the baud rate as per AN205 Table 1
>   */
> @@ -535,7 +593,7 @@ static void cp210x_get_termios_port(struct 
> usb_serial_port *port,
>  
>   cflag = *cflagp;
>  
> - cp210x_get_config(port, CP210X_GET_LINE_CTL, , 2);
> + cp210x_get_line_ctl(port, );
>   cflag &= ~CSIZE;
>   switch (bits & BITS_DATA_MASK) {
>   case BITS_DATA_5:
> @@ -703,7 +761,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
>  
>   /* If the number of data bits is to be updated */
>   if ((cflag & CSIZE) != (old_cflag & CSIZE)) {
> - cp210x_get_config(port, CP210X_GET_LINE_CTL, , 2);
> + cp210x_get_line_ctl(port, );
>   bits &= ~BITS_DATA_MASK;
>   switch (cflag & CSIZE) {
>   case CS5:
> @@ -737,7 +795,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
>  
>   if ((cflag & (PARENB|PARODD|CMSPAR)) !=
>   (old_cflag & (PARENB|PARODD|CMSPAR))) {
> - cp210x_get_config(port, CP210X_GET_LINE_CTL, , 2);
> + cp210x_get_line_ctl(port, );
>   bits &= ~BITS_PARITY_MASK;
>   if (cflag & PARENB) {
>   if (cflag & CMSPAR) {
> @@ -763,7 +821,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
>   }
>  
>   if ((cflag & CSTOPB) != 

[PATCH 3/3] USB: serial: cp210x: Workaround cp2108 GET_LINE_CTL bug

[Konstantin Shkolnyy]

cp2108 GET_LINE_CTL returns the 16-bit value with the 2 bytes swapped.
However, SET_LINE_CTL functions properly. When the driver tries to modify
the register, it reads it, modifies some bits and writes back. Because the
read bytes were swapped, this often results in an invalid value to be
written. In turn, this causes cp2108 respond with a stall. The stall
sometimes doesn't clear properly and cp2108 starts responding to following
valid commands also with stalls, effectively failing.

Signed-off-by: Konstantin Shkolnyy 
---
 drivers/usb/serial/cp210x.c | 73 ++---
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 352fe63..eb02184 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -199,6 +199,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct cp210x_port_private {
__u8bInterfaceNumber;
+   bool  cp2108_with_get_line_ctl_swap;
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -419,6 +420,63 @@ static inline int cp210x_set_config_single(struct 
usb_serial_port *port,
 }
 
 /*
+ * Detect CP2108 GET_LINE_CTL bug and activate workaround.
+ * Write a known good value 0x800, read it back.
+ * If it comes back swapped the bug is detected.
+ * Preserve the original register value.
+ */
+static int cp210x_activate_workarounds(struct usb_serial_port *port)
+{
+   struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+   unsigned int line_ctl_save;
+   unsigned int line_ctl_test;
+   int err;
+
+   port_priv->cp2108_with_get_line_ctl_swap = false;
+
+   err = cp210x_get_config(port, CP210X_GET_LINE_CTL, _ctl_save, 2);
+   if (err)
+   return err;
+
+   line_ctl_test = 0x800;
+   err = cp210x_set_config(port, CP210X_SET_LINE_CTL, _ctl_test, 2);
+   if (err)
+   return err;
+
+   err = cp210x_get_config(port, CP210X_GET_LINE_CTL, _ctl_test, 2);
+   if (err)
+   return err;
+
+   if (line_ctl_test == 8) {
+   port_priv->cp2108_with_get_line_ctl_swap = true;
+   swab16s((u16 *)(_ctl_save));
+   }
+
+   return cp210x_set_config(port, CP210X_SET_LINE_CTL, _ctl_save, 2);
+}
+
+/*
+ * Must always be called instead of cp210x_get_config(CP210X_GET_LINE_CTL)
+ * to workaround cp2108 bug and get correct value.
+ */
+static int cp210x_get_line_ctl(struct usb_serial_port *port,
+   unsigned int *pctrl)
+{
+   struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+   int err;
+
+   err = cp210x_get_config(port, CP210X_GET_LINE_CTL, pctrl, 2);
+   if (err)
+   return err;
+
+   /* Workaround swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
+   if (port_priv->cp2108_with_get_line_ctl_swap)
+   swab16s((u16 *) pctrl);
+
+   return 0;
+}
+
+/*
  * cp210x_quantise_baudrate
  * Quantises the baud rate as per AN205 Table 1
  */
@@ -535,7 +593,7 @@ static void cp210x_get_termios_port(struct usb_serial_port 
*port,
 
cflag = *cflagp;
 
-   cp210x_get_config(port, CP210X_GET_LINE_CTL, , 2);
+   cp210x_get_line_ctl(port, );
cflag &= ~CSIZE;
switch (bits & BITS_DATA_MASK) {
case BITS_DATA_5:
@@ -703,7 +761,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 
/* If the number of data bits is to be updated */
if ((cflag & CSIZE) != (old_cflag & CSIZE)) {
-   cp210x_get_config(port, CP210X_GET_LINE_CTL, , 2);
+   cp210x_get_line_ctl(port, );
bits &= ~BITS_DATA_MASK;
switch (cflag & CSIZE) {
case CS5:
@@ -737,7 +795,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 
if ((cflag & (PARENB|PARODD|CMSPAR)) !=
(old_cflag & (PARENB|PARODD|CMSPAR))) {
-   cp210x_get_config(port, CP210X_GET_LINE_CTL, , 2);
+   cp210x_get_line_ctl(port, );
bits &= ~BITS_PARITY_MASK;
if (cflag & PARENB) {
if (cflag & CMSPAR) {
@@ -763,7 +821,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
}
 
if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
-   cp210x_get_config(port, CP210X_GET_LINE_CTL, , 2);
+   cp210x_get_line_ctl(port, );
bits &= ~BITS_STOP_MASK;
if (cflag & CSTOPB) {
bits |= BITS_STOP_2;
@@ -883,16 +941,23 @@ static int cp210x_port_probe(struct usb_serial_port *port)
struct usb_serial *serial = port->serial;
struct usb_host_interface *cur_altsetting;
struct cp210x_port_private *port_priv;
+   int err;
 
port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
if (!port_priv)
return -ENOMEM;
 
+   /* register access