serial: samsung: drop the spinlock around uart_write_wakeup

From: Anand Moon <[email protected]>

drop the spin_unlock/lock around uart_write_wakeup to protect write wakeup for uart port.

Signed-off-by Anand Moon <[email protected]>

---
changes logs: drop the previous approch of spin_unlock_irqrestore/save

---
 drivers/tty/serial/samsung.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/tty/serial/samsung.c +1 -4

index d72cd73..0cb70a9 100644
@@ -758,11 +758,8 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		goto out;
 	}
 
-	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-		spin_unlock(&port->lock);
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
-		spin_lock(&port->lock);
-	}
 
 	if (uart_circ_empty(xmit))
 		s3c24xx_serial_stop_tx(port);

[ +cc Krzysztof Kozlowski ]

Quoted 4 lines · Anand Moon

From: Anand Moon <[email protected]>

drop the spin_unlock/lock around uart_write_wakeup to protect write wakeup for uart port.

What Krzysztof was saying wrt v1 of this patch is that the changelog should provide as much information as possible to the maintainer(s) and driver author(s), and that you should test that outcome.

Here's what I would have written for a commit message:

Remove deadlock workaround for line disciplines that invoke the tty driver's write() method directly from their write_wakeup() method. As documented for the write_wakeup() line discipline method in tty_ldisc.h, line disciplines must not attempt i/o directly from write_wakeup() as this will deadlock. Reviews of in-tree line disciplines confirm all defer i/o.

NB: This workaround was added in commit c15c3747ee32 ("serial: samsung: fix potential soft lockup during uart write") which notes both slip and bluetooth hci attempt i/o directly from write_wakeup(). These issues were fixed in commits 661f7fda21b1 ("slip: Fix deadlock in write_wakeup") and da64c27d3c93 ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.

Regards,
Peter Hurley

PS - Minor procedural note: please cc those who have reviewed previous
patch versions when you submit new versions, ok? Not a big deal, lots of submitters don't, but it's still preferred.

Quoted 22 lines

Signed-off-by Anand Moon <[email protected]>

---
changes logs: drop the previous approch of spin_unlock_irqrestore/save

---
 drivers/tty/serial/samsung.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/tty/serial/samsung.c +1 -4

index d72cd73..0cb70a9 100644
@@ -758,11 +758,8 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
              goto out;
      }
 
-     if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-             spin_unlock(&port->lock);
+     if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
              uart_write_wakeup(port);
-             spin_lock(&port->lock);
-     }
 

drivers/tty/serial/samsung.c static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) · line 765

Quoted 3 lines

drivers/tty/serial/samsung.c +0 -0

      if (uart_circ_empty(xmit))
              s3c24xx_serial_stop_tx(port);

[ +cc Krzysztof Kozlowski ]

Quoted 4 lines · Anand Moon

From: Anand Moon <[email protected]>

drop the spin_unlock/lock around uart_write_wakeup to protect write wakeup for uart port.

What Krzysztof was saying wrt v1 of this patch is that the changelog should provide as much information as possible to the maintainer(s) and driver author(s), and that you should test that outcome.

Here's what I would have written for a commit message:

Remove deadlock workaround for line disciplines that invoke the tty driver's write() method directly from their write_wakeup() method. As documented for the write_wakeup() line discipline method in tty_ldisc.h, line disciplines must not attempt i/o directly from write_wakeup() as this will deadlock. Reviews of in-tree line disciplines confirm all defer i/o.

NB: This workaround was added in commit c15c3747ee32 ("serial: samsung: fix potential soft lockup during uart write") which notes both slip and bluetooth hci attempt i/o directly from write_wakeup(). These issues were fixed in commits 661f7fda21b1 ("slip: Fix deadlock in write_wakeup") and da64c27d3c93 ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.

2016-02-20 4:30 GMT+09:00 Peter Hurley <[email protected]>:

Quoted 23 lines

[ +cc Krzysztof Kozlowski ]

Quoted 4 lines · Anand Moon

From: Anand Moon <[email protected]>

drop the spin_unlock/lock around uart_write_wakeup to protect write wakeup for uart port.

What Krzysztof was saying wrt v1 of this patch is that the changelog should provide as much information as possible to the maintainer(s) and driver author(s), and that you should test that outcome.

Here's what I would have written for a commit message:

Remove deadlock workaround for line disciplines that invoke the tty driver's write() method directly from their write_wakeup() method. As documented for the write_wakeup() line discipline method in tty_ldisc.h, line disciplines must not attempt i/o directly from write_wakeup() as this will deadlock. Reviews of in-tree line disciplines confirm all defer i/o.

Quoted 6 lines

NB: This workaround was added in commit c15c3747ee32 ("serial: samsung: fix potential soft lockup during uart write") which notes both slip and bluetooth hci attempt i/o directly from write_wakeup(). These issues were fixed in commits 661f7fda21b1 ("slip: Fix deadlock in write_wakeup") and da64c27d3c93 ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.

