[PATCH v2 0/3] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[PATCH v2 0/3] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[Michael Pratt]

Hi all,

On Thu, Jan 11, 2024 I submitted a single patch solution
for this issue of data overrun because of the default timeout
value which is insufficient for very low baud rates
with the internal fifo device enabled.

Following the advice of Vamshi Gajjela in the v1 thread,
I tried to find a way to limit the amount of math
being done within the write operation, and this is a possibility.

In this series, I'm proposing we bring back the
"timeout" member of struct uart_port, and also that we
add a new member of struct uart_8250_port in order
to store whether the fifo device has been enabled.

This way, in the function wait_for_lsr()
all we need to do is convert the value from jiffies to usecs
in order to have the appropriate timeout value.

I'm hoping to get this in during the RC phase of v6.9
and I'm trying to keep this fix as simple as possible,
or at least I think this is the appropriate time to
bring this issue up again...

Thanks in advance for your time.

--
MCP

Michael Pratt (3):
  serial: core: Store fifo timeout again
  serial: 8250: Store whether fifo device is enabled
  serial: 8250: Set fifo timeout using uart_fifo_timeout()

 drivers/tty/serial/8250/8250_port.c | 7 ++++++-
 drivers/tty/serial/serial_core.c    | 3 +++
 include/linux/serial_8250.h         | 1 +
 include/linux/serial_core.h         | 1 +
 4 files changed, 11 insertions(+), 1 deletion(-)


base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
--
2.30.2

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

[Michael Pratt]

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

[PATCH v2 2/3] serial: 8250: Store whether fifo device is enabled

[Michael Pratt]

Currently, there are 7 checks for whether to enable
the internal fifo device of a 8250/16550 type uart.

Instead of checking all 7 values again whenever
we need to know whether we have the fifo device enabled,
store the result as a struct member of uart_8250_port.

This can, for example, lessen the amount
of calculations done during a write operation.

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

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

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index fc9dd5d45295..5b0cfe6bc98c 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		 */
 		!(up->port.flags & UPF_CONS_FLOW);
 
