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

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

[Esben Haabendal]

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(-)

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)
-- 
2.44.0

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Esben Haabendal]

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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Fabio Estevam]

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?

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

[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
>
> 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

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

[Esben Haabendal]

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

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

[Esben Haabendal]

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

[PATCH v2] serial: imx: Introduce timeout when waiting on transmitter empty

[Esben Haabendal]

By waiting at most 1 second for USR2_TXDC to be set, we avoid a potential
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.

v2:
- Fixed commit message typo
- Remove reference to patch series it originated from. This is a
  stand-alone patch

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(-)

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)
-- 
2.44.0

Re: [PATCH v2] serial: imx: Introduce timeout when waiting on transmitter empty

[Greg Kroah-Hartman]

On Wed, Apr 10, 2024 at 09:18:32AM +0200, Esben Haabendal wrote:
> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potential
> 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.
> 
> v2:
> - Fixed commit message typo
> - Remove reference to patch series it originated from. This is a
>   stand-alone patch

The "v2:" stuff needs to go below the --- line, so it doesn't show up in
the kernel changelog.  The kernel documentation should describe this,
right?

Please fix up and send a v3.

thanks,

greg k-h

Re: [PATCH v2] serial: imx: Introduce timeout when waiting on transmitter empty

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Wed, Apr 10, 2024 at 09:18:32AM +0200, Esben Haabendal wrote:
>> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potential
>> 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.
>> 
>> v2:
>> - Fixed commit message typo
>> - Remove reference to patch series it originated from. This is a
>>   stand-alone patch
>
> The "v2:" stuff needs to go below the --- line, so it doesn't show up in
> the kernel changelog.  The kernel documentation should describe this,
> right?

Right. It is described in Documentation/process/submitting-patches.rst.
Sorry about that.

> Please fix up and send a v3.

On its way.

/Esben

[PATCH v3] serial: imx: Introduce timeout when waiting on transmitter empty

[Esben Haabendal]

By waiting at most 1 second for USR2_TXDC to be set, we avoid a potential
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>
---

v2:
- Fixed commit message typo
- Remove reference to patch series it originated from. This is a
  stand-alone patch

v3:
- Moved this version information into the correct patch section

 drivers/tty/serial/imx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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)
-- 
2.44.0

Re: [PATCH v3] serial: imx: Introduce timeout when waiting on transmitter empty

[Esben Haabendal]

Esben Haabendal <esben@geanix.com> writes:

> By waiting at most 1 second for USR2_TXDC to be set, we avoid a potential
> 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>
> ---
>
> v2:
> - Fixed commit message typo
> - Remove reference to patch series it originated from. This is a
>   stand-alone patch
>
> v3:
> - Moved this version information into the correct patch section

Anything more needed in order to get this merged?

/Esben

>
>  drivers/tty/serial/imx.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> 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)

Re: [PATCH v3] serial: imx: Introduce timeout when waiting on transmitter empty

[Greg Kroah-Hartman]

On Thu, May 02, 2024 at 11:14:26AM +0200, Esben Haabendal wrote:
> Esben Haabendal <esben@geanix.com> writes:
> 
> > By waiting at most 1 second for USR2_TXDC to be set, we avoid a potential
> > 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>
> > ---
> >
> > v2:
> > - Fixed commit message typo
> > - Remove reference to patch series it originated from. This is a
> >   stand-alone patch
> >
> > v3:
> > - Moved this version information into the correct patch section
> 
> Anything more needed in order to get this merged?

Sorry, but I don't see this in my review queue anymore.  If this isn't
already accepted, please resend it, sorry about that.

greg k-h