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 helper generators:
* DEFINE_UART_PORT_TX_HELPER_LIMITED() -- it performs the above taking
  the written characters limit into account, and
* DEFINE_UART_PORT_TX_HELPER() -- the same as above, except it only
  checks the HW readiness, not the characters limit.

The HW specific operations (as stated as "differences" above) are passed
as arguments to the macros. They are:
* 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.

Note that the above macros are generators. This means the code is
generated in place and the above 3 arguments are "inlined". I.e. no
added penalty by generating call instructions for every single
character. Nor any indirect calls. (As in previous versions of this
patchset.)

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---

Notes:
    [v2] instead of a function (uart_port_tx_limit()) in serial_core,
         generate these in-place using macros. Thus eliminating "call"
         penalty.

 Documentation/driver-api/serial/driver.rst |  3 +
 include/linux/serial_core.h                | 86 ++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst
index 23c6b956cd90..25775bf1fcc6 100644
--- a/Documentation/driver-api/serial/driver.rst
+++ b/Documentation/driver-api/serial/driver.rst
@@ -78,6 +78,9 @@ Other functions
            uart_get_lsr_info uart_handle_dcd_change uart_handle_cts_change
            uart_try_toggle_sysrq uart_get_console
 
+.. kernel-doc:: include/linux/serial_core.h
+   :identifiers: DEFINE_UART_PORT_TX_HELPER_LIMITED DEFINE_UART_PORT_TX_HELPER
+
 Other notes
 -----------
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 6e4f4765d209..715778160ae1 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -646,6 +646,92 @@ struct uart_driver {
 
 void uart_write_wakeup(struct uart_port *port);
 
+#define __DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,  \
+		tx_done, for_test, for_post, ...)			  \
+unsigned int name(struct uart_port *port __VA_OPT__(,) __VA_ARGS__)	  \
+{									  \
+	struct circ_buf *xmit = &port->state->xmit;			  \
+	unsigned int pending;						  \
+	u8 ch;								  \
+									  \
+	for (; (for_test) && (tx_ready); (for_post), port->icount.tx++) { \
+		if (port->x_char) {					  \
+			ch = port->x_char;				  \
+			(put_char);					  \
+			port->x_char = 0;				  \
+			continue;					  \
+		}							  \
+									  \
+		if (uart_circ_empty(xmit) || uart_tx_stopped(port))	  \
+			break;						  \
+									  \
+		ch = xmit->buf[xmit->tail];				  \
+		(put_char);						  \
+		xmit->tail = (xmit->tail + 1) % UART_XMIT_SIZE;		  \
+	}								  \
+									  \
+	(tx_done);							  \
+									  \
+	pending = uart_circ_chars_pending(xmit);			  \
+	if (pending < WAKEUP_CHARS) {					  \
+		uart_write_wakeup(port);				  \
+									  \
+		if (pending == 0)					  \
+			port->ops->stop_tx(port);			  \
+	}								  \
+									  \
+	return pending;							  \
+}
+
+/**
+ * DEFINE_UART_PORT_TX_HELPER_LIMITED -- generate transmit helper for uart_port
+ *	with count limiting
+ * @name: name of the helper to generate
+ * @port: name of variable holding uart_port
+ * @ch: name of variable holding a character to write
+ * @tx_ready: can HW accept more data function
+ * @put_char: function to write a character
+ * @tx_done: function to call after the loop is done
+ *
+ * This macro generates a function @name. The generated function is meant as a
+ * helper to transmit characters from the xmit buffer to the hardware using
+ * @put_char(). It does so until count (passed to @name) characters are sent
+ * and while @tx_ready() still returns non-zero (if non-NULL).
+ *
+ * The generated function returns the number of characters in the xmit buffer
+ * when done.
+ *
+ * The functions in parameters shall be designed as follows:
+ *  * **tx_ready(port):** the function shall return true if the HW can accept
+ *    more data to be sent. This function can be %NULL, which means the HW is
+ *    always ready.
+ *  * **put_char(port, ch):** the function shall write @ch to the device.
+ *  * **tx_done(port):** when the write loop is done, this function can
+ *    perform arbitrary action before potential invocation of ops->stop_tx()
+ *    happens. This function can be %NULL.
+ *
+ * For all of them, @port->lock is held, interrupts are locally disabled and
+ * the functions must not sleep.
+ */
+#define DEFINE_UART_PORT_TX_HELPER_LIMITED(name, port, ch, tx_ready, put_char, \
+		                tx_done)				       \
+	__DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,       \
+			tx_done, count, count--, unsigned int count)
+
+/**
+ * DEFINE_UART_PORT_TX_HELPER -- generate transmit helper for uart_port
+ * @name: name of the helper to generate
+ * @port: name of variable holding uart_port
+ * @ch: name of variable holding a character to write
+ * @tx_ready: can HW accept more data function
+ * @put_char: function to write a character
+ *
+ * See DEFINE_UART_PORT_TX_HELPER_LIMITED() for more details.
+ */
+#define DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char)	 \
+	__DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char, \
+			({}), true, ({}))
+
 /*
  * Baud rate helpers.
  */
