[PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Geert Uytterhoeven]

	Hi Greg, Jiri,

When submitting a DMA request fails, the sh-sci 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 patch series fixes fallback to PIO in the receive path (cases 1 and
2).
Fallback to PIO in the transmit path (case 3) already seems to work
fine.

Changes compared to v3:
  - Let sci_submit_rx() return -EAGAIN instead of -1 on failure,
  - Check for negative error in sci_submit_rx() caller.

Changes compared to v2:
  - Add missing definition of "u16 scr" to sci_dma_rx_complete(),
  - Move label handle_pio inside #ifdef to kill defined but not used
    compiler warning when CONFIG_SERIAL_SH_SCI_DMA=n,
  - Move call to dmaengine_terminate_async() in sci_dma_rx_complete()
    inside the spinlock, for symmetry with sci_submit_rx(),
  - Move the call sci_submit_rx() in sci_rx_interrupt() up, as it may
    fail, rendering the modification of scr unused,
  - Split in multiple patches,
  - Drop RFC status.

Changes compared to v1:
  - Fix fallback in sci_dma_rx_complete(),
  - Fallback in the transmit path already works fine,
  - Widen audience, but keep RFC.

This has been tested on r8a7791/koelsch, using SCIF1 on debug serial 1,
and SCIFA3 on EXIO-B, by introducing random failures in DMA submission
code.

Thanks!

Geert Uytterhoeven (4):
  serial: sh-sci: Fix locking in sci_submit_rx()
  serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
  serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
  serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()

 drivers/tty/serial/sh-sci.c | 39 +++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Geert Uytterhoeven]

	Hi Greg, Jiri,

When submitting a DMA request fails, the sh-sci 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 patch series fixes fallback to PIO in the receive path (cases 1 and
2).
Fallback to PIO in the transmit path (case 3) already seems to work
fine.

Changes compared to v3:
  - Let sci_submit_rx() return -EAGAIN instead of -1 on failure,
  - Check for negative error in sci_submit_rx() caller.

Changes compared to v2:
  - Add missing definition of "u16 scr" to sci_dma_rx_complete(),
  - Move label handle_pio inside #ifdef to kill defined but not used
    compiler warning when CONFIG_SERIAL_SH_SCI_DMA=n,
  - Move call to dmaengine_terminate_async() in sci_dma_rx_complete()
    inside the spinlock, for symmetry with sci_submit_rx(),
  - Move the call sci_submit_rx() in sci_rx_interrupt() up, as it may
    fail, rendering the modification of scr unused,
  - Split in multiple patches,
  - Drop RFC status.

Changes compared to v1:
  - Fix fallback in sci_dma_rx_complete(),
  - Fallback in the transmit path already works fine,
  - Widen audience, but keep RFC.

This has been tested on r8a7791/koelsch, using SCIF1 on debug serial 1,
and SCIFA3 on EXIO-B, by introducing random failures in DMA submission
code.

Thanks!

Geert Uytterhoeven (4):
  serial: sh-sci: Fix locking in sci_submit_rx()
  serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
  serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
  serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()

 drivers/tty/serial/sh-sci.c | 39 +++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx()

[Geert Uytterhoeven]

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.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - No changes,

v3:
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 8146d9cef0cbe2d0..2a08039f792235e5 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1331,7 +1331,7 @@ static void sci_tx_dma_release(struct sci_port *s)
 	dma_release_channel(chan);
 }
 
-static void sci_submit_rx(struct sci_port *s)
+static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 {
 	struct dma_chan *chan = s->chan_rx;
 	struct uart_port *port = &s->port;
@@ -1362,16 +1362,18 @@ static void sci_submit_rx(struct sci_port *s)
 	return;
 
 fail:
+	/* Switch to PIO */
+	if (!port_lock_held)
+		spin_lock_irqsave(&port->lock, flags);
 	if (i)
 		dmaengine_terminate_async(chan);
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
 	s->active_rx = -EINVAL;
-	/* Switch to PIO */
-	spin_lock_irqsave(&port->lock, flags);
 	s->chan_rx = NULL;
 	sci_start_rx(port);
-	spin_unlock_irqrestore(&port->lock, flags);
+	if (!port_lock_held)
+		spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static void work_fn_tx(struct work_struct *work)
@@ -1491,7 +1493,7 @@ static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
 	}
 
 	if (port->type = PORT_SCIFA || port->type = PORT_SCIFB)