+	up->fifo_enable = use_fifo;
+
 	if (likely(use_fifo))
 		serial8250_console_fifo_write(up, s, count);
 	else
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index fd59ed2cca53..017429f0e743 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -127,6 +127,7 @@ struct uart_8250_port {
 	struct list_head	list;		/* ports on this IRQ */
 	u32			capabilities;	/* port capabilities */
 	u16			bugs;		/* port bugs */
+	unsigned int		fifo_enable;	/* fifo enabled if capable */
 	unsigned int		tx_loadsz;	/* transmit fifo load size */
 	unsigned char		acr;
 	unsigned char		fcr;
-- 
2.30.2

[PATCH v2 3/3] serial: 8250: Set fifo timeout using uart_fifo_timeout()

[Michael Pratt]

Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
reworked functions for basic 8250 and 16550 type serial devices
in order to enable and use the internal fifo device for buffering,
however the default timeout of 10 ms remained, which is proving
to be insufficient for low baud rates like 9600, causing data overrun.

Unforunately, that commit was written and accepted just before commit
31f6bd7fad3b ("serial: Store character timing information to uart_port")
which introduced the frame_time member of the uart_port struct
in order to store the amount of time it takes to send one uart frame
relative to the baud rate and other serial port configuration,
and commit f9008285bb69 ("serial: Drop timeout from uart_port")
which established function uart_fifo_timeout() in order to
calculate a reasonable timeout to wait until flushing
the fifo device before writing data again when partially filled,
using the now stored frame_time value and size of the buffer.

Fix this by using the stored timeout value made with this new function
to calculate the timeout for the fifo device, when enabled, and when
the buffer is larger than 1 byte (unknown port default).

The previous 2 commits add the struct members used here
in order to store the values, so that the calculations
are offloaded from the functions that are called
during a write operation for better performance.

Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.

Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
Signed-off-by: Michael Pratt <mcpratt@pm.me>
---
V1 -> V2:
 Use stored values instead of calling uart_fifo_timeout()
 or checking capability flags.
 The existence of the timeout value satisfies fifosize > 1.

 drivers/tty/serial/8250/8250_port.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 5b0cfe6bc98c..cf67911a74f5 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2066,7 +2066,10 @@ static void wait_for_lsr(struct uart_8250_port *up, int bits)
 {
 	unsigned int status, tmout = 10000;
 
-	/* Wait up to 10ms for the character(s) to be sent. */
+	/* Wait for a time relative to buffer size and baud */
+	if (up->fifo_enable && up->port.timeout)
+		tmout = jiffies_to_usecs(up->port.timeout);
+
 	for (;;) {
 		status = serial_lsr_in(up);
 
-- 
2.30.2

Re: [PATCH v2 2/3] serial: 8250: Store whether fifo device is enabled

[Andy Shevchenko]

On Tue, Apr 16, 2024 at 06:29:56PM +0000, Michael Pratt wrote:
> Currently, there are 7 checks for whether to enable
> the internal fifo device of a 8250/16550 type uart.
> 
> Instead of checking all 7 values again whenever
> we need to know whether we have the fifo device enabled,
> store the result as a struct member of uart_8250_port.
> 
> This can, for example, lessen the amount
> of calculations done during a write operation.

...

> @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,

> +	up->fifo_enable = use_fifo;

This seems incorrect / not the only one place to assign this. What if the
console not enabled at compile time? What if it's not enabled at boot time?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] serial: 8250: Set fifo timeout using uart_fifo_timeout()

[Andy Shevchenko]

On Tue, Apr 16, 2024 at 06:34:10PM +0000, Michael Pratt wrote:
> Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
> reworked functions for basic 8250 and 16550 type serial devices
> in order to enable and use the internal fifo device for buffering,
> however the default timeout of 10 ms remained, which is proving
> to be insufficient for low baud rates like 9600, causing data overrun.
> 
> Unforunately, that commit was written and accepted just before commit
> 31f6bd7fad3b ("serial: Store character timing information to uart_port")
> which introduced the frame_time member of the uart_port struct
> in order to store the amount of time it takes to send one uart frame
> relative to the baud rate and other serial port configuration,
> and commit f9008285bb69 ("serial: Drop timeout from uart_port")
> which established function uart_fifo_timeout() in order to
> calculate a reasonable timeout to wait until flushing
> the fifo device before writing data again when partially filled,
> using the now stored frame_time value and size of the buffer.
> 
> Fix this by using the stored timeout value made with this new function
> to calculate the timeout for the fifo device, when enabled, and when
> the buffer is larger than 1 byte (unknown port default).
> 
> The previous 2 commits add the struct members used here
> in order to store the values, so that the calculations
> are offloaded from the functions that are called
> during a write operation for better performance.
> 
> Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.

...

>  	unsigned int status, tmout = 10000;
>  
> -	/* Wait up to 10ms for the character(s) to be sent. */
> +	/* Wait for a time relative to buffer size and baud */
> +	if (up->fifo_enable && up->port.timeout)
> +		tmout = jiffies_to_usecs(up->port.timeout);

Why do we still use that default? Can't we calculate timeout even for\
FIFO-less / FIFO-disabled devices?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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 2/3] serial: 8250: Store whether fifo device is enabled

[Michael Pratt]

Hi Andy,

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

> 
> > @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> 
> > + up->fifo_enable = use_fifo;
> 
> 
> This seems incorrect / not the only one place to assign this. What if the
> console not enabled at compile time? What if it's not enabled at boot time?
>

This is 8250 specific, and currently, it's the only place there
where it's decided whether or not to use the fifo device
by checking a bunch of flags and values.

If you're suggesting that these checks are moved out of this function somewhere else,
I would probably agree with that, but let's save that idea for the future...

If you're suggesting that there could be a null pointer, I don't think that's possible
in this function... (the name of the pointer being "up" might be confusing?)

Sorry if I'm misunderstanding what you mean.

> --
> With Best Regards,
> Andy Shevchenko

--
MCP

Re: [PATCH v2 3/3] serial: 8250: Set fifo timeout using uart_fifo_timeout()

[Michael Pratt]

Hi Andy,

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

> 
> > unsigned int status, tmout = 10000;
> > 
> > - /* Wait up to 10ms for the character(s) to be sent. /
> > + / Wait for a time relative to buffer size and baud */
> > + if (up->fifo_enable && up->port.timeout)
> > + tmout = jiffies_to_usecs(up->port.timeout);
> 
> 
> Why do we still use that default? Can't we calculate timeout even for\
> FIFO-less / FIFO-disabled devices?

Maybe it's possible that there is some kind of rare case where the LSR register
is not working or not configured properly for a device in which support
is being worked on...without a timeout, that would result in an infinite loop.

AFAIK, when everything is working properly, there is no such thing as needing
a timeout for a uart device without fifo, as every single byte written would trigger
an interrupt anyway.

> --
> With Best Regards,
> Andy Shevchenko

--
MCP

Re: [PATCH v2 2/3] serial: 8250: Store whether fifo device is enabled

[Andy Shevchenko]

On Tue, Apr 16, 2024 at 07:09:52PM +0000, Michael Pratt wrote:
> On Tuesday, April 16th, 2024 at 14:55, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> > > @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> > 
> > > + up->fifo_enable = use_fifo;
> > 
> > This seems incorrect / not the only one place to assign this. What if the
> > console not enabled at compile time? What if it's not enabled at boot time?
> 
> This is 8250 specific, and currently, it's the only place there
> where it's decided whether or not to use the fifo device
> by checking a bunch of flags and values.

Exactly, as initial commit is related to the kernel console _only_.
While your code, IIUC (correct me, if I'm wrong) is for any use of the port.

> If you're suggesting that these checks are moved out of this function somewhere else,
> I would probably agree with that, but let's save that idea for the future...

Not really (again, IIUC above), as console can be not enabled, and hence
serial8250_console_write() never been called and you will have false impression
that there is no FIFO in use.

> If you're suggesting that there could be a null pointer, I don't think that's possible
> in this function... (the name of the pointer being "up" might be confusing?)
> 
> Sorry if I'm misunderstanding what you mean.

-- 
With Best Regards,
Andy Shevchenko

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

[Michael Pratt]

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 2/3] serial: 8250: Store whether fifo device is enabled

[Michael Pratt]

Hi Andy,

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

> 
> 
> On Tue, Apr 16, 2024 at 07:09:52PM +0000, Michael Pratt wrote:
> 
> > On Tuesday, April 16th, 2024 at 14:55, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
> 
> > > > @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> > > 
> > > > + up->fifo_enable = use_fifo;
> > > 
> > > This seems incorrect / not the only one place to assign this. What if the
> > > console not enabled at compile time? What if it's not enabled at boot time?
> > 
> > This is 8250 specific, and currently, it's the only place there
> > where it's decided whether or not to use the fifo device
> > by checking a bunch of flags and values.
> 
> 
> Exactly, as initial commit is related to the kernel console only.
> While your code, IIUC (correct me, if I'm wrong) is for any use of the port.
> 
> > If you're suggesting that these checks are moved out of this function somewhere else,
> > I would probably agree with that, but let's save that idea for the future...
> 
> 
> Not really (again, IIUC above), as console can be not enabled, and hence
> serial8250_console_write() never been called and you will have false impression
> that there is no FIFO in use.

Ah ok, I understand now...

So there are cases where the function with the checks will never be called,
yet the device itself will be configured the same way and the struct member I am adding
will still be instantiated with value of 0 and never be set elsewhere... and because
it is declared in a major struct "uart_8250_port", it appears to apply to a larger scope
compared to the way it is actually being used...
(or at the very least, the name "fifo_enable" would be misleading).

Thanks for pointing that out, I'll take a deeper dive into the file...


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

--
MCP

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

[Ilpo Järvinen]

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 3/3] serial: 8250: Set fifo timeout using uart_fifo_timeout()

[Ilpo Järvinen]

On Tue, 16 Apr 2024, Michael Pratt wrote:

> Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
> reworked functions for basic 8250 and 16550 type serial devices
> in order to enable and use the internal fifo device for buffering,
> however the default timeout of 10 ms remained, which is proving
> to be insufficient for low baud rates like 9600, causing data overrun.
> 
> Unforunately, that commit was written and accepted just before commit
> 31f6bd7fad3b ("serial: Store character timing information to uart_port")
> which introduced the frame_time member of the uart_port struct
> in order to store the amount of time it takes to send one uart frame
> relative to the baud rate and other serial port configuration,
> and commit f9008285bb69 ("serial: Drop timeout from uart_port")
> which established function uart_fifo_timeout() in order to
> calculate a reasonable timeout to wait until flushing
> the fifo device before writing data again when partially filled,
> using the now stored frame_time value and size of the buffer.
> 
> Fix this by using the stored timeout value made with this new function
> to calculate the timeout for the fifo device, when enabled, and when
> the buffer is larger than 1 byte (unknown port default).
> 
> The previous 2 commits add the struct members used here
> in order to store the values, so that the calculations
> are offloaded from the functions that are called
> during a write operation for better performance.
> 
> Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.

Did you actually see some perfomance issue with 115000 bps? Or did you 
just make the "better performance" claim up?

> Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> V1 -> V2:
>  Use stored values instead of calling uart_fifo_timeout()
>  or checking capability flags.
>  The existence of the timeout value satisfies fifosize > 1.
> 
>  drivers/tty/serial/8250/8250_port.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 5b0cfe6bc98c..cf67911a74f5 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2066,7 +2066,10 @@ static void wait_for_lsr(struct uart_8250_port *up, int bits)
>  {
>  	unsigned int status, tmout = 10000;
>  
> -	/* Wait up to 10ms for the character(s) to be sent. */
> +	/* Wait for a time relative to buffer size and baud */
> +	if (up->fifo_enable && up->port.timeout)
> +		tmout = jiffies_to_usecs(up->port.timeout);
> +
>  	for (;;) {
>  		status = serial_lsr_in(up);

Hi,

Is this the only reason for adding the timeout field? I think you should 
just refactor the code such that you don't need to recalculate the timeout 
for each characted when writing n characters?

-- 
 i.

Re: [PATCH v2 3/3] serial: 8250: Set fifo timeout using uart_fifo_timeout()

[Ilpo Järvinen]

On Tue, 16 Apr 2024, Michael Pratt wrote:
> On Tuesday, April 16th, 2024 at 14:57, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > > unsigned int status, tmout = 10000;
> > > 
> > > - /* Wait up to 10ms for the character(s) to be sent. /
> > > + / Wait for a time relative to buffer size and baud */
> > > + if (up->fifo_enable && up->port.timeout)
> > > + tmout = jiffies_to_usecs(up->port.timeout);
> > 
> > 
> > Why do we still use that default? Can't we calculate timeout even for\
> > FIFO-less / FIFO-disabled devices?

Yes we definitely should be able to. Unfortunately these patches just keep 
coming back not in the form that follows the review feedback, but they 
come up their own way of doing things which is way worse and ignores the 
given feedback.

> Maybe it's possible that there is some kind of rare case where the LSR register
> is not working or not configured properly for a device in which support
> is being worked on...without a timeout, that would result in an infinite loop.

"without a timeout" is not what Andy said. He said you should always have 
a timeout, regardless of there being FIFO or not. And that timeout should 
be derived in the same manner from baudrate and FIFO size (to address the
cases w/o FIFO, the fifosize should be lower bounded to 1 while 
calculating the FIFO timeout).

> AFAIK, when everything is working properly, there is no such thing as needing
> a timeout for a uart device without fifo, as every single byte written would trigger
> an interrupt anyway.

While I agree the general principle, that this is backup that should not 
even be needed, the statement is partly incorrect. We don't get interrupts 
during console write because they're disabled. But LSR should still change 
and allow progress without the backup timeout.

-- 
 i.

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

[Ilpo Järvinen]

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.

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

[Wander Lairson Costa]

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
>