Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457808 > unrolled thread

[PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop dirk.eibach@gdsys.cc - 2016-08-08 15:50 +0200
    Re: [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop m.brock@vanmierlo.com - 2016-08-09 11:30 +0200

#1457808 — [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop

From: Dirk Eibach <dirk.eibach@gdsys.cc>

sc16is7xx_port_irq() is laid out as an endless loop. It will exit only
when there is no more interrupt left to service. This not common
practice.
In our case it lead to some strange hangup situation when there was an
unexpected XOFF-interrupt that could not be handled.
So let's service interrupts only once and report XOFF-interrupts that
should never happen since they are never enabled.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>

Conflicts:
	drivers/tty/serial/sc16is7xx.c
---
 drivers/tty/serial/sc16is7xx.c | 48 +++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index f36e6df..098c6dc 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -664,35 +664,31 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 {
 	struct uart_port *port = &s->p[portno].port;
+	unsigned int iir, ier, msr, rxlen;
 
-	do {
-		unsigned int iir, rxlen;
-
-		iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
-		if (iir & SC16IS7XX_IIR_NO_INT_BIT)
-			break;
+	iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
+	if (iir & SC16IS7XX_IIR_NO_INT_BIT)
+		return;
 
-		iir &= SC16IS7XX_IIR_ID_MASK;
+	iir &= SC16IS7XX_IIR_ID_MASK;
 
-		switch (iir) {
-		case SC16IS7XX_IIR_RDI_SRC:
-		case SC16IS7XX_IIR_RLSE_SRC:
-		case SC16IS7XX_IIR_RTOI_SRC:
-		case SC16IS7XX_IIR_XOFFI_SRC:
-			rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
-			if (rxlen)
-				sc16is7xx_handle_rx(port, rxlen, iir);
-			break;
-		case SC16IS7XX_IIR_THRI_SRC:
-			sc16is7xx_handle_tx(port);
-			break;
-		default:
-			dev_err_ratelimited(port->dev,
-					    "ttySC%i: Unexpected interrupt: %x",
-					    port->line, iir);
-			break;
-		}
-	} while (1);
+	switch (iir) {
+	case SC16IS7XX_IIR_RDI_SRC:
+	case SC16IS7XX_IIR_RLSE_SRC:
+	case SC16IS7XX_IIR_RTOI_SRC:
+		rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
+		if (rxlen)
+			sc16is7xx_handle_rx(port, rxlen, iir);
+		break;
+	case SC16IS7XX_IIR_THRI_SRC:
+		sc16is7xx_handle_tx(port);
+		break;
+	default:
+		dev_err_ratelimited(port->dev,
+				    "Port %i: Unexpected interrupt: iir %02x, ier %02x",
+				    port->line, iir, ier);
+		break;
+	}
 }
 
 static void sc16is7xx_ist(struct kthread_work *ws)
-- 
2.1.3

[toc] | [next] | [standalone]

#1458543

On 2016-08-08 15:32, dirk.eibach@gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
> 
> sc16is7xx_port_irq() is laid out as an endless loop. It will exit only
> when there is no more interrupt left to service. This not common
> practice.
> In our case it lead to some strange hangup situation when there was an
> unexpected XOFF-interrupt that could not be handled.
> So let's service interrupts only once and report XOFF-interrupts that
> should never happen since they are never enabled.

The reason for such an endless loop in an interrupt handler usually 
means
that multiple sources can generate an interrupt and the interrupt 
controller
is configured for edge. During handling the interrupt of one source 
(e.g.
data received) but before it is cleared, another interrupt can trigger 
(e.g.
transmit done). This will not generate a new edge as the interrupt line 
is
still active! Thus re-reading the interrupt status is required. And with
interrupt sharing this problem gets even worse. The fact that the 
sc16is7xx
uses an indirect interface like i2c or spi doesn't help either.

When configured for level interrupts the interrupt will automatically
re-trigger. But the risk of interrupt storm when the hardware is broken
(interrupt line stuck active) seems to keep developers away from setting
level interrupt. IMHO an interrupt storm should be detectable by the
interrupt handler so nobody needs to fear using level sensitive 
interrupts.

Maarten

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web