-		sci_submit_rx(s);
+		sci_submit_rx(s, true);
 
 	/* Direct new serial port interrupts back to CPU */
 	scr = serial_port_in(port, SCSCR);
@@ -1617,7 +1619,7 @@ static void sci_request_dma(struct uart_port *port)
 		s->chan_rx_saved = s->chan_rx = chan;
 
 		if (port->type = PORT_SCIFA || port->type = PORT_SCIFB)
-			sci_submit_rx(s);
+			sci_submit_rx(s, false);
 	}
 }
 
@@ -1667,7 +1669,7 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 			scr |= SCSCR_RDRQE;
 		} else {
 			scr &= ~SCSCR_RIE;
-			sci_submit_rx(s);
+			sci_submit_rx(s, false);
 		}
 		serial_port_out(port, SCSCR, scr);
 		/* Clear current interrupt */
-- 
2.17.1

[PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx()

[Geert Uytterhoeven]

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.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - No changes,

v3:
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 8146d9cef0cbe2d0..2a08039f792235e5 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1331,7 +1331,7 @@ static void sci_tx_dma_release(struct sci_port *s)
 	dma_release_channel(chan);
 }
 
-static void sci_submit_rx(struct sci_port *s)
+static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 {
 	struct dma_chan *chan = s->chan_rx;
 	struct uart_port *port = &s->port;
@@ -1362,16 +1362,18 @@ static void sci_submit_rx(struct sci_port *s)
 	return;
 
 fail:
+	/* Switch to PIO */
+	if (!port_lock_held)
+		spin_lock_irqsave(&port->lock, flags);
 	if (i)
 		dmaengine_terminate_async(chan);
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
 	s->active_rx = -EINVAL;
-	/* Switch to PIO */
-	spin_lock_irqsave(&port->lock, flags);
 	s->chan_rx = NULL;
 	sci_start_rx(port);
-	spin_unlock_irqrestore(&port->lock, flags);
+	if (!port_lock_held)
+		spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static void work_fn_tx(struct work_struct *work)
@@ -1491,7 +1493,7 @@ static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
 	}
 
 	if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
-		sci_submit_rx(s);
+		sci_submit_rx(s, true);
 
 	/* Direct new serial port interrupts back to CPU */
 	scr = serial_port_in(port, SCSCR);
@@ -1617,7 +1619,7 @@ static void sci_request_dma(struct uart_port *port)
 		s->chan_rx_saved = s->chan_rx = chan;
 
 		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
-			sci_submit_rx(s);
+			sci_submit_rx(s, false);
 	}
 }
 
@@ -1667,7 +1669,7 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 			scr |= SCSCR_RDRQE;
 		} else {
 			scr &= ~SCSCR_RIE;
-			sci_submit_rx(s);
+			sci_submit_rx(s, false);
 		}
 		serial_port_out(port, SCSCR, scr);
 		/* Clear current interrupt */
-- 
2.17.1

