Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1248246 > unrolled thread

[PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com> - 2015-10-16 01:30 +0200
    Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure  due to GET_LINE_CTL bug Johan Hovold <johan@kernel.org> - 2015-10-16 15:30 +0200
      Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due  to GET_LINE_CTL bug Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com> - 2015-10-16 17:20 +0200
        Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure  due to GET_LINE_CTL bug Johan Hovold <johan@kernel.org> - 2015-10-16 17:30 +0200
          Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due  to GET_LINE_CTL bug Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com> - 2015-10-16 17:40 +0200
            Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure  due to GET_LINE_CTL bug Johan Hovold <johan@kernel.org> - 2015-10-16 17:50 +0200

#1248246 — [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

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 | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eac7cca..f028490 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_serial_private {
 	__u8			bInterfaceNumber;
+	bool			swap_get_line_ctl; /* set if CP2108 swaps bytes due to a bug */
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -343,6 +344,10 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
 		return result;
 	}
 
+	/* Workaround for swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
+	if (spriv->swap_get_line_ctl && request == CP210X_GET_LINE_CTL && size == 2)
+		swab16s((u16 *)data);
+
 	return 0;
 }
 
@@ -865,18 +870,53 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
 
 static int cp210x_startup(struct usb_serial *serial)
 {
+	struct usb_serial_port *port;
 	struct usb_host_interface *cur_altsetting;
 	struct cp210x_serial_private *spriv;
+	unsigned int  line_ctl;
+	int           err;
+
+	/* We always expect a single port only */
+	if (serial->num_ports != 1) {
+		dev_err(&serial->dev->dev, "%s - expected 1 port, found %d\n",
+			__func__, serial->num_ports);
+		return -EINVAL;
+	}
+	port = serial->port[0];
 
 	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
 	if (!spriv)
 		return -ENOMEM;
 
+	/* get_config and set_config rely on this spriv field */
 	cur_altsetting = serial->interface->cur_altsetting;
 	spriv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
 
+	/* Detect CP2108 bug and activate workaround.
+	 * Write a known good value 0x800, read it back.
+	 * If it comes back swapped the bug is detected.
+	 */
+
+	/* The following get_config won't swap the bytes */
+	spriv->swap_get_line_ctl = false;
+
+	/* must be set before calling get_config and set_config */
 	usb_set_serial_data(serial, spriv);
 
+	line_ctl = 0x800;
+
+	err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl, 2);
+	if (err)
+		return err;
+
+	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl, 2);
+	if (err)
+		return err;
+
+	if ((line_ctl & 0xffff) == 8)
+		/* Future get_config calls will swap the bytes */
+		spriv->swap_get_line_ctl = true;
+
 	return 0;
 }
 
-- 
1.8.4.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1248763 — Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

On Thu, Oct 15, 2015 at 06:23:31PM -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.

That sounds weird. Are you saying that all or only some cp2108 devices
would be affected by this? And only for the line-control register?

What is the endianess of your host by the way?

The control-request and endianess handling of this driver is messy to
say the least, but it does look like it would work for both LE and BE as
long as the data fields are either single 2-byte little-endian, or
multiple 4-byte little-endian.

> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eac7cca..f028490 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_serial_private {
>  	__u8			bInterfaceNumber;
> +	bool			swap_get_line_ctl; /* set if CP2108 swaps bytes due to a bug */
>  };
>  
>  static struct usb_serial_driver cp210x_device = {
> @@ -343,6 +344,10 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
>  		return result;
>  	}
>  
> +	/* Workaround for swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
> +	if (spriv->swap_get_line_ctl && request == CP210X_GET_LINE_CTL && size == 2)
> +		swab16s((u16 *)data);
> +

If we determine we need this, then it should probably be isolated to a
dedicated line-control helper function.

>  	return 0;
>  }
>  
> @@ -865,18 +870,53 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
>  
>  static int cp210x_startup(struct usb_serial *serial)
>  {
> +	struct usb_serial_port *port;
>  	struct usb_host_interface *cur_altsetting;
>  	struct cp210x_serial_private *spriv;
> +	unsigned int  line_ctl;
> +	int           err;
> +
> +	/* We always expect a single port only */
> +	if (serial->num_ports != 1) {
> +		dev_err(&serial->dev->dev, "%s - expected 1 port, found %d\n",
> +			__func__, serial->num_ports);
> +		return -EINVAL;
> +	}

This is not needed as all devices are one-port per interface.

> +	port = serial->port[0];
>
>  	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
>  	if (!spriv)
>  		return -ENOMEM;
>  
> +	/* get_config and set_config rely on this spriv field */
>  	cur_altsetting = serial->interface->cur_altsetting;
>  	spriv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
>  
> +	/* Detect CP2108 bug and activate workaround.
> +	 * Write a known good value 0x800, read it back.
> +	 * If it comes back swapped the bug is detected.
> +	 */
> +
> +	/* The following get_config won't swap the bytes */
> +	spriv->swap_get_line_ctl = false;
> +
> +	/* must be set before calling get_config and set_config */
>  	usb_set_serial_data(serial, spriv);
>  
> +	line_ctl = 0x800;
> +
> +	err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl, 2);

Calling port functions before the ports have been fully initialised
isn't very nice, even if it works for this driver. This should probably
be done in a helper function that accesses this register directly (and
perhaps also avoids overwriting the default setting).

> +	if (err)
> +		return err;

You leak spriv here.

> +
> +	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl, 2);
> +	if (err)
> +		return err;

And here.

> +
> +	if ((line_ctl & 0xffff) == 8)
> +		/* Future get_config calls will swap the bytes */

Just drop the comment.

> +		spriv->swap_get_line_ctl = true;
> +
>  	return 0;
>  }

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248853 — Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

On Fri, Oct 16, 2015 at 8:27 AM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Oct 15, 2015 at 06:23:31PM -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.
>
> That sounds weird. Are you saying that all or only some cp2108 devices
> would be affected by this? And only for the line-control register?

The bug exists in all current cp2108 devices and affects only
GET_LINE_CTL. But it may be fixed in a future revision.

> What is the endianess of your host by the way?

I'm using an Intel-based Dell desktop, so its LE.

[...]
>> @@ -343,6 +344,10 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
>>               return result;
>>       }
>>
>> +     /* Workaround for swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
>> +     if (spriv->swap_get_line_ctl && request == CP210X_GET_LINE_CTL && size == 2)
>> +             swab16s((u16 *)data);
>> +
>
> If we determine we need this, then it should probably be isolated to a
> dedicated line-control helper function.

I feel that you want to kill cp210x_get_config. I can easily see why
you don't like it :-) I'll gladly replace all
"cp210x_get_config(GET_LINE_CTL)" calls with a new
"cp210x_get_line_ctl" helper if you don';t mind.

[...]
>> +     port = serial->port[0];
>>
>>       spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
>>       if (!spriv)
>>               return -ENOMEM;
>>
>> +     /* get_config and set_config rely on this spriv field */
>>       cur_altsetting = serial->interface->cur_altsetting;
>>       spriv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
>>
>> +     /* Detect CP2108 bug and activate workaround.
>> +      * Write a known good value 0x800, read it back.
>> +      * If it comes back swapped the bug is detected.
>> +      */
>> +
>> +     /* The following get_config won't swap the bytes */
>> +     spriv->swap_get_line_ctl = false;
>> +
>> +     /* must be set before calling get_config and set_config */
>>       usb_set_serial_data(serial, spriv);
>>
>> +     line_ctl = 0x800;
>> +
>> +     err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl, 2);
>
> Calling port functions before the ports have been fully initialised
> isn't very nice, even if it works for this driver. This should probably
> be done in a helper function that accesses this register directly (and

So usb_control_msg should be fine to call here, right?

> perhaps also avoids overwriting the default setting).

You mean save and restore the register setting around this bug detection?

[...]

Thanks,
Konstantin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248886 — Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

On Fri, Oct 16, 2015 at 10:11:12AM -0500, Konstantin Shkolnyy wrote:
> On Fri, Oct 16, 2015 at 8:27 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Oct 15, 2015 at 06:23:31PM -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.
> >
> > That sounds weird. Are you saying that all or only some cp2108 devices
> > would be affected by this? And only for the line-control register?
> 
> The bug exists in all current cp2108 devices and affects only
> GET_LINE_CTL. But it may be fixed in a future revision.

