serial/amba-pl011: Disable interrupts around TX softirq

PATCH New Dave Martin 4 participants 8 messages Jun 5, 2015 – Jun 15, 2015

Subsystems (2)

TTY layer and serial drivers Arm primecell UART PL010 and PL011 drivers

lore

Share & tools

lore

Dave Martin

author

2015-06-05 14:07 UTC #0 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks.

This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete.

Signed-off-by Dave Martin <[email protected]>

---

Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring.

This patch conflicts with tty-next like the previous patch, since it fixes code that is removed by tty-next. The correct resolution for the resulting merge conflict is to keep the code from tty-next.

I am not 100% certain yet whether some rare deadlocks that Robin is seeing are caused by this issue, or whether this patch fixes them -- he's testing atm.

The patch is straightforward and using non _irq() locking in a workitem that expects the lock to protect against interrupt handlers on the same CPU is clearly wrong.

Cheers
---Dave

 drivers/tty/serial/amba-pl011.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tty/serial/amba-pl011.c +2 -2

index 763eb20..0cc622a 100644
@@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work)
 	struct uart_amba_port *uap =
 		container_of(dwork, struct uart_amba_port, tx_softirq_work);
 
-	spin_lock(&uap->port.lock);
+	spin_lock_irq(&uap->port.lock);
 	while (pl011_tx_chars(uap)) ;
-	spin_unlock(&uap->port.lock);
+	spin_unlock_irq(&uap->port.lock);
 }
 
 static void pl011_tx_irq_seen(struct uart_amba_port *uap)

Signature · 1 line

1.7.10.4

Robin Murphy

2015-06-05 18:03 UTC #1 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

Quoted 15 lines · Dave P Martin

pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks.
This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete.
Signed-off-by Dave Martin <[email protected]>
---
Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring.

Quoted 8 lines

This patch conflicts with tty-next like the previous patch, since it fixes code that is removed by tty-next. The correct resolution for the resulting merge conflict is to keep the code from tty-next.
I am not 100% certain yet whether some rare deadlocks that Robin is seeing are caused by this issue, or whether this patch fixes them -- he's testing atm.

FWIW, I've been running Juno in a startup/shutdown loop with a very noisy systemd all afternoon and haven't hit a problem yet with this patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep splats in about the same amount of time. I'll leave it going over the weekend just to make sure, though.

Robin.

Quoted 30 lines

The patch is straightforward and using non _irq() locking in a workitem that expects the lock to protect against interrupt handlers on the same CPU is clearly wrong.

Cheers
---Dave

  drivers/tty/serial/amba-pl011.c |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tty/serial/amba-pl011.c +2 -2

index 763eb20..0cc622a 100644
@@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work)
  	struct uart_amba_port *uap =
  		container_of(dwork, struct uart_amba_port, tx_softirq_work);
 
-	spin_lock(&uap->port.lock);
+	spin_lock_irq(&uap->port.lock);
  	while (pl011_tx_chars(uap)) ;
-	spin_unlock(&uap->port.lock);
+	spin_unlock_irq(&uap->port.lock);
  }
 
  static void pl011_tx_irq_seen(struct uart_amba_port *uap)

Robin Murphy

2015-06-08 10:34 UTC #2 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

Quoted 25 lines · Robin Murphy

Quoted 15 lines · Dave P Martin
pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks.
This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete.
Signed-off-by Dave Martin <[email protected]>
---
Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring.
Quoted 8 lines
This patch conflicts with tty-next like the previous patch, since it fixes code that is removed by tty-next. The correct resolution for the resulting merge conflict is to keep the code from tty-next.
I am not 100% certain yet whether some rare deadlocks that Robin is seeing are caused by this issue, or whether this patch fixes them -- he's testing atm.

Quoted 5 lines

FWIW, I've been running Juno in a startup/shutdown loop with a very noisy systemd all afternoon and haven't hit a problem yet with this patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep splats in about the same amount of time. I'll leave it going over the weekend just to make sure, though.

Having still seen nothing over several hundred more reboot cycles, I'm satisfied that the irq vs. softirq race explains the deadlock and that this patch fixes it, so;

Tested-by Robin Murphy <[email protected]>

Quoted 40 lines

Robin.
Quoted 20 lines
The patch is straightforward and using non _irq() locking in a workitem that expects the lock to protect against interrupt handlers on the same CPU is clearly wrong.
Cheers
---Dave
   drivers/tty/serial/amba-pl011.c |    4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/amba-pl011.c +0 -0
