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: <fcfa2fec-7267-4d16-9f01-898b4223313d@kernel.org>
Date: Mon, 4 Nov 2024 07:44:33 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: John Ogness <john.ogness@...utronix.de>,
 "Maciej W. Rozycki" <macro@...am.me.uk>
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

On 31. 10. 24, 9:49, John Ogness wrote:
>>>> +	/* 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 you wait for THRE, so effectively waiting for FIFO to flush. The 
difference is only one byte (TSR), or what am I missing?

>>> 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.

For the first loop, that's all right. But why would you want to wait for 
the FIFO to flush at the end of the function? It's not only the last 
byte, it's the last batch (aka 'tx_count'), right?

> 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.

OK, let's see then :).

thanks,
-- 
js
suse labs