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
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
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]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[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 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
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 <[email protected]>
---
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
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 <[email protected]>
Reported-by: Jon Hunter <[email protected]>
Signed-off-by: Wander Lairson Costa <[email protected]>
---
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
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
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 <[email protected]>
Co-author: Phil Elwell <[email protected]>
Signed-off-by: Wander Lairson Costa <[email protected]>
---
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
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 <[email protected]>
> Reported-by: Jon Hunter <[email protected]>
> Signed-off-by: Wander Lairson Costa <[email protected]>
> ---
> 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.
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 <[email protected]>
> ---
> 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.
On Thu, Mar 17, 2022 at 4:06 AM Jiri Slaby <[email protected]> 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 <[email protected]>
> > ---
> > 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
>
On Thu, Mar 17, 2022 at 5:44 AM Ilpo Järvinen
<[email protected]> 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 <[email protected]>
> > ---
> > 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.
>
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 <[email protected]>
> ---
> 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
On Wed, 16 Mar 2022 11:36:39 -0300
Wander Lairson Costa <[email protected]> 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
On Thu, Mar 17, 2022 at 5:31 AM Ilpo Järvinen
<[email protected]> 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 <[email protected]>
> > Reported-by: Jon Hunter <[email protected]>
> > Signed-off-by: Wander Lairson Costa <[email protected]>
> > ---
> > 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.