Thanks Peter for thorough analysis. It shouldn't be done by you but by the patch submitter... and I have big worries that Anand did not perform that analysis.

Anand, could you at least test that this lockup does not happen anymore? You will need board with Bluetooth for that (and not USB Bluetooth...). If you cannot test it, maybe guys from Polish R&D could help you (Cc-ed), because they were working on DMA for serial used in Bluetooth.

Best regards,
Krzysztof

Hi Krzysztof,

Quoted 35 lines · Krzysztof Kozlowski

2016-02-20 4:30 GMT+09:00 Peter Hurley <[email protected]>:

Quoted 23 lines

[ +cc Krzysztof Kozlowski ]

Quoted 4 lines · Anand Moon

From: Anand Moon <[email protected]>

drop the spin_unlock/lock around uart_write_wakeup to protect write wakeup for uart port.

What Krzysztof was saying wrt v1 of this patch is that the changelog should provide as much information as possible to the maintainer(s) and driver author(s), and that you should test that outcome.

Here's what I would have written for a commit message:

Remove deadlock workaround for line disciplines that invoke the tty driver's write() method directly from their write_wakeup() method. As documented for the write_wakeup() line discipline method in tty_ldisc.h, line disciplines must not attempt i/o directly from write_wakeup() as this will deadlock. Reviews of in-tree line disciplines confirm all defer i/o.

Quoted 6 lines

NB: This workaround was added in commit c15c3747ee32 ("serial: samsung: fix potential soft lockup during uart write") which notes both slip and bluetooth hci attempt i/o directly from write_wakeup(). These issues were fixed in commits 661f7fda21b1 ("slip: Fix deadlock in write_wakeup") and da64c27d3c93 ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.

Thanks Peter for thorough analysis. It shouldn't be done by you but by the patch submitter... and I have big worries that Anand did not perform that analysis.

Quoted 8 lines

Anand, could you at least test that this lockup does not happen anymore? You will need board with Bluetooth for that (and not USB Bluetooth...). If you cannot test it, maybe guys from Polish R&D could help you (Cc-ed), because they were working on DMA for serial used in Bluetooth.

Best regards,
Krzysztof

I have looked into the history of the changes,
commit c15c3747ee32 (serial: samsung: fix potential soft lockup during uart write)
was added long time ago, that's why have missed this.

I don't have on-board Bluetooth enable boards with me, so if their is potential lockup you people observer do not consider this patch.

Quoted 48 lines · Anand Moon

Hi Krzysztof,

Quoted 35 lines · Krzysztof Kozlowski

2016-02-20 4:30 GMT+09:00 Peter Hurley <[email protected]>:

Quoted 23 lines

[ +cc Krzysztof Kozlowski ]

Quoted 4 lines · Anand Moon

From: Anand Moon <[email protected]>

drop the spin_unlock/lock around uart_write_wakeup to protect write wakeup for uart port.

What Krzysztof was saying wrt v1 of this patch is that the changelog should provide as much information as possible to the maintainer(s) and driver author(s), and that you should test that outcome.

Here's what I would have written for a commit message:

Remove deadlock workaround for line disciplines that invoke the tty driver's write() method directly from their write_wakeup() method. As documented for the write_wakeup() line discipline method in tty_ldisc.h, line disciplines must not attempt i/o directly from write_wakeup() as this will deadlock. Reviews of in-tree line disciplines confirm all defer i/o.

Quoted 6 lines

NB: This workaround was added in commit c15c3747ee32 ("serial: samsung: fix potential soft lockup during uart write") which notes both slip and bluetooth hci attempt i/o directly from write_wakeup(). These issues were fixed in commits 661f7fda21b1 ("slip: Fix deadlock in write_wakeup") and da64c27d3c93 ("bluetooth: hci_ldisc: fix deadlock condition"), respectively.

Thanks Peter for thorough analysis. It shouldn't be done by you but by the patch submitter... and I have big worries that Anand did not perform that analysis.

Quoted 8 lines

Anand, could you at least test that this lockup does not happen anymore? You will need board with Bluetooth for that (and not USB Bluetooth...). If you cannot test it, maybe guys from Polish R&D could help you (Cc-ed), because they were working on DMA for serial used in Bluetooth.

Best regards,
Krzysztof

Quoted 8 lines

I have looked into the history of the changes,
commit c15c3747ee32 (serial: samsung: fix potential soft lockup during uart write)
was added long time ago, that's why have missed this.

I don't have on-board Bluetooth enable boards with me, so if their is potential lockup you people observer do not consider this patch.

Which means that you cannot test it... Find someone who can test it with Bluetooth (or SLIP) and will provide you a Tested-by tag.

Best regards,
Krzysztof