[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; }