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

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

[Jon Hunter]

On 29/10/2021 21:14, wander@redhat.com wrote:
> From: Wander Lairson Costa <wander@redhat.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.
> 
> While investigating a bug in the RHEL kernel, I noticed that the serial
> console throughput is way below the configured speed of 115200 bps in
> a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but
> I got 2.5KB/s.
> 
> $ time ./sertest -n 2500 /tmp/serco
> 
> real    0m0.997s
> user    0m0.000s
> sys     0m0.997s
> 
> With the help of the function tracer, I then noticed the serial
> controller was taking around 410us seconds to dispatch one single byte:
> 
> $ trace-cmd record -p function_graph -g serial8250_console_write \
>     ./sertest -n 1 /tmp/serco
> 
> $ trace-cmd report
> 
>              |  serial8250_console_write() {
>   0.384 us   |    _raw_spin_lock_irqsave();
>   1.836 us   |    io_serial_in();
>   1.667 us   |    io_serial_out();
>              |    uart_console_write() {
>              |      serial8250_console_putchar() {
>              |        wait_for_xmitr() {
>   1.870 us   |          io_serial_in();
>   2.238 us   |        }
>   1.737 us   |        io_serial_out();
>   4.318 us   |      }
>   4.675 us   |    }
>              |    wait_for_xmitr() {
>   1.635 us   |      io_serial_in();
>              |      __const_udelay() {
>   1.125 us   |        delay_tsc();
>   1.429 us   |      }
> ...
> ...
> ...
>   1.683 us   |      io_serial_in();
>              |      __const_udelay() {
>   1.248 us   |        delay_tsc();
>   1.486 us   |      }
>   1.671 us   |      io_serial_in();
>   411.342 us |    }
> 
> In another machine, I measured a throughput of 11.5KB/s, with the serial
> controller taking between 80-90us to send each byte. That matches the
> expected throughput for a configuration of 115200 bps.
> 
> This patch changes the serial8250_console_write to use the 16550 fifo
> if available. In my benchmarks I got around 25% improvement in the slow
> machine, and no performance penalty in the fast machine.
> 
> Signed-off-by: Wander Lairson Costa <wander@redhat.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@jonathanh-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@j-linux-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.

Cheers
Jon

-- 
nvpublic

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

[Greg Kroah-Hartman]

On Tue, Jan 25, 2022 at 08:39:24AM +0000, Jon Hunter wrote:
> 
> On 29/10/2021 21:14, wander@redhat.com wrote:
> > From: Wander Lairson Costa <wander@redhat.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.
> > 
> > While investigating a bug in the RHEL kernel, I noticed that the serial
> > console throughput is way below the configured speed of 115200 bps in
> > a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but
> > I got 2.5KB/s.
> > 
> > $ time ./sertest -n 2500 /tmp/serco
> > 
> > real    0m0.997s
> > user    0m0.000s
> > sys     0m0.997s
> > 
> > With the help of the function tracer, I then noticed the serial
> > controller was taking around 410us seconds to dispatch one single byte:
> > 
> > $ trace-cmd record -p function_graph -g serial8250_console_write \
> >     ./sertest -n 1 /tmp/serco
> > 
> > $ trace-cmd report
> > 
> >              |  serial8250_console_write() {
> >   0.384 us   |    _raw_spin_lock_irqsave();
> >   1.836 us   |    io_serial_in();
> >   1.667 us   |    io_serial_out();
> >              |    uart_console_write() {
> >              |      serial8250_console_putchar() {
> >              |        wait_for_xmitr() {
> >   1.870 us   |          io_serial_in();
> >   2.238 us   |        }
> >   1.737 us   |        io_serial_out();
> >   4.318 us   |      }
> >   4.675 us   |    }
> >              |    wait_for_xmitr() {
> >   1.635 us   |      io_serial_in();
> >              |      __const_udelay() {
> >   1.125 us   |        delay_tsc();
> >   1.429 us   |      }
> > ...
> > ...
> > ...
> >   1.683 us   |      io_serial_in();
> >              |      __const_udelay() {
> >   1.248 us   |        delay_tsc();
> >   1.486 us   |      }
> >   1.671 us   |      io_serial_in();
> >   411.342 us |    }
> > 
> > In another machine, I measured a throughput of 11.5KB/s, with the serial
> > controller taking between 80-90us to send each byte. That matches the
> > expected throughput for a configuration of 115200 bps.
> > 
> > This patch changes the serial8250_console_write to use the 16550 fifo
> > if available. In my benchmarks I got around 25% improvement in the slow
> > machine, and no performance penalty in the fast machine.
> > 
> > Signed-off-by: Wander Lairson Costa <wander@redhat.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@jonathanh-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@j-linux-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.

Ick.  Does this uart have any other quirks assigned to it that are
somehow not getting assigned here?

thanks,

greg k-h

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

[Jon Hunter]

On 25/01/2022 08:50, Greg Kroah-Hartman wrote:
> On Tue, Jan 25, 2022 at 08:39:24AM +0000, Jon Hunter wrote:
>>
>> On 29/10/2021 21:14, wander@redhat.com wrote:
>>> From: Wander Lairson Costa <wander@redhat.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.
>>>
>>> While investigating a bug in the RHEL kernel, I noticed that the serial
>>> console throughput is way below the configured speed of 115200 bps in
>>> a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but
>>> I got 2.5KB/s.
>>>
>>> $ time ./sertest -n 2500 /tmp/serco
>>>
>>> real    0m0.997s
>>> user    0m0.000s
>>> sys     0m0.997s
>>>
>>> With the help of the function tracer, I then noticed the serial
>>> controller was taking around 410us seconds to dispatch one single byte:
>>>
>>> $ trace-cmd record -p function_graph -g serial8250_console_write \
>>>      ./sertest -n 1 /tmp/serco
>>>
>>> $ trace-cmd report
>>>
>>>               |  serial8250_console_write() {
>>>    0.384 us   |    _raw_spin_lock_irqsave();
>>>    1.836 us   |    io_serial_in();
>>>    1.667 us   |    io_serial_out();
>>>               |    uart_console_write() {
>>>               |      serial8250_console_putchar() {
>>>               |        wait_for_xmitr() {
>>>    1.870 us   |          io_serial_in();
>>>    2.238 us   |        }
>>>    1.737 us   |        io_serial_out();
>>>    4.318 us   |      }
>>>    4.675 us   |    }
>>>               |    wait_for_xmitr() {
>>>    1.635 us   |      io_serial_in();
>>>               |      __const_udelay() {
>>>    1.125 us   |        delay_tsc();
>>>    1.429 us   |      }
>>> ...
>>> ...
>>> ...
>>>    1.683 us   |      io_serial_in();
>>>               |      __const_udelay() {
>>>    1.248 us   |        delay_tsc();
>>>    1.486 us   |      }
>>>    1.671 us   |      io_serial_in();
>>>    411.342 us |    }
>>>
>>> In another machine, I measured a throughput of 11.5KB/s, with the serial
>>> controller taking between 80-90us to send each byte. That matches the
>>> expected throughput for a configuration of 115200 bps.
>>>
>>> This patch changes the serial8250_console_write to use the 16550 fifo
>>> if available. In my benchmarks I got around 25% improvement in the slow
>>> machine, and no performance penalty in the fast machine.
>>>
>>> Signed-off-by: Wander Lairson Costa <wander@redhat.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@jonathanh-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@j-linux-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.
> 
> Ick.  Does this uart have any other quirks assigned to it that are
> somehow not getting assigned here?


Not that I know of, but I can have a look. I did check to see if there 
are any known issues that could be related but I have not found any so far.

Jon

-- 
nvpublic

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

[Jiri Slaby]

On 25. 01. 22, 9:39, Jon Hunter wrote:
> 
> On 29/10/2021 21:14, wander@redhat.com wrote:
>> From: Wander Lairson Costa <wander@redhat.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.
>>
>> While investigating a bug in the RHEL kernel, I noticed that the serial
>> console throughput is way below the configured speed of 115200 bps in
>> a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but
>> I got 2.5KB/s.
>>
>> $ time ./sertest -n 2500 /tmp/serco
>>
>> real    0m0.997s
>> user    0m0.000s
>> sys     0m0.997s
>>
>> With the help of the function tracer, I then noticed the serial
>> controller was taking around 410us seconds to dispatch one single byte:
>>
>> $ trace-cmd record -p function_graph -g serial8250_console_write \
>>     ./sertest -n 1 /tmp/serco
>>
>> $ trace-cmd report
>>
>>              |  serial8250_console_write() {
>>   0.384 us   |    _raw_spin_lock_irqsave();
>>   1.836 us   |    io_serial_in();
>>   1.667 us   |    io_serial_out();
>>              |    uart_console_write() {
>>              |      serial8250_console_putchar() {
>>              |        wait_for_xmitr() {
>>   1.870 us   |          io_serial_in();
>>   2.238 us   |        }
>>   1.737 us   |        io_serial_out();
>>   4.318 us   |      }
>>   4.675 us   |    }
>>              |    wait_for_xmitr() {
>>   1.635 us   |      io_serial_in();
>>              |      __const_udelay() {
>>   1.125 us   |        delay_tsc();
>>   1.429 us   |      }
>> ...
>> ...
>> ...
>>   1.683 us   |      io_serial_in();
>>              |      __const_udelay() {
>>   1.248 us   |        delay_tsc();
>>   1.486 us   |      }
>>   1.671 us   |      io_serial_in();
>>   411.342 us |    }
>>
>> In another machine, I measured a throughput of 11.5KB/s, with the serial
>> controller taking between 80-90us to send each byte. That matches the
>> expected throughput for a configuration of 115200 bps.
>>
>> This patch changes the serial8250_console_write to use the 16550 fifo
>> if available. In my benchmarks I got around 25% improvement in the slow
>> machine, and no performance penalty in the fast machine.
>>
>> Signed-off-by: Wander Lairson Costa <wander@redhat.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@jonathanh-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@j-linux-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.

regards,
-- 
js
suse labs

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

[Jiri Slaby]

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@redhat.com wrote:
>>> From: Wander Lairson Costa <wander@redhat.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@redhat.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@jonathanh-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@j-linux-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

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

[Jon Hunter]

On 25/01/2022 09:36, Jiri Slaby wrote:

...

>> 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. Unfortunately, this did not fix it. The port type is PORT_TEGRA ...

  70006000.serial: ttyS0 at MMIO 0x70006000 (irq = 72, base_baud = 25500000) is a Tegra

Jon

-- 
nvpublic

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

[Wander Costa]

On Tue, Jan 25, 2022 at 7:06 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 25/01/2022 09:36, Jiri Slaby wrote:
>
> ...
>
> >> 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. Unfortunately, this did not fix it. The port type is PORT_TEGRA ...
>
>   70006000.serial: ttyS0 at MMIO 0x70006000 (irq = 72, base_baud = 25500000) is a Tegra

I see PORT_TEGRA has different values for fifosize and tx_loadsz.
Maybe we should use tx_loadsz.
Could you please give a try to this patch:

diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 2abb3de11a48..d3a93e5d55f7 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3343,7 +3343,7 @@ static void serial8250_console_fifo_write(struct
uart_8250_port *up,
 {
        int i;
        const char *end = s + count;
-       unsigned int fifosize = up->port.fifosize;
+       unsigned int fifosize = up->tx_loadsz;
        bool cr_sent = false;

        while (s != end) {
@@ -3409,8 +3409,8 @@ 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->tx_loadsz > 1 &&
+               (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



>
> Jon
>
> --
> nvpublic
>

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

[Jon Hunter]

On 25/01/2022 10:29, Wander Costa wrote:
> On Tue, Jan 25, 2022 at 7:06 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 25/01/2022 09:36, Jiri Slaby wrote:
>>
>> ...
>>
>>>> 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. Unfortunately, this did not fix it. The port type is PORT_TEGRA ...
>>
>>    70006000.serial: ttyS0 at MMIO 0x70006000 (irq = 72, base_baud = 25500000) is a Tegra
> 
> I see PORT_TEGRA has different values for fifosize and tx_loadsz.
> Maybe we should use tx_loadsz.
> Could you please give a try to this patch:
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index 2abb3de11a48..d3a93e5d55f7 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3343,7 +3343,7 @@ static void serial8250_console_fifo_write(struct
> uart_8250_port *up,
>   {
>          int i;
>          const char *end = s + count;
> -       unsigned int fifosize = up->port.fifosize;
> +       unsigned int fifosize = up->tx_loadsz;
>          bool cr_sent = false;
> 
>          while (s != end) {
> @@ -3409,8 +3409,8 @@ 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->tx_loadsz > 1 &&
> +               (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
> 


Thanks. Yes that does fix it.

Andy, does this work for X86?

Jon

-- 
nvpublic

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

[Andy Shevchenko]

On Tue, Jan 25, 2022 at 12:40:27PM +0000, Jon Hunter wrote:
> 
> On 25/01/2022 10:29, Wander Costa wrote:
> > On Tue, Jan 25, 2022 at 7:06 AM Jon Hunter <jonathanh@nvidia.com> wrote:
> > > 
> > > 
> > > On 25/01/2022 09:36, Jiri Slaby wrote:
> > > 
> > > ...
> > > 
> > > > > 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. Unfortunately, this did not fix it. The port type is PORT_TEGRA ...
> > > 
> > >    70006000.serial: ttyS0 at MMIO 0x70006000 (irq = 72, base_baud = 25500000) is a Tegra
> > 
> > I see PORT_TEGRA has different values for fifosize and tx_loadsz.
> > Maybe we should use tx_loadsz.
> > Could you please give a try to this patch:
> > 
> > diff --git a/drivers/tty/serial/8250/8250_port.c
> > b/drivers/tty/serial/8250/8250_port.c
> > index 2abb3de11a48..d3a93e5d55f7 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -3343,7 +3343,7 @@ static void serial8250_console_fifo_write(struct
> > uart_8250_port *up,
> >   {
> >          int i;
> >          const char *end = s + count;
> > -       unsigned int fifosize = up->port.fifosize;
> > +       unsigned int fifosize = up->tx_loadsz;
> >          bool cr_sent = false;
> > 
> >          while (s != end) {
> > @@ -3409,8 +3409,8 @@ 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->tx_loadsz > 1 &&
> > +               (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
> > 
> 
> 
> Thanks. Yes that does fix it.
> 
> Andy, does this work for X86?

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

No, it does NOT fix an issue (I see it on a handful x86) with the legacy UART
(means the 8250_pnp is in use). And I believe the same will be the case on LPSS
ones (8250_dw / 8250_lpss) and HSU (8250_mid), because the patch influences on
all of them.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 25, 2022 at 06:53:48PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 25, 2022 at 12:40:27PM +0000, Jon Hunter wrote:
> > On 25/01/2022 10:29, Wander Costa wrote:

...

> > Andy, does this work for X86?
> 
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> No, it does NOT fix an issue (I see it on a handful x86) with the legacy UART
> (means the 8250_pnp is in use). And I believe the same will be the case on LPSS
> ones (8250_dw / 8250_lpss) and HSU (8250_mid), because the patch influences on
> all of them.

Shall I send a revert and we can continue with a new approach later on?

-- 
With Best Regards,
Andy Shevchenko

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

[Wander Costa]

On Tue, Jan 25, 2022 at 1:56 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Jan 25, 2022 at 06:53:48PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 25, 2022 at 12:40:27PM +0000, Jon Hunter wrote:
> > > On 25/01/2022 10:29, Wander Costa wrote:
>
> ...
>
> > > Andy, does this work for X86?
> >
> > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > No, it does NOT fix an issue (I see it on a handful x86) with the legacy UART
> > (means the 8250_pnp is in use). And I believe the same will be the case on LPSS
> > ones (8250_dw / 8250_lpss) and HSU (8250_mid), because the patch influences on
> > all of them.
>
> Shall I send a revert and we can continue with a new approach later on?
>

Tomorrow (or maybe after tomorrow) I am going to post the fixes I
already have, and an additional patch adding a build option
(disabled to default) so people maybe if they want to use the FIFO on
console write. But I understand if people decide to go
ahead and revert the patch.

> --
> With Best Regards,
> Andy Shevchenko
>
>

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

[Greg Kroah-Hartman]

On Tue, Jan 25, 2022 at 03:40:36PM -0300, Wander Costa wrote:
> On Tue, Jan 25, 2022 at 1:56 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Jan 25, 2022 at 06:53:48PM +0200, Andy Shevchenko wrote:
> > > On Tue, Jan 25, 2022 at 12:40:27PM +0000, Jon Hunter wrote:
> > > > On 25/01/2022 10:29, Wander Costa wrote:
> >
> > ...
> >
> > > > Andy, does this work for X86?
> > >
> > > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > No, it does NOT fix an issue (I see it on a handful x86) with the legacy UART
> > > (means the 8250_pnp is in use). And I believe the same will be the case on LPSS
> > > ones (8250_dw / 8250_lpss) and HSU (8250_mid), because the patch influences on
> > > all of them.
> >
> > Shall I send a revert and we can continue with a new approach later on?
> >
> 
> Tomorrow (or maybe after tomorrow) I am going to post the fixes I
> already have, and an additional patch adding a build option
> (disabled to default) so people maybe if they want to use the FIFO on
> console write. But I understand if people decide to go
> ahead and revert the patch.

Let me revert this for now.  And no new config options please, this
should "just work".

thanks,

greg k-h

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

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 09:52:26AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 25, 2022 at 03:40:36PM -0300, Wander Costa wrote:
> > On Tue, Jan 25, 2022 at 1:56 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Jan 25, 2022 at 06:53:48PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Jan 25, 2022 at 12:40:27PM +0000, Jon Hunter wrote:
> > > > > On 25/01/2022 10:29, Wander Costa wrote:
> > >
> > > ...
> > >
> > > > > Andy, does this work for X86?
> > > >
> > > > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > >
> > > > No, it does NOT fix an issue (I see it on a handful x86) with the legacy UART
> > > > (means the 8250_pnp is in use). And I believe the same will be the case on LPSS
> > > > ones (8250_dw / 8250_lpss) and HSU (8250_mid), because the patch influences on
> > > > all of them.
> > >
> > > Shall I send a revert and we can continue with a new approach later on?
> > >
> > 
> > Tomorrow (or maybe after tomorrow) I am going to post the fixes I
> > already have, and an additional patch adding a build option
> > (disabled to default) so people maybe if they want to use the FIFO on
> > console write. But I understand if people decide to go
> > ahead and revert the patch.
> 
> Let me revert this for now.  And no new config options please, this
> should "just work".

Thanks!

Wander, if you need a test for something new, I may help to perform on
our (sub)set of x86 machines.

-- 
With Best Regards,
Andy Shevchenko

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

[Wander Costa]

On Wed, Jan 26, 2022 at 9:10 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Jan 26, 2022 at 09:52:26AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 25, 2022 at 03:40:36PM -0300, Wander Costa wrote:
> > > On Tue, Jan 25, 2022 at 1:56 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Tue, Jan 25, 2022 at 06:53:48PM +0200, Andy Shevchenko wrote:
> > > > > On Tue, Jan 25, 2022 at 12:40:27PM +0000, Jon Hunter wrote:
> > > > > > On 25/01/2022 10:29, Wander Costa wrote:
> > > >
> > > > ...
> > > >
> > > > > > Andy, does this work for X86?
> > > > >
> > > > > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > >
> > > > > No, it does NOT fix an issue (I see it on a handful x86) with the legacy UART
> > > > > (means the 8250_pnp is in use). And I believe the same will be the case on LPSS
> > > > > ones (8250_dw / 8250_lpss) and HSU (8250_mid), because the patch influences on
> > > > > all of them.
> > > >
> > > > Shall I send a revert and we can continue with a new approach later on?
> > > >
> > >
> > > Tomorrow (or maybe after tomorrow) I am going to post the fixes I
> > > already have, and an additional patch adding a build option
> > > (disabled to default) so people maybe if they want to use the FIFO on
> > > console write. But I understand if people decide to go
> > > ahead and revert the patch.
> >
> > Let me revert this for now.  And no new config options please, this
> > should "just work".
>
> Thanks!
>
> Wander, if you need a test for something new, I may help to perform on
> our (sub)set of x86 machines.
>

Thanks, Andy. I will let you know when I have new patches.

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

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 10:23:57AM -0300, Wander Costa wrote:
> On Wed, Jan 26, 2022 at 9:10 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Jan 26, 2022 at 09:52:26AM +0100, Greg Kroah-Hartman wrote:

...

> > Wander, if you need a test for something new, I may help to perform on
> > our (sub)set of x86 machines.
> 
> Thanks, Andy. I will let you know when I have new patches.

Just Cc me that time.

-- 
With Best Regards,
Andy Shevchenko

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

[Theodore Y. Ts'o]

On Wed, Jan 26, 2022 at 09:52:26AM +0100, Greg Kroah-Hartman wrote:
> 
> Let me revert this for now.  And no new config options please, this
> should "just work".

I'm not sure the commit is actually worth the extra complexity, to be
honest.  The reason for the FIFO is to improve interrupt latency, and
in the console write path, we're busy looping.  There is something
seriously wrong serial port of the HP Proliant DL380 Gen 9.  Per the
commit description for 5021d709b31b: ("tty: serial: Use fifo in 8250
console driver"), on the "fast machine" (read: the one with a
propertly working serial port), we were getting over 10 KB/s without
the patch.  And on the "slow machine" it was getting only 2.5 KB/s,
and with the patch it only improved things by 25% (so only 3.1 KB/s).

I assume what must be going on is this machine is emulating the UART
and is extremely slow to set the Trasmitter Holding Register Empty
(THRE) bit after the UART is finished sending the byte out the serial
port.

So we're adding a lot of complexity for what is obviously broken
hardware, and we risk breaking the serial console for other machines
with a properly implemented serial port.  How common are UART's which
are broken in this way?  Is it unique to the HP Proliant DL380 Gen 9?
Or is a common misimplementation which is unfortunately quite common?
If it's the former, maybe the FIFO hack should only be done via a
quirk?

If it's really the case that the HP Proliant's nasty performance is
due to a badly implemented emulation layer, is there any way to do
better, perhaps via a more direct path to the serial port?  Or is the
problem that the serial port on this motherboard is connected via some
super-slow internal path and it would be faster if you could talk to
it directly via a UEFI call, or some other mechanism?  Whether it's
2.5 KB/s or 3.1 KB/s, it's really quite pathetic....

     	      	     	   	       - Ted

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

[Wander Costa]

On Tue, Jan 25, 2022 at 6:36 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> 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@redhat.com wrote:
> >>> From: Wander Lairson Costa <wander@redhat.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@redhat.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@jonathanh-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@j-linux-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
>

Indeed I made a mistake here. Independent of the reported this, this
should be fixed.
Jiri, do you intend to send an official patch or should I do so?

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

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

[Jiri Slaby]

On 25. 01. 22, 11:18, Wander Costa wrote:
>> 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
>>
> 
> Indeed I made a mistake here. Independent of the reported this, this
> should be fixed.
> Jiri, do you intend to send an official patch or should I do so?

Please you send the fix after testing the fifo mode still works with 
that fix.

thanks,
-- 
js