From patchwork Fri Mar 8 16:43:35 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Mosberger X-Patchwork-Id: 10845135 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 43B94922 for ; Fri, 8 Mar 2019 16:43:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2A7672891F for ; Fri, 8 Mar 2019 16:43:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1A4912F4FB; Fri, 8 Mar 2019 16:43:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.7 required=2.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 80CBE2891F for ; Fri, 8 Mar 2019 16:43:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726452AbfCHQni (ORCPT ); Fri, 8 Mar 2019 11:43:38 -0500 Received: from mail-io1-f65.google.com ([209.85.166.65]:34146 "EHLO mail-io1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726375AbfCHQni (ORCPT ); Fri, 8 Mar 2019 11:43:38 -0500 Received: by mail-io1-f65.google.com with SMTP id e1so17264968iok.1; Fri, 08 Mar 2019 08:43:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:subject:cc:message-id:date; bh=GvjgaIm0PZzbK3wIiZOwscsBYpk6GVkYrk+of9HUnuE=; b=sOG3UpAbYQ3FbkNh6RqLhKrLbuDBMztTurXqbpBGCmVuT13cYeiLG9C4yzyBC2HUCB jc/kU+LuYo51egRF5+82BqLLMnOdiMa8ypXN/iVGi3lvJojgfhvLy42F/J/G/T5aV40h r7PNUnsB7aLNiA/CqKf1Dk4bEygNig7qjtYpbnlxRotfsJHsnwGxBUkOHyT3r6oxGR2I hPoipkbcJalnTuBFKiiI8s1hDFhJ4e2QW1XXY+PuHMWR/H1nGWxJ70fj8i6hpP+0oE3l 8khcKuGZcXlx4GeoGKjWfntvA4f8gVVAC7BnFey8LHSvA/GHBvMiIxIw46C+sqv6/xyd f4PQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:cc:message-id:date; bh=GvjgaIm0PZzbK3wIiZOwscsBYpk6GVkYrk+of9HUnuE=; b=DKqoO9qJknLAo80SwMuiFvLKqBWwEZ3L8yXKnjUGhpvsCjGC4DXv1a0gK99ZIXP77K vVJBXbjEeSryFvMzZ517g75TEOatRbLjQUlttlB2q8mJQ86fPUmR9CKmay6UI5dNurSy Oli9gy6xmH+c6rxnH54ST74tJORob1ZypKRuKH1v8zHDHgdV1vB2lSynlEwss/8BW6YL 885pQlID5/H6uaHbqwRaLNzOfDmIuGGQfAOvWmQr0KWC8K3N0s5FwrWgubWgQDbjewT3 biopt98rGYhnyPt+Cr4yhZurs2Ww+U/yuA42Nr/Ku1N0cGYA+gVey0xMCB7CGc/QHDBX MfSw== X-Gm-Message-State: APjAAAXtp6B0QED1Fx0bvioVbYDbsydtQoxD80ELUPOsbVULAJwsMnPZ UX4tCNPYOFYrTxg9FSElad4= X-Google-Smtp-Source: APXvYqwTDBeMU9bex7EC9J2qgZI3mwOZHAK7kU4MAUtTEzLZ3ResL1K/O1WmqMh3GVDlC1Xi6f60vw== X-Received: by 2002:a5e:9b0b:: with SMTP id j11mr9716143iok.65.1552063416699; Fri, 08 Mar 2019 08:43:36 -0800 (PST) Received: from egauge.net (c-73-181-115-211.hsd1.co.comcast.net. [73.181.115.211]) by smtp.gmail.com with ESMTPSA id 10sm4173524itv.10.2019.03.08.08.43.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Mar 2019 08:43:36 -0800 (PST) From: egaugesystems@gmail.com X-Google-Original-From: davidm@egauge.net Received: by egauge.net (Postfix, from userid 1000) id 1DC0A7003A9; Fri, 8 Mar 2019 09:43:35 -0700 (MST) To: johan@kernel.org Subject: drivers: usb: serial: ftdi_sio.c error flagging Cc: linux-serial@vger.kernel.org, linux-serial@vger.kernel.org, linux-usb@vger.kernel.org Message-Id: <20190308164335.1DC0A7003A9@egauge.net> Date: Fri, 8 Mar 2019 09:43:35 -0700 (MST) Sender: linux-usb-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP [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 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++) {