Thread (15 messages) 15 messages, 3 authors, 2015-08-17

[PATCH 4/4] serial: xuartps: Rewrite the interrupt handling logic

From: Peter Hurley <hidden>
Date: 2015-08-17 15:56:09
Also in: linux-serial, lkml

On 08/17/2015 03:22 AM, Michal Simek wrote:
From: Anirudha Sarangi <redacted>

The existing interrupt handling logic has followins issues.
- Upon a parity error with default configuration, the control
  never comes out of the ISR thereby hanging Linux.
- The error handling logic around framing and parity error are buggy.
  There are chances that the errors will never be captured.
- The existing ISR is just too long.
This patch fixes all these concerns.
This patch is unreviewable. Please break this down into multiple patches.

Regards,
Peter Hurley
quoted hunk ↗ jump to hunk
It separates out the Tx and Rx
hanling logic into separate functions. It ensures that the status
registers are cleared on all cases so that a hang situation never
arises.

Signed-off-by: Anirudha Sarangi <redacted>
Signed-off-by: Michal Simek <redacted>
---

 drivers/tty/serial/xilinx_uartps.c | 194 ++++++++++++++++++++-----------------
 1 file changed, 104 insertions(+), 90 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2dc26e5f1384..c771dbbf6161 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -173,61 +173,86 @@ struct cdns_uart {
 		clk_rate_change_nb);
 
 /**
- * cdns_uart_isr - Interrupt handler
- * @irq: Irq number
- * @dev_id: Id of the port
- *
- * Return: IRQHANDLED
+ * cdns_uart_handle_tx - Handle the bytes to be Txed.
+ * @dev_id: Id of the UART port
+ * Return: None
  */
-static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+static void cdns_uart_handle_tx(void *dev_id)
 {
 	struct uart_port *port = (struct uart_port *)dev_id;
-	unsigned long flags;
-	unsigned int isrstatus, numbytes;
-	unsigned int data;
-	char status = TTY_NORMAL;
+	unsigned int numbytes;
 
-	spin_lock_irqsave(&port->lock, flags);
+	if (uart_circ_empty(&port->state->xmit)) {
+		writel(CDNS_UART_IXR_TXEMPTY, port->membase +
+		       CDNS_UART_IDR_OFFSET);
+	} else {
+		numbytes = port->fifosize;
+		/* Break if no more data available in the UART buffer */
+		while (numbytes--) {
+			if (uart_circ_empty(&port->state->xmit))
+				break;
+			/*
+			 * Get the data from the UART circular buffer
+			 * and write it to the cdns_uart's TX_FIFO
+			 * register.
+			 */
+			writel(port->state->xmit.buf[port->state->xmit.tail],
+			       port->membase + CDNS_UART_FIFO_OFFSET);
 
-	/* Read the interrupt status register to determine which
-	 * interrupt(s) is/are active.
-	 */
-	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
+			port->icount.tx++;
 
-	/*
-	 * There is no hardware break detection, so we interpret framing
-	 * error with all-zeros data as a break sequence. Most of the time,
-	 * there's another non-zero byte at the end of the sequence.
-	 */
-	if (isrstatus & CDNS_UART_IXR_FRAMING) {
-		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
-					CDNS_UART_SR_RXEMPTY)) {
-			if (!readl(port->membase + CDNS_UART_FIFO_OFFSET)) {
-				port->read_status_mask |= CDNS_UART_IXR_BRK;
-				isrstatus &= ~CDNS_UART_IXR_FRAMING;
-			}
+			/*
+			 * Adjust the tail of the UART buffer and wrap
+			 * the buffer if it reaches limit.
+			 */
+			port->state->xmit.tail =
+				(port->state->xmit.tail + 1) &
+					(UART_XMIT_SIZE - 1);
 		}
-		writel(CDNS_UART_IXR_FRAMING,
-				port->membase + CDNS_UART_ISR_OFFSET);
-	}
 
-	/* drop byte with parity error if IGNPAR specified */
-	if (isrstatus & port->ignore_status_mask & CDNS_UART_IXR_PARITY)
-		isrstatus &= ~(CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT);
+		if (uart_circ_chars_pending(
+				&port->state->xmit) < WAKEUP_CHARS)
+			uart_write_wakeup(port);
+	}
+}
 
