linux-kernel - Re: [PATCH tty-next v3 1/6] serial: 8250: Adjust the timeout for FIFO mode

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: <84sesclkqx.fsf@jogness.linutronix.de>
Date: Thu, 31 Oct 2024 09:55:58 +0106
From: John Ogness <john.ogness@...utronix.de>
To: "Maciej W. Rozycki" <macro@...am.me.uk>, Jiri Slaby <jirislaby@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Petr Mladek
 <pmladek@...e.com>, 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>, Rengarajan S
 <rengarajan.s@...rochip.com>, Jeff Johnson <quic_jjohnson@...cinc.com>,
 Serge Semin <fancer.lancer@...il.com>, Lino Sanfilippo
 <l.sanfilippo@...bus.com>, Wander Lairson Costa <wander@...hat.com>
Subject: Re: [PATCH tty-next v3 1/6] serial: 8250: Adjust the timeout for
 FIFO mode

Hi Maciej,

Thanks for jumping in with some ref-manual quotes. Some more comments
from me below...

On 2024-10-31, "Maciej W. Rozycki" <macro@...am.me.uk> wrote:
> On Wed, 30 Oct 2024, Jiri Slaby wrote:
>> > @@ -3306,13 +3310,18 @@ static void serial8250_console_restore(struct
>> > uart_8250_port *up)
>> >   static void serial8250_console_fifo_write(struct uart_8250_port *up,
>> >   					  const char *s, unsigned int count)
>> >   {
>> > -	int i;
>> >   	const char *end = s + count;
>> >   	unsigned int fifosize = up->tx_loadsz;
>> > +	unsigned int tx_count = 0;
>> >   	bool cr_sent = false;
>> > +	unsigned int i;
>> >     	while (s != end) {
>> > -		wait_for_lsr(up, UART_LSR_THRE);
>> > +		/* Allow timeout for each byte of a possibly full FIFO. */
>> > +		for (i = 0; i < fifosize; i++) {
>> > +			if (wait_for_lsr(up, UART_LSR_THRE))
>> > +				break;
>> > +		}
>> 
>> THRE only signals there is a space for one character.
>
>  Nope[1]:
>
> "In the FIFO mode, THRE is set when the transmit FIFO is empty; it is 
> cleared when at least one byte is written to the transmit FIFO."
>
> It seems common enough a misconception that once I actually had to fix the 
> bad interpretation of THRE in an unpublished platform driver to get decent 
> performance out of it at higher rates such as 230400bps, as it only pushed 
> one byte at a time to the FIFO while it had it all available once THRE has 
> been set.

I do not know if this is true for all 8250-variants. If there is some
variant where it functions as Jiri expected, then it would mean
significant text loss during longer messages. But that would already be
a problem in the current mainline driver.

>> > +	/* Allow timeout for each byte written. */
>> > +	for (i = 0; i < tx_count; i++) {
>> > +		if (wait_for_lsr(up, UART_LSR_THRE))
>> 
>> This ensures you sent one character from the FIFO. The FIFO still can contain
>> plenty of them. Did you want UART_LSR_TEMT?
>
>  The difference between THRE and TEMT is the state of the shift register 
> only[2]:
>
> "In the FIFO mode, TEMT is set when the transmitter FIFO and shift 
> register are both empty."

If we wait for TEMT, we lose significant advantages of having the FIFO.

>> But what's the purpose of spinning _here_? The kernel can run and FIFO
>> too. Without the kernel waiting for the FIFO.

When serial8250_console_fifo_write() exits, the caller just does a
single wait_for_xmitr() ... with a 10ms timeout. In the FIFO case, for
<=56k baudrates, it can easily hit the timeout and thus continue before
the FIFO has been emptied.

By waiting on UART_LSR_THRE after filling the FIFO,
serial8250_console_fifo_write() waits until the hardware has had a
chance to shift out all the data. Then the final wait_for_xmitr() in the
caller only waits for the final byte to go out on the line.

Please keep in mind that none of these timeouts should trigger during
normal operation.

For v4 I am doing some refactoring (as suggested by Andy) so that the
wait-code looks a bit cleaner.

John

> References:
>
> [1] "TL16C550C, TL16C550CI Asynchronous Communications Element with 
>     Autoflow Control", Texas Instruments, SLLS177F -- March 1994 -- 
>     Revised March 2001, p. 30
>
> [2] same