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

[Johan Hovold <johan@kernel.org>]

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