linux-kernel - Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in pl011_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220102182801.GA22268@wunner.de>
Date:   Sun, 2 Jan 2022 19:28:01 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Lino Sanfilippo <LinoSanfilippo@....de>
Cc:     jmades <jochen@...es.net>, gregkh@...uxfoundation.org,
        Russell King <linux@...linux.org.uk>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Philipp Rosenberger <p.rosenberger@...bus.com>
Subject: Re: [PATCH] Bugfix RTS line config in RS485 mode is overwritten in
 pl011_set_mctrl() function.

On Sun, Jan 02, 2022 at 04:06:53PM +0100, Lino Sanfilippo wrote:
> On 02.01.22 at 11:07, Lukas Wunner wrote:
> > On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote:
> > > --- a/drivers/tty/serial/amba-pl011.c
> > > +++ b/drivers/tty/serial/amba-pl011.c
> > > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > >  	    container_of(port, struct uart_amba_port, port);
> > >  	unsigned int cr;
> > >
> > > -	if (port->rs485.flags & SER_RS485_ENABLED)
> > > -		mctrl &= ~TIOCM_RTS;
> > > +	if (port->rs485.flags & SER_RS485_ENABLED) {
> > > +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > > +			mctrl &= ~TIOCM_RTS;
> > > +		else
> > > +			mctrl |= TIOCM_RTS;
> > > +	}
> > >
> > >  	cr = pl011_read(uap, REG_CR);
> 
> Does this logic really have to be implemented in the driver?

No, it doesn't have to be and indeed I'm working towards consolidating
it in the serial core with this collection of patches:

https://git.kernel.org/gregkh/tty/c/d3b3404df318
https://lore.kernel.org/all/f49f945375f5ccb979893c49f1129f51651ac738.1641129062.git.lukas@wunner.de
https://lore.kernel.org/all/e22089ab49e6e78822c50c8c4db46bf3ee885623.1641129328.git.lukas@wunner.de
https://lore.kernel.org/all/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de
https://github.com/l1k/linux/commit/532ef2ad757f

The last of these removes the rs485 logic from pl011_set_mctrl().
I'll post it once the others (and Jochen Mades' patch) have landed.

Even though the logic is eventually removed from pl011_set_mctrl(),
Jochen's patch makes sense as a backportable fix for v5.15.

> It looks as if the serial core already takes RS485 into account before
> calling set_mctrls(). At least I get the impression when looking
> at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx
> simply seem to ignore RTS in case of RS485.

The logic in uart_port_dtr_rts() is broken.  That's fixed by d3b3404df318,
which is queued up in tty-next for v5.17.

The pl011 driver papered over it with its own rs485-specific logic in
pl011_set_mctrl().  But as Jochen Mades correctly pointed out, that
only worked correctly if RTS is driven high on idle.

The logic in uart_tiocmset() is correct, but not sufficient because
uart_throttle(), uart_unthrottle and uart_set_termios() need to become
rs485-aware as well.  That's also addressed by the above-linked
GitHub commit.

Thanks,

Lukas