On Tue, Oct 16, 2018 at 3:24 PM Geert Uytterhoeven
<geert+renesas@xxxxxxxxx> wrote:
> When submitting a DMA request fails, the driver is supposed to fall back
> to PIO. However, this never really worked due to various reasons
> (sh-sci driver issues and dmaengine framework limitations).
>
> There are three places where DMA submission can fail, and the driver
> should fall back to PIO:
> 1. sci_dma_rx_complete(),
> 2. sci_submit_rx(),
> 3. work_fn_tx().
>
> This RFC fixes fallback to PIO in the receive path (cases 1 and 2).
> Fallback to PIO in the transmit path (case 3) already works fine.
>
> Case 1: sci_dma_rx_complete()
>
> A. PIO cannot take over on SCIF if any DMA transactions are pending,
> hence they must be terminated first.
>
> B. The active cookies must be invalidated, else rx_timer_fn() may
> trigger a NULL pointer dereference.
>
> C. Restarting the port is not needed, as it is already running, but
> serial port interrupts must be directed back from the DMA engine to
> the CPU.
>
> Case 2: sci_submit_rx()
>
> D. Some callers of sci_submit_rx() hold the port spinlock, others
> don't. During fallback to PIO, the driver needs to obtain the port
> spinlock. If the lock was already held, spinlock recursion is
> detected, causing a deadlock: BUG: spinlock recursion on CPU#0.
>
> Fix this by adding a flag parameter to sci_submit_rx() for the
> caller to indicate the port spinlock is already held, so spinlock
> recursion can be avoided.
>
> Move the spin_lock_irqsave() up, so all DMA disable steps are
> protected, which is safe as the recently introduced
> dmaengine_terminate_async() can be called in atomic context.
>
> E. When falling back to PIO, active_rx must be set to a different
> value than cookie_rx[i], else sci_dma_rx_find_active() will
> incorrectly find a match, leading to a NULL pointer dereference in
> rx_timer_fn() later.
>
> F. On (H)SCIF, sci_submit_rx() is called in the receive interrupt
> handler. Hence if DMA submission fails, the interrupt handler
> should handle reception using PIO.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> ---
> Against tty/next + "serial: sh-sci: Fix receive on SCIFA/SCIFB variants
> with DMA".
>
> All testing was done on r8a7791/koelsch using SCIF1 on debug serial 1,
> and SCIFA3 on EXIO-B, by introducing random failures in DMA submission
> code.
>
> Thanks for your comments!
>
> v2:
> - Fix fallback in sci_dma_rx_complete(),
> - Fallback in the transmit path already works fine,
> - Widen audience, but keep RFC.
> ---
> drivers/tty/serial/sh-sci.c | 34 +++++++++++++++++++++++++---------
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 3aad48e64b9b71ff..30b2728c20d6e982 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1272,6 +1272,7 @@ static void sci_dma_rx_complete(void *arg)
> struct dma_async_tx_descriptor *desc;
> unsigned long flags;
> int active, count = 0;
> + unsigned int i;
Oops, there's a "u16 scr;" missing here.
>
> dev_dbg(port->dev, "%s(%d) active cookie %d\n", __func__, port->line,
> s->active_rx);
> @@ -1309,12 +1310,22 @@ static void sci_dma_rx_complete(void *arg)
> return;
>
> fail:
> + dmaengine_terminate_async(chan);
> spin_unlock_irqrestore(&port->lock, flags);
> dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
> /* Switch to PIO */
> spin_lock_irqsave(&port->lock, flags);
> + for (i = 0; i < 2; i++)
> + s->cookie_rx[i] = -EINVAL;
> + s->active_rx = 0;
> s->chan_rx = NULL;
> - sci_start_rx(port);
> + /* Direct new serial port interrupts back to CPU */
> + scr = serial_port_in(port, SCSCR);
> + if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
> + scr &= ~SCSCR_RDRQE;
> + enable_irq(s->irqs[SCIx_RXI_IRQ]);
> + }
> + serial_port_out(port, SCSCR, scr | SCSCR_RIE);
> spin_unlock_irqrestore(&port->lock, flags);
> }
>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
