[v2] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[PATCH v2 1/3] serial: core: Store fifo timeout again

Posted by Michael Pratt 1 year, 8 months ago

This is a partial revert of Commit f9008285bb69
("serial: Drop timeout from uart_port").

In order to prevent having to calculate a timeout
for the fifo device during a write operation, if enabled,
calculate it ahead of time and store the value of the timeout
in a struct member of uart_port.

Signed-off-by: Michael Pratt <mcpratt@pm.me>
---
V1 -> V2: new commit

 drivers/tty/serial/serial_core.c | 3 +++
 include/linux/serial_core.h      | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ff85ebd3a007..9b3176d684a4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -414,6 +414,9 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
 
 	temp *= NSEC_PER_SEC;
 	port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);
+
+	if (port->fifosize > 1)
+		port->timeout = uart_fifo_timeout(port);
 }
 EXPORT_SYMBOL(uart_update_timeout);
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 0a0f6e21d40e..c6422021152f 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -561,6 +561,7 @@ struct uart_port {
 
 	bool			hw_stopped;		/* sw-assisted CTS flow state */
 	unsigned int		mctrl;			/* current modem ctrl settings */
+	unsigned long		timeout;		/* character-based timeout */
 	unsigned int		frame_time;		/* frame timing in ns */
 	unsigned int		type;			/* port type */
 	const struct uart_ops	*ops;
-- 
2.30.2

Re: [PATCH v2 1/3] serial: core: Store fifo timeout again

Posted by Ilpo Järvinen 1 year, 8 months ago

On Tue, 16 Apr 2024, Michael Pratt wrote:

> This is a partial revert of Commit f9008285bb69
> ("serial: Drop timeout from uart_port").
> 
> In order to prevent having to calculate a timeout
> for the fifo device during a write operation, if enabled,
> calculate it ahead of time and store the value of the timeout
> in a struct member of uart_port.

Hi,

Why is calculating during write bad/wrong, you don't give any real 
justification for this change? You're talking about "low rates" in cover 
letter, which makes it even more confusing because writes occur very 
infrequently so a few math operations are nothing to be concerned of (in 
timescales UARTs operate in, no math penalty is really worth even 
discussing IMO).

-- 
 i.

> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> V1 -> V2: new commit
> 
>  drivers/tty/serial/serial_core.c | 3 +++
>  include/linux/serial_core.h      | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index ff85ebd3a007..9b3176d684a4 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -414,6 +414,9 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
>  
>  	temp *= NSEC_PER_SEC;
>  	port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);
> +
> +	if (port->fifosize > 1)
> +		port->timeout = uart_fifo_timeout(port);
>  }
>  EXPORT_SYMBOL(uart_update_timeout);
>  
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 0a0f6e21d40e..c6422021152f 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -561,6 +561,7 @@ struct uart_port {
>  
>  	bool			hw_stopped;		/* sw-assisted CTS flow state */
>  	unsigned int		mctrl;			/* current modem ctrl settings */
> +	unsigned long		timeout;		/* character-based timeout */
>  	unsigned int		frame_time;		/* frame timing in ns */
>  	unsigned int		type;			/* port type */
>  	const struct uart_ops	*ops;
>

Re: [PATCH v2 1/3] serial: core: Store fifo timeout again

Posted by Andy Shevchenko 1 year, 8 months ago

On Tue, Apr 16, 2024 at 06:29:28PM +0000, Michael Pratt wrote:
> This is a partial revert of Commit f9008285bb69
> ("serial: Drop timeout from uart_port").
> 
> In order to prevent having to calculate a timeout
> for the fifo device during a write operation, if enabled,
> calculate it ahead of time and store the value of the timeout
> in a struct member of uart_port.

...

> +	if (port->fifosize > 1)
> +		port->timeout = uart_fifo_timeout(port);

	else
		port->timeout = port->frame_time;

?

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] serial: core: Store fifo timeout again

Posted by Michael Pratt 1 year, 8 months ago

Hi Andy,

On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> ...
> 
> > + if (port->fifosize > 1)
> > + port->timeout = uart_fifo_timeout(port);
> 
> 
> else
> port->timeout = port->frame_time;
> 

Consistent with what I said in the other reply, the only reason that
I have an if statement here, is to avoid doing extra math for devices
without a fifo, as a specifically calculated timeout value would be useless
in those cases.

However, if you don't like the 10 ms default timeout, perhaps port->frame_time
could actually be a more reasonable default value? That is, provided that we have a process
for calculating the proper value already in place...

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

Thanks for taking a look :D

--
MCP

Re: [PATCH v2 1/3] serial: core: Store fifo timeout again

Posted by Wander Lairson Costa 1 year, 8 months ago

On Tue, Apr 16, 2024 at 07:20:18PM +0000, Michael Pratt wrote:
> Hi Andy,
> 
> On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > ...
> > 
> > > + if (port->fifosize > 1)
> > > + port->timeout = uart_fifo_timeout(port);
> > 
> > 
> > else
> > port->timeout = port->frame_time;
> > 
> 
> 
> Consistent with what I said in the other reply, the only reason that
> I have an if statement here, is to avoid doing extra math for devices
> without a fifo, as a specifically calculated timeout value would be useless
> in those cases.
> 

As we are talking about a device with a scale of KB/s, I am attempted to
suggest to call uart_fifo_timeout() unconditionally.

> However, if you don't like the 10 ms default timeout, perhaps port->frame_time
> could actually be a more reasonable default value? That is, provided that we have a process
> for calculating the proper value already in place...
> 
> > 
> > --
> > With Best Regards,
> > Andy Shevchenko
> 
> 
> 
> Thanks for taking a look :D
> 
> --
> MCP
>

Re: [PATCH v2 1/3] serial: core: Store fifo timeout again

Posted by Ilpo Järvinen 1 year, 8 months ago

On Tue, 16 Apr 2024, Michael Pratt wrote:
> On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > > + if (port->fifosize > 1)
> > > + port->timeout = uart_fifo_timeout(port);
> > 
> > 
> > else
> > port->timeout = port->frame_time;
> > 
> 
> 
> Consistent with what I said in the other reply, the only reason that
> I have an if statement here, is to avoid doing extra math for devices
> without a fifo, as a specifically calculated timeout value would be useless
> in those cases.

Please benchmark to show this actually matters if want to make this claim. 
Otherwise just do the math always.

> However, if you don't like the 10 ms default timeout, perhaps port->frame_time
> could actually be a more reasonable default value? That is, provided 
> that we have a process 
> for calculating the proper value already in place...

While it would be a step toward the correct direction, you'd still need to 
add the safety there which is already done by uart_fifo_timeout(). So no, 
I don't think there's advantage of using port->frame_time over just 
calling uart_fifo_timeout() and ensuring uart_fifo_timeout() is always 
using at least 1 as the FIFO size when it does the calculations.

-- 
 i.