diff mbox series

[1/2] serial: imx: Introduce timeout when waiting on transmitter empty

Message ID 76cf9ce9cbf9dcdf78bc00ce7a919db1776ebce1.1712309058.git.esben@geanix.com (mailing list archive)
State Superseded
Headers show
Series [1/2] serial: imx: Introduce timeout when waiting on transmitter empty | expand

Commit Message

Esben Haabendal April 5, 2024, 9:25 a.m. UTC
By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
deadlock.

In case of the timeout, there is not much we can do, so we simply ignore
the transmitter state and optimistically try to continue.

Signed-off-by: Esben Haabendal <esben@geanix.com>
Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/tty/serial/imx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Marc Kleine-Budde April 5, 2024, 9:49 a.m. UTC | #1
On 05.04.2024 11:25:13, Esben Haabendal wrote:
> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
> deadlock.
> 
> In case of the timeout, there is not much we can do, so we simply ignore
> the transmitter state and optimistically try to continue.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

Where's the cover letter and patch 2/2? Have a look at b4 [1], it's a
great tool to help you with sending git patch series.

[1] https://b4.docs.kernel.org/en/latest/

Marc
Esben Haabendal April 5, 2024, 5:22 p.m. UTC | #2
Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 05.04.2024 11:25:13, Esben Haabendal wrote:
>> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
>> deadlock.
>> 
>> In case of the timeout, there is not much we can do, so we simply ignore
>> the transmitter state and optimistically try to continue.
>> 
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
>
> Where's the cover letter and patch 2/2? Have a look at b4 [1], it's a
> great tool to help you with sending git patch series.

It is left out on purpose.

This patch is a stand-alone patch as it is. The other part of the series
you are talking about is not going to mainline for now. It needs still
quite some work, and will only go in after all the other printk stuff.

I hope we can merge this patch as it to mainline now, instead of piling
up more than necessary in the rt tree.

/Esben
Marc Kleine-Budde April 5, 2024, 5:33 p.m. UTC | #3
On 05.04.2024 19:22:29, Esben Haabendal wrote:
> Marc Kleine-Budde <mkl@pengutronix.de> writes:
> 
> > On 05.04.2024 11:25:13, Esben Haabendal wrote:
> >> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
> >> deadlock.
> >> 
> >> In case of the timeout, there is not much we can do, so we simply ignore
> >> the transmitter state and optimistically try to continue.
> >> 
> >> Signed-off-by: Esben Haabendal <esben@geanix.com>
> >> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >
> > Where's the cover letter and patch 2/2? Have a look at b4 [1], it's a
> > great tool to help you with sending git patch series.
> 
> It is left out on purpose.
> 
> This patch is a stand-alone patch as it is. The other part of the series
> you are talking about is not going to mainline for now. It needs still
> quite some work, and will only go in after all the other printk stuff.
> 
> I hope we can merge this patch as it to mainline now, instead of piling
> up more than necessary in the rt tree.

Ok, then send it as patch 1/1.

Marc
Fabio Estevam April 5, 2024, 5:38 p.m. UTC | #4
On Fri, Apr 5, 2024 at 6:25 AM Esben Haabendal <esben@geanix.com> wrote:
>
> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital

s/potentital/potential

Could you elaborate on this deadlock? Have you seen it in practice?

Should a Fixes tag be added?
Sergey Organov April 5, 2024, 7:05 p.m. UTC | #5
Fabio Estevam <festevam@gmail.com> writes:

> On Fri, Apr 5, 2024 at 6:25 AM Esben Haabendal <esben@geanix.com> wrote:
>>
>> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
>
> s/potentital/potential
>
> Could you elaborate on this deadlock? Have you seen it in practice?

I've stumped upon this piece of code a long time ago, and it's indeed
broken. However, to actually see a "deadlock", I believe one needs to
enable hardware RTS/CTS handshake on the port, then, say, not connect
RS232 cable, and then printk(), if enabled to this port, will soon
result in the loop to be executed forever, that in turn will hang
single-CPU machine entirely (provided this code is still executed with
interrupts disabled, as it was at the time I investigated severe
printk()-induced ISR delays).

-- Sergey Organov
Esben Haabendal April 8, 2024, 8:56 a.m. UTC | #6
Fabio Estevam <festevam@gmail.com> writes:

> On Fri, Apr 5, 2024 at 6:25 AM Esben Haabendal <esben@geanix.com> wrote:
>>
>> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
>
> s/potentital/potential

Thanks, fixing.

> Could you elaborate on this deadlock? Have you seen it in practice?

I cannot say for sure if I have seen it. But in some cases, that is
exactly what you would see. Nothing.

If it would occur during shutdown, the console would simply stop/block,
and you would see nothing.

> Should a Fixes tag be added?

Which commit should I add to that tag? The polling without timeout dates
back to at least 2.6.12-rc2.

/Esben
Esben Haabendal April 8, 2024, 8:57 a.m. UTC | #7
Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 05.04.2024 19:22:29, Esben Haabendal wrote:
>> Marc Kleine-Budde <mkl@pengutronix.de> writes:
>> 
>> > On 05.04.2024 11:25:13, Esben Haabendal wrote:
>> >> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potentital
>> >> deadlock.
>> >> 
>> >> In case of the timeout, there is not much we can do, so we simply ignore
>> >> the transmitter state and optimistically try to continue.
>> >> 
>> >> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> >> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> >
>> > Where's the cover letter and patch 2/2? Have a look at b4 [1], it's a
>> > great tool to help you with sending git patch series.
>> 
>> It is left out on purpose.
>> 
>> This patch is a stand-alone patch as it is. The other part of the series
>> you are talking about is not going to mainline for now. It needs still
>> quite some work, and will only go in after all the other printk stuff.
>> 
>> I hope we can merge this patch as it to mainline now, instead of piling
>> up more than necessary in the rt tree.
>
> Ok, then send it as patch 1/1.

Sure. Sorry about that.

/Esben
diff mbox series

Patch

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e14813250616..09c1678ddfd4 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -26,6 +26,7 @@ 
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/dma-mapping.h>
 
 #include <asm/irq.h>
@@ -2010,7 +2011,7 @@  imx_uart_console_write(struct console *co, const char *s, unsigned int count)
 	struct imx_port *sport = imx_uart_ports[co->index];
 	struct imx_port_ucrs old_ucr;
 	unsigned long flags;
-	unsigned int ucr1;
+	unsigned int ucr1, usr2;
 	int locked = 1;
 
 	if (sport->port.sysrq)
@@ -2041,8 +2042,8 @@  imx_uart_console_write(struct console *co, const char *s, unsigned int count)
 	 *	Finally, wait for transmitter to become empty
 	 *	and restore UCR1/2/3
 	 */
-	while (!(imx_uart_readl(sport, USR2) & USR2_TXDC));
-
+	read_poll_timeout_atomic(imx_uart_readl, usr2, usr2 & USR2_TXDC,
+				 0, USEC_PER_SEC, false, sport, USR2);
 	imx_uart_ucrs_restore(sport, &old_ucr);
 
 	if (locked)