linux-kernel - Re: [PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start

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: <84ldzproiy.fsf@jogness.linutronix.de>
Date: Wed, 18 Sep 2024 17:10:53 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby
 <jirislaby@...nel.org>, Sergey Senozhatsky <senozhatsky@...omium.org>,
 Steven Rostedt <rostedt@...dmis.org>, Thomas Gleixner
 <tglx@...utronix.de>, Esben Haabendal <esben@...nix.com>,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org, Andy
 Shevchenko <andriy.shevchenko@...ux.intel.com>, Sunil V L
 <sunilvl@...tanamicro.com>, Arnd Bergmann <arnd@...db.de>, Florian
 Fainelli <f.fainelli@...il.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, Lino Sanfilippo
 <l.sanfilippo@...bus.com>, Rengarajan S <rengarajan.s@...rochip.com>,
 Serge Semin <fancer.lancer@...il.com>
Subject: Re: [PATCH next v2 1/4] serial: 8250: Split out IER from
 rs485_start_tx()

On 2024-09-17, Petr Mladek <pmladek@...e.com> wrote:
> Sigh, I am trying to review this patch but I am not familiar with the
> code. Feel free to ignore me when the questions are completely off.

I appreciate you researching where the code came from. I made my changes
based on what I see the code doing now.

>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>>  void serial8250_em485_start_tx(struct uart_8250_port *up)
>>  {
>>  	unsigned char mcr = serial8250_in_MCR(up);
>>  
>> +	/*
>> +	 * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
>> +	 * disabled, so explicitly mask it.
>> +	 */
>>  	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> -		serial8250_stop_rx(&up->port);
>> +		up->port.read_status_mask &= ~UART_LSR_DR;
>
> This change is related to disabling UART_IER_RDI but we do not longer
> disable it in this code path.

Correct. It will be disabled in the new wrapper
serial8250_em485_start_tx(). For the console write() callback, RDI is
already being disabled (IER is cleared). It will not use the wrapper.

> Why do we need to do it here, please?

Because the console write() callback also needs to clear LSR_DR. That
part of the callback needs to stay.

> Why is it needed only in the em485-specific path, please?

Only RS485 deals with controlling TX/RX directions.

> On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
> so seems to be relater.

I do not know if the LSR_DR modify is strictly necessary. I am just
preserving the existing behavior (and related comment). The disabling of
IER_RDI will still happen (via wrapper or explicitly as in the console
write() callback).

>>  static bool start_tx_rs485(struct uart_port *port)
>>  {
>> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
>>  	if (em485->tx_stopped) {
>>  		em485->tx_stopped = false;
>>  
>> -		up->rs485_start_tx(up);
>> +		serial8250_rs485_start_tx(up);
>
> If I get this correctly then this keeps the existing behavior when
>
>     up->rs485_start_tx == serial8250_em485_start_tx

Correct.

> Is this always the case, please?

Yes.

> Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?

Yes.

> Will it still work as expected?

Yes, but it does perform an extra read. And since someone added a
comment just to mention that, I assume it was important for some use
case.

John