Re: [PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx()

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:41PM +0100, Geert Uytterhoeven wrote:
> 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.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
> v4:
>   - No changes,
> 
> v3:
>   - Split in multiple patches.
> ---
>  drivers/tty/serial/sh-sci.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 8146d9cef0cbe2d0..2a08039f792235e5 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1331,7 +1331,7 @@ static void sci_tx_dma_release(struct sci_port *s)
>  	dma_release_channel(chan);
>  }
>  
> -static void sci_submit_rx(struct sci_port *s)
> +static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
>  {
>  	struct dma_chan *chan = s->chan_rx;
>  	struct uart_port *port = &s->port;
> @@ -1362,16 +1362,18 @@ static void sci_submit_rx(struct sci_port *s)
>  	return;
>  
>  fail:
> +	/* Switch to PIO */
> +	if (!port_lock_held)
> +		spin_lock_irqsave(&port->lock, flags);
>  	if (i)
>  		dmaengine_terminate_async(chan);
>  	for (i = 0; i < 2; i++)
>  		s->cookie_rx[i] = -EINVAL;
>  	s->active_rx = -EINVAL;
> -	/* Switch to PIO */
> -	spin_lock_irqsave(&port->lock, flags);
>  	s->chan_rx = NULL;
>  	sci_start_rx(port);
> -	spin_unlock_irqrestore(&port->lock, flags);
> +	if (!port_lock_held)
> +		spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
>  static void work_fn_tx(struct work_struct *work)
> @@ -1491,7 +1493,7 @@ static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
>  	}
>  
>  	if (port->type = PORT_SCIFA || port->type = PORT_SCIFB)
> -		sci_submit_rx(s);
> +		sci_submit_rx(s, true);
>  
>  	/* Direct new serial port interrupts back to CPU */
>  	scr = serial_port_in(port, SCSCR);
> @@ -1617,7 +1619,7 @@ static void sci_request_dma(struct uart_port *port)
>  		s->chan_rx_saved = s->chan_rx = chan;
>  
>  		if (port->type = PORT_SCIFA || port->type = PORT_SCIFB)
> -			sci_submit_rx(s);
> +			sci_submit_rx(s, false);
>  	}
>  }
>  
> @@ -1667,7 +1669,7 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
>  			scr |= SCSCR_RDRQE;
>  		} else {
>  			scr &= ~SCSCR_RIE;
> -			sci_submit_rx(s);
> +			sci_submit_rx(s, false);
>  		}
>  		serial_port_out(port, SCSCR, scr);
>  		/* Clear current interrupt */
> -- 
> 2.17.1
> 

Re: [PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx()

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:41PM +0100, Geert Uytterhoeven wrote:
> 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.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
> v4:
>   - No changes,
> 
> v3:
>   - Split in multiple patches.
> ---
>  drivers/tty/serial/sh-sci.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 8146d9cef0cbe2d0..2a08039f792235e5 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1331,7 +1331,7 @@ static void sci_tx_dma_release(struct sci_port *s)
>  	dma_release_channel(chan);
>  }
>  
> -static void sci_submit_rx(struct sci_port *s)
> +static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
>  {
>  	struct dma_chan *chan = s->chan_rx;
>  	struct uart_port *port = &s->port;
> @@ -1362,16 +1362,18 @@ static void sci_submit_rx(struct sci_port *s)
>  	return;
>  
>  fail:
> +	/* Switch to PIO */
> +	if (!port_lock_held)
> +		spin_lock_irqsave(&port->lock, flags);
>  	if (i)
>  		dmaengine_terminate_async(chan);
>  	for (i = 0; i < 2; i++)
>  		s->cookie_rx[i] = -EINVAL;
>  	s->active_rx = -EINVAL;
> -	/* Switch to PIO */
> -	spin_lock_irqsave(&port->lock, flags);
>  	s->chan_rx = NULL;
>  	sci_start_rx(port);
> -	spin_unlock_irqrestore(&port->lock, flags);
> +	if (!port_lock_held)
> +		spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
>  static void work_fn_tx(struct work_struct *work)
> @@ -1491,7 +1493,7 @@ static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
>  	}
>  
>  	if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
> -		sci_submit_rx(s);
> +		sci_submit_rx(s, true);
>  
>  	/* Direct new serial port interrupts back to CPU */
>  	scr = serial_port_in(port, SCSCR);
> @@ -1617,7 +1619,7 @@ static void sci_request_dma(struct uart_port *port)
>  		s->chan_rx_saved = s->chan_rx = chan;
>  
>  		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
> -			sci_submit_rx(s);
> +			sci_submit_rx(s, false);
>  	}
>  }
>  
> @@ -1667,7 +1669,7 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
>  			scr |= SCSCR_RDRQE;
>  		} else {
>  			scr &= ~SCSCR_RIE;
> -			sci_submit_rx(s);
> +			sci_submit_rx(s, false);
>  		}
>  		serial_port_out(port, SCSCR, scr);
>  		/* Clear current interrupt */
> -- 
> 2.17.1
> 

[PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Geert Uytterhoeven]

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.

Use zero instead, which is the same value as after driver
initialization.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - No changes,

v3:
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 2a08039f792235e5..e353e03ce2602559 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1369,7 +1369,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 		dmaengine_terminate_async(chan);
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
-	s->active_rx = -EINVAL;
+	s->active_rx = 0;
 	s->chan_rx = NULL;
 	sci_start_rx(port);
 	if (!port_lock_held)
