* [PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx()
2018-12-13 18:44 [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
@ 2018-12-13 18:44 ` Geert Uytterhoeven
2018-12-17 13:17 ` Simon Horman
2018-12-13 18:44 ` [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback Geert Uytterhoeven
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2018-12-13 18:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ulrich Hecht, Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato,
linux-renesas-soc, linux-sh, linux-serial, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx()
2018-12-13 18:44 ` [PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx() Geert Uytterhoeven
@ 2018-12-17 13:17 ` Simon Horman
0 siblings, 0 replies; 15+ messages in thread
From: Simon Horman @ 2018-12-17 13:17 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Ulrich Hecht, Wolfram Sang,
Yoshihiro Shimoda, Yoshinori Sato, linux-renesas-soc, linux-sh,
linux-serial
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
2018-12-13 18:44 [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
2018-12-13 18:44 ` [PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx() Geert Uytterhoeven
@ 2018-12-13 18:44 ` Geert Uytterhoeven
2018-12-17 13:22 ` Simon Horman
2018-12-13 18:44 ` [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2018-12-13 18:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ulrich Hecht, Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato,
linux-renesas-soc, linux-sh, linux-serial, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
2018-12-13 18:44 ` [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback Geert Uytterhoeven
@ 2018-12-17 13:22 ` Simon Horman
2018-12-17 13:27 ` Geert Uytterhoeven
0 siblings, 1 reply; 15+ messages in thread
From: Simon Horman @ 2018-12-17 13:22 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Ulrich Hecht, Wolfram Sang,
Yoshihiro Shimoda, Yoshinori Sato, linux-renesas-soc, linux-sh,
linux-serial
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
2018-12-17 13:22 ` Simon Horman
@ 2018-12-17 13:27 ` Geert Uytterhoeven
2018-12-17 14:50 ` Simon Horman
0 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2018-12-17 13:27 UTC (permalink / raw)
To: Simon Horman
Cc: Geert Uytterhoeven, Greg Kroah-Hartman, Jiri Slaby, Ulrich Hecht,
Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato, Linux-Renesas,
Linux-sh list, open list:SERIAL DRIVERS
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
2018-12-17 13:27 ` Geert Uytterhoeven
@ 2018-12-17 14:50 ` Simon Horman
0 siblings, 0 replies; 15+ messages in thread
From: Simon Horman @ 2018-12-17 14:50 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Greg Kroah-Hartman, Jiri Slaby, Ulrich Hecht,
Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato, Linux-Renesas,
Linux-sh list, open list:SERIAL DRIVERS
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-13 18:44 [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
2018-12-13 18:44 ` [PATCH v4 1/4] serial: sh-sci: Fix locking in sci_submit_rx() Geert Uytterhoeven
2018-12-13 18:44 ` [PATCH v4 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback Geert Uytterhoeven
@ 2018-12-13 18:44 ` Geert Uytterhoeven
2018-12-14 16:14 ` Wolfram Sang
2018-12-17 13:25 ` Simon Horman
2018-12-13 18:44 ` [PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete() Geert Uytterhoeven
` (2 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Geert Uytterhoeven @ 2018-12-13 18:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ulrich Hecht, Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato,
linux-renesas-soc, linux-sh, linux-serial, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-13 18:44 ` [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
@ 2018-12-14 16:14 ` Wolfram Sang
2018-12-17 13:25 ` Simon Horman
1 sibling, 0 replies; 15+ messages in thread
From: Wolfram Sang @ 2018-12-14 16:14 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Ulrich Hecht, Wolfram Sang,
Yoshihiro Shimoda, Yoshinori Sato, linux-renesas-soc, linux-sh,
linux-serial
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-13 18:44 ` [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
2018-12-14 16:14 ` Wolfram Sang
@ 2018-12-17 13:25 ` Simon Horman
1 sibling, 0 replies; 15+ messages in thread
From: Simon Horman @ 2018-12-17 13:25 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Ulrich Hecht, Wolfram Sang,
Yoshihiro Shimoda, Yoshinori Sato, linux-renesas-soc, linux-sh,
linux-serial
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()
2018-12-13 18:44 [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
` (2 preceding siblings ...)
2018-12-13 18:44 ` [PATCH v4 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
@ 2018-12-13 18:44 ` Geert Uytterhoeven
2018-12-17 13:29 ` Simon Horman
2018-12-16 2:22 ` [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Rob Landley
2018-12-17 14:05 ` Greg Kroah-Hartman
5 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2018-12-13 18:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ulrich Hecht, Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato,
linux-renesas-soc, linux-sh, linux-serial, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()
2018-12-13 18:44 ` [PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete() Geert Uytterhoeven
@ 2018-12-17 13:29 ` Simon Horman
0 siblings, 0 replies; 15+ messages in thread
From: Simon Horman @ 2018-12-17 13:29 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Ulrich Hecht, Wolfram Sang,
Yoshihiro Shimoda, Yoshinori Sato, linux-renesas-soc, linux-sh,
linux-serial
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure
2018-12-13 18:44 [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
` (3 preceding siblings ...)
2018-12-13 18:44 ` [PATCH v4 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete() Geert Uytterhoeven
@ 2018-12-16 2:22 ` Rob Landley
2018-12-16 8:27 ` Geert Uytterhoeven
2018-12-17 14:05 ` Greg Kroah-Hartman
5 siblings, 1 reply; 15+ messages in thread
From: Rob Landley @ 2018-12-16 2:22 UTC (permalink / raw)
To: Geert Uytterhoeven, Greg Kroah-Hartman, Jiri Slaby
Cc: Ulrich Hecht, Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato,
linux-renesas-soc, linux-sh, linux-serial
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure
2018-12-16 2:22 ` [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Rob Landley
@ 2018-12-16 8:27 ` Geert Uytterhoeven
0 siblings, 0 replies; 15+ messages in thread
From: Geert Uytterhoeven @ 2018-12-16 8:27 UTC (permalink / raw)
To: Rob Landley
Cc: Geert Uytterhoeven, Greg KH, Jiri Slaby, uli+renesas,
Wolfram Sang, Yoshihiro Shimoda, Yoshinori Sato, Linux-Renesas,
Linux-sh list, open list:SERIAL DRIVERS
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure
2018-12-13 18:44 [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
` (4 preceding siblings ...)
2018-12-16 2:22 ` [PATCH v4 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Rob Landley
@ 2018-12-17 14:05 ` Greg Kroah-Hartman
5 siblings, 0 replies; 15+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-17 14:05 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jiri Slaby, Ulrich Hecht, Wolfram Sang, Yoshihiro Shimoda,
Yoshinori Sato, linux-renesas-soc, linux-sh, linux-serial
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
^ permalink raw reply [flat|nested] 15+ messages in thread