Re: [PATCH v4] usbserial: cp210x - icount support for parity error checking

[Johan Hovold <johan@kernel.org>]

On Mon, Jun 22, 2020 at 05:13:41PM +0200, Jaromír Škorpil wrote:
> The current version of cp210x driver doesn't provide any way to detect
> a parity error in received data from userspace. Some serial protocols like
> STM32 bootloader protect data only by parity so application needs to
> know whether parity error happened to repeat peripheral data reading.
> 
> Added support for icount (ioctl TIOCGICOUNT) which sends GET_COMM_STATUS
> command to CP210X and according received flags increments fields for
> parity error, frame error, break and overrun. An application can detect
> an error condition after reading data from ttyUSB and reacts adequately.
> There is no impact for applications which don't call ioctl TIOCGICOUNT.
> 
> The flag "hardware overrun" is not examined because CP2102 sets this bit
> for the first received byte after openning of port which was previously
> closed with some unreaded data in buffer. This is confusing and checking
> "queue overrun" flag seems be enough.
> 
> Signed-off-by: Jaromír Škorpil <Jerry@jrr.cz>
> ---
> v2: Simplified counting - only queue overrun checked
> v3: Changed description + UTF8 name
> v4: Corrected endian

So let's go with this and then I can add support for in-band reporting
on top.

I was gonna apply it and the missing locking, but noticed that the patch
is white-space damaged. At least some leading tabs have been converted.

Try sending the patch yourself (e.g. using git-send-email) and make sure
you can apply it using git-am.

>   cp210x.c |   43 ++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 38 insertions(+), 5 deletions(-)
> 
> diff -up linux-5.8-rc1/drivers/usb/serial/cp210x.c j/drivers/usb/serial/cp210x.c
> --- linux-5.8-rc1/drivers/usb/serial/cp210x.c
> +++ j/drivers/usb/serial/cp210x.c
> @@ -43,6 +43,8 @@ static int cp210x_tiocmget(struct tty_st
>   static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
>   static int cp210x_tiocmset_port(struct usb_serial_port *port,
>   		unsigned int, unsigned int);
> +static int cp210x_get_icount(struct tty_struct *tty,
> +		struct serial_icounter_struct *icount);
>   static void cp210x_break_ctl(struct tty_struct *, int);
>   static int cp210x_attach(struct usb_serial *);
>   static void cp210x_disconnect(struct usb_serial *);
> @@ -274,6 +276,7 @@ static struct usb_serial_driver cp210x_d
>   	.tx_empty		= cp210x_tx_empty,
>   	.tiocmget		= cp210x_tiocmget,
>   	.tiocmset		= cp210x_tiocmset,
> +	.get_icount		= cp210x_get_icount,
>   	.attach			= cp210x_attach,
>   	.disconnect		= cp210x_disconnect,
>   	.release		= cp210x_release,
> @@ -393,6 +396,13 @@ struct cp210x_comm_status {
>   	u8       bReserved;
>   } __packed;
>   
> +/* cp210x_comm_status::ulErrors */
> +#define CP210X_SERIAL_ERR_BREAK	BIT(0)
> +#define CP210X_SERIAL_ERR_FRAME	BIT(1)
> +#define CP210X_SERIAL_ERR_HW_OVERRUN	BIT(2)
> +#define CP210X_SERIAL_ERR_QUEUE_OVERRUN	BIT(3)
> +#define CP210X_SERIAL_ERR_PARITY	BIT(4)

Can you drop the SERIAL_ infix here?

> +
>   /*
>    * CP210X_PURGE - 16 bits passed in wValue of USB request.
>    * SiLabs app note AN571 gives a strange description of the 4 bits:
> @@ -836,10 +846,10 @@ static void cp210x_close(struct usb_seri
>   }
>   
>   /*
> - * Read how many bytes are waiting in the TX queue.
> + * Read how many bytes are waiting in the TX queue and update error counters.
>    */
> -static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port,
> -		u32 *count)
> +static int cp210x_get_comm_status(struct usb_serial_port *port,
> +		u32 *tx_count)
>   {
>   	struct usb_serial *serial = port->serial;
>   	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> @@ -855,7 +865,17 @@ static int cp210x_get_tx_queue_byte_coun
>   			0, port_priv->bInterfaceNumber, sts, sizeof(*sts),
>   			USB_CTRL_GET_TIMEOUT);
>   	if (result == sizeof(*sts)) {
> -		*count = le32_to_cpu(sts->ulAmountInOutQueue);
> +		u32 flags = le32_to_cpu(sts->ulErrors);

Here should be a newline.

> +		if (flags & CP210X_SERIAL_ERR_BREAK)
> +			port->icount.brk++;
> +		if (flags & CP210X_SERIAL_ERR_FRAME)
> +			port->icount.frame++;
> +		if (flags & CP210X_SERIAL_ERR_QUEUE_OVERRUN)
> +			port->icount.overrun++;
> +		if (flags & CP210X_SERIAL_ERR_PARITY)
> +			port->icount.parity++;

And these icount increments should be done under the port->lock as
TIOCGICOUNT can be called in parallel.

> +		if (tx_count)
> +			*tx_count = le32_to_cpu(sts->ulAmountInOutQueue);
>   		result = 0;
>   	} else {
>   		dev_err(&port->dev, "failed to get comm status: %d\n", result);
> @@ -873,13 +893,26 @@ static bool cp210x_tx_empty(struct usb_s
>   	int err;
>   	u32 count;
>   
> -	err = cp210x_get_tx_queue_byte_count(port, &count);
> +	err = cp210x_get_comm_status(port, &count);
>   	if (err)
>   		return true;
>   
>   	return !count;
>   }
>   
> +static int cp210x_get_icount(struct tty_struct *tty,
> +		struct serial_icounter_struct *icount)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	int result;
> +
> +	result = cp210x_get_comm_status(port, NULL);
> +	if (result)
> +		return result;
> +
> +	return usb_serial_generic_get_icount(tty, icount);
> +}
> +
>   /*
>    * cp210x_get_termios
>    * Reads the baud rate, data bits, parity, stop bits and flow control mode
> 
> 
> 

Johan