-- 
2.17.1

[PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Geert Uytterhoeven]

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.

Use zero instead, which is the same value as after driver
initialization.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - No changes,

v3:
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 2a08039f792235e5..e353e03ce2602559 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1369,7 +1369,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 		dmaengine_terminate_async(chan);
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
-	s->active_rx = -EINVAL;
+	s->active_rx = 0;
 	s->chan_rx = NULL;
 	sci_start_rx(port);
 	if (!port_lock_held)
-- 
2.17.1

Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:42PM +0100, Geert Uytterhoeven wrote:
> 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.
> 
> Use zero instead, which is the same value as after driver
> initialization.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

This looks good so to me long as dmaengine_submit() doesn't return 0.
Is that the case?

> ---
> v4:
>   - No changes,
> 
> v3:
>   - Split in multiple patches.
> ---
>  drivers/tty/serial/sh-sci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 2a08039f792235e5..e353e03ce2602559 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1369,7 +1369,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
>  		dmaengine_terminate_async(chan);
>  	for (i = 0; i < 2; i++)
>  		s->cookie_rx[i] = -EINVAL;
> -	s->active_rx = -EINVAL;
> +	s->active_rx = 0;
>  	s->chan_rx = NULL;
>  	sci_start_rx(port);
>  	if (!port_lock_held)
> -- 
> 2.17.1
> 

Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:42PM +0100, Geert Uytterhoeven wrote:
> 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.
> 
> Use zero instead, which is the same value as after driver
> initialization.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

This looks good so to me long as dmaengine_submit() doesn't return 0.
Is that the case?

> ---
> v4:
>   - No changes,
> 
> v3:
>   - Split in multiple patches.
> ---
>  drivers/tty/serial/sh-sci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 2a08039f792235e5..e353e03ce2602559 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1369,7 +1369,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
>  		dmaengine_terminate_async(chan);
>  	for (i = 0; i < 2; i++)
>  		s->cookie_rx[i] = -EINVAL;
> -	s->active_rx = -EINVAL;
> +	s->active_rx = 0;
>  	s->chan_rx = NULL;
>  	sci_start_rx(port);
>  	if (!port_lock_held)
> -- 
> 2.17.1
> 

Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Geert Uytterhoeven]

Hi Simon,

On Mon, Dec 17, 2018 at 2:22 PM Simon Horman <horms@verge.net.au> wrote:
> On Thu, Dec 13, 2018 at 07:44:42PM +0100, Geert Uytterhoeven wrote:
> > 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.
> >
> > Use zero instead, which is the same value as after driver
> > initialization.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> This looks good so to me long as dmaengine_submit() doesn't return 0.
> Is that the case?

include/linux/dmaengine.h:

/**
 * typedef dma_cookie_t - an opaque DMA cookie
 *
 * if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code
 */
typedef s32 dma_cookie_t;
#define DMA_MIN_COOKIE  1

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Geert Uytterhoeven]

Hi Simon,

On Mon, Dec 17, 2018 at 2:22 PM Simon Horman <horms@verge.net.au> wrote:
> On Thu, Dec 13, 2018 at 07:44:42PM +0100, Geert Uytterhoeven wrote:
> > 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.
> >
> > Use zero instead, which is the same value as after driver
> > initialization.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> This looks good so to me long as dmaengine_submit() doesn't return 0.
> Is that the case?

include/linux/dmaengine.h:

/**
 * typedef dma_cookie_t - an opaque DMA cookie
 *
 * if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code
 */
typedef s32 dma_cookie_t;
#define DMA_MIN_COOKIE  1

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Simon Horman]

On Mon, Dec 17, 2018 at 02:27:47PM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Mon, Dec 17, 2018 at 2:22 PM Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Dec 13, 2018 at 07:44:42PM +0100, Geert Uytterhoeven wrote:
> > > 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.
> > >
> > > Use zero instead, which is the same value as after driver
> > > initialization.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > This looks good so to me long as dmaengine_submit() doesn't return 0.
> > Is that the case?
> 
> include/linux/dmaengine.h:
> 
> /**
>  * typedef dma_cookie_t - an opaque DMA cookie
>  *
>  * if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code
>  */
> typedef s32 dma_cookie_t;
> #define DMA_MIN_COOKIE  1

