[PATCH v4 0/5] tty/8250: Use fifo in 8250 console driver

[PATCH v4 0/5] tty/8250: Use fifo in 8250 console driver

[Wander Lairson Costa]

This version fixes the bugs reported in version v3. The first patch
is the same patch of v3 as is. The following commits fix the issues in the
original patch. For details, please check the commit log of each patch.

I tested these patches in the following systems:

* IBM X3550 M3
* HP ProLiant DL380 Gen9
* HP ProLiant BL480c G1
* Dell PowerEdge R910
* Cisco UCSC-C220-M3S

I cc everybody that reported problems with the previous version of this
patch so they can retest and confirm their systems work flawlessly.

Wander Lairson Costa (5):
  serial/8250: Use fifo in 8250 console driver
  serial/8250: Use the cache value of the FCR register
  serial/8250: Use tx_loadsz as the transmitter fifo size
  serial/8250: exclude BCM283x from console_fifo_write
  serial/8250: Only use fifo after the port is initialized in
    console_write

 drivers/tty/serial/8250/8250_port.c | 67 ++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 6 deletions(-)

-- 
2.35.1

[PATCH v4 1/5] serial/8250: Use fifo in 8250 console driver

[Wander Lairson Costa]

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>
Link: https://lore.kernel.org/r/20211222112831.1968392-2-wander@redhat.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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 3b12bfc1ed67..2abb3de11a48 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2056,10 +2056,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;
 
@@ -2076,6 +2073,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) {
@@ -3325,6 +3332,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...
@@ -3340,7 +3376,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();
@@ -3372,7 +3408,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.35.1

[PATCH v4 2/5] serial/8250: Use the cache value of the FCR register

[Wander Lairson Costa]

commit 5021d709b31b ("tty: serial: Use fifo in 8250 console driver")
erroneous tries to read the FCR register content, but this register is
write-only.

This patch fixes that by reading the content from the port struct fcr
field.

Thanks to Jon Hunter and Jiri Slaby.

Suggested-by: Jiri Slaby <jirislaby@kernel.org>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 2abb3de11a48..9f3fa9fe2a4e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3410,7 +3410,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
-- 
2.35.1

[PATCH v4 3/5] serial/8250: Use tx_loadsz as the transmitter fifo size

[Wander Lairson Costa]

Using port_fifosize as the fifo size to transmit data to the serial
console causes data loss in some controllers. Use the correct tx_loadsz
field.

Thanks to Jon Hunter for reporting the issue.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Fixes: 5021d709b31b ("tty: serial: Use fifo in 8250 console driver")
Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 drivers/tty/serial/8250/8250_port.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 9f3fa9fe2a4e..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,7 +3409,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	}
 
 	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
