linux-kernel - Re: [PATCH] tty: serial: Use fifo in 8250 console driver

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: <8e57400f-d6a8-bd42-6214-fca1fe37a972@kernel.org>
Date:   Tue, 25 Jan 2022 10:36:27 +0100
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Jon Hunter <jonathanh@...dia.com>, wander@...hat.com
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Maciej W. Rozycki" <macro@...am.me.uk>,
        Johan Hovold <johan@...nel.org>,
        Andrew Jeffery <andrew@...id.au>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] tty: serial: Use fifo in 8250 console driver

On 25. 01. 22, 10:08, Jiri Slaby wrote:
> On 25. 01. 22, 9:39, Jon Hunter wrote:
>>
>> On 29/10/2021 21:14, wander@...hat.com wrote:
>>> From: Wander Lairson Costa <wander@...hat.com>
>>>
>>> Note: I am using a small test app + driver located at [0] for the
>>> problem description. serco is a driver whose write function dispatches
>>> to the serial controller. sertest is a user-mode app that writes n bytes
>>> to the serial console using the serco driver.
...
>>> Signed-off-by: Wander Lairson Costa <wander@...hat.com>
>>
>>
>> On the current mainline and -next branches, I have noticed that the
>> serial output on many of our Tegra boards is corrupted and so
>> parsing the serial output is failing.
>>
>> Before this change the serial console would appear as follows ...
>>
>> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x411fd071]
>> [    0.000000] Linux version 5.16.0-rc6-00091-gadbfddc757ae 
>> (jonathanh@...athanh-vm-01) (aarch64-linux-gnu-gcc (Linaro GCC 
>> 6.4-2017.08) 6.4.1 20170707, GNU ld (Linaro_Binutils-2017.08) 
>> 2.27.0.20161019) #15 SMP PREEMPT Tue Jan 25 00:15:25 PST 2022
>> [    0.000000] Machine model: NVIDIA Jetson TX1 Developer Kit
>>
>> And now I see ...
>>
>> [    0.000000] Booting Linux on physicalfd071]
>> [    0.000000] Linux version 5.16.0-rc6-athanh@...inux-g017.08) 
>> Linaro_B20161019n 25 00:[    0.000000] Machine model: NVIDIA Jet[ 
>> 0.000000] efi: UEFI not found.
>> [    0.000000] NUMA: No NUMA configurati[    0.000000] NUMA: Faking a 
>> node at [m00000001[    0.000000] NUMA: NODE_DATA [mem 0x17[    
>> 0.000000] Zone ranges:
>>
>> Bisecting is pointing to this commit. Let me know if there are any
>> tests I can run. Otherwise we may need to disable this at least
>> for Tegra.
> 
> 
> The test is bogus:
>          use_fifo = (up->capabilities & UART_CAP_FIFO) &&
>                  port->fifosize > 1 &&
>                  (serial_port_in(port, UART_FCR) & UART_FCR_ENABLE_FIFO)
> 
> FCR is write only. Reading it, one gets IIR contents.

In particular, the test is checking whether there is no interrupt 
pending (UART_FCR_ENABLE_FIFO == UART_IIR_NO_INT). So it oscillates 
between use_fifo and not, depending on the interrupt state of the chip.

Could you change it into something like this:
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3396,7 +3396,7 @@ void serial8250_console_write(struct 
uart_8250_port *up, const char *s,

         use_fifo = (up->capabilities & UART_CAP_FIFO) &&
                 port->fifosize > 1 &&
-               (serial_port_in(port, UART_FCR) & UART_FCR_ENABLE_FIFO) &&
+               (up->fcr & UART_FCR_ENABLE_FIFO) &&
                 /*
                  * After we put a data in the fifo, the controller will 
send
                  * it regardless of the CTS state. Therefore, only use fifo


And see whether it fixes the issue. Anyway, of what port type is the 
serial port (what says dmesg/setserial about that)?

thanks,
-- 
js
suse labs