[PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Konstantin Shkolnyy]

Occasionally, writing data and immediately closing the port makes cp2108
stop responding. The device had to be unplugged to clear the error.
The failure is induced by shutting down the device while its Tx queue still has
unsent data. Reporting the correct amount of those data avoids the problem.
Adding tx_empty() has no adverse effect on other cp210x devices.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
---
 drivers/usb/serial/cp210x.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eac7cca..0189e64 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -38,6 +38,7 @@ static void cp210x_change_speed(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios *);
 static void cp210x_set_termios(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios*);
+static bool cp210x_tx_empty(struct usb_serial_port *port);
 static int cp210x_tiocmget(struct tty_struct *);
 static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
 static int cp210x_tiocmset_port(struct usb_serial_port *port,
@@ -214,6 +215,7 @@ static struct usb_serial_driver cp210x_device = {
 	.close			= cp210x_close,
 	.break_ctl		= cp210x_break_ctl,
 	.set_termios		= cp210x_set_termios,
+	.tx_empty		= cp210x_tx_empty,
 	.tiocmget		= cp210x_tiocmget,
 	.tiocmset		= cp210x_tiocmset,
 	.attach			= cp210x_startup,
@@ -249,6 +251,16 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_GET_CHARS	0x0E
 #define CP210X_GET_PROPS	0x0F
 #define CP210X_GET_COMM_STATUS	0x10
+/* Data returned by CP210X_GET_COMM_STATUS -- h/w doc says it's 0x13 bytes */
+struct cp210x_comm_status {
+	u32     errors;
+	u32     hold_reasons;
+	u32     amount_in_in_queue;
+	u32     amount_in_out_queue;
+	u8      eof_received;
+	u8      wait_for_immediate;
+	u8      reserved;
+};
 #define CP210X_RESET		0x11
 #define CP210X_PURGE		0x12
 #define CP210X_SET_FLOW		0x13
@@ -479,6 +491,24 @@ static void cp210x_close(struct usb_serial_port *port)
 	cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
 }
 
+static bool cp210x_tx_empty(struct usb_serial_port *port)
+{
+	int err;
+
+	/* get_config needs "array of integers large enough", so pad to 0x14 bytes */
+	struct cp210x_comm_status_container {
+		struct cp210x_comm_status  sts; /* 0x13 bytes */
+		u8                         pad_to_0x14_bytes;
+	} comm_sts_cont;
+
+	err = cp210x_get_config(port, CP210X_GET_COMM_STATUS, (unsigned int *) &comm_sts_cont, 0x13);
+
+	if (!err)
+		if (comm_sts_cont.sts.amount_in_out_queue)
+			return false;
+	return true;
+}
+
 /*
  * cp210x_get_termios
  * Reads the baud rate, data bits, parity, stop bits and flow control mode
-- 
1.9.1

[PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Konstantin Shkolnyy]

Occasionally, writing data and immediately closing the port makes cp2108
stop responding. The device had to be unplugged to clear the error.
The failure is induced by shutting down the device while its Tx queue still has
unsent data. Reporting the correct amount of those data avoids the problem.
Adding tx_empty() has no adverse effect on other cp210x devices.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
---
 drivers/usb/serial/cp210x.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eac7cca..0189e64 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -38,6 +38,7 @@ static void cp210x_change_speed(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios *);
 static void cp210x_set_termios(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios*);
+static bool cp210x_tx_empty(struct usb_serial_port *port);
 static int cp210x_tiocmget(struct tty_struct *);
 static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
 static int cp210x_tiocmset_port(struct usb_serial_port *port,
@@ -214,6 +215,7 @@ static struct usb_serial_driver cp210x_device = {
 	.close			= cp210x_close,
 	.break_ctl		= cp210x_break_ctl,
 	.set_termios		= cp210x_set_termios,
+	.tx_empty		= cp210x_tx_empty,
 	.tiocmget		= cp210x_tiocmget,
 	.tiocmset		= cp210x_tiocmset,
 	.attach			= cp210x_startup,
@@ -249,6 +251,16 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_GET_CHARS	0x0E
 #define CP210X_GET_PROPS	0x0F
 #define CP210X_GET_COMM_STATUS	0x10
+/* Data returned by CP210X_GET_COMM_STATUS -- h/w doc says it's 0x13 bytes */
+struct cp210x_comm_status {
+	u32     errors;
+	u32     hold_reasons;
+	u32     amount_in_in_queue;
+	u32     amount_in_out_queue;
+	u8      eof_received;
+	u8      wait_for_immediate;
+	u8      reserved;
+};
 #define CP210X_RESET		0x11
 #define CP210X_PURGE		0x12
 #define CP210X_SET_FLOW		0x13
@@ -479,6 +491,24 @@ static void cp210x_close(struct usb_serial_port *port)
 	cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
 }
 