In that case I have no objections.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback

[Simon Horman]

On Mon, Dec 17, 2018 at 02:27:47PM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Mon, Dec 17, 2018 at 2:22 PM Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Dec 13, 2018 at 07:44:42PM +0100, Geert Uytterhoeven wrote:
> > > 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.
> > >
> > > Use zero instead, which is the same value as after driver
> > > initialization.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > This looks good so to me long as dmaengine_submit() doesn't return 0.
> > Is that the case?
> 
> include/linux/dmaengine.h:
> 
> /**
>  * typedef dma_cookie_t - an opaque DMA cookie
>  *
>  * if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code
>  */
> typedef s32 dma_cookie_t;
> #define DMA_MIN_COOKIE  1

In that case I have no objections.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

[PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure

[Geert Uytterhoeven]

On (H)SCIF, sci_submit_rx() is called in the receive interrupt handler.
Hence if DMA submission fails, the interrupt handler should resume
handling reception using PIO, else no more data is received.

Make sci_submit_rx() return an error indicator, so the receive interrupt
handler can act appropriately.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - Let sci_submit_rx() return -EAGAIN instead of -1 on failure,
  - Check for negative error in sci_submit_rx() caller,

v3:
  - Move label handle_pio inside #ifdef to kill defined but not used
    compiler warning when CONFIG_SERIAL_SH_SCI_DMA=n,
  - Move the call sci_submit_rx() in sci_rx_interrupt() up, as it may
    fail, rendering the modification of scr unused,
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index e353e03ce2602559..8df0fd8245203f8b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1331,7 +1331,7 @@ static void sci_tx_dma_release(struct sci_port *s)
 	dma_release_channel(chan);
 }
 
-static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
+static int sci_submit_rx(struct sci_port *s, bool port_lock_held)
 {
 	struct dma_chan *chan = s->chan_rx;
 	struct uart_port *port = &s->port;
@@ -1359,7 +1359,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 	s->active_rx = s->cookie_rx[0];
 
 	dma_async_issue_pending(chan);
-	return;
+	return 0;
 
 fail:
 	/* Switch to PIO */
@@ -1374,6 +1374,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 	sci_start_rx(port);
 	if (!port_lock_held)
 		spin_unlock_irqrestore(&port->lock, flags);
+	return -EAGAIN;
 }
 
 static void work_fn_tx(struct work_struct *work)
@@ -1668,8 +1669,10 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 			disable_irq_nosync(irq);
 			scr |= SCSCR_RDRQE;
 		} else {
+			if (sci_submit_rx(s, false) < 0)
+				goto handle_pio;
+
 			scr &= ~SCSCR_RIE;
-			sci_submit_rx(s, false);
 		}
 		serial_port_out(port, SCSCR, scr);
 		/* Clear current interrupt */
@@ -1681,6 +1684,8 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 
 		return IRQ_HANDLED;
 	}
+
+handle_pio:
 #endif
 
 	if (s->rx_trigger > 1 && s->rx_fifo_timeout > 0) {
-- 
2.17.1

[PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure

[Geert Uytterhoeven]

On (H)SCIF, sci_submit_rx() is called in the receive interrupt handler.
Hence if DMA submission fails, the interrupt handler should resume
handling reception using PIO, else no more data is received.

Make sci_submit_rx() return an error indicator, so the receive interrupt
handler can act appropriately.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - Let sci_submit_rx() return -EAGAIN instead of -1 on failure,
  - Check for negative error in sci_submit_rx() caller,

v3:
  - Move label handle_pio inside #ifdef to kill defined but not used
    compiler warning when CONFIG_SERIAL_SH_SCI_DMA=n,
  - Move the call sci_submit_rx() in sci_rx_interrupt() up, as it may
    fail, rendering the modification of scr unused,
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index e353e03ce2602559..8df0fd8245203f8b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1331,7 +1331,7 @@ static void sci_tx_dma_release(struct sci_port *s)
 	dma_release_channel(chan);
 }
 
-static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
+static int sci_submit_rx(struct sci_port *s, bool port_lock_held)
 {
 	struct dma_chan *chan = s->chan_rx;
 	struct uart_port *port = &s->port;
@@ -1359,7 +1359,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 	s->active_rx = s->cookie_rx[0];
 
 	dma_async_issue_pending(chan);
-	return;
+	return 0;
 
 fail:
 	/* Switch to PIO */
@@ -1374,6 +1374,7 @@ static void sci_submit_rx(struct sci_port *s, bool port_lock_held)
 	sci_start_rx(port);
 	if (!port_lock_held)
 		spin_unlock_irqrestore(&port->lock, flags);
+	return -EAGAIN;
 }
 
 static void work_fn_tx(struct work_struct *work)
@@ -1668,8 +1669,10 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 			disable_irq_nosync(irq);
 			scr |= SCSCR_RDRQE;
 		} else {
+			if (sci_submit_rx(s, false) < 0)
+				goto handle_pio;
+
 			scr &= ~SCSCR_RIE;
-			sci_submit_rx(s, false);
 		}
 		serial_port_out(port, SCSCR, scr);
 		/* Clear current interrupt */
