LinuxLists.cc - [PATCH] tty: serial: Use fifo in 8250 console driver

[permalink] [raw]

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

Em sáb., 30 de out. de 2021 04:41, Andy Shevchenko
<[email protected]> escreveu:
>
>
>
> On Friday, October 29, 2021, <[email protected]> wrote:
>>
>> From: Wander Lairson Costa <[email protected]>
>>
>> Note: I am using a small test app + driver located at [0] for the
>
>
> I don't see any links.

Oops, sorry about that. I must have accidentally deleted it while
editing the commit message.
Here it is https://github.com/walac/serial-console-test.
I will update the patch with the link.

> While at it, why can't you integrate your changes to [1], for example?
>
> [1]: https://github.com/cbrake/linux-serial-test
>
First of all, I was not aware of it, but afaik, /dev/ttyS doesn't
follow the same code path as
printk, which was my main goal here (I should have made this clear in
the commit message, my bad).

>

[snip]

>>
>
>
> On how many different UARTs have you tested this? Have you tested oops and NMI contexts?
>
I only tested in a half dozen machines that I have available. I tried
it in panic, warnings, IRQ contexts, etc. Theoretically, this change
should not be affected by the context. Theoretically...

> What I would like to say here is that the code is being used on zillions of different 8250 implementations here and I would be rather skeptical about enabling the feature for everyone.
>
I did my homework and studied the 16550 datasheets, but yes, there is
always this risk. Maybe people more experienced with PC serial ports
than me might think the patch is not worth the risk of breaking some
unknown number of devices out there, and I am ok with that. It is a
valid point.

