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

[Johan Hovold <johan@kernel.org>]

On Fri, Jul 03, 2020 at 09:45:39AM +0200, Johan Hovold wrote:
> On Wed, Jul 01, 2020 at 09:28:25PM +0200, Jerry wrote:
> > Johan Hovold wrote on 7/1/20 5:42 PM:
> > > It would be better if could detect both types of overrun.
> > >
> > > Did you try moving the purge command at close to after disabling the
> > > uart?
> > >
> > > Or perhaps we can add a "dummy" comm-status command after disabling the
> > > uart?
> 
> > I try to describe more details about this overrun problem:
> > 1) I tried only CP2102 because our company uses it, I have no idea whether 
> > the same apply for CP2104,2105... or not, I don't have another chip.
> > 2) Maybe I should note I'm always using even parity (because of STM32 
> > bootloader protocol).
> > 3) I thought the problem is created by unreaded data when closing because 
> > overrun was reported after closing if GET_COMM_STATUS shown positive 
> > ulAmountInInQueue before closing. Later I discovered that if I close the 
> > port, wait, send some data from outside, then open it, I will also get 
> > HW_OVERRUN flag.
> > 4) So currently I suppose that problem is usually created by any incoming 
> > data after disabling interface. If I remove UART_DISABLE from close method, 
> > no overrun ever reported.
> > 5) Unfortunately this overrun is not reported immediately after port 
> > opening but later after receiving first byte. I didn't find any way how to 
> > purge nor dummy read this flag before transmitting data.
> > 6) I didn't find this problem in a chip errata and nobody complaining about it.
> > 7) I successfully reproduced the same problem in MS Windows 10. If some 
> > data arrived to closed port, then I open it, everything is OK but after 
> > sending request and receiving reply I often get overrun indication from 
> > Win32 API ClearCommError()
> > 8) I removed HW_OVERRUN checking because I don't want to break anything but 
> > maybe Linux driver should work the same way as Windows and don't hide this 
> > problem?
> > 9) I needed just to detect parity error from user space and things 
> > complicate.  :-)
> 
> Heh, yeah, it tends to be that way. :) But thanks for the great summary
> of your findings!
> 
> I think it probably makes most sense to keep the error in as it's a
> quirk of the device rather than risk missing an actual overrun.
> 
> The problem here is that we're sort of violating the API and turning
> TIOCGICOUNT into a polling interface instead of just returning our error
> and interrupt counters. This means will always undercount any errors for
> a start.
> 
> The chip appears to have a mechanism for reporting errors in-band, but
> that would amount to processing every character received to look for the
> escape char, which adds overhead. I'm guessing that interface would also
> insert an overrun error when returning the first character.
> 
> But perhaps that it was we should be using as it allows parity the
> errors to be reported using the normal in-band methods. If we only
> enable it when parity checking is enabled then the overhead seems
> justified.
> 
> I did a quick test here and the event insertion appears to work well for
> parity errors. Didn't manage to get it to report break events yet, and
> modem-status changes are being buffered and not reported until the next
> character. But in theory we could expand the implementation to provide
> more features later.
> 
> I'll see if I can cook something up quickly.

Would you mind giving the below a try and see how that works in your
setup?

With this patch you'd be able to use PARMRK to be notified of any parity
errors, but I also wired up TIOCGICOUNT if you prefer to use that.

Also, could try and see if your device detects breaks properly? Mine
just return a NUL char.

Johan



From 5fc7de670489a6651e023c325e674666d65cfe14 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Fri, 3 Jul 2020 16:39:14 +0200
Subject: [PATCH] USB: serial: add support for line-status events

Add support for line-status events that can be used to detect and report
parity errors.

Enable the device's event-insertion mode whenever input-parity checking
is requested. This will insert line and modem status events into the
data stream.

Note that modem-status changes appear to be buffered until a character
is received and is therefore left unimplemented.

On at least one type of these chips, line breaks are also not detected
properly and is just reported as a NUL character. I'm therefore not
enabling event-insertion when !IGNBRK is requested.

Also wire up TIOCGICOUNT to allow for reading out the line-status
counters.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 207 +++++++++++++++++++++++++++++++++++-
 1 file changed, 204 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index f5143eedbc48..b5f8176ee7ab 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -50,6 +50,9 @@ static void cp210x_release(struct usb_serial *);
 static int cp210x_port_probe(struct usb_serial_port *);
 static int cp210x_port_remove(struct usb_serial_port *);
 static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