@@ -1681,6 +1684,8 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 
 		return IRQ_HANDLED;
 	}
+
+handle_pio:
 #endif
 
 	if (s->rx_trigger > 1 && s->rx_fifo_timeout > 0) {
-- 
2.17.1

Re: [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Thu, Dec 13, 2018 at 07:44:43PM +0100, Geert Uytterhoeven wrote:
> On (H)SCIF, sci_submit_rx() is called in the receive interrupt handler.
> Hence if DMA submission fails, the interrupt handler should resume
> handling reception using PIO, else no more data is received.
> 
> Make sci_submit_rx() return an error indicator, so the receive interrupt
> handler can act appropriately.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Thu, Dec 13, 2018 at 07:44:43PM +0100, Geert Uytterhoeven wrote:
> On (H)SCIF, sci_submit_rx() is called in the receive interrupt handler.
> Hence if DMA submission fails, the interrupt handler should resume
> handling reception using PIO, else no more data is received.
> 
> Make sci_submit_rx() return an error indicator, so the receive interrupt
> handler can act appropriately.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:43PM +0100, Geert Uytterhoeven wrote:
> On (H)SCIF, sci_submit_rx() is called in the receive interrupt handler.
> Hence if DMA submission fails, the interrupt handler should resume
> handling reception using PIO, else no more data is received.
> 
> Make sci_submit_rx() return an error indicator, so the receive interrupt
> handler can act appropriately.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:43PM +0100, Geert Uytterhoeven wrote:
> On (H)SCIF, sci_submit_rx() is called in the receive interrupt handler.
> Hence if DMA submission fails, the interrupt handler should resume
> handling reception using PIO, else no more data is received.
> 
> Make sci_submit_rx() return an error indicator, so the receive interrupt
> handler can act appropriately.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

[PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()

[Geert Uytterhoeven]

When submitting a DMA request fails in sci_dma_rx_complete(), the driver
tries to fall back to PIO, but that does not work: no more data will be
received, or the kernel will even crash.

Fix this similar as in (but not identical to) sci_submit_rx():
  - On SCIF, PIO cannot take over if any DMA transactions are pending,
    hence they must be terminated first.
  - All active cookies must be invalidated, else rx_timer_fn() may
    trigger a NULL pointer dereference.
  - 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.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - No changes,

v3:
  - Add missing definition of "u16 scr",
  - Move call to dmaengine_terminate_async() inside the spinlock, for
    symmetry with sci_submit_rx(),
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 8df0fd8245203f8b..44c29ce3fcc5d894 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1272,6 +1272,8 @@ static void sci_dma_rx_complete(void *arg)
 	struct dma_async_tx_descriptor *desc;
 	unsigned long flags;
 	int active, count = 0;
+	unsigned int i;
+	u16 scr;
 
 	dev_dbg(port->dev, "%s(%d) active cookie %d\n", __func__, port->line,
 		s->active_rx);
@@ -1313,8 +1315,18 @@ static void sci_dma_rx_complete(void *arg)
 	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
 	/* Switch to PIO */
 	spin_lock_irqsave(&port->lock, flags);
+	dmaengine_terminate_async(chan);
+	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);
 }
 
-- 
2.17.1

[PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()

[Geert Uytterhoeven]

When submitting a DMA request fails in sci_dma_rx_complete(), the driver
tries to fall back to PIO, but that does not work: no more data will be
received, or the kernel will even crash.

Fix this similar as in (but not identical to) sci_submit_rx():
  - On SCIF, PIO cannot take over if any DMA transactions are pending,
    hence they must be terminated first.
  - All active cookies must be invalidated, else rx_timer_fn() may
    trigger a NULL pointer dereference.
  - 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.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - No changes,

v3:
  - Add missing definition of "u16 scr",
  - Move call to dmaengine_terminate_async() inside the spinlock, for
    symmetry with sci_submit_rx(),
  - Split in multiple patches.
---
 drivers/tty/serial/sh-sci.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 8df0fd8245203f8b..44c29ce3fcc5d894 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1272,6 +1272,8 @@ static void sci_dma_rx_complete(void *arg)
 	struct dma_async_tx_descriptor *desc;
 	unsigned long flags;
 	int active, count = 0;
+	unsigned int i;
+	u16 scr;
 
 	dev_dbg(port->dev, "%s(%d) active cookie %d\n", __func__, port->line,
 		s->active_rx);
@@ -1313,8 +1315,18 @@ static void sci_dma_rx_complete(void *arg)
 	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
 	/* Switch to PIO */
 	spin_lock_irqsave(&port->lock, flags);
+	dmaengine_terminate_async(chan);
+	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);
 }
 