-- 
2.37.2

On Thu, 1 Sep 2022, 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 helper generators:
> * DEFINE_UART_PORT_TX_HELPER_LIMITED() -- it performs the above taking
>   the written characters limit into account, and
> * DEFINE_UART_PORT_TX_HELPER() -- the same as above, except it only
>   checks the HW readiness, not the characters limit.
> 
> The HW specific operations (as stated as "differences" above) are passed
> as arguments to the macros. They are:
> * 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.
> 
> Note that the above macros are generators. This means the code is
> generated in place and the above 3 arguments are "inlined". I.e. no
> added penalty by generating call instructions for every single
> character. Nor any indirect calls. (As in previous versions of this
> patchset.)
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
> 
> Notes:
>     [v2] instead of a function (uart_port_tx_limit()) in serial_core,
>          generate these in-place using macros. Thus eliminating "call"
>          penalty.
> 
>  Documentation/driver-api/serial/driver.rst |  3 +
>  include/linux/serial_core.h                | 86 ++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
> 
> diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst
> index 23c6b956cd90..25775bf1fcc6 100644
> --- a/Documentation/driver-api/serial/driver.rst
> +++ b/Documentation/driver-api/serial/driver.rst
> @@ -78,6 +78,9 @@ Other functions
>             uart_get_lsr_info uart_handle_dcd_change uart_handle_cts_change
>             uart_try_toggle_sysrq uart_get_console
>  
> +.. kernel-doc:: include/linux/serial_core.h
> +   :identifiers: DEFINE_UART_PORT_TX_HELPER_LIMITED DEFINE_UART_PORT_TX_HELPER
> +
>  Other notes
>  -----------
>  
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 6e4f4765d209..715778160ae1 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -646,6 +646,92 @@ struct uart_driver {
>  
>  void uart_write_wakeup(struct uart_port *port);
>  
> +#define __DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,  \
> +		tx_done, for_test, for_post, ...)			  \
> +unsigned int name(struct uart_port *port __VA_OPT__(,) __VA_ARGS__)	  \
> +{									  \
> +	struct circ_buf *xmit = &port->state->xmit;			  \
> +	unsigned int pending;						  \
> +	u8 ch;								  \
> +									  \
> +	for (; (for_test) && (tx_ready); (for_post), port->icount.tx++) { \

> + * The functions in parameters shall be designed as follows:
> + *  * **tx_ready(port):** the function shall return true if the HW can accept
> + *    more data to be sent. This function can be %NULL, which means the HW is
> + *    always ready.

So if tx_ready can be NULL, how does that for() loop above work??

-- 
 i.

On 02. 09. 22, 12:22, Ilpo Järvinen wrote:
> On Thu, 1 Sep 2022, 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 helper generators:
>> * DEFINE_UART_PORT_TX_HELPER_LIMITED() -- it performs the above taking
>>    the written characters limit into account, and
>> * DEFINE_UART_PORT_TX_HELPER() -- the same as above, except it only
>>    checks the HW readiness, not the characters limit.
>>
>> The HW specific operations (as stated as "differences" above) are passed
>> as arguments to the macros. They are:
>> * 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.
>>
>> Note that the above macros are generators. This means the code is
>> generated in place and the above 3 arguments are "inlined". I.e. no
>> added penalty by generating call instructions for every single
>> character. Nor any indirect calls. (As in previous versions of this
>> patchset.)
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> ---
>>
>> Notes:
>>      [v2] instead of a function (uart_port_tx_limit()) in serial_core,
>>           generate these in-place using macros. Thus eliminating "call"
>>           penalty.
>>
>>   Documentation/driver-api/serial/driver.rst |  3 +
>>   include/linux/serial_core.h                | 86 ++++++++++++++++++++++
>>   2 files changed, 89 insertions(+)
>>
>> diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst
>> index 23c6b956cd90..25775bf1fcc6 100644
>> --- a/Documentation/driver-api/serial/driver.rst
>> +++ b/Documentation/driver-api/serial/driver.rst
>> @@ -78,6 +78,9 @@ Other functions
>>              uart_get_lsr_info uart_handle_dcd_change uart_handle_cts_change
>>              uart_try_toggle_sysrq uart_get_console
>>   
>> +.. kernel-doc:: include/linux/serial_core.h
>> +   :identifiers: DEFINE_UART_PORT_TX_HELPER_LIMITED DEFINE_UART_PORT_TX_HELPER
>> +
>>   Other notes
>>   -----------
>>   
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index 6e4f4765d209..715778160ae1 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -646,6 +646,92 @@ struct uart_driver {
>>   
>>   void uart_write_wakeup(struct uart_port *port);
>>   
>> +#define __DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,  \
>> +		tx_done, for_test, for_post, ...)			  \
>> +unsigned int name(struct uart_port *port __VA_OPT__(,) __VA_ARGS__)	  \
>> +{									  \
>> +	struct circ_buf *xmit = &port->state->xmit;			  \
>> +	unsigned int pending;						  \
>> +	u8 ch;								  \
>> +									  \
>> +	for (; (for_test) && (tx_ready); (for_post), port->icount.tx++) { \
> 
>> + * The functions in parameters shall be designed as follows:
>> + *  * **tx_ready(port):** the function shall return true if the HW can accept
>> + *    more data to be sent. This function can be %NULL, which means the HW is
>> + *    always ready.
> 
> So if tx_ready can be NULL, how does that for() loop above work??

Sorry, the docs is wrong (corresponded to v1). I fixed it locally already:

https://git.kernel.org/pub/scm/linux/kernel/git/jirislaby/linux.git/commit/?h=devel&id=989c488c69faf2987648e09358c79d75b4a3b5c7

thanks,
-- 
js
suse labs

On Thu, Sep 01, 2022 at 01:06:55PM +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 helper generators:
> * DEFINE_UART_PORT_TX_HELPER_LIMITED() -- it performs the above taking
>   the written characters limit into account, and
> * DEFINE_UART_PORT_TX_HELPER() -- the same as above, except it only
>   checks the HW readiness, not the characters limit.
> 
> The HW specific operations (as stated as "differences" above) are passed
> as arguments to the macros. They are:
> * 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.
> 
> Note that the above macros are generators. This means the code is
> generated in place and the above 3 arguments are "inlined". I.e. no
> added penalty by generating call instructions for every single
> character. Nor any indirect calls. (As in previous versions of this
> patchset.)
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
> 
> Notes:
>     [v2] instead of a function (uart_port_tx_limit()) in serial_core,
>          generate these in-place using macros. Thus eliminating "call"
>          penalty.

Much nicer, but:

> +#define __DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,  \
> +		tx_done, for_test, for_post, ...)			  \

Do you really need "port" and "ch" as part of this macro?  You always
set that to the same thing in your patches, so is it really needed?

thanks,

greg k-h

On 01. 09. 22, 14:25, Greg KH wrote:
> Much nicer, but:
> 
>> +#define __DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,  \
>> +		tx_done, for_test, for_post, ...)			  \
> 
> Do you really need "port" and "ch" as part of this macro?  You always
> set that to the same thing in your patches, so is it really needed?

Not really, just to make obvious that those are the names that can be 
used in tx_ready, put_char... I can remove it, if you prefer, of course.

thanks,
-- 
js
suse labs

On Fri, Sep 02, 2022 at 07:16:58AM +0200, Jiri Slaby wrote:
> On 01. 09. 22, 14:25, Greg KH wrote:
> > Much nicer, but:
> > 
> > > +#define __DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,  \
> > > +		tx_done, for_test, for_post, ...)			  \
> > 
> > Do you really need "port" and "ch" as part of this macro?  You always
> > set that to the same thing in your patches, so is it really needed?
> 
> Not really, just to make obvious that those are the names that can be used
> in tx_ready, put_char... I can remove it, if you prefer, of course.

I'd recommend just removing it as it's a hard macro to read as-is.  That
would make it a bit more simple as then you are just passing in the name
and the callback functions, which makes a bit more sense to me.

thanks,

greg k-h

On 02. 09. 22, 7:23, Greg KH wrote:
> On Fri, Sep 02, 2022 at 07:16:58AM +0200, Jiri Slaby wrote:
>> On 01. 09. 22, 14:25, Greg KH wrote:
>>> Much nicer, but:
>>>
>>>> +#define __DEFINE_UART_PORT_TX_HELPER(name, port, ch, tx_ready, put_char,  \
>>>> +		tx_done, for_test, for_post, ...)			  \
>>>
>>> Do you really need "port" and "ch" as part of this macro?  You always
>>> set that to the same thing in your patches, so is it really needed?
>>
>> Not really, just to make obvious that those are the names that can be used
>> in tx_ready, put_char... I can remove it, if you prefer, of course.
> 
> I'd recommend just removing it as it's a hard macro to read as-is.  That
> would make it a bit more simple as then you are just passing in the name
> and the callback functions, which makes a bit more sense to me.

OK, I'll wait a couple of days if more comments/acks/reviews come and 
will send a v3.

thanks,
-- 
js
suse labs