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

[Peter Hurley <peter@hurleysoftware.com>]

On 08/17/2015 03:22 AM, Michal Simek wrote:
> From: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> 
> 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

> 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 <anirudh@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>  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;
>  }
>  
>