-- 
2.17.1

Re: [PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:44PM +0100, Geert Uytterhoeven wrote:
> When submitting a DMA request fails in sci_dma_rx_complete(), the driver
> tries to fall back to PIO, but that does not work: no more data will be
> received, or the kernel will even crash.
> 
> Fix this similar as in (but not identical to) sci_submit_rx():
>   - On SCIF, PIO cannot take over if any DMA transactions are pending,
>     hence they must be terminated first.
>   - All active cookies must be invalidated, else rx_timer_fn() may
>     trigger a NULL pointer dereference.
>   - 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.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - No changes,
> 
> v3:
>   - Add missing definition of "u16 scr",
>   - Move call to dmaengine_terminate_async() inside the spinlock, for
>     symmetry with sci_submit_rx(),
>   - Split in multiple patches.
> ---
>  drivers/tty/serial/sh-sci.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 8df0fd8245203f8b..44c29ce3fcc5d894 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1272,6 +1272,8 @@ static void sci_dma_rx_complete(void *arg)
>  	struct dma_async_tx_descriptor *desc;
>  	unsigned long flags;
>  	int active, count = 0;
> +	unsigned int i;
> +	u16 scr;
>  
>  	dev_dbg(port->dev, "%s(%d) active cookie %d\n", __func__, port->line,
>  		s->active_rx);
> @@ -1313,8 +1315,18 @@ static void sci_dma_rx_complete(void *arg)
>  	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
>  	/* Switch to PIO */
>  	spin_lock_irqsave(&port->lock, flags);
> +	dmaengine_terminate_async(chan);
> +	for (i = 0; i < 2; i++)
> +		s->cookie_rx[i] = -EINVAL;

Invalidating cookie_rx already appears twice, in two different forms.
Perhaps a helper is appropriate.

> +	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);

Likewise, logic ti redirect interrupts seems to already be present in
rx_timer_fn().

>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()

[Simon Horman]

