Re: [Xen-devel] [PATCH 26/27 v8] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 28/08/17 09:56, Bhupinder Thakur wrote:
> This patch fixes the issue observed when pl011 patches were tested on
> the junos hardware by Andre/Julien. It was observed that when large output is
> generated such as on running 'find /', output was getting truncated 
> intermittently
> due to OUT ring buffer getting full.
> 
> This issue was due to the fact that the SBSA UART driver expects that when
> a TX interrupt is asserted then the FIFO queue should be atleast half empty 
> and
> that it can write N bytes in the FIFO, where N is half the FIFO queue size, 
> without
> the bytes getting dropped due to FIFO getting full.
> 
> This requirement is as per section 3.4.2 of [1], which is:
> 
> -------------------------------------------------------------------------------
> UARTTXINTR
> 
> If the FIFOs are enabled and the transmit FIFO reaches the programmed
> trigger level. When this happens, the transmit interrupt is asserted HIGH. The
> transmit interrupt is cleared by writing data to the transmit FIFO until it
> becomes greater than the trigger level, or by clearing the interrupt.
> -------------------------------------------------------------------------------
> 
> The SBSA UART fifo size is 32 bytes and so it expects that space for 16 bytes
> should be available when TX interrupt is asserted.
> 
> The pl011 emulation logic was asserting the TX interrupt as soon as
> any space became available in the FIFO and the SBSA UART driver tried to write
> more data (upto 16 bytes) in the FIFO expecting that there is enough space
> available.
> 
> The fix was to ensure that the TX interriupt is raised only when there
> is space available for 16 bytes or more in the FIFO.
> 
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Andre Przywara <andre.przywara@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
>  xen/arch/arm/vpl011.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 56d9cbe..1e72fca 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -152,12 +152,20 @@ static void vpl011_write_data(struct domain *d, uint8_t 
> data)
>      else
>          gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
> -    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
> -         sizeof (intf->out) )
> -    {
> -        vpl011->uartfr |= TXFF;
> +    /*
> +     * Ensure that there is space for atleast 16 bytes before asserting the
> +     * TXI interrupt status bit because the SBSA UART driver may write
> +     * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
> +     * a TX interrupt.
> +     */
> +    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) <=
> +         (sizeof (intf->out) - 16) )
> +        vpl011->uartris |= TXI;
> +    else if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
> +              sizeof (intf->out) )

Now this is really hard to read. Can't you use:

    fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));

Also I think you could start the patch a few lines above, where you
check for any free space in the buffer.

>          vpl011->uartris &= ~TXI;
> -    }
> +    else
> +        vpl011->uartfr |= TXFF;

And I believe we should separate the FIFO full condition from the
interrupt condition. I think it should more look like:

    vpl011->uartfr |= BUSY;
    vpl011->uartfr &= ~TXFE;

    if ( fifo_level == sizeof(intf->out) )
        vpl011->uartfr |= TXFF;

    if ( fifo_level >= sizeof(intf->out) - 16 )
        vpl011->uartris &= ~TXI;

Which is much easier to read and understand, also follows the spec
closely. The "16" should be either expressed at FIFOSIZE / 2 or
explained in a comment.

>  
>      vpl011->uartfr |= BUSY;
>  
> @@ -368,7 +376,16 @@ static void vpl011_data_avail(struct domain *d)
>      if ( out_ring_qsize != sizeof(intf->out) )
>      {
>          vpl011->uartfr &= ~TXFF;
> -        vpl011->uartris |= TXI;
> +
> +        /*
> +         * Ensure that there is space for atleast 16 bytes before asserting 
> the
> +         * TXI interrupt status bit because the SBSA UART driver may write 
> upto
> +         * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
> +         * a TX interrupt.

The comment sounds a bit like this is hack, where it actually is a
totally legit spec requirement (the interrupt is asserted/deasserted
around the *trigger level*, which is half way by default and always half
for the SBSA).

Also I think the same logic/fix needs to be applied to the receiving side.

And while I see that Julien requested a follow-up patch, I believe this
should eventually be squashed into 02/27, to not have wrong code in the
repo. But can could be done at commit time, I guess.

Cheers,
Andre.

> +         */
> +        if ( out_ring_qsize <= (sizeof(intf->out) - 16) )
> +            vpl011->uartris |= TXI;
> +
>          if ( out_ring_qsize == 0 )
>          {
>              vpl011->uartfr &= ~BUSY;
> 

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