>>
>> Signed-off-by: Wander Lairson Costa <[email protected]>
>> ---
>> drivers/tty/serial/8250/8250_port.c | 61 ++++++++++++++++++++++++++---
>> 1 file changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 66374704747e..edf88a8338a2 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -2063,10 +2063,7 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
>> serial8250_rpm_put(up);
>> }
>>
>> -/*
>> - * Wait for transmitter & holding register to empty
>> - */
>> -static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>> +static void wait_for_lsr(struct uart_8250_port *up, int bits)
>> {
>> unsigned int status, tmout = 10000;
>>
>> @@ -2083,6 +2080,16 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>> udelay(1);
>> touch_nmi_watchdog();
>> }
>> +}
>> +
>> +/*
>> + * Wait for transmitter & holding register to empty
>> + */
>> +static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>> +{
>> + unsigned int tmout;
>> +
>> + wait_for_lsr(up, bits);
>>
>> /* Wait up to 1s for flow control if necessary */
>> if (up->port.flags & UPF_CONS_FLOW) {
>> @@ -3319,6 +3326,35 @@ static void serial8250_console_restore(struct uart_8250_port *up)
>> serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS);
>> }
>>
>> +/*
>> + * Print a string to the serial port using the device FIFO
>> + *
>> + * It sends fifosize bytes and then waits for the fifo
>> + * to get empty.
>> + */
>> +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->port.fifosize;
>> + bool cr_sent = false;
>> +
>> + while (s != end) {
>> + wait_for_lsr(up, UART_LSR_THRE);
>> +
>> + for (i = 0; i < fifosize && s != end; ++i) {
>> + if (*s == '\n' && !cr_sent) {
>> + serial_out(up, UART_TX, '\r');
>> + cr_sent = true;
>> + } else {
>> + serial_out(up, UART_TX, *s++);
>> + cr_sent = false;
>> + }
>> + }
>> + }
>> +}
>> +
>> /*
>> * Print a string to the serial port trying not to disturb
>> * any possible real use of the port...
>> @@ -3334,7 +3370,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>> struct uart_8250_em485 *em485 = up->em485;
>> struct uart_port *port = &up->port;
>> unsigned long flags;
>> - unsigned int ier;
>> + unsigned int ier, use_fifo;
>> int locked = 1;
>>
>> touch_nmi_watchdog();
>> @@ -3366,7 +3402,20 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>> mdelay(port->rs485.delay_rts_before_send);
>> }
>>
>> - uart_console_write(port, s, count, serial8250_console_putchar);
>> + use_fifo = (up->capabilities & UART_CAP_FIFO)
>> + && port->fifosize > 1
>> + && (serial_port_in(port, UART_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
>> + * if we don't use control flow.
>> + */
>> + && !(up->port.flags & UPF_CONS_FLOW);
>> +
>> + if (likely(use_fifo))
>> + serial8250_console_fifo_write(up, s, count);
>> + else
>> + uart_console_write(port, s, count, serial8250_console_putchar);
>>
>> /*
>> * Finally, wait for transmitter to become empty
>> --
>> 2.27.0
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

2021-11-01 15:37:57

[permalink] [raw]

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

On Mon, Nov 1, 2021 at 5:22 PM Wander Costa <[email protected]> wrote:
> Em sáb., 30 de out. de 2021 04:41, Andy Shevchenko
> <[email protected]> escreveu:
> > On Friday, October 29, 2021, <[email protected]> wrote:

...

> > I don't see any links.
>
> Oops, sorry about that. I must have accidentally deleted it while
> editing the commit message.
> Here it is https://github.com/walac/serial-console-test.
> I will update the patch with the link.

Thanks!

...

> > On how many different UARTs have you tested this? Have you tested oops and NMI contexts?
> >
> I only tested in a half dozen machines that I have available. I tried
> it in panic, warnings, IRQ contexts, etc. Theoretically, this change
> should not be affected by the context. Theoretically...
>
> > What I would like to say here is that the code is being used on zillions of different 8250 implementations here and I would be rather skeptical about enabling the feature for everyone.
> >
> I did my homework and studied the 16550 datasheets, but yes, there is
> always this risk. Maybe people more experienced with PC serial ports
> than me might think the patch is not worth the risk of breaking some
> unknown number of devices out there, and I am ok with that. It is a
> valid point.

Here is a translation of my comment to a roadmap.

1. Introduce yet another UART quirk or capability (see corresponding
UART_CAP_* or UART_*_QUIRK definitions)
2. Add your patch conditionally based on the above
3. Enable it on UART(s) you _have tested_

--
With Best Regards,
Andy Shevchenko

2021-11-10 12:12:07

[permalink] [raw]

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

On Mon, Nov 1, 2021 at 12:33 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Nov 1, 2021 at 5:22 PM Wander Costa <[email protected]> wrote:
> > Em sáb., 30 de out. de 2021 04:41, Andy Shevchenko
> > <[email protected]> escreveu:
> > > On Friday, October 29, 2021, <[email protected]> wrote:
>
> ...
>
> > > I don't see any links.
> >
> > Oops, sorry about that. I must have accidentally deleted it while
> > editing the commit message.
> > Here it is https://github.com/walac/serial-console-test.
> > I will update the patch with the link.
>
> Thanks!
>
> ...
>
> > > On how many different UARTs have you tested this? Have you tested oops and NMI contexts?
> > >
> > I only tested in a half dozen machines that I have available. I tried
> > it in panic, warnings, IRQ contexts, etc. Theoretically, this change
> > should not be affected by the context. Theoretically...
> >
> > > What I would like to say here is that the code is being used on zillions of different 8250 implementations here and I would be rather skeptical about enabling the feature for everyone.
> > >
> > I did my homework and studied the 16550 datasheets, but yes, there is
> > always this risk. Maybe people more experienced with PC serial ports
> > than me might think the patch is not worth the risk of breaking some
> > unknown number of devices out there, and I am ok with that. It is a
> > valid point.
>
> Here is a translation of my comment to a roadmap.
>
> 1. Introduce yet another UART quirk or capability (see corresponding
> UART_CAP_* or UART_*_QUIRK definitions)
> 2. Add your patch conditionally based on the above
> 3. Enable it on UART(s) you _have tested_
>
Thank you for the feedback, I submitted a v2 patch with your proposed changes,

Cheers,
Wander

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

2021-11-12 11:58:13

by David Laight

[permalink] [raw]

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

> > Here is a translation of my comment to a roadmap.
> >
> > 1. Introduce yet another UART quirk or capability (see corresponding
> > UART_CAP_* or UART_*_QUIRK definitions)
> > 2. Add your patch conditionally based on the above
> > 3. Enable it on UART(s) you _have tested_
> >
> Thank you for the feedback, I submitted a v2 patch with your proposed changes,

Or put the UART into loopback and see how many characters will
actually fit in the fifos.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2022-01-25 09:32:28

[permalink] [raw]

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

On 29/10/2021 21:14, [email protected] wrote:
> From: Wander Lairson Costa <[email protected]>
>
> 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 <[email protected]>

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 [email protected]) 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

2022-01-25 13:11:09

[permalink] [raw]

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

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, [email protected] wrote:
>>> From: Wander Lairson Costa <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>>
>> 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 [email protected]) 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

2022-01-25 13:39:28

by Greg Kroah-Hartman

[permalink] [raw]

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

On Tue, Jan 25, 2022 at 08:39:24AM +0000, Jon Hunter wrote:
>
> On 29/10/2021 21:14, [email protected] wrote:
> > From: Wander Lairson Costa <[email protected]>
> >
> > 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 <[email protected]>
>
>
> 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 [email protected]) 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

2022-01-25 14:21:53

by Jiri Slaby

[permalink] [raw]

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

On 25. 01. 22, 9:39, Jon Hunter wrote:
>
> On 29/10/2021 21:14, [email protected] wrote:
>> From: Wander Lairson Costa <[email protected]>
>>
>> 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 <[email protected]>
>
>
> 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 [email protected])
> 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

2022-01-25 14:30:52

by Jiri Slaby

[permalink] [raw]

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, [email protected] wrote:
>>> From: Wander Lairson Costa <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>>
>> 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 [email protected])
>> 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

2022-01-25 14:42:07

[permalink] [raw]

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

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

2022-01-25 14:46:08

[permalink] [raw]

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

On Tue, Jan 25, 2022 at 6:36 AM Jiri Slaby <[email protected]> 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, [email protected] wrote:
> >>> From: Wander Lairson Costa <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>
> >>
> >> 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 [email protected])
> >> 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
>

2022-01-25 14:54:05

[permalink] [raw]

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

On Tue, Jan 25, 2022 at 7:06 AM Jon Hunter <[email protected]> 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
>

2022-01-25 14:54:43

by Jiri Slaby

[permalink] [raw]

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

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

2022-01-25 17:01:21

[permalink] [raw]

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

On 25/01/2022 10:29, Wander Costa wrote:
> On Tue, Jan 25, 2022 at 7:06 AM Jon Hunter <[email protected]> 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

2022-01-26 01:22:41

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]>

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

2022-01-26 01:23:01

[permalink] [raw]

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

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 <[email protected]>
>
> 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

2022-01-26 03:38:09

[permalink] [raw]

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

On Tue, Jan 25, 2022 at 1:56 PM Andy Shevchenko
<[email protected]> 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 <[email protected]>
> >
> > 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
>
>

2022-01-26 20:37:50

by Greg Kroah-Hartman

[permalink] [raw]

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

On Tue, Jan 25, 2022 at 03:40:36PM -0300, Wander Costa wrote:
> On Tue, Jan 25, 2022 at 1:56 PM Andy Shevchenko
> <[email protected]> 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 <[email protected]>
> > >
> > > 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

2022-01-26 21:12:41

[permalink] [raw]

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

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
> > <[email protected]> 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 <[email protected]>
> > > >
> > > > 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

2022-01-26 21:19:47

[permalink] [raw]

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

On Wed, Jan 26, 2022 at 9:10 AM Andy Shevchenko
<[email protected]> 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
> > > <[email protected]> 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 <[email protected]>
> > > > >
> > > > > 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.

2022-01-26 21:23:57