On Thu, Dec 13, 2018 at 07:44:44PM +0100, Geert Uytterhoeven wrote:
> When submitting a DMA request fails in sci_dma_rx_complete(), the driver
> tries to fall back to PIO, but that does not work: no more data will be
> received, or the kernel will even crash.
> 
> Fix this similar as in (but not identical to) sci_submit_rx():
>   - On SCIF, PIO cannot take over if any DMA transactions are pending,
>     hence they must be terminated first.
>   - All active cookies must be invalidated, else rx_timer_fn() may
>     trigger a NULL pointer dereference.
>   - 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.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - No changes,
> 
> v3:
>   - Add missing definition of "u16 scr",
>   - Move call to dmaengine_terminate_async() inside the spinlock, for
>     symmetry with sci_submit_rx(),
>   - Split in multiple patches.
> ---
>  drivers/tty/serial/sh-sci.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 8df0fd8245203f8b..44c29ce3fcc5d894 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1272,6 +1272,8 @@ static void sci_dma_rx_complete(void *arg)
>  	struct dma_async_tx_descriptor *desc;
>  	unsigned long flags;
>  	int active, count = 0;
> +	unsigned int i;
> +	u16 scr;
>  
>  	dev_dbg(port->dev, "%s(%d) active cookie %d\n", __func__, port->line,
>  		s->active_rx);
> @@ -1313,8 +1315,18 @@ static void sci_dma_rx_complete(void *arg)
>  	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
>  	/* Switch to PIO */
>  	spin_lock_irqsave(&port->lock, flags);
> +	dmaengine_terminate_async(chan);
> +	for (i = 0; i < 2; i++)
> +		s->cookie_rx[i] = -EINVAL;

Invalidating cookie_rx already appears twice, in two different forms.
Perhaps a helper is appropriate.

> +	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);

Likewise, logic ti redirect interrupts seems to already be present in
rx_timer_fn().

>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Rob Landley]

On 12/13/18 12:44 PM, Geert Uytterhoeven wrote:
> 	Hi Greg, Jiri,
> 
> When submitting a DMA request fails, the sh-sci 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).

Is it possible to test this under qemu-system-sh4? (Does that emulate the DMA
controller?)

Rob

Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Rob Landley]

On 12/13/18 12:44 PM, Geert Uytterhoeven wrote:
> 	Hi Greg, Jiri,
> 
> When submitting a DMA request fails, the sh-sci 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).

Is it possible to test this under qemu-system-sh4? (Does that emulate the DMA
controller?)

Rob

Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Geert Uytterhoeven]

Hi Rob,

On Sun, Dec 16, 2018 at 3:22 AM Rob Landley <rob@landley.net> wrote:
> On 12/13/18 12:44 PM, Geert Uytterhoeven wrote:
> > When submitting a DMA request fails, the sh-sci 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).
>
> Is it possible to test this under qemu-system-sh4? (Does that emulate the DMA
> controller?)

No, the sh-sci emulation in QEMU is very limited, and does not include DMA.

My patch series does not affect PIO mode, only errors paths in DMA code.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Geert Uytterhoeven]

Hi Rob,

On Sun, Dec 16, 2018 at 3:22 AM Rob Landley <rob@landley.net> wrote:
> On 12/13/18 12:44 PM, Geert Uytterhoeven wrote:
> > When submitting a DMA request fails, the sh-sci 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).
>
> Is it possible to test this under qemu-system-sh4? (Does that emulate the DMA
> controller?)

No, the sh-sci emulation in QEMU is very limited, and does not include DMA.

My patch series does not affect PIO mode, only errors paths in DMA code.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Greg Kroah-Hartman]

On Thu, Dec 13, 2018 at 07:44:40PM +0100, Geert Uytterhoeven wrote:
> 	Hi Greg, Jiri,
> 
> When submitting a DMA request fails, the sh-sci 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 patch series fixes fallback to PIO in the receive path (cases 1 and
> 2).
> Fallback to PIO in the transmit path (case 3) already seems to work
> fine.
> 
> Changes compared to v3:
>   - Let sci_submit_rx() return -EAGAIN instead of -1 on failure,
>   - Check for negative error in sci_submit_rx() caller.

First 3 patches now queued up, thanks.

I'll wait for a respin or something for patch 4.

thanks,

greg k-h

Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure

[Greg Kroah-Hartman]

On Thu, Dec 13, 2018 at 07:44:40PM +0100, Geert Uytterhoeven wrote:
> 	Hi Greg, Jiri,
> 
> When submitting a DMA request fails, the sh-sci 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 patch series fixes fallback to PIO in the receive path (cases 1 and
> 2).
> Fallback to PIO in the transmit path (case 3) already seems to work
> fine.
> 
> Changes compared to v3:
>   - Let sci_submit_rx() return -EAGAIN instead of -1 on failure,
>   - Check for negative error in sci_submit_rx() caller.

First 3 patches now queued up, thanks.

I'll wait for a respin or something for patch 4.

thanks,

greg k-h