| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <55D2040F.1010205@hurleysoftware.com>
Date: Mon, 17 Aug 2015 11:55:59 -0400
From: Peter Hurley <peter@...leysoftware.com>
To: Michal Simek <michal.simek@...inx.com>
CC: linux-kernel@...r.kernel.org, monstr@...str.eu,
Anirudha Sarangi <anirudha.sarangi@...inx.com>,
Sören Brinkmann <soren.brinkmann@...inx.com>,
Jiri Slaby <jslaby@...e.com>, linux-serial@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 4/4] serial: xuartps: Rewrite the interrupt handling logic
On 08/17/2015 03:22 AM, Michal Simek wrote:
> From: Anirudha Sarangi <anirudha.sarangi@...inx.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@...inx.com>
> Signed-off-by: Michal Simek <michal.simek@...inx.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;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists