linux-kernel - Re: [PATCH] tty: serial: atmel: Preserve previous USART mode if RS485 disabled

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: <2cab23a2-7a82-9321-dca1-5c7c71484ad@linux.intel.com>
Date:   Wed, 24 Aug 2022 15:02:30 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Sergiu Moga <sergiu.moga@...rochip.com>
cc:     richard.genoud@...il.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>, nicolas.ferre@...rochip.com,
        alexandre.belloni@...tlin.com, claudiu.beznea@...rochip.com,
        rmk+kernel@....linux.org.uk, claudio@...dence.eu.com, rick@....org,
        michael@...dence.eu.com, ryan@...ewatersys.com,
        linux-serial <linux-serial@...r.kernel.org>,
        linux-arm-kernel@...ts.infradead.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tty: serial: atmel: Preserve previous USART mode if
 RS485 disabled

On Wed, 24 Aug 2022, Sergiu Moga wrote:

> Whenever the atmel_rs485_config driver method would be called,
> the USART mode is reset to normal mode before even checking if
> RS485 flag is set, thus resulting in losing the previous USART
> mode in the case where the checking fails. Some tools, such as
> `linux-serial-test`, lead to the driver calling this method
> when doing the setup of the serial port: after setting the port
> mode (Hardware Flow Control, Normal Mode, RS485 Mode, etc.),
> `linux-serial-test` tries to enable/disable RS485 depending on
> the commandline arguments passed. If we were to, for example, enable
> Hardware Flow Control through `linux-serial-test`, the tool would
> make the driver set the corresponding bit to 1 (ATMEL_US_USMODE_HWHS
> bit in the ATMEL_US_MR register) through the atmel_set_termios method
> and then proceed to disabling RS485. This, in turn, causes the
> ATMEL_US_USMODE_HWHS bit of the ATMEL_US_MR mode register to be unset
> and, if the checking for RS485 fails, leads to having the mode set
> back to the ATMEL_US_USMODE_NORMAL normal mode. Since in hardware
> flow control mode the meanings of the ATMEL_US_RTSDIS and
> ATMEL_US_RTSEN bits are swapped, this leads to our endpoint leaving
> the RTS line to high when wanting to receive, which is the opposite
> of what the other endpoint is expecting in order to start transmitting.
> This fix ensures that this reset is done only if the checking for RS485
> succeeds.

Could you please try to split this long paragraph to a slightly shorter 
bits such that it would be easier to read.

> Fixes: e8faff7330a35 ("ARM: 6092/1: atmel_serial: support for RS485 communications")
> Signed-off-by: Sergiu Moga <sergiu.moga@...rochip.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 0a0b46ee0955..c29b1fb48694 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -298,9 +298,6 @@ static int atmel_config_rs485(struct uart_port *port, struct ktermios *termios,
>  
>  	mode = atmel_uart_readl(port, ATMEL_US_MR);
>  
> -	/* Resetting serial mode to RS232 (0x0) */
> -	mode &= ~ATMEL_US_USMODE;
> -
>  	if (rs485conf->flags & SER_RS485_ENABLED) {
>  		dev_dbg(port->dev, "Setting UART to RS485\n");
>  		if (rs485conf->flags & SER_RS485_RX_DURING_TX)
> @@ -310,6 +307,7 @@ static int atmel_config_rs485(struct uart_port *port, struct ktermios *termios,
>  
>  		atmel_uart_writel(port, ATMEL_US_TTGR,
>  				  rs485conf->delay_rts_after_send);
> +		mode &= ~ATMEL_US_USMODE;
>  		mode |= ATMEL_US_USMODE_RS485;
>  	} else {
>  		dev_dbg(port->dev, "Setting UART to RS232\n");
> 

Makes sense.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

Unrelated to this patch but I came across it while reviewing yours... Do 
you BTW have any idea why atmel_serial_probe() sets ATMEL_US_USMODE_NORMAL 
inside rs485_enabled block? I'd have expected it wanted to do 
ATMEL_US_USMODE_RS485 there too like is done in atmel_config_rs485().


-- 
 i.