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

[PATCH v4 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 <konstantin.shkolnyy@gmail.com>
---
 drivers/usb/serial/cp210x.c | 70 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 352fe63..fd76a35 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 {
 	__u8			bInterfaceNumber;
+	bool			has_swapped_line_ctl;
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -419,6 +420,62 @@ 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_detect_swapped_line_ctl(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;
+
+	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_save, 2);
+	if (err)
+		return err;
+
+	line_ctl_test = 0x800;
+	err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl_test, 2);
+	if (err)
+		return err;
+
+	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_test, 2);
+	if (err)
+		return err;
+
+	/* has_swapped_line_ctl is 0 here because port_priv was kzalloced */
+	if (line_ctl_test == 8) {
+		port_priv->has_swapped_line_ctl = true;
+		line_ctl_save = swab16((u16)line_ctl_save);
+	}
+
+	return cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_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 *ctl)
+{
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	int err;
+
+	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, ctl, 2);
+	if (err)
+		return err;
+
+	/* Workaround swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
+	if (port_priv->has_swapped_line_ctl)
+		*ctl = swab16((u16)(*ctl));
+
+	return 0;
+}
+
+/*
  * cp210x_quantise_baudrate
  * Quantises the baud rate as per AN205 Table 1
  */
@@ -535,7 +592,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 
 	cflag = *cflagp;
 
-	cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+	cp210x_get_line_ctl(port, &bits);
 	cflag &= ~CSIZE;
 	switch (bits & BITS_DATA_MASK) {
 	case BITS_DATA_5:
@@ -703,7 +760,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, &bits, 2);
+		cp210x_get_line_ctl(port, &bits);
 		bits &= ~BITS_DATA_MASK;
 		switch (cflag & CSIZE) {
 		case CS5:
@@ -737,7 +794,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, &bits, 2);
+		cp210x_get_line_ctl(port, &bits);
 		bits &= ~BITS_PARITY_MASK;
 		if (cflag & PARENB) {
 			if (cflag & CMSPAR) {
@@ -763,7 +820,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 	}
 
 	if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
-		cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+		cp210x_get_line_ctl(port, &bits);
 		bits &= ~BITS_STOP_MASK;
 		if (cflag & CSTOPB) {
 			bits |= BITS_STOP_2;
@@ -883,6 +940,7 @@ 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)
@@ -893,6 +951,10 @@ static int cp210x_port_probe(struct usb_serial_port *port)
 
 	usb_set_serial_port_data(port, port_priv);
 
+	err = cp210x_detect_swapped_line_ctl(port);
+	if (err)
+		kfree(port_priv);
+
 	return 0;
 }
 
-- 
1.8.4.5

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

[Johan Hovold]

On Tue, Oct 27, 2015 at 04:53:34PM -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 <konstantin.shkolnyy@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 70 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 66 insertions(+), 4 deletions(-)

> +/*
> + * 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 *ctl)

No need to break this line anymore.

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

> @@ -883,6 +940,7 @@ 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)
> @@ -893,6 +951,10 @@ static int cp210x_port_probe(struct usb_serial_port *port)
>  
>  	usb_set_serial_port_data(port, port_priv);
>  
> +	err = cp210x_detect_swapped_line_ctl(port);
> +	if (err)
> +		kfree(port_priv);

You forgot to return err here...

> +
>  	return 0;
>  }

I gave the series a really quick test on an cp2102-device I had lying
around. I assume you verified the changes on some other older devices as
well?

Fix the above, and I'll queue this up for 4.4-rc.

Thanks,
Johan

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

[Johan Hovold]

On Wed, Oct 28, 2015 at 09:51:26AM +0100, Johan Hovold wrote:
> On Tue, Oct 27, 2015 at 04:53:34PM -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 <konstantin.shkolnyy@gmail.com>

> I gave the series a really quick test on an cp2102-device I had lying
> around. I assume you verified the changes on some other older devices as
> well?

Did you test this series on the older device types as well?

Thanks,
Johan

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

[Konstantin Shkolnyy]

I tested it on cp2102, cp2105 and cp2108.
I'm a little worried about that extra PURGE command, so I did several
manual tests on each with a standard PC serial port on the other end
of the cable.
- run several open/write/close iteration (the test that used to break
cp2108), observe data on the other end;
- open minicom and see that cp210x can still receive data from the other end.

BTW, can you suggest (preferably automated) serial port test s/w?

On Thu, Oct 29, 2015 at 2:19 AM, Johan Hovold <johan@kernel.org> wrote:
> On Wed, Oct 28, 2015 at 09:51:26AM +0100, Johan Hovold wrote:
>> On Tue, Oct 27, 2015 at 04:53:34PM -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 <konstantin.shkolnyy@gmail.com>
>
>> I gave the series a really quick test on an cp2102-device I had lying
>> around. I assume you verified the changes on some other older devices as
>> well?
>
> Did you test this series on the older device types as well?
>
> Thanks,
> Johan

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

[Johan Hovold]

[ Please avoid top-posting. ]

On Thu, Oct 29, 2015 at 08:39:04AM -0500, Konstantin Shkolnyy wrote:
> I tested it on cp2102, cp2105 and cp2108.
> I'm a little worried about that extra PURGE command, so I did several
> manual tests on each with a standard PC serial port on the other end
> of the cable.
> - run several open/write/close iteration (the test that used to break
> cp2108), observe data on the other end;
> - open minicom and see that cp210x can still receive data from the other end.

Sounds good.

If this turns out to cause any problems, we can always enable this based
on chip type, right? I saw something about a vendor request for that in
your (silabs') vendor driver.

> BTW, can you suggest (preferably automated) serial port test s/w?

No, sorry. I have a bunch of test code that I intend to organise and
clean up at some point, but I never seem to find the time to actually do
it.

Johan

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

[Konstantin Shkolnyy]

On Sat, Oct 31, 2015 at 7:16 AM, Johan Hovold <johan@kernel.org> wrote:
> [ Please avoid top-posting. ]
>
> On Thu, Oct 29, 2015 at 08:39:04AM -0500, Konstantin Shkolnyy wrote:
>> I tested it on cp2102, cp2105 and cp2108.
>> I'm a little worried about that extra PURGE command, so I did several
>> manual tests on each with a standard PC serial port on the other end
>> of the cable.
>> - run several open/write/close iteration (the test that used to break
>> cp2108), observe data on the other end;
>> - open minicom and see that cp210x can still receive data from the other end.
>
> Sounds good.
>
> If this turns out to cause any problems, we can always enable this based
> on chip type, right? I saw something about a vendor request for that in
> your (silabs') vendor driver.

Yes, there is a register with the chip type.

[...]

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

[Konstantin Shkolnyy]

[...]

Hi Johan,

I'd like to add tx_empty() and replace cp210x_get/set_config with
simpler functions, as we discussed before. While the tx_empty patch
could be made against the current kernel, the latter would have to be
made against my previous patch set. How should this be done?

Thanks,
Konstantin

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

[Johan Hovold]

On Tue, Nov 03, 2015 at 07:26:00AM -0600, Konstantin Shkolnyy wrote:
> [...]
> 
> Hi Johan,
> 
> I'd like to add tx_empty() and replace cp210x_get/set_config with
> simpler functions, as we discussed before. While the tx_empty patch
> could be made against the current kernel, the latter would have to be
> made against my previous patch set. How should this be done?

I'll apply your patches for 4.4-rc as soon as the merge window closes
(in about two weeks) and also merge this is into my usb-next branch
which is what you should be working against:

	git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git

Just base it off of what you submitted meanwhile.

Thanks,
Johan