Re: [Xen-devel] [PATCH v2 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5)

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.09.17 at 11:37, <awais.masood@xxxxxxxxxx> wrote:

First of all please only send to one of xen-devel@xxxxxxxxxxxxx and
xen-devel@xxxxxxxxxxxxxxxxxxxx.

> On Allwinner H5 (Orange Pi PC2) serial driver goes into an
> infinite loop when interrupts are enabled. The reason is a
> residual "busy detect" interrupt. Since the condition
> UART_IIR_NOINT will not be true unless this interrupt is
> cleared, the interrupt handler will remain locked up in this
> while loop.
> 
> A hw quirk fix was previously added for designware uart under
> commit:
> 50417cd978aa54930d065ac1f139f935d14af76d
> 
> It checks for a busy condition during setup and clears the
> condition by reading UART_USR register.
> 
> On Allwinner H5 (and H3), the "busy detect" condition occurs
> later because an LCR write is performed during setup 'after'
> this clear and if uart is busy, the "busy detect" condition
> will trigger again and cause the ISR lockup.
> 
> To solve this problem, the same UART_USR read operation is
> added within the interrupt handler to clear the condition.
> 
> Linux dw 8250 driver also handles this condition within
> interrupt handler
> http://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/8250/ 
> 8250_dw.c#L233
> 
> Tested on Orange Pi PC2 (H5). Earlier this issue was seen on H3
> as well and the same fix should help.

So the description almost exclusively talks about Allwinner hardware,
but ...

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -505,6 +505,19 @@ static int ns16550_ioport_invalid(struct ns16550 *uart)
>      return ns_read_reg(uart, UART_IER) == 0xff;
>  }
>  
> +static void ns16550_handle_dw_usr_busy_quirk(struct ns16550 *uart)
> +{
> +    if ( uart->dw_usr_bsy &&
> +         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
> +    {
> +        /* DesignWare 8250 detects if LCR is written while the UART is
> +         * busy and raises a "busy detect" interrupt. Read the UART
> +         * Status Register to clear this state.
> +         */
> +        ns_read_reg(uart, UART_USR);
> +    }
> +}

... here you don't mention it at all, and ...

> @@ -521,6 +534,9 @@ static void ns16550_interrupt(
>              serial_tx_interrupt(port, regs);
>          if ( lsr & UART_LSR_DR )
>              serial_rx_interrupt(port, regs);
> +
> +        /* Handle the DesignWare 8250 'busy-detect' quirk. */
> +        ns16550_handle_dw_usr_busy_quirk(uart);
>      }
>  }

... here, according to the description, the issue doesn't even apply
to DesignWare hardware.

Also while indeed there are many example to the contrary in this
file, please don't follow the bad habit of prefixing local functions
with something derived from the file name.
handle_dw_usr_busy_quirk() is perfectly fine a name for the new
function.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel