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: <2fab2ef8-d0d6-4b94-90b6-7c16641a2f68@kernel.org>
Date: Mon, 4 Nov 2024 07:34:26 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: John Ogness <john.ogness@...utronix.de>,
 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, 5:44, Maciej W. Rozycki 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."

Hmm, I was confused by NXP's 16c650b [1] datasheet then (or I cannot parse):
===
The THR empty flag in the LSR register will be set to a logic 1 when the 
transmitter is empty or when data is transferred to the TSR. Note that a 
write operation can be performed when the THR empty flag is set
(logic 0 = FIFO full; logic 1 = at least one FIFO location available).
===

But indeed in the LSR[5] bit description, they state:
===
In the FIFO mode, this bit is set when the transmit FIFO is
empty; it is cleared when at least 1 byte is written to the transmit FIFO.
===

Anyway, it still does not answer the original question: Instead of 
looping fifosize multiplied by random timeout, can we re-use 
port->frame_time?

[1] SC16C650B -- 5 V, 3.3 V and 2.5 V UART with 32-byte FIFOs and 
infrared (IrDA) encoder/decoder; Rev. 04 — 14 September 2009; Product 
data sheet

>>> +	/* 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."

Sure. The question still holds:

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

If we want to wait for fifo to empty, why not *also* the TSR. Meaning:

 > Did you want UART_LSR_TEMT?

thanks,
-- 
js
suse labs