-	isrstatus &= port->read_status_mask;
-	isrstatus &= ~port->ignore_status_mask;
+/**
+ * cdns_uart_handle_rx - Handle the received bytes along with Rx errors.
+ * @dev_id: Id of the UART port
+ * @isrstatus: The interrupt status register value as read
+ * Return: None
+ */
+static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
+{
+	struct uart_port *port = (struct uart_port *)dev_id;
+	unsigned int data;
+	unsigned int framerrprocessed = 0;
+	char status = TTY_NORMAL;
 
-	if ((isrstatus & CDNS_UART_IXR_TOUT) ||
-		(isrstatus & CDNS_UART_IXR_RXTRIG)) {
-		/* Receive Timeout Interrupt */
-		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
-					CDNS_UART_SR_RXEMPTY)) {
-			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+	while ((readl(port->membase + CDNS_UART_SR_OFFSET) &
+		CDNS_UART_SR_RXEMPTY) != CDNS_UART_SR_RXEMPTY) {
+		data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+		port->icount.rx++;
+		/*
+		 * There is no hardware break detection, so we interpret
+		 * framing error with all-zeros data as a break sequence.
+		 * Most of the time, there's another non-zero byte at the
+		 * end of the sequence.
+		 */
+		if (isrstatus & CDNS_UART_IXR_FRAMING) {
+			if (!data) {
+				port->read_status_mask |= CDNS_UART_IXR_BRK;
+				framerrprocessed = 1;
+				continue;
+			}
+		}
+		isrstatus &= port->read_status_mask;
+		isrstatus &= ~port->ignore_status_mask;
 
-			/* Non-NULL byte after BREAK is garbage (99%) */
-			if (data && (port->read_status_mask &
-						CDNS_UART_IXR_BRK)) {
+		if ((isrstatus & CDNS_UART_IXR_TOUT) ||
+		    (isrstatus & CDNS_UART_IXR_RXTRIG)) {
+			if (data &&
+			    (port->read_status_mask & CDNS_UART_IXR_BRK)) {
 				port->read_status_mask &= ~CDNS_UART_IXR_BRK;
 				port->icount.brk++;
 				if (uart_handle_break(port))
@@ -249,67 +274,56 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 				spin_lock(&port->lock);
 			}
 #endif
-
-			port->icount.rx++;
-
 			if (isrstatus & CDNS_UART_IXR_PARITY) {
 				port->icount.parity++;
 				status = TTY_PARITY;
-			} else if (isrstatus & CDNS_UART_IXR_FRAMING) {
+			}
+			if ((isrstatus & CDNS_UART_IXR_FRAMING) &&
+			    !framerrprocessed) {
 				port->icount.frame++;
 				status = TTY_FRAME;
-			} else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
+			}
+			if (isrstatus & CDNS_UART_IXR_OVERRUN) {
 				port->icount.overrun++;
+				tty_insert_flip_char(&port->state->port, 0,
+						     TTY_OVERRUN);
 			}
-
-			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
-					data, status);
+			tty_insert_flip_char(&port->state->port, data, status);
 		}
-		spin_unlock(&port->lock);
-		tty_flip_buffer_push(&port->state->port);
-		spin_lock(&port->lock);
 	}
+	spin_unlock(&port->lock);
+	tty_flip_buffer_push(&port->state->port);
+	spin_lock(&port->lock);
+}
 
-	/* Dispatch an appropriate handler */
-	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
-		if (uart_circ_empty(&port->state->xmit)) {
-			writel(CDNS_UART_IXR_TXEMPTY,
-					port->membase + CDNS_UART_IDR_OFFSET);
-		} else {
-			numbytes = port->fifosize;
-			/* Break if no more data available in the UART buffer */
-			while (numbytes--) {
-				if (uart_circ_empty(&port->state->xmit))
-					break;
-				/* Get the data from the UART circular buffer
-				 * and write it to the cdns_uart's TX_FIFO
-				 * register.
-				 */
-				writel(port->state->xmit.buf[
-						port->state->xmit.tail],
-					port->membase + CDNS_UART_FIFO_OFFSET);
-
-				port->icount.tx++;
-
-				/* Adjust the tail of the UART buffer and wrap
-				 * the buffer if it reaches limit.
-				 */
-				port->state->xmit.tail =
-					(port->state->xmit.tail + 1) &
-						(UART_XMIT_SIZE - 1);
-			}
+/**
+ * cdns_uart_isr - Interrupt handler
+ * @irq: Irq number
+ * @dev_id: Id of the port
+ *
+ * Return: IRQHANDLED
+ */
+static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+{
+	struct uart_port *port = (struct uart_port *)dev_id;
+	unsigned int isrstatus;
 
-			if (uart_circ_chars_pending(
-					&port->state->xmit) < WAKEUP_CHARS)
-				uart_write_wakeup(port);
-		}
-	}
+	spin_lock(&port->lock);
 
+	/* Read the interrupt status register to determine which
+	 * interrupt(s) is/are active and clear them.
+	 */
+	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
 	writel(isrstatus, port->membase + CDNS_UART_ISR_OFFSET);
 
-	/* be sure to release the lock and tty before leaving */
-	spin_unlock_irqrestore(&port->lock, flags);
+	if (isrstatus & CDNS_UART_IXR_TXEMPTY) {
+		cdns_uart_handle_tx(dev_id);
+		isrstatus &= ~CDNS_UART_IXR_TXEMPTY;
+	}
+	if (isrstatus & CDNS_UART_IXR_MASK)
+		cdns_uart_handle_rx(dev_id, isrstatus);
 
+	spin_unlock(&port->lock);
 	return IRQ_HANDLED;
 }
 
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help