diff mbox series

drivers: usb: serial: ftdi_sio.c error flagging

Message ID 20190308164335.1DC0A7003A9@egauge.net (mailing list archive)
State New, archived
Headers show
Series drivers: usb: serial: ftdi_sio.c error flagging | expand

Commit Message

David Mosberger March 8, 2019, 4:43 p.m. UTC
[Resend with From address corrected.  Sorry about that]

Johan,

Some of our customers are experiencing communication issues on RS485
that could be solved quite nicely by turning on termios.PARMRK.  I'd
be happy to go into details, but I don't think they're necessary for
the discussion below.

The problem we encountered is that the error flagging produced by
ftdi_sio.c over-marks errors to the point that PARMRK becomes unusable
(in our cases, everything ends up being flagged as errors, even the
actual, good data).

The issue in particular is that the driver marks *all* characters
received in a single USB packet with an error flag based on whether a
BREAK, frame error, or parity error condition is reported by that USB
packet.  In actuality, the FTDI chip's error condition seems to apply
to the last byte in the received packet (I tested with an FT230X
chip).  Unfortunately, it's more complicated than that.  I was unable
to find any useful documentation on the USB packets the FTDI chips
generate, but from trial and error, the BREAK condition handling seems
to work something like this:

	When a break condition is reported (FTDI_RS_BI is set), the
	last byte received MAY be a BREAK.  To confirm, wait for the
	next status packet.  IF that status packet has no data AND
	FTDI_RS_BI is still set, then the previous character was
	indeed a BREAK.  Otherwise, the previous character was a
	normal data byte.

This seems rather byzantine, but in my testing, it's been the only
algorithm that was able to accurately identify BREAKs.

I attached some code that worked for the test cases I tried.  Of
course, I'm not overly confident that this will work in all cases.

As for parity errors: I have not been able to figure out how to mark
ALL parity errors characters without also sometimes accidentally
marking correctly received bytes as erroneous.

My sense is that the FTDI chips simply can't reliably flag all error
characters (and only error characters) and we're probably not going to
use PARMRK to try to handle the communications issue mentioned at the
outset, so I'm on the fence whether is worthwhile to apply a patch
along the lines of the below.  However, I'd suggest that at least a
comment be added in the driver making it clear that the error flagging
is not accurate as implemented and that it will mark too many
characters as erroneous, but that, perhaps, it's the best that can be
done.

Thoughts?

  --david

Comments

Johan Hovold April 2, 2019, 9:23 a.m. UTC | #1
Hi,

and sorry about the late reply.

On Fri, Mar 08, 2019 at 09:43:35AM -0700, egaugesystems@gmail.com wrote:
> [Resend with From address corrected.  Sorry about that]
> 
> Johan,
> 
> Some of our customers are experiencing communication issues on RS485
> that could be solved quite nicely by turning on termios.PARMRK.  I'd
> be happy to go into details, but I don't think they're necessary for
> the discussion below.
> 
> The problem we encountered is that the error flagging produced by
> ftdi_sio.c over-marks errors to the point that PARMRK becomes unusable
> (in our cases, everything ends up being flagged as errors, even the
> actual, good data).
> 
> The issue in particular is that the driver marks *all* characters
> received in a single USB packet with an error flag based on whether a
> BREAK, frame error, or parity error condition is reported by that USB
> packet.  In actuality, the FTDI chip's error condition seems to apply
> to the last byte in the received packet (I tested with an FT230X
> chip).  Unfortunately, it's more complicated than that.  I was unable
> to find any useful documentation on the USB packets the FTDI chips
> generate, but from trial and error, the BREAK condition handling seems
> to work something like this:
> 
> 	When a break condition is reported (FTDI_RS_BI is set), the
> 	last byte received MAY be a BREAK.  To confirm, wait for the
> 	next status packet.  IF that status packet has no data AND
> 	FTDI_RS_BI is still set, then the previous character was
> 	indeed a BREAK.  Otherwise, the previous character was a
> 	normal data byte.
> 
> This seems rather byzantine, but in my testing, it's been the only
> algorithm that was able to accurately identify BREAKs.
> 
> I attached some code that worked for the test cases I tried.  Of
> course, I'm not overly confident that this will work in all cases.
> 
> As for parity errors: I have not been able to figure out how to mark
> ALL parity errors characters without also sometimes accidentally
> marking correctly received bytes as erroneous.
> 
> My sense is that the FTDI chips simply can't reliably flag all error
> characters (and only error characters) and we're probably not going to
> use PARMRK to try to handle the communications issue mentioned at the
> outset, so I'm on the fence whether is worthwhile to apply a patch
> along the lines of the below.  However, I'd suggest that at least a
> comment be added in the driver making it clear that the error flagging
> is not accurate as implemented and that it will mark too many
> characters as erroneous, but that, perhaps, it's the best that can be
> done.
> 
> Thoughts?

I'm aware that the current implementation flags all characters in the
receive buffer when we detect an error, but I'm not sure we can do much
better either.

Apparently, you get similar behaviour using their own drivers:

	https://highfieldtales.wordpress.com/2014/09/27/lets-dig-into-an-issue-of-the-ft232-chip/

Having FTDI provide the required documentation would help (perhaps you
can ask about this specific issue), otherwise it's down to tedious
reverse engineering of behaviour which may not even be consistent
between device types.

Johan
diff mbox series

Patch

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 2e2f736384ab..aca2613225f3 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -65,6 +65,8 @@  struct ftdi_private {
 	int flags;		/* some ASYNC_xxxx flags are supported */
 	unsigned long last_dtr_rts;	/* saved modem control outputs */
 	char prev_status;        /* Used for TIOCMIWAIT */
+	char prev_errs;
+	char held;
 	char transmit_empty;	/* If transmitter is empty or not */
 	__u16 interface;	/* FT2232C, FT2232H or FT4232H port interface
 				   (0 for FT232/245) */
@@ -2045,6 +2047,7 @@  static int ftdi_process_packet(struct usb_serial_port *port,
 	int i;
 	char status;
 	char flag;
+	char errs;
 	char *ch;
 
 	if (len < 2) {
@@ -2086,6 +2089,21 @@  static int ftdi_process_packet(struct usb_serial_port *port,
 	else
 		priv->transmit_empty = 0;
 
+	if (priv->prev_errs) {
+		errs = priv->prev_errs & packet[1];
+		flag = TTY_NORMAL;
+		if ((len <= 2) && (errs & FTDI_RS_BI)) {
+			printk("BREAK\n");
+			flag = TTY_BREAK;
+			port->icount.brk++;
+			usb_serial_handle_break(port);
+		}
+		printk("%s: flag=%d data=%02x\n", __func__, flag, priv->held);
+		tty_insert_flip_char(&port->port, priv->held, flag);
+		++port->icount.rx;
+		priv->prev_errs = 0;
+	}
+
 	len -= 2;
 	if (!len)
 		return 0;	/* status only */
@@ -2094,18 +2112,22 @@  static int ftdi_process_packet(struct usb_serial_port *port,
 	 * Break and error status must only be processed for packets with
 	 * data payload to avoid over-reporting.
 	 */
+	errs = packet[1] & FTDI_RS_ERR_MASK;
 	flag = TTY_NORMAL;
-	if (packet[1] & FTDI_RS_ERR_MASK) {
-		/* Break takes precedence over parity, which takes precedence
-		 * over framing errors */
-		if (packet[1] & FTDI_RS_BI) {
-			flag = TTY_BREAK;
-			port->icount.brk++;
-			usb_serial_handle_break(port);
-		} else if (packet[1] & FTDI_RS_PE) {
+	ch = packet + 2;
+	if (errs) {
+		if (errs & FTDI_RS_BI) {
+			priv->prev_errs = errs;
+			priv->held = ch[len - 1];
+			if (--len == 0)
+				return 0;
+		}
+		if (errs & FTDI_RS_PE) {
+			printk("PARITY\n");
 			flag = TTY_PARITY;
 			port->icount.parity++;
-		} else if (packet[1] & FTDI_RS_FE) {
+		} else if (errs & FTDI_RS_FE) {
+			printk("FRAME\n");
 			flag = TTY_FRAME;
 			port->icount.frame++;
 		}
@@ -2116,8 +2138,19 @@  static int ftdi_process_packet(struct usb_serial_port *port,
 		}
 	}
 
+
 	port->icount.rx += len;
-	ch = packet + 2;
+
+	{
+		char buf[128], *dst = buf, *end = &buf[128];
+		int i;
+
+		for (i = 0; i < len; ++i) {
+			snprintf (dst, end - dst, " %02x", ch[i]);
+			dst += 3;
+		}
+		printk("%s: flag=%d data=[%s ]\n", __func__, flag, buf);
+	}
 
 	if (port->port.console && port->sysrq) {
 		for (i = 0; i < len; i++, ch++) {