Re: [PATCH 1/3] tty: serial: introduce uart_port_tx{,_limit}() helpers

From: Jiri Slaby
Date: Fri Apr 15 2022 - 03:47:38 EST


On 14. 04. 22, 18:32, Greg KH wrote:
On Mon, Apr 11, 2022 at 12:54:03PM +0200, Jiri Slaby wrote:
Many serial drivers do the same thing:
* send x_char if set
* keep sending from the xmit circular buffer until either
- the loop reaches the end of the xmit buffer
- TX is stopped
- HW fifo is full
* check for pending characters and:
- wake up tty writers to fill for more data into xmit buffer
- stop TX if there is nothing in the xmit buffer

The only differences are:
* how to write the character to the HW fifo
* the check of the end condition:
- is the HW fifo full?
- is limit of the written characters reached?

So unify the above into two helpers:
* uart_port_tx_limit() -- the generic one, it performs the above taking
into account the written characters limit
* uart_port_tx() -- calls the above with ~0 as the limit. So it only
checks the HW fullness.

We need three more hooks in struct uart_ops for all this to work:
* tx_ready() -- returns true if HW can accept more data.
* put_char() -- write a character to the device.
* tx_done() -- when the write loop is done, perform arbitrary action
before potential invocation of ops->stop_tx() happens.

NOTE1: Maybe the three hooks in uart_ops above are overkill. We can
instead pass pointers to the three functions directly to the new helpers
as they are not used elsewhere. Similar to uart_console_write() and its
putchar().

NOTE2: These two new helper functions call the hooks per every character
processed. I was unable to measure any difference, provided most time is
spent by readb (or alike) in the hooks themselves. First, LTO might
help to eliminate these explicit calls (we might need NOTE1 to be
implemented for this to be true). Second, if this turns out to be a
problem, we can introduce a macro to build the helper in the driver's
code instead of serial_core. That is, similar to wait_event().

Signed-off-by: Jiri Slaby <jslaby@xxxxxxx>
---
Documentation/driver-api/serial/driver.rst | 28 ++++++++++++
drivers/tty/serial/serial_core.c | 53 ++++++++++++++++++++++
include/linux/serial_core.h | 9 ++++
3 files changed, 90 insertions(+)

diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst
index 06ec04ba086f..7dc3791addeb 100644
--- a/Documentation/driver-api/serial/driver.rst
+++ b/Documentation/driver-api/serial/driver.rst
@@ -80,6 +80,34 @@ hardware.
This call must not sleep
+ tx_ready(port)
+ The driver returns true if the HW can accept more data to be sent.
+
+ Locking: port->lock taken.
+
+ Interrupts: locally disabled.
+
+ This call must not sleep.
+
+ put_char(port, ch)
+ The driver is asked to write ch to the device.
+
+ Locking: port->lock taken.
+
+ Interrupts: locally disabled.
+
+ This call must not sleep.
+
+ tx_done(port)
+ When the write loop is done, the driver can perform arbitrary action
+ here before potential invocation of ops->stop_tx() happens.
+
+ Locking: port->lock taken.
+
+ Interrupts: locally disabled.
+
+ This call must not sleep.
+
set_mctrl(port, mctrl)
This function sets the modem control lines for port described
by 'port' to the state described by mctrl. The relevant bits
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6a8963caf954..1be14e90066c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -107,6 +107,59 @@ void uart_write_wakeup(struct uart_port *port)
}
EXPORT_SYMBOL(uart_write_wakeup);
+static bool uart_port_tx_always_ready(struct uart_port *port)
+{
+ return true;
+}
+
+/**
+ * uart_port_tx_limit -- transmit helper for uart_port
+ * @port: from which port to transmit
+ * @count: limit count
+ *
+ * uart_port_tx_limit() transmits characters from the xmit buffer to the
+ * hardware using @uart_port::ops::put_char(). It does so until @count
+ * characters are sent and while @uart_port::ops::tx_ready() still returns
+ * non-zero (if non-NULL).
+ *
+ * Return: number of characters in the xmit buffer when done.
+ */
+unsigned int uart_port_tx_limit(struct uart_port *port, unsigned int count)
+{
+ struct circ_buf *xmit = &port->state->xmit;
+ bool (*tx_ready)(struct uart_port *) = port->ops->tx_ready ? :
+ uart_port_tx_always_ready;
+ unsigned int pending;
+
+ for (; count && tx_ready(port); count--, port->icount.tx++) {
+ if (port->x_char) {
+ port->ops->put_char(port, port->x_char);
+ port->x_char = 0;
+ continue;
+ }
+
+ if (uart_circ_empty(xmit) || uart_tx_stopped(port))
+ break;
+
+ port->ops->put_char(port, xmit->buf[xmit->tail]);

That's a lot of redirection and function pointer mess per each character
sent now. With the spectre overhead here (and only getting worse), this
feels like a step backwards.

I doubt throughput matters here given cpu speeds now, _but_ the cpu load
should go up.

Although on smaller cpus with slower Mhz and faster line rates, this
feels like a lot of extra work happening for no real good reason.

I know… Did you miss NOTE2 in the commit log? Any idea on that?

thanks,
--
js
suse labs