-		port->fifosize > 1 &&
+		up->tx_loadsz > 1 &&
 		(up->fcr & UART_FCR_ENABLE_FIFO) &&
 		/*
 		 * After we put a data in the fifo, the controller will send
-- 
2.35.1

[PATCH v4 4/5] serial/8250: exclude BCM283x from console_fifo_write

[Wander Lairson Costa]

From Phil's original patch:

"""
The mini-UART on BCM283x is doubly crippled - it has 8-byte FIFOs and
the THRE bit indicates that the TX FIFO is not-full rather than empty.

The optimisation to enable the use of the FIFO assumes that it is safe
to write fifosize bytes whenever THRE is set, but the BCM283x quirk
(indicated by the presence of UART_CAP_MINI) makes it necessary to
check the FIFO state after each byte.

See: https://github.com/raspberrypi/linux/issues/4849
"""

Thanks to Phil Elwell for reporting the issue and providing the original
patch.

Reported-by: Phil Elwell <phil@raspberrypi.com>
Co-author: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 drivers/tty/serial/8250/8250_port.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index d3a93e5d55f7..4acf620be241 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3409,6 +3409,11 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	}
 
 	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
+		/*
+		 * BCM283x requires to check the fifo
+		 * after each byte.
+		 */
+		!(up->capabilities & UART_CAP_MINI) &&
 		up->tx_loadsz > 1 &&
 		(up->fcr & UART_FCR_ENABLE_FIFO) &&
 		/*
-- 
2.35.1

[PATCH v4 5/5] serial/8250: Only use fifo after the port is initialized in console_write

[Wander Lairson Costa]

The serial driver set the value of uart_8250_port.fcr in the function
serial8250_config_port, but only writes the value to the controller
register later in the initalization code.

That opens a small window in which is not safe to use the fifo for
console write.

Make sure the port is initialized correctly before reading the FCR
cached value.

Unfortunately, I lost track of who originally reported the issue. If
s/he is reading this, please speak up so I can give you the due credit.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 drivers/tty/serial/8250/8250_port.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4acf620be241..7e2227161555 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3416,6 +3416,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		!(up->capabilities & UART_CAP_MINI) &&
 		up->tx_loadsz > 1 &&
 		(up->fcr & UART_FCR_ENABLE_FIFO) &&
+		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
 		/*
 		 * After we put a data in the fifo, the controller will send
 		 * it regardless of the CTS state. Therefore, only use fifo
-- 
2.35.1

RE: [PATCH v4 1/5] serial/8250: Use fifo in 8250 console driver

[David Laight]

From: Wander Lairson Costa
> Sent: 16 March 2022 14:37
> 
> 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:

Did you verify the baud rate?

Or is there some horrid serial redirection going on.
It is even possible there is a bios smm interrupt
chugging through on another cpu core.

	David

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

Re: [PATCH v4 0/5] tty/8250: Use fifo in 8250 console driver

[Andy Shevchenko]

On Wed, Mar 16, 2022 at 11:36:39AM -0300, Wander Lairson Costa wrote:
> This version fixes the bugs reported in version v3. The first patch
> is the same patch of v3 as is. The following commits fix the issues in the
> original patch. For details, please check the commit log of each patch.
> 
> I tested these patches in the following systems:
> 
> * IBM X3550 M3
> * HP ProLiant DL380 Gen9
> * HP ProLiant BL480c G1
> * Dell PowerEdge R910
> * Cisco UCSC-C220-M3S
> 
> I cc everybody that reported problems with the previous version of this
> patch so they can retest and confirm their systems work flawlessly.

I have got this only message and I don't see any good changelog what has
been done between v3 and v4.

> Wander Lairson Costa (5):
>   serial/8250: Use fifo in 8250 console driver
>   serial/8250: Use the cache value of the FCR register
>   serial/8250: Use tx_loadsz as the transmitter fifo size
>   serial/8250: exclude BCM283x from console_fifo_write
>   serial/8250: Only use fifo after the port is initialized in
>     console_write

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/5] tty/8250: Use fifo in 8250 console driver

[Andy Shevchenko]

On Wed, Mar 16, 2022 at 06:13:19PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 16, 2022 at 11:36:39AM -0300, Wander Lairson Costa wrote:
> > This version fixes the bugs reported in version v3. The first patch
> > is the same patch of v3 as is. The following commits fix the issues in the
> > original patch. For details, please check the commit log of each patch.
> > 
> > I tested these patches in the following systems:
> > 
> > * IBM X3550 M3
> > * HP ProLiant DL380 Gen9
> > * HP ProLiant BL480c G1
> > * Dell PowerEdge R910
> > * Cisco UCSC-C220-M3S
> > 
> > I cc everybody that reported problems with the previous version of this
> > patch so they can retest and confirm their systems work flawlessly.
> 
> I have got this only message and I don't see any good changelog what has
> been done between v3 and v4.
> 
> > Wander Lairson Costa (5):
> >   serial/8250: Use fifo in 8250 console driver
> >   serial/8250: Use the cache value of the FCR register
> >   serial/8250: Use tx_loadsz as the transmitter fifo size
> >   serial/8250: exclude BCM283x from console_fifo_write
> >   serial/8250: Only use fifo after the port is initialized in
> >     console_write

If you are going to (re-)send a new version, please Cc to Ilpo as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/5] tty/8250: Use fifo in 8250 console driver

[Wander Costa]

On Wed, Mar 16, 2022 at 1:15 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 16, 2022 at 11:36:39AM -0300, Wander Lairson Costa wrote:
> > This version fixes the bugs reported in version v3. The first patch
> > is the same patch of v3 as is. The following commits fix the issues in the
> > original patch. For details, please check the commit log of each patch.
> >
> > I tested these patches in the following systems:
> >
> > * IBM X3550 M3
> > * HP ProLiant DL380 Gen9
> > * HP ProLiant BL480c G1
> > * Dell PowerEdge R910
> > * Cisco UCSC-C220-M3S
> >
> > I cc everybody that reported problems with the previous version of this
> > patch so they can retest and confirm their systems work flawlessly.
>
> I have got this only message and I don't see any good changelog what has
> been done between v3 and v4.
>

Weird, the patches were sent [1,2,3,4,5] and reached out my inbox.

[1] https://lore.kernel.org/all/20220316143646.13301-2-wander@redhat.com/
[2] https://lore.kernel.org/all/20220316143646.13301-3-wander@redhat.com/
[3] https://lore.kernel.org/all/20220316143646.13301-4-wander@redhat.com/
[4] https://lore.kernel.org/all/20220316143646.13301-5-wander@redhat.com/
[5] https://lore.kernel.org/all/20220316143646.13301-6-wander@redhat.com/

[snip]

Re: [PATCH v4 5/5] serial/8250: Only use fifo after the port is initialized in console_write

[Jiri Slaby]

On 16. 03. 22, 15:36, Wander Lairson Costa wrote:
> The serial driver set the value of uart_8250_port.fcr in the function
> serial8250_config_port, but only writes the value to the controller
> register later in the initalization code.
> 
> That opens a small window in which is not safe to use the fifo for
> console write.
> 
> Make sure the port is initialized correctly before reading the FCR
> cached value.
> 
> Unfortunately, I lost track of who originally reported the issue. If
> s/he is reading this, please speak up so I can give you the due credit.
> 
> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> ---
>   drivers/tty/serial/8250/8250_port.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 4acf620be241..7e2227161555 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3416,6 +3416,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>   		!(up->capabilities & UART_CAP_MINI) &&
>   		up->tx_loadsz > 1 &&
>   		(up->fcr & UART_FCR_ENABLE_FIFO) &&
> +		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&

Cannot be port->state be NULL sometimes here?

>   		/*
>   		 * After we put a data in the fifo, the controller will send
>   		 * it regardless of the CTS state. Therefore, only use fifo


-- 
js
suse labs

Re: [PATCH v4 2/5] serial/8250: Use the cache value of the FCR register

[Ilpo Järvinen]

On Wed, 16 Mar 2022, Wander Lairson Costa wrote:

> commit 5021d709b31b ("tty: serial: Use fifo in 8250 console driver")
> erroneous tries to read the FCR register content, but this register is
> write-only.
> 
> This patch fixes that by reading the content from the port struct fcr
> field.
> 
> Thanks to Jon Hunter and Jiri Slaby.
> 
> Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 2abb3de11a48..9f3fa9fe2a4e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3410,7 +3410,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) &&

Didn't you just add this line in 1/5? Please merge this kind of fixes that 
are due to development history of a change to the main patch itself.


-- 
 i.

Re: [PATCH v4 5/5] serial/8250: Only use fifo after the port is initialized in console_write

[Ilpo Järvinen]

On Wed, 16 Mar 2022, Wander Lairson Costa wrote:

> The serial driver set the value of uart_8250_port.fcr in the function
> serial8250_config_port, but only writes the value to the controller
> register later in the initalization code.
> 
> That opens a small window in which is not safe to use the fifo for
> console write.
> 
> Make sure the port is initialized correctly before reading the FCR
> cached value.
> 
> Unfortunately, I lost track of who originally reported the issue. If
> s/he is reading this, please speak up so I can give you the due credit.
> 
> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 4acf620be241..7e2227161555 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3416,6 +3416,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>  		!(up->capabilities & UART_CAP_MINI) &&
>  		up->tx_loadsz > 1 &&
>  		(up->fcr & UART_FCR_ENABLE_FIFO) &&
> +		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
>  		/*
>  		 * After we put a data in the fifo, the controller will send
>  		 * it regardless of the CTS state. Therefore, only use fifo

So it looks like 2-5 just contain your development history and should all 
be merged to 1/5 (perhaps with Co-developed-by: tags where appropriate).

And please don't just merge them "silently" there w/o describing in the 
message _why_ you ended up doing the things the way you did in the end.
The messages you've written for patches 2-5 will serve you as great source 
material (with small mods, obviously).


-- 
 i.

Re: [PATCH v4 2/5] serial/8250: Use the cache value of the FCR register

[Wander Costa]

On Thu, Mar 17, 2022 at 5:31 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Wed, 16 Mar 2022, Wander Lairson Costa wrote:
>
> > commit 5021d709b31b ("tty: serial: Use fifo in 8250 console driver")
> > erroneous tries to read the FCR register content, but this register is
> > write-only.
> >
> > This patch fixes that by reading the content from the port struct fcr
> > field.
> >
> > Thanks to Jon Hunter and Jiri Slaby.
> >
> > Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> > Reported-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > ---
> >  drivers/tty/serial/8250/8250_port.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 2abb3de11a48..9f3fa9fe2a4e 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -3410,7 +3410,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) &&
>
> Didn't you just add this line in 1/5? Please merge this kind of fixes that
> are due to development history of a change to the main patch itself.
>

The reason is that 1/5 has been applied in 5.17 and then reverted, so
I thought it would make it easier for reviewers if I sent the new
fixes in different commits. If that's not the case, I can send a
squashed version with the changelog described in 0/5.

Re: [PATCH v4 5/5] serial/8250: Only use fifo after the port is initialized in console_write

[Wander Costa]

On Thu, Mar 17, 2022 at 4:06 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 16. 03. 22, 15:36, Wander Lairson Costa wrote:
> > The serial driver set the value of uart_8250_port.fcr in the function
> > serial8250_config_port, but only writes the value to the controller
> > register later in the initalization code.
> >
> > That opens a small window in which is not safe to use the fifo for
> > console write.
> >
> > Make sure the port is initialized correctly before reading the FCR
> > cached value.
> >
> > Unfortunately, I lost track of who originally reported the issue. If
> > s/he is reading this, please speak up so I can give you the due credit.
> >
> > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > ---
> >   drivers/tty/serial/8250/8250_port.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 4acf620be241..7e2227161555 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -3416,6 +3416,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> >               !(up->capabilities & UART_CAP_MINI) &&
> >               up->tx_loadsz > 1 &&
> >               (up->fcr & UART_FCR_ENABLE_FIFO) &&
> > +             test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
>
> Cannot be port->state be NULL sometimes here?
>

IIUC, state is assigned at early port registration in
uart_add_one_port(), so this function wouldn't be called when state is
NULL. But I think it causes no harm to add an extra check. Thanks!

> >               /*
> >                * After we put a data in the FIFO, the controller will send
> >                * it regardless of the CTS state. Therefore, only use fifo
>
>
> --
> js
> suse labs
>

Re: [PATCH v4 5/5] serial/8250: Only use fifo after the port is initialized in console_write

[Wander Costa]

On Thu, Mar 17, 2022 at 5:44 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Wed, 16 Mar 2022, Wander Lairson Costa wrote:
>
> > The serial driver set the value of uart_8250_port.fcr in the function
> > serial8250_config_port, but only writes the value to the controller
> > register later in the initalization code.
> >
> > That opens a small window in which is not safe to use the fifo for
> > console write.
> >
> > Make sure the port is initialized correctly before reading the FCR
> > cached value.
> >
> > Unfortunately, I lost track of who originally reported the issue. If
> > s/he is reading this, please speak up so I can give you the due credit.
> >
> > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > ---
> >  drivers/tty/serial/8250/8250_port.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 4acf620be241..7e2227161555 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -3416,6 +3416,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> >               !(up->capabilities & UART_CAP_MINI) &&
> >               up->tx_loadsz > 1 &&
> >               (up->fcr & UART_FCR_ENABLE_FIFO) &&
> > +             test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
> >               /*
> >                * After we put a data in the fifo, the controller will send
> >                * it regardless of the CTS state. Therefore, only use fifo
>
> So it looks like 2-5 just contain your development history and should all
> be merged to 1/5 (perhaps with Co-developed-by: tags where appropriate).
>
> And please don't just merge them "silently" there w/o describing in the
> message _why_ you ended up doing the things the way you did in the end.
> The messages you've written for patches 2-5 will serve you as great source
> material (with small mods, obviously).
>

Ok, I will merge them in v5.

>
> --
>  i.
>

Re: [PATCH v4 1/5] serial/8250: Use fifo in 8250 console driver

[Wander Costa]

On Wed, Mar 16, 2022 at 12:27 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Wander Lairson Costa
> > Sent: 16 March 2022 14:37
> >
> > 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:
>
> Did you verify the baud rate?
>

Yes.



> Or is there some horrid serial redirection going on.
> It is even possible there is a bios smm interrupt
> chugging through on another cpu core.
>

I would be surprised if that were the case because I see the problem
even in low system activity. Someone in a previous patch-revision
theorized this might be due to a bad serial controller emulator
implemented in hardware (or something in this line of thought).

Re: [PATCH v4 0/5] tty/8250: Use fifo in 8250 console driver

[Steven Rostedt]

On Wed, 16 Mar 2022 11:36:39 -0300
Wander Lairson Costa <wander@redhat.com> wrote:

> I cc everybody that reported problems with the previous version of this
> patch so they can retest and confirm their systems work flawlessly.

I didn't do an extensive test, but I quickly applied them and did not see
any issues with the two machines that had problems with your first commits.

But I'll do a more extensive testing on your v5 before I give a tested-by.

Thanks,

-- Steve