Re: [PATCH v2] tty: pl011: Avoid spuriously stuck-off interrupts

[Wei Xu <xuwei5@hisilicon.com>]

Hi Dave,

On 2018/5/10 18:08, Dave Martin wrote:
> Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> clears the RX and receive timeout interrupts on pl011 startup, to
> avoid a screaming-interrupt scenario that can occur when the
> firmware or bootloader leaves these interrupts asserted.
> 
> This has been noted as an issue when running Linux on qemu [1].
> 
> Unfortunately, the above fix seems to lead to potential
> misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously
> on driver startup, if the RX FIFO is also already full to the
> trigger level.
> 
> Clearing the RX FIFO interrupt does not change the FIFO fill level.
> In this scenario, because the interrupt is now clear and because
> the FIFO is already full to the trigger level, no new assertion of
> the RX FIFO interrupt can occur unless the FIFO is drained back
> below the trigger level.  This never occurs because the pl011
> driver is waiting for an RX FIFO interrupt to tell it that there is
> something to read, and does not read the FIFO at all until that
> interrupt occurs.
> 
> Thus, simply clearing "spurious" interrupts on startup may be
> misguided, since there is no way to be sure that the interrupts are
> truly spurious, and things can go wrong if they are not.
> 
> This patch instead clears the interrupt condition by draining the
> RX FIFO during UART startup, after clearing any potentially
> spurious interrupt.  This should ensure that an interrupt will
> definitely be asserted if the RX FIFO subsequently becomes
> sufficiently full.
> 
> The drain is done at the point of enabling interrupts only.  This
> means that it will occur any time the UART is newly opened through
> the tty layer.  It will not apply to polled-mode use of the UART by
> kgdboc: since that scenario cannot use interrupts by design, this
> should not matter.  kgdboc will interact badly with "normal" use of
> the UART in any case: this patch makes no attempt to paper over
> such issues.
> 
> This patch does not attempt to address the case where the RX FIFO
> fills faster than it can be drained: that is a pathological
> hardware design problem that is beyond the scope of the driver to
> work around.  As a failsafe, the number of poll iterations for
> draining the FIFO is limited to twice the FIFO size.  This will
> ensure that the kernel at least boots even if it is impossible to
> drain the FIFO for some reason.
> 
> [1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
> before enabled the interruption
> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
> 
> Reported-by: Wei Xu <xuwei5@hisilicon.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Thanks!
Tested on hisilicon D05 board.

Tested-by: Wei Xu <xuwei5@hisilicon.com>

Best Regards,
Wei

> 
> ---
> 
> Changes since v1 [1]
> 
>  * Deleted Andrew Jones' Reviewed/Tested-bys due to the following
>    change.
> 
>    If you can please retest that the updated patch fixes your
>    issue that would be appreciated.
> 
> Suggested by Russell King:
> 
>  * Only drain 2 * FIFO size, to avoid going into an infinite spin
>    if something does wrong.  If the FIFO still couldn't be drained
>    sufficiently then pl011 RX won't work properly, but the kernel
>    will at least boot.
> 
> 
> [1] [PATCH] tty: pl011: Avoid spuriously stuck-off interrupts
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/574362.html
> 
> ---
>  drivers/tty/serial/amba-pl011.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 4b40a5b..ebd33c0 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1727,10 +1727,26 @@ static int pl011_allocate_irq(struct uart_amba_port *uap)
>   */
>  static void pl011_enable_interrupts(struct uart_amba_port *uap)
>  {
> +	unsigned int i;
> +
>  	spin_lock_irq(&uap->port.lock);
>  
>  	/* Clear out any spuriously appearing RX interrupts */
>  	pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
> +
> +	/*
> +	 * RXIS is asserted only when the RX FIFO transitions from below
> +	 * to above the trigger threshold.  If the RX FIFO is already
> +	 * full to the threshold this can't happen and RXIS will now be
> +	 * stuck off.  Drain the RX FIFO explicitly to fix this:
> +	 */
> +	for (i = 0; i < uap->fifosize * 2; ++i) {
> +		if (pl011_read(uap, REG_FR) & UART01x_FR_RXFE)
> +			break;
> +
> +		pl011_read(uap, REG_DR);
> +	}
> +
>  	uap->im = UART011_RTIM;
>  	if (!pl011_dma_rx_running(uap))
>  		uap->im |= UART011_RXIM;
>