+static bool cp210x_tx_empty(struct usb_serial_port *port)
+{
+	int err;
+
+	/* get_config needs "array of integers large enough", so pad to 0x14 bytes */
+	struct cp210x_comm_status_container {
+		struct cp210x_comm_status  sts; /* 0x13 bytes */
+		u8                         pad_to_0x14_bytes;
+	} comm_sts_cont;
+
+	err = cp210x_get_config(port, CP210X_GET_COMM_STATUS, (unsigned int *) &comm_sts_cont, 0x13);
+
+	if (!err)
+		if (comm_sts_cont.sts.amount_in_out_queue)
+			return false;
+	return true;
+}
+
 /*
  * cp210x_get_termios
  * Reads the baud rate, data bits, parity, stop bits and flow control mode
-- 
1.9.1

Re: [PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Sergei Shtylyov]

Hello.

On 10/16/2015 1:07 AM, Konstantin Shkolnyy wrote:

> Occasionally, writing data and immediately closing the port makes cp2108
> stop responding. The device had to be unplugged to clear the error.
> The failure is induced by shutting down the device while its Tx queue still has
> unsent data. Reporting the correct amount of those data avoids the problem.
> Adding tx_empty() has no adverse effect on other cp210x devices.
>
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
>   drivers/usb/serial/cp210x.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eac7cca..0189e64 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
[...]
> @@ -249,6 +251,16 @@ static struct usb_serial_driver * const serial_drivers[] = {
>   #define CP210X_GET_CHARS	0x0E
>   #define CP210X_GET_PROPS	0x0F
>   #define CP210X_GET_COMM_STATUS	0x10
> +/* Data returned by CP210X_GET_COMM_STATUS -- h/w doc says it's 0x13 bytes */
> +struct cp210x_comm_status {
> +	u32     errors;
> +	u32     hold_reasons;
> +	u32     amount_in_in_queue;
> +	u32     amount_in_out_queue;
> +	u8      eof_received;
> +	u8      wait_for_immediate;
> +	u8      reserved;
> +};

    Please don't declare structures amidst of the command #define's.

>   #define CP210X_RESET		0x11
>   #define CP210X_PURGE		0x12
>   #define CP210X_SET_FLOW		0x13
> @@ -479,6 +491,24 @@ static void cp210x_close(struct usb_serial_port *port)
>   	cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
>   }
>
> +static bool cp210x_tx_empty(struct usb_serial_port *port)
> +{
> +	int err;
> +

    Empty line hardly needed here.

> +	/* get_config needs "array of integers large enough", so pad to 0x14 bytes */
> +	struct cp210x_comm_status_container {
> +		struct cp210x_comm_status  sts; /* 0x13 bytes */
> +		u8                         pad_to_0x14_bytes;
> +	} comm_sts_cont;
> +
> +	err = cp210x_get_config(port, CP210X_GET_COMM_STATUS, (unsigned int *) &comm_sts_cont, 0x13);
> +

    Empty line hardly needed here.

> +	if (!err)
> +		if (comm_sts_cont.sts.amount_in_out_queue)

    Why not collapse these two *if* statements into one?

> +			return false;
> +	return true;
> +}
> +
[...]

MBR, Sergei

Re: [PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Konstantin Shkolnyy]

Hello,

On Fri, Oct 16, 2015 at 6:19 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
 [...]
