Subject: [PATCH v2] drivers/serial/atmel_serial.c: Fix PDC wrap around issue

From: [email-masked] (Nicolas Ferre)
Date: Mon, 24 Jun 2013 19:02:45 +0200

On 20/06/2013 12:30, Retallack, Mark :
> Resubmited using correct format.

Hi Mark,

> We have found a small problem with the Atmel serial driver (drivers/tty/serial/atmel_serial.c) when using PDC. The
> problem showed itself on our system when it is heavily loaded and the softirq's are implemented as threads.
>
> The DMA buffers (2 off) are dynamically allocated when the driver is initialized. The relative location of these
> buffers is down to the whims of the memory allocation software i.e. the buffers could be in contiguous memory or
> separated by a number of bytes.
> What if the buffers are contiguous and the PDC managed to fill both buffers before the handling tasklet could run,
> the tasklet would be unable to identify this condition.
>
> For example:
> Assume DMA buffers A and B are both 0x200 bytes long and reside at addresses 0x100 and 0x300 respectively. Now also
> assume that the PDC has managed to fill both buffers starting with B then moving to A (at which point the PDC
> asserts the ENDRX and RXBUFF flags, resulting in the ENDRX interrupt). At this time the PDC has advanced the RPR to
> one byte beyond the end of buffer A i.e. address 0x200.
> When the tasklet runs, as far as it is concerned it is still processing buffer B. The first thing it does is to
> work out whether the PDC has completely filled buffer B, which it does by subtracting buffer B address from the RPR
> i.e. 0x200 - 0x200 = 0. So the tasklet believes that the buffer is empty rather than actually being full. In this
> event, the tasklet does not update RNPR and RNCR to inform the PDC that buffer B can now be used (side effect of
> which is that the ENDRX and RXBUFF flags remain set). When the tasklet reenables the ENDRX interrupt (when it
> exits) the UART immediately generates an interrupt (based on the ENDRX flag being set). The whole process then
> repeats while no further data is received.

Thanks a lot for this detailed description which nails the issue: it if 
very valuable to have such an example.
Actually, I may need to move the "example" part of your description in 
the comment part of the commit message (ie: after the "---" below).

Acked-by: Nicolas Ferre <[email-masked]>

I will re-send the message to the tty maintainer.

Thanks, best regards,

> Thanks
> Mark Retallack
>
> Signed-off-by: Mark Retallack <[email-masked]>
> ---
>   drivers/tty/serial/atmel_serial.c |   37 ++++++++++++++++++++++++++++++++-----
>   1 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 3467462..3f1f65b 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -121,6 +121,7 @@ struct atmel_dma_buffer {
>          dma_addr_t      dma_addr;
>          unsigned int    dma_size;
>          unsigned int    ofs;
> +       unsigned int    dma_full;
>   };
>
>   struct atmel_uart_char {
> @@ -393,7 +394,7 @@ static void atmel_start_rx(struct uart_port *port)
>          if (atmel_use_dma_rx(port)) {
>                  /* enable PDC controller */
>                  UART_PUT_IER(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT |
> -                       port->read_status_mask);
> +                       ATMEL_US_RXBUFF | port->read_status_mask);
>                  UART_PUT_PTCR(port, ATMEL_PDC_RXTEN);
>          } else {
>                  UART_PUT_IER(port, ATMEL_US_RXRDY);
> @@ -411,7 +412,7 @@ static void atmel_stop_rx(struct uart_port *port)
>                  /* disable PDC receive */
>                  UART_PUT_PTCR(port, ATMEL_PDC_RXTDIS);
>                  UART_PUT_IDR(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT |
> -                       port->read_status_mask);
> +                       ATMEL_US_RXBUFF | port->read_status_mask);
>          } else {
>                  UART_PUT_IDR(port, ATMEL_US_RXRDY);
>          }
> @@ -574,15 +575,27 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
>
>          if (atmel_use_dma_rx(port)) {
>                  /*
> +                * Check for PDC full error
> +                */
> +               if (pending & (ATMEL_US_RXBUFF)) {
> +                       int i;
> +                       port->icount.buf_overrun++;
> +                       for (i = 0; i < 2; i++)
> +                               atmel_port->pdc_rx[i].dma_full = 1;
> +               }
> +
> +               /*
>                   * PDC receive. Just schedule the tasklet and let it
>                   * figure out the details.
>                   *
>                   * TODO: We're not handling error flags correctly at
>                   * the moment.
>                   */
> -               if (pending & (ATMEL_US_ENDRX | ATMEL_US_TIMEOUT)) {
> +               if (pending & (ATMEL_US_ENDRX | ATMEL_US_TIMEOUT |
> +                               ATMEL_US_RXBUFF)) {
>                          UART_PUT_IDR(port, (ATMEL_US_ENDRX
> -                                               | ATMEL_US_TIMEOUT));
> +                                               | ATMEL_US_TIMEOUT
> +                                               | ATMEL_US_RXBUFF));
>                          tasklet_schedule(&atmel_port->tasklet);
>                  }
>
> @@ -808,6 +821,19 @@ static void atmel_rx_from_dma(struct uart_port *port)
>                   */
>                  head = min(head, pdc->dma_size);
>
> +               /*
> +                * If we have a RXBUFF set, it means that both buffers are full
> +                * and cannot take anymore data, force a clean.
> +                * This protects against the situation where both buffers
> +                * are full and the head matches the tail because the head
> +                * is pointing to the first byte in the second buffer.
> +                * This makes it look like the current buffer is still empty
> +                */
> +               if (pdc->dma_full) {
> +                       head = pdc->dma_size;
> +                       pdc->dma_full = 0;
> +               }
> +
>                  if (likely(head != tail)) {
>                          dma_sync_single_for_cpu(port->dev, pdc->dma_addr,
>                                          pdc->dma_size, DMA_FROM_DEVICE);
> @@ -852,7 +878,7 @@ static void atmel_rx_from_dma(struct uart_port *port)
>          tty_flip_buffer_push(tport);
>          spin_lock(&port->lock);
>
> -       UART_PUT_IER(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT);
> +       UART_PUT_IER(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT | ATMEL_US_RXBUFF);
>   }
>
>   /*
> @@ -954,6 +980,7 @@ static int atmel_startup(struct uart_port *port)
>                                                         DMA_FROM_DEVICE);
>                          pdc->dma_size = PDC_BUFFER_SIZE;
>                          pdc->ofs = 0;
> +                       pdc->dma_full = 0;
>                  }
>
>                  atmel_port->pdc_rx_idx = 0;
>

-- 
Nicolas Ferre