Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner <lukas@wunner.de>]

On Sun, Nov 21, 2021 at 10:00:51AM +0100, Jan Kiszka wrote:
> Meanwhile reproduced myself, and now I believe your patch is broken in
> ignoring the internal call path to serial8250_set_mctrl, coming from
> uart_port_dtr_rts:
[...]
> This case is not triggered by userspace setting a custom RTS value but
> by the uart-internal machinery selecting it based on the rs485 mode,
> among other things. That path must not be intercepted and made
> conditional using the current MCR state but has to write the request
> value *as is*.

Thanks for the analysis and sorry for the breakage.  I'm proposing the
fix below.  Let me know if that works for you.

However I believe that omap_8250_startup() should be amended to not set
up->mcr = 0 unconditionally.  Rather, it should set the RTS bit if rs485
is enabled and RTS polarity is inverted (as seems to be the case on your
product).  Right now, even with the fix below you'll see a brief glitch
wherein RTS is asserted (so the transceiver's driver is enabled) and
immediately deasserted when opening the port.  This may disturb the
communication of other devices on the bus.  Do you agree?  If so, I can
prepare a separate fix for that.  Note that we may have never noticed
that without f45709df7731, so... ;)

Thanks,

Lukas

-- >8 --
Subject: [PATCH] serial: 8250: Fix RTS modem control while in rs485 mode

Commit f45709df7731 ("serial: 8250: Don't touch RTS modem control while
in rs485 mode") sought to prevent user space from interfering with rs485
communication by ignoring a TIOCMSET ioctl() which changes RTS polarity.

It did so in serial8250_do_set_mctrl(), which turns out to be too deep
in the call stack:  When a uart_port is opened, RTS polarity is set by
the rs485-aware function uart_port_dtr_rts().  It calls down to
serial8250_do_set_mctrl() and that particular RTS polarity change should
*not* be ignored.

The user-visible result is that on 8250_omap ports which use rs485 with
inverse polarity (RTS bit in MCR register is 1 to receive, 0 to send),
a newly opened port initially sets up RTS for sending instead of
receiving.  That's because omap_8250_startup() sets the cached value
up->mcr to 0 and omap_8250_restore_regs() subsequently writes it to the
MCR register.  Due to the commit, serial8250_do_set_mctrl() preserves
that incorrect register value:

do_sys_openat2
  do_filp_open
    path_openat
      vfs_open
        do_dentry_open
	  chrdev_open
	    tty_open
	      uart_open
	        tty_port_open
		  uart_port_activate
		    uart_startup
		      uart_port_startup
		        serial8250_startup
			  omap_8250_startup # up->mcr = 0
			uart_change_speed
			  serial8250_set_termios
			    omap_8250_set_termios
			      omap_8250_restore_regs
			        serial8250_out_MCR # up->mcr written
		  tty_port_block_til_ready
		    uart_dtr_rts
		      uart_port_dtr_rts
		        serial8250_set_mctrl
			  omap8250_set_mctrl
			    serial8250_do_set_mctrl # mcr[1] = 1 ignored

Fix by intercepting RTS changes from user space in uart_tiocmset()
instead.

Fixes: f45709df7731 ("serial: 8250: Don't touch RTS modem control while in rs485 mode")
Reported-by: Su Bao Cheng <baocheng.su@siemens.com>
Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chao Zeng <chao.zeng@siemens.com>
Cc: stable@vger.kernel.org # v5.7+
---
 drivers/tty/serial/8250/8250_port.c | 7 -------
 drivers/tty/serial/serial_core.c    | 5 +++++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 5775cbff8f6e..46e2079ad1aa 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2024,13 +2024,6 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned char mcr;
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
-		if (serial8250_in_MCR(up) & UART_MCR_RTS)
-			mctrl |= TIOCM_RTS;
-		else
-			mctrl &= ~TIOCM_RTS;
-	}
-
 	mcr = serial8250_TIOCM_to_MCR(mctrl);
 
 	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 1e738f265eea..6a38e9d7b87a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1075,6 +1075,11 @@ uart_tiocmset(struct tty_struct *tty, unsigned int set, unsigned int clear)
 		goto out;
 
 	if (!tty_io_error(tty)) {
+		if (uport->rs485.flags & SER_RS485_ENABLED) {
+			set &= ~TIOCM_RTS;
+			clear &= ~TIOCM_RTS;
+		}
+
 		uart_update_mctrl(uport, set, clear);
 		ret = 0;
 	}
-- 
2.33.0