>>
>> @@ -249,6 +251,16 @@ static struct usb_serial_driver * const
>> serial_drivers[] = {
>>   #define CP210X_GET_CHARS      0x0E
>>   #define CP210X_GET_PROPS      0x0F
>>   #define CP210X_GET_COMM_STATUS        0x10
>> +/* Data returned by CP210X_GET_COMM_STATUS -- h/w doc says it's 0x13
>> bytes */
>> +struct cp210x_comm_status {
>> +       u32     errors;
>> +       u32     hold_reasons;
>> +       u32     amount_in_in_queue;
>> +       u32     amount_in_out_queue;
>> +       u8      eof_received;
>> +       u8      wait_for_immediate;
>> +       u8      reserved;
>> +};
>
>
>    Please don't declare structures amidst of the command #define's.
>
[...]

I agree with all suggestions except this one. I find it very
convenient, when reading code, to have the command code and its data
declared in the same place.

Best regards,
Konstantin

Re: [PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Johan Hovold]

On Fri, Oct 16, 2015 at 07:35:02AM -0500, Konstantin Shkolnyy wrote:
> Hello,
> 
> On Fri, Oct 16, 2015 at 6:19 AM, Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
>  [...]
> >>
> >> @@ -249,6 +251,16 @@ static struct usb_serial_driver * const
> >> serial_drivers[] = {
> >>   #define CP210X_GET_CHARS      0x0E
> >>   #define CP210X_GET_PROPS      0x0F
> >>   #define CP210X_GET_COMM_STATUS        0x10
> >> +/* Data returned by CP210X_GET_COMM_STATUS -- h/w doc says it's 0x13
> >> bytes */
> >> +struct cp210x_comm_status {
> >> +       u32     errors;
> >> +       u32     hold_reasons;
> >> +       u32     amount_in_in_queue;
> >> +       u32     amount_in_out_queue;
> >> +       u8      eof_received;
> >> +       u8      wait_for_immediate;
> >> +       u8      reserved;
> >> +};
> >
> >
> >    Please don't declare structures amidst of the command #define's.
> >
> [...]
> 
> I agree with all suggestions except this one. I find it very
> convenient, when reading code, to have the command code and its data
> declared in the same place.

No, I agree with Sergei on this. Please place the struct after the
request defines (just like the various request values after are defined
after the request list).

I'll try to provide some more feedback on your patches shortly. Sorry
for the delay.

Johan

Re: [PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Johan Hovold]

On Thu, Oct 15, 2015 at 05:07:08PM -0500, Konstantin Shkolnyy wrote:
> Occasionally, writing data and immediately closing the port makes cp2108
> stop responding. The device had to be unplugged to clear the error.
> The failure is induced by shutting down the device while its Tx queue still has
> unsent data. Reporting the correct amount of those data avoids the problem.
> Adding tx_empty() has no adverse effect on other cp210x devices.

Thanks for the patch. Please always run scripts/checkpatch.pl on your
patches before submitting. It would have let you known there are some
minor style issues.

Also when resending, please indicate that in the patch subject (e.g.
[PATCH RESEND]). If you make changes, include a version (e.g. v2) and
add short change log after the cut off line.

While tx_empty is a nice feature to have this does not seem to fully
address the problem you have identified. Specifically, you also need to
consider what happens if flow control is enabled. Then the TX buffer may
never drain, and you'd end up in the same situation.

Could you first see if a simple purge command (0x12) to clear the tx
fifo from close is enough to fix the problem you're seeing? If so that
fix would be preferred as it is both more general and makes for smaller
patch more suitable for backporting.

You can implement tx_empty is a follow up feature patch.

> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eac7cca..0189e64 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -38,6 +38,7 @@ static void cp210x_change_speed(struct tty_struct *, struct usb_serial_port *,
>  							struct ktermios *);
>  static void cp210x_set_termios(struct tty_struct *, struct usb_serial_port *,
>  							struct ktermios*);
> +static bool cp210x_tx_empty(struct usb_serial_port *port);
>  static int cp210x_tiocmget(struct tty_struct *);
>  static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
>  static int cp210x_tiocmset_port(struct usb_serial_port *port,
> @@ -214,6 +215,7 @@ static struct usb_serial_driver cp210x_device = {
>  	.close			= cp210x_close,
>  	.break_ctl		= cp210x_break_ctl,
>  	.set_termios		= cp210x_set_termios,
> +	.tx_empty		= cp210x_tx_empty,
>  	.tiocmget		= cp210x_tiocmget,
>  	.tiocmset		= cp210x_tiocmset,
>  	.attach			= cp210x_startup,
> @@ -249,6 +251,16 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CP210X_GET_CHARS	0x0E
>  #define CP210X_GET_PROPS	0x0F
>  #define CP210X_GET_COMM_STATUS	0x10
> +/* Data returned by CP210X_GET_COMM_STATUS -- h/w doc says it's 0x13 bytes */
> +struct cp210x_comm_status {
> +	u32     errors;
> +	u32     hold_reasons;
> +	u32     amount_in_in_queue;
> +	u32     amount_in_out_queue;

Use __le32 here (more details below).

> +	u8      eof_received;
> +	u8      wait_for_immediate;
> +	u8      reserved;
> +};

Place the struct last after the other request defines.

>  #define CP210X_RESET		0x11
>  #define CP210X_PURGE		0x12
>  #define CP210X_SET_FLOW		0x13
> @@ -479,6 +491,24 @@ static void cp210x_close(struct usb_serial_port *port)
>  	cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
>  }
>  
> +static bool cp210x_tx_empty(struct usb_serial_port *port)
> +{
> +	int err;
> +
> +	/* get_config needs "array of integers large enough", so pad to 0x14 bytes */
> +	struct cp210x_comm_status_container {
> +		struct cp210x_comm_status  sts; /* 0x13 bytes */
> +		u8                         pad_to_0x14_bytes;
> +	} comm_sts_cont;
> +
> +	err = cp210x_get_config(port, CP210X_GET_COMM_STATUS, (unsigned int *) &comm_sts_cont, 0x13);

You should not use cp210x_get_config here at all and rather add a new
helper to read out the status that you call here after allocating a
buffer to store the result. Then use le32_to_cpu() to access the field
you're interested in.

The byte fields at the end of the message will also be incorrectly
ordered otherwise.

Yes, the current control-request handling is a bit of a mess...

> +
> +	if (!err)
> +		if (comm_sts_cont.sts.amount_in_out_queue)
> +			return false;
> +	return true;
> +}
> +
>  /*
>   * cp210x_get_termios
>   * Reads the baud rate, data bits, parity, stop bits and flow control mode

Thanks,
Johan

Re: [PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Konstantin Shkolnyy]

On Fri, Oct 16, 2015 at 7:55 AM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Oct 15, 2015 at 05:07:08PM -0500, Konstantin Shkolnyy wrote:
>> Occasionally, writing data and immediately closing the port makes cp2108
>> stop responding. The device had to be unplugged to clear the error.
>> The failure is induced by shutting down the device while its Tx queue still has
>> unsent data. Reporting the correct amount of those data avoids the problem.
>> Adding tx_empty() has no adverse effect on other cp210x devices.
[...]
>
> While tx_empty is a nice feature to have this does not seem to fully
> address the problem you have identified. Specifically, you also need to
> consider what happens if flow control is enabled. Then the TX buffer may
> never drain, and you'd end up in the same situation.
>
> Could you first see if a simple purge command (0x12) to clear the tx
> fifo from close is enough to fix the problem you're seeing? If so that
> fix would be preferred as it is both more general and makes for smaller
> patch more suitable for backporting.

I did consider the purge, but didn't want to kill bytes that could
otherwise be successfully sent. By the description of tx_empty(), it
sounded like something that just simply reports the status of the
queue. It shouldn't cause any harm, if implemented. And, conveniently,
it got rid of the failure I was seeing.

I'm new to this code. I don't know how flow control interacts with the
rest of the logic. If it gets close() called even if there are still
data in the Tx queue, then the purge may indeed be needed in close().
Is this how it works?
[...]

>> +static bool cp210x_tx_empty(struct usb_serial_port *port)
>> +{
>> +     int err;
>> +
>> +     /* get_config needs "array of integers large enough", so pad to 0x14 bytes */
>> +     struct cp210x_comm_status_container {
>> +             struct cp210x_comm_status  sts; /* 0x13 bytes */
>> +             u8                         pad_to_0x14_bytes;
>> +     } comm_sts_cont;
>> +
>> +     err = cp210x_get_config(port, CP210X_GET_COMM_STATUS, (unsigned int *) &comm_sts_cont, 0x13);
>
> You should not use cp210x_get_config here at all and rather add a new
> helper to read out the status that you call here after allocating a
> buffer to store the result. Then use le32_to_cpu() to access the field
> you're interested in.
>
> The byte fields at the end of the message will also be incorrectly
> ordered otherwise.
[...]

Do you mean that I should call cp210x_get_config from the helper?

Thanks,
Konstantin

Re: [PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Johan Hovold]

On Fri, Oct 16, 2015 at 08:48:39AM -0500, Konstantin Shkolnyy wrote:
> On Fri, Oct 16, 2015 at 7:55 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Oct 15, 2015 at 05:07:08PM -0500, Konstantin Shkolnyy wrote:
> >> Occasionally, writing data and immediately closing the port makes cp2108
> >> stop responding. The device had to be unplugged to clear the error.
> >> The failure is induced by shutting down the device while its Tx queue still has
> >> unsent data. Reporting the correct amount of those data avoids the problem.
> >> Adding tx_empty() has no adverse effect on other cp210x devices.
> [...]
> >
> > While tx_empty is a nice feature to have this does not seem to fully
> > address the problem you have identified. Specifically, you also need to
> > consider what happens if flow control is enabled. Then the TX buffer may
> > never drain, and you'd end up in the same situation.
> >
> > Could you first see if a simple purge command (0x12) to clear the tx
> > fifo from close is enough to fix the problem you're seeing? If so that
> > fix would be preferred as it is both more general and makes for smaller
> > patch more suitable for backporting.
> 
> I did consider the purge, but didn't want to kill bytes that could
> otherwise be successfully sent. By the description of tx_empty(), it
> sounded like something that just simply reports the status of the
> queue. It shouldn't cause any harm, if implemented. And, conveniently,
> it got rid of the failure I was seeing.

You're reasoning is sound, but if using purge works, it would be a more
generic (handling flow control) and less intrusive change, and as such
would be more suitable for backporting to stable.

We'd still want tx_empty (the basis for wait_until_sent) to avoid
unnecessarily dropping any data. But note that this would be a new
feature of the driver (and as such is not stable material) and should be
implemented on top of the fix.

> I'm new to this code. I don't know how flow control interacts with the
> rest of the logic. If it gets close() called even if there are still
> data in the Tx queue, then the purge may indeed be needed in close().
> Is this how it works?

Exactly. If you implement tx_empty the generic wait-until-sent
implementation will wait for the buffer to drain, but there would still
be a timeout in case the connection has stalled (e.g. due to flow
control). After that close() will always be called.

> [...]
> 
> >> +static bool cp210x_tx_empty(struct usb_serial_port *port)
> >> +{
> >> +     int err;
> >> +
> >> +     /* get_config needs "array of integers large enough", so pad to 0x14 bytes */
> >> +     struct cp210x_comm_status_container {
> >> +             struct cp210x_comm_status  sts; /* 0x13 bytes */
> >> +             u8                         pad_to_0x14_bytes;
> >> +     } comm_sts_cont;
> >> +
> >> +     err = cp210x_get_config(port, CP210X_GET_COMM_STATUS, (unsigned int *) &comm_sts_cont, 0x13);
> >
> > You should not use cp210x_get_config here at all and rather add a new
> > helper to read out the status that you call here after allocating a
> > buffer to store the result. Then use le32_to_cpu() to access the field
> > you're interested in.
> >
> > The byte fields at the end of the message will also be incorrectly
> > ordered otherwise.
> [...]
> 
> Do you mean that I should call cp210x_get_config from the helper?

No, just do the usb_control_msg call directly from your helper. We can
refactor that into a generic helper later (and kill off get_config).

Don't hesitate to ask if you have any further questions.

Thanks,
Johan