Ok, have you some kind of confirmation from silabs on this already?

> >> @@ -343,6 +344,10 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
> >>               return result;
> >>       }
> >>
> >> +     /* Workaround for swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
> >> +     if (spriv->swap_get_line_ctl && request == CP210X_GET_LINE_CTL && size == 2)
> >> +             swab16s((u16 *)data);
> >> +
> >
> > If we determine we need this, then it should probably be isolated to a
> > dedicated line-control helper function.
> 
> I feel that you want to kill cp210x_get_config. I can easily see why
> you don't like it :-) I'll gladly replace all
> "cp210x_get_config(GET_LINE_CTL)" calls with a new
> "cp210x_get_line_ctl" helper if you don';t mind.

That would be great.

> [...]
> >> +     port = serial->port[0];
> >>
> >>       spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
> >>       if (!spriv)
> >>               return -ENOMEM;
> >>
> >> +     /* get_config and set_config rely on this spriv field */
> >>       cur_altsetting = serial->interface->cur_altsetting;
> >>       spriv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
> >>
> >> +     /* Detect CP2108 bug and activate workaround.
> >> +      * Write a known good value 0x800, read it back.
> >> +      * If it comes back swapped the bug is detected.
> >> +      */
> >> +
> >> +     /* The following get_config won't swap the bytes */
> >> +     spriv->swap_get_line_ctl = false;
> >> +
> >> +     /* must be set before calling get_config and set_config */
> >>       usb_set_serial_data(serial, spriv);
> >>
> >> +     line_ctl = 0x800;
> >> +
> >> +     err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl, 2);
> >
> > Calling port functions before the ports have been fully initialised
> > isn't very nice, even if it works for this driver. This should probably
> > be done in a helper function that accesses this register directly (and
> 
> So usb_control_msg should be fine to call here, right?

Yes.

> > perhaps also avoids overwriting the default setting).
> 
> You mean save and restore the register setting around this bug detection?

Precisely. The driver currently reads out the default settings and
updates it's terminal settings accordingly. I'm sure 8N1 is likely the
default setting, but let's keep the current behaviour for now in case we
end up having to backport this fix as well (arguably, these devices have
never been supported so that may not be necessary).

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248901 — Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

On Fri, Oct 16, 2015 at 10:27 AM, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Oct 16, 2015 at 10:11:12AM -0500, Konstantin Shkolnyy wrote:
>> On Fri, Oct 16, 2015 at 8:27 AM, Johan Hovold <johan@kernel.org> wrote:
>> > On Thu, Oct 15, 2015 at 06:23:31PM -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.
>> >
>> > That sounds weird. Are you saying that all or only some cp2108 devices
>> > would be affected by this? And only for the line-control register?
>>
>> The bug exists in all current cp2108 devices and affects only
>> GET_LINE_CTL. But it may be fixed in a future revision.
>
> Ok, have you some kind of confirmation from silabs on this already?

I work for SiLabs.

Thanks,
Konstantin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248918 — Re: [PATCH v2] USB: serial: cp210x: Workaround for cp2108 failure due to GET_LINE_CTL bug

On Fri, Oct 16, 2015 at 10:39:13AM -0500, Konstantin Shkolnyy wrote:
> On Fri, Oct 16, 2015 at 10:27 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Fri, Oct 16, 2015 at 10:11:12AM -0500, Konstantin Shkolnyy wrote:
> >> On Fri, Oct 16, 2015 at 8:27 AM, Johan Hovold <johan@kernel.org> wrote:
> >> > On Thu, Oct 15, 2015 at 06:23:31PM -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.
> >> >
> >> > That sounds weird. Are you saying that all or only some cp2108 devices
> >> > would be affected by this? And only for the line-control register?
> >>
> >> The bug exists in all current cp2108 devices and affects only
> >> GET_LINE_CTL. But it may be fixed in a future revision.
> >
> > Ok, have you some kind of confirmation from silabs on this already?
> 
> I work for SiLabs.

Heh. :)

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web