index 763eb20..0cc622a 100644
@@ -1360,9 +1360,9 @@ static void pl011_tx_softirq(struct work_struct *work)
   	struct uart_amba_port *uap =
drivers/tty/serial/amba-pl011.c static void pl011_tx_softirq(struct work_struct *work) · line 1368
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +2 -2
   		container_of(dwork, struct uart_amba_port, tx_softirq_work);
 
-	spin_lock(&uap->port.lock);
+	spin_lock_irq(&uap->port.lock);
   	while (pl011_tx_chars(uap)) ;
-	spin_unlock(&uap->port.lock);
+	spin_unlock_irq(&uap->port.lock);
   }
 
   static void pl011_tx_irq_seen(struct uart_amba_port *uap)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Dave P Martin

author

2015-06-08 11:34 UTC #3 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

Quoted 32 lines · Robin Murphy

Quoted 25 lines · Robin Murphy
Quoted 15 lines · Dave P Martin
pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks.
This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete.
Signed-off-by Dave Martin <[email protected]>
---
Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring.
Quoted 8 lines
This patch conflicts with tty-next like the previous patch, since it fixes code that is removed by tty-next. The correct resolution for the resulting merge conflict is to keep the code from tty-next.
I am not 100% certain yet whether some rare deadlocks that Robin is seeing are caused by this issue, or whether this patch fixes them -- he's testing atm.
Quoted 5 lines
FWIW, I've been running Juno in a startup/shutdown loop with a very noisy systemd all afternoon and haven't hit a problem yet with this patch applied. Testing without this patch yesterday I saw 3 or 4 lockdep splats in about the same amount of time. I'll leave it going over the weekend just to make sure, though.

Quoted 5 lines

Having still seen nothing over several hundred more reboot cycles, I'm satisfied that the irq vs. softirq race explains the deadlock and that this patch fixes it, so;
Tested-by Robin Murphy <[email protected]>

Thanks for this

Cheers
---Dave

Greg Kroah-Hartman

maintainer

2015-06-13 00:39 UTC #4 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

In reply to Dave Martin

Quoted 9 lines · Dave Martin

pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks.
This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete.

Quoted 7 lines

Signed-off-by Dave Martin <[email protected]>
Tested-by Robin Murphy <[email protected]>
Cc stable <[email protected]> # 4.1
---
Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring.

Too late for 4.1, sorry. This doesn't apply to my tty-next branch, otherwise I would have queued it up there. Care to redo it and mark it for -stable, like I did above, and resend it?

thanks,

greg k-h

Dave P Martin

author

2015-06-15 10:53 UTC #5 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

Quoted 15 lines · Greg Kroah-Hartman

Quoted 14 lines · Dave Martin
pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks.
This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete.
Signed-off-by Dave Martin <[email protected]>
Tested-by Robin Murphy <[email protected]>
Cc stable <[email protected]> # 4.1
---

Quoted 4 lines

Quoted 2 lines
Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring.
Too late for 4.1, sorry. This doesn't apply to my tty-next branch,

No worries -- I was aware it was getting very late. The code this fixes is obsoleted by -next, so it is relevant only for stable now.

Quoted 2 lines

otherwise I would have queued it up there. Care to redo it and mark it for -stable, like I did above, and resend it?

Will do, thanks.
---Dave

Andre Przywara

2015-06-15 11:09 UTC #6 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

In reply to Greg Kroah-Hartman

Hi Greg,

Quoted 15 lines · Greg Kroah-Hartman

Quoted 14 lines · Dave Martin
pl011_tx_softirq() currently uses spin_{,un}lock(), which are not sufficient to inhibit pl011_int() from being triggered by a local IRQ and trying to re-take the same lock. This can lead to deadlocks.
This patch uses the _irq() locking variants instead to ensure that pl011_int() handling for a given port is deferred until any pl011_tx_softirq() work for that port is complete.
Signed-off-by Dave Martin <[email protected]>
Tested-by Robin Murphy <[email protected]>
Cc stable <[email protected]> # 4.1
---

Quoted 4 lines

Quoted 2 lines
Another candidate for v4.1 if possible (sorry!) -- I thought this change was already in, but it went astray when I was refactoring.
Too late for 4.1, sorry.

So if I get this correctly, we go from "some characters lost from systemd messages" behaviour in 4.0 to "may deadlock" in 4.1 now? Is that right?

I understand that you are reluctant to take this patch this late in the game, but wouldn't it actually be better now to revert PL011 to the state of 4.0 to avoid ending up with worse-than-4.0 behaviour in the 4.1 release?
We could still backport the solution ending up in 4.2-rc to 4.1.x.

Cheers,
Andre.

Quoted 8 lines

This doesn't apply to my tty-next branch,
otherwise I would have queued it up there. Care to redo it and mark it for -stable, like I did above, and resend it?
thanks,
greg k-h

Dave Martin

author

2015-06-15 14:03 UTC #7 lore raw

PATCH 1/1 serial/amba-pl011: Disable interrupts around TX softirq

On Mon, Jun 15, 2015 at 12:09:59PM +0100, Andre Przywara wrote:

[...]

Quoted 1 line · Greg Kroah-Hartman

[...]

Quoted 5 lines

Quoted 1 line
Too late for 4.1, sorry.
So if I get this correctly, we go from "some characters lost from systemd messages" behaviour in 4.0 to "may deadlock" in 4.1 now? Is that right?

So it seems. It's my bad that we hit this so late in the cycle.

Quoted 5 lines

I understand that you are reluctant to take this patch this late in the game, but wouldn't it actually be better now to revert PL011 to the state of 4.0 to avoid ending up with worse-than-4.0 behaviour in the 4.1 release?
We could still backport the solution ending up in 4.2-rc to 4.1.x.

If neither this patch, nor what is in -next, can be applied in 4.1 (and I understand where Greg was coming from in saying "no"), then this seems inevitable.

The only other alternative that might be viable is to revert the relevant changes from 4.1 as you suggest, and optionally try to fix the systemd console corruption issue via stable. However, all the fixes currently available involve a lot of code -- I'm not sure whether they are good candidates for stable.

There's also a risk of getting into a worse mess with that late revert if it interacts with any other patches.

However, the revert [1] doesn't look complicated. There is a 1-line context clash with some of the dma changes that needs to be resolved, but that's it.

Opinions?

To avoid further confusion I'll hold off on sending any further patch for the moment -- I'd like to see some sort of consensus on the way forward if possible.

Cheers
---Dave

[1] For me, I can build and boot Juno with the v4.0 behaviour with the following reverts. These would clash with next again, but the resolution is the same as before: the code in -next overrides these changes in master.

Quoted 1 line

From 89cd9714f223117389f24b191ff97f6969d81c3c Mon Sep 17 00:00:00 2001

From: Dave Martin <[email protected]>
Date: Mon, 15 Jun 2015 14:06:25 +0100
Subject: [REVERT 1/2] Revert "serial/amba-pl011: Unconditionally poll for
FIFO space before each TX char"

This reverts commit 43dd1f9a5b05d6db2cb258354a01ace63baa5c0b.

---
 drivers/tty/serial/amba-pl011.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/tty/serial/amba-pl011.c +8 -8

index 763eb20..6f5a072 100644
@@ -1249,19 +1249,20 @@ __acquires(&uap->port.lock)
 
 /*
  * Transmit a character
+ * There must be at least one free entry in the TX FIFO to accept the char.
  *
- * Returns true if the character was successfully queued to the FIFO.
- * Returns false otherwise.
+ * Returns true if the FIFO might have space in it afterwards;
+ * returns false if the FIFO definitely became full.
  */
 static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c)
 {
-       if (readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF)
-               return false; /* unable to transmit character */
-
        writew(c, uap->port.membase + UART01x_DR);
        uap->port.icount.tx++;
 
-       return true;
+       if (likely(uap->tx_irq_seen > 1))
+               return true;
+
+       return !(readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF);
 }
 
 static bool pl011_tx_chars(struct uart_amba_port *uap)

@@ -1295,8 +1296,7 @@ static bool pl011_tx_chars(struct uart_amba_port *uap)
                return false;
 
        if (uap->port.x_char) {
-               if (!pl011_tx_char(uap, uap->port.x_char))
-                       goto done;
+               pl011_tx_char(uap, uap->port.x_char);
                uap->port.x_char = 0;
                --count;
        }

--
1.7.10.4

Quoted 1 line

From ece86a450686f37c3761a25c64af013bc556ee61 Mon Sep 17 00:00:00 2001

From: Dave Martin <[email protected]>
Date: Mon, 15 Jun 2015 14:07:39 +0100
Subject: [REVERT 2/2] Revert "serial/amba-pl011: Activate TX IRQ passively"

This reverts commit 734745caeb9f155ab58918834a8c70e83fa6afd3.

Conflicts:

        drivers/tty/serial/amba-pl011.c
---
 drivers/tty/serial/amba-pl011.c |  165 ++++++++++++---------------------------
 1 file changed, 50 insertions(+), 115 deletions(-)

drivers/tty/serial/amba-pl011.c +50 ~~-115~~

280 lines not on this page · raw message

--
1.7.10.4