From: Su Bao Cheng <baocheng_su@163.com>
To: Lukas Wunner <lukas@wunner.de>, Jan Kiszka <jan.kiszka@siemens.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Su Bao Cheng <baocheng.su@siemens.com>,
linux-serial@vger.kernel.org, chao.zeng@siemens.com
Subject: Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"
Date: Mon, 22 Nov 2021 17:01:07 +0800 [thread overview]
Message-ID: <10469f11-aefc-3b45-b7e1-516c918e4dc2@163.com> (raw)
In-Reply-To: <20211121174324.GA17258@wunner.de>
On 11/22/21 1:43 AM, Lukas Wunner wrote:
> 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
>
The new patch works on our setup.
Thanks,
Baocheng Su
> -- >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;
> }
>
next prev parent reply other threads:[~2021-11-22 9:01 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-27 11:16 [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode" Su Bao Cheng
2021-10-27 11:39 ` Lukas Wunner
2021-11-12 6:14 ` Su Bao Cheng
2021-11-19 8:00 ` Jan Kiszka
2021-11-19 8:43 ` Jan Kiszka
2021-11-19 11:17 ` Lukas Wunner
2021-11-19 11:12 ` Lukas Wunner
2021-11-20 17:18 ` Lukas Wunner
2021-11-21 9:00 ` Jan Kiszka
2021-11-21 17:43 ` Lukas Wunner
2021-11-22 9:01 ` Su Bao Cheng [this message]
2021-11-22 17:11 ` Lukas Wunner
2021-12-13 16:12 ` Lukas Wunner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10469f11-aefc-3b45-b7e1-516c918e4dc2@163.com \
--to=baocheng_su@163.com \
--cc=baocheng.su@siemens.com \
--cc=chao.zeng@siemens.com \
--cc=gregkh@linuxfoundation.org \
--cc=jan.kiszka@siemens.com \
--cc=linux-serial@vger.kernel.org \
--cc=lukas@wunner.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).