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 <[email protected]>
Acked-by: Marc Kleine-Budde <[email protected]>
---
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
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 <[email protected]>
> Acked-by: Marc Kleine-Budde <[email protected]>
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 |
Marc Kleine-Budde <[email protected]> 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 <[email protected]>
>> Acked-by: Marc Kleine-Budde <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
> >> Acked-by: Marc Kleine-Budde <[email protected]>
> >
> > 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 |
On Fri, Apr 5, 2024 at 6:25 AM Esben Haabendal <[email protected]> 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 <[email protected]> writes:
> On Fri, Apr 5, 2024 at 6:25 AM Esben Haabendal <[email protected]> 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 <[email protected]> writes:
> On Fri, Apr 5, 2024 at 6:25 AM Esben Haabendal <[email protected]> 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 <[email protected]> writes:
> On 05.04.2024 19:22:29, Esben Haabendal wrote:
>> Marc Kleine-Budde <[email protected]> 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 <[email protected]>
>> >> Acked-by: Marc Kleine-Budde <[email protected]>
>> >
>> > 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
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 <[email protected]>
Acked-by: Marc Kleine-Budde <[email protected]>
---
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
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
Greg Kroah-Hartman <[email protected]> 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
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 <[email protected]>
Acked-by: Marc Kleine-Budde <[email protected]>
---
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
Esben Haabendal <[email protected]> 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 <[email protected]>
> Acked-by: Marc Kleine-Budde <[email protected]>
> ---
>
> 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)