+static void cp210x_process_read_urb(struct urb *urb);
+static void cp210x_enable_event_mode(struct usb_serial_port *port);
+static void cp210x_disable_event_mode(struct usb_serial_port *port);
 
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x045B, 0x0053) }, /* Renesas RX610 RX-Stick */
@@ -253,9 +256,21 @@ struct cp210x_serial_private {
 	bool			use_actual_rate;
 };
 
+enum cp210x_event_state {
+	ES_DATA,
+	ES_ESCAPE,
+	ES_LSR,
+	ES_LSR_DATA_0,
+	ES_LSR_DATA_1,
+	ES_MSR
+};
+
 struct cp210x_port_private {
 	__u8			bInterfaceNumber;
 	bool			has_swapped_line_ctl;
+	bool			event_mode;
+	enum cp210x_event_state event_state;
+	u8 lsr;
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -274,12 +289,14 @@ static struct usb_serial_driver cp210x_device = {
 	.tx_empty		= cp210x_tx_empty,
 	.tiocmget		= cp210x_tiocmget,
 	.tiocmset		= cp210x_tiocmset,
+	.get_icount		= usb_serial_generic_get_icount,
 	.attach			= cp210x_attach,
 	.disconnect		= cp210x_disconnect,
 	.release		= cp210x_release,
 	.port_probe		= cp210x_port_probe,
 	.port_remove		= cp210x_port_remove,
-	.dtr_rts		= cp210x_dtr_rts
+	.dtr_rts		= cp210x_dtr_rts,
+	.process_read_urb	= cp210x_process_read_urb,
 };
 
 static struct usb_serial_driver * const serial_drivers[] = {
@@ -401,6 +418,15 @@ struct cp210x_comm_status {
  */
 #define PURGE_ALL		0x000f
 
+/* CP210X_EMBED_EVENTS */
+#define CP210X_ESCCHAR		0xff
+
+#define CP210X_LSR_OVERRUN	BIT(1)
+#define CP210X_LSR_PARITY	BIT(2)
+#define CP210X_LSR_FRAME	BIT(3)
+#define CP210X_LSR_BREAK	BIT(4)
+
+
 /* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
 struct cp210x_flow_ctl {
 	__le32	ulControlHandshake;
@@ -807,6 +833,7 @@ static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *ctl)
 
 static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
 	int result;
 
 	result = cp210x_write_u16_reg(port, CP210X_IFC_ENABLE, UART_ENABLE);
@@ -819,20 +846,151 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 	cp210x_get_termios(tty, port);
 
 	/* The baud rate must be initialised on cp2104 */
-	if (tty)
+	if (tty) {
 		cp210x_change_speed(tty, port, NULL);
 
-	return usb_serial_generic_open(tty, port);
+		/* FIXME: handle from set_termios() only */
+		if (I_INPCK(tty))
+			cp210x_enable_event_mode(port);
+	}
+
+	result = usb_serial_generic_open(tty, port);
+	if (result)
+		goto err_disable;
+
+	return 0;
+
+err_disable:
+	cp210x_write_u16_reg(port, CP210X_IFC_ENABLE, UART_DISABLE);
+	port_priv->event_mode = false;
+
+	return result;
 }
 
 static void cp210x_close(struct usb_serial_port *port)
 {
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+
 	usb_serial_generic_close(port);
 
 	/* Clear both queues; cp2108 needs this to avoid an occasional hang */
 	cp210x_write_u16_reg(port, CP210X_PURGE, PURGE_ALL);
 
 	cp210x_write_u16_reg(port, CP210X_IFC_ENABLE, UART_DISABLE);
+
+	/* Disabling the interface disables event-insertion mode. */
+	port_priv->event_mode = false;
+}
+
+static char cp210x_process_lsr(struct usb_serial_port *port, unsigned char lsr)
+{
+	char flag = TTY_NORMAL;
+
+	if (lsr & CP210X_LSR_BREAK) {
+		flag = TTY_BREAK;
+		port->icount.brk++;
+		/* FIXME: no need to process sysrq chars without breaks... */
+		usb_serial_handle_break(port);
+	} else if (lsr & CP210X_LSR_PARITY) {
+		flag = TTY_PARITY;
+		port->icount.parity++;
+	} else if (lsr & CP210X_LSR_FRAME) {
+		flag = TTY_FRAME;
+		port->icount.frame++;
+	}
+
+	if (lsr & CP210X_LSR_OVERRUN) {
+		port->icount.overrun++;
+		tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
+	}
+
+	return flag;
+}
+
+static bool cp210x_process_event_char(struct usb_serial_port *port, unsigned char *ch, char *flag)
+{
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+
+	switch(port_priv->event_state) {
+	case ES_DATA:
+		if (*ch == CP210X_ESCCHAR) {
+			port_priv->event_state = ES_ESCAPE;
+			break;
+		}
+		return false;
+	case ES_ESCAPE:
+		switch (*ch) {
+		case 0:
+			dev_dbg(&port->dev, "%s - escape char\n", __func__);
+			*ch = CP210X_ESCCHAR;
+			port_priv->event_state = ES_DATA;
+			return false;
+		case 1:
+			port_priv->event_state = ES_LSR_DATA_0;
+			break;
+		case 2:
+			port_priv->event_state = ES_LSR;
+			break;
+		case 3:
+			port_priv->event_state = ES_MSR;
+			break;
+		default:
+			dev_err(&port->dev, "malformed event 0x%02x\n", *ch);
+			port_priv->event_state = ES_DATA;
+			break;
+		}
+		break;
+	case ES_LSR_DATA_0:
+		port_priv->lsr = *ch;
+		port_priv->event_state = ES_LSR_DATA_1;
+		break;
+	case ES_LSR_DATA_1:
+		dev_dbg(&port->dev, "%s - lsr = 0x%02x, data = 0x%02x\n",
+				__func__, port_priv->lsr, *ch);
+		*flag = cp210x_process_lsr(port, port_priv->lsr);
+		port_priv->event_state = ES_DATA;
+		return false;
+	case ES_LSR:
+		dev_dbg(&port->dev, "%s - lsr = 0x%02x\n", __func__, *ch);
+		port_priv->lsr = *ch;
+		cp210x_process_lsr(port, port_priv->lsr);
+		port_priv->event_state = ES_DATA;
+		break;
+	case ES_MSR:
+		dev_dbg(&port->dev, "%s - msr = 0x%02x\n", __func__, *ch);
+		/* unimplemented */
+		port_priv->event_state = ES_DATA;
+		break;
+	}
+
+	return true;
+}
+
+static void cp210x_process_read_urb(struct urb *urb)
+{
+	struct usb_serial_port *port = urb->context;
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	unsigned char *ch = urb->transfer_buffer;
+	char flag;
+	int i;
+
+	if (!urb->actual_length)
+		return;
+
+	if (!port_priv->event_mode && !(port->port.console && port->sysrq)) {
+		tty_insert_flip_string(&port->port, ch, urb->actual_length);
+	} else {
+		for (i = 0; i < urb->actual_length; i++, ch++) {
+			flag = TTY_NORMAL;
+
+			if (cp210x_process_event_char(port, ch, &flag))
+				continue;
+
+			if (!usb_serial_handle_sysrq_char(port, *ch))
+				tty_insert_flip_char(&port->port, *ch, flag);
+		}
+	}
+	tty_flip_buffer_push(&port->port);
 }
 
 /*
@@ -1148,6 +1306,41 @@ static void cp210x_change_speed(struct tty_struct *tty,
 	tty_encode_baud_rate(tty, baud, baud);
 }
 
+static void cp210x_enable_event_mode(struct usb_serial_port *port)
+{
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+
+	if (port_priv->event_mode)
+		return;
+
+	port_priv->event_state = ES_DATA;
+	port_priv->event_mode = true;
+
+	ret = cp210x_write_u16_reg(port, CP210X_EMBED_EVENTS, CP210X_ESCCHAR);
+	if (ret) {
+		dev_err(&port->dev, "failed to enable events: %d\n", ret);
+		port_priv->event_mode = false;
+	}
+}
+
+static void cp210x_disable_event_mode(struct usb_serial_port *port)
+{
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+
+	if (!port_priv->event_mode)
+		return;
+
+	ret = cp210x_write_u16_reg(port, CP210X_EMBED_EVENTS, 0);
+	if (ret) {
+		dev_err(&port->dev, "failed to disable events: %d\n", ret);
+		return;
+	}
+
+	port_priv->event_mode = false;
+}
+
 static void cp210x_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
@@ -1270,6 +1463,14 @@ static void cp210x_set_termios(struct tty_struct *tty,
 				sizeof(flow_ctl));
 	}
 
+	/*
+	 * Enable event-insertion mode only if input parity checking is
+	 * enabled for now.
+	 */
+	if (I_INPCK(tty))
+		cp210x_enable_event_mode(port);
+	else
+		cp210x_disable_event_mode(port);
 }
 
 static int cp210x_tiocmset(struct tty_struct *tty,
-- 
2.26.2