| Message ID | 76cf9ce9cbf9dcdf78bc00ce7a919db1776ebce1.1712309058.git.esben@geanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] serial: imx: Introduce timeout when waiting on transmitter empty | expand |
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
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
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
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?
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
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
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 --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)