* [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()
@ 2024-01-11 0:27 Michael Pratt
2024-01-11 6:52 ` Greg Kroah-Hartman
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Michael Pratt @ 2024-01-11 0:27 UTC (permalink / raw)
To: linux-kernel, linux-serial, Greg Kroah-Hartman, Jiri Slaby
Cc: Wander Lairson Costa, Ilpo Järvinen, Andy Shevchenko, 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 for all frames
in the FIFO device to flush before writing data again
using the now stored frame_time value and size of the buffer.
Fix this by using the new function to calculate the timeout
whenever the buffer is larger than 1 byte (unknown port default).
Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.
Signed-off-by: Michael Pratt <mcpratt@pm.me>
---
v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
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 8ca061d3bbb9..777b61a79c5e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2076,7 +2076,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->port.fifosize > 1)
+ tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
+
for (;;) {
status = serial_lsr_in(up);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()
2024-01-11 0:27 [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout() Michael Pratt
@ 2024-01-11 6:52 ` Greg Kroah-Hartman
2024-01-11 7:37 ` Michael Pratt
2024-01-11 9:53 ` VAMSHI GAJJELA
2024-01-11 11:32 ` Ilpo Järvinen
2 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2024-01-11 6:52 UTC (permalink / raw)
To: Michael Pratt
Cc: linux-kernel, linux-serial, Jiri Slaby, Wander Lairson Costa,
Ilpo Järvinen, Andy Shevchenko
On Thu, Jan 11, 2024 at 12:27:07AM +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 for all frames
> in the FIFO device to flush before writing data again
> using the now stored frame_time value and size of the buffer.
>
> Fix this by using the new function to calculate the timeout
> whenever the buffer is larger than 1 byte (unknown port default).
>
> Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.
>
> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
What commit id does this fix?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()
2024-01-11 6:52 ` Greg Kroah-Hartman
@ 2024-01-11 7:37 ` Michael Pratt
2024-01-11 11:38 ` Ilpo Järvinen
0 siblings, 1 reply; 6+ messages in thread
From: Michael Pratt @ 2024-01-11 7:37 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, linux-serial, Jiri Slaby, Wander Lairson Costa,
Ilpo Järvinen, Andy Shevchenko
On Thursday, January 11th, 2024 at 01:52, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Thu, Jan 11, 2024 at 12:27:07AM +0000, Michael Pratt wrote:
>
> > Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
This is the commit that made the issue present itself.
I'm not sure whether it's right to say that this "fixes" that commit
since it only caused the issue indirectly, and the diff for this patch
doesn't touch any lines that the other commit touched, and the method
I'm using to fix the issue was not available at the time, and also that
for high baud rates like 115200 everything is still fine...
(the 10 ms timeout is as old as the tree)
If that's enough for a "Fixes" tag then go ahead (or tell me to add it),
but maybe a "Ref" tag would be enough?
You can see the other thread linked for more discussion on that point if you like...
> > 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 for all frames
> > in the FIFO device to flush before writing data again
> > using the now stored frame_time value and size of the buffer.
> >
> > Fix this by using the new function to calculate the timeout
> > whenever the buffer is larger than 1 byte (unknown port default).
> >
> > Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.
> >
> > Signed-off-by: Michael Pratt mcpratt@pm.me
> > ---
> > v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
>
>
> What commit id does this fix?
>
> thanks,
>
> greg k-h
--
MCP
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()
2024-01-11 0:27 [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout() Michael Pratt
2024-01-11 6:52 ` Greg Kroah-Hartman
@ 2024-01-11 9:53 ` VAMSHI GAJJELA
2024-01-11 11:32 ` Ilpo Järvinen
2 siblings, 0 replies; 6+ messages in thread
From: VAMSHI GAJJELA @ 2024-01-11 9:53 UTC (permalink / raw)
To: Michael Pratt
Cc: linux-kernel, linux-serial, Greg Kroah-Hartman, Jiri Slaby,
Wander Lairson Costa, Ilpo Järvinen, Andy Shevchenko
On Thu, Jan 11, 2024 at 5:57 AM Michael Pratt <mcpratt@pm.me> 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 for all frames
> in the FIFO device to flush before writing data again
> using the now stored frame_time value and size of the buffer.
>
> Fix this by using the new function to calculate the timeout
> whenever the buffer is larger than 1 byte (unknown port default).
>
> Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.
>
> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
>
> 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 8ca061d3bbb9..777b61a79c5e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2076,7 +2076,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->port.fifosize > 1)
> + tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
> +
Michael, I have copied you onto a patch addressing the similar issue, where
it was considered to avoid the computation of timeout on call to wait_for_lsr.
> for (;;) {
> status = serial_lsr_in(up);
>
> --
> 2.30.2
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()
2024-01-11 0:27 [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout() Michael Pratt
2024-01-11 6:52 ` Greg Kroah-Hartman
2024-01-11 9:53 ` VAMSHI GAJJELA
@ 2024-01-11 11:32 ` Ilpo Järvinen
2 siblings, 0 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2024-01-11 11:32 UTC (permalink / raw)
To: Michael Pratt
Cc: LKML, linux-serial, Greg Kroah-Hartman, Jiri Slaby,
Wander Lairson Costa, Andy Shevchenko
On Thu, 11 Jan 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
Unfortunately,
--
i.
> 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 for all frames
> in the FIFO device to flush before writing data again
> using the now stored frame_time value and size of the buffer.
>
> Fix this by using the new function to calculate the timeout
> whenever the buffer is larger than 1 byte (unknown port default).
>
> Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.
>
> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
> 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 8ca061d3bbb9..777b61a79c5e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2076,7 +2076,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->port.fifosize > 1)
> + tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
> +
> for (;;) {
> status = serial_lsr_in(up);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()
2024-01-11 7:37 ` Michael Pratt
@ 2024-01-11 11:38 ` Ilpo Järvinen
0 siblings, 0 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2024-01-11 11:38 UTC (permalink / raw)
To: Michael Pratt
Cc: Greg Kroah-Hartman, LKML, linux-serial, Jiri Slaby,
Wander Lairson Costa, Andy Shevchenko
On Thu, 11 Jan 2024, Michael Pratt wrote:
>
>
> On Thursday, January 11th, 2024 at 01:52, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> > On Thu, Jan 11, 2024 at 12:27:07AM +0000, Michael Pratt wrote:
> >
> > > Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
>
> This is the commit that made the issue present itself.
>
> I'm not sure whether it's right to say that this "fixes" that commit
> since it only caused the issue indirectly, and the diff for this patch
> doesn't touch any lines that the other commit touched, and the method
> I'm using to fix the issue was not available at the time, and also that
> for high baud rates like 115200 everything is still fine...
> (the 10 ms timeout is as old as the tree)
>
> If that's enough for a "Fixes" tag then go ahead (or tell me to add it),
> but maybe a "Ref" tag would be enough?
>
> You can see the other thread linked for more discussion on that point if you like...
Besides Fixes tags, you can add dependency information for stable folks
with Cc tag using the format described in
Documentation/process/stable-kernel-rules.rst
--
i.
> > > 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 for all frames
> > > in the FIFO device to flush before writing data again
> > > using the now stored frame_time value and size of the buffer.
> > >
> > > Fix this by using the new function to calculate the timeout
> > > whenever the buffer is larger than 1 byte (unknown port default).
> > >
> > > Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.
> > >
> > > Signed-off-by: Michael Pratt mcpratt@pm.me
> > > ---
> > > v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
> >
> >
> > What commit id does this fix?
> >
> > thanks,
> >
> > greg k-h
>
> --
> MCP
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-11 11:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-11 0:27 [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout() Michael Pratt
2024-01-11 6:52 ` Greg Kroah-Hartman
2024-01-11 7:37 ` Michael Pratt
2024-01-11 11:38 ` Ilpo Järvinen
2024-01-11 9:53 ` VAMSHI GAJJELA
2024-01-11 11:32 ` Ilpo Järvinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).