* [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure
@ 2018-12-10 18:21 Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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
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 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. For testing, this series is also available in the
topic/scif-pio-fallback-v3 branch of my renesas-drivers git repository
at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure
2018-12-10 18:21 [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 1/4] serial: sh-sci: Fix locking in sci_submit_rx() Geert Uytterhoeven
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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
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 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. For testing, this series is also available in the
topic/scif-pio-fallback-v3 branch of my renesas-drivers git repository
at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/4] serial: sh-sci: Fix locking in sci_submit_rx()
2018-12-10 18:21 [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback Geert Uytterhoeven
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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] 14+ messages in thread
* [PATCH v3 1/4] serial: sh-sci: Fix locking in sci_submit_rx()
2018-12-10 18:21 ` [PATCH v3 1/4] serial: sh-sci: Fix locking in sci_submit_rx() Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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] 14+ messages in thread
* [PATCH v3 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
2018-12-10 18:21 [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 1/4] serial: sh-sci: Fix locking in sci_submit_rx() Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete() Geert Uytterhoeven
4 siblings, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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] 14+ messages in thread
* [PATCH v3 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback
2018-12-10 18:21 ` [PATCH v3 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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] 14+ messages in thread
* [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-10 18:21 [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
` (2 preceding siblings ...)
2018-12-10 18:21 ` [PATCH v3 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 19:00 ` Wolfram Sang
2018-12-10 18:21 ` [PATCH v3 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete() Geert Uytterhoeven
4 siblings, 2 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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..d8672336d4ff9b65 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 -1;
}
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))
+ 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] 14+ messages in thread
* [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-10 18:21 ` [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 19:00 ` Wolfram Sang
1 sibling, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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..d8672336d4ff9b65 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 -1;
}
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))
+ 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] 14+ messages in thread
* [PATCH v3 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()
2018-12-10 18:21 [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
` (3 preceding siblings ...)
2018-12-10 18:21 ` [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
4 siblings, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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 d8672336d4ff9b65..946cdb0c7250955f 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] 14+ messages in thread
* [PATCH v3 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete()
2018-12-10 18:21 ` [PATCH v3 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete() Geert Uytterhoeven
@ 2018-12-10 18:21 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-10 18:21 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>
---
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 d8672336d4ff9b65..946cdb0c7250955f 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] 14+ messages in thread
* Re: [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-10 18:21 ` [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
@ 2018-12-10 19:00 ` Wolfram Sang
2018-12-10 19:00 ` Wolfram Sang
2018-12-11 8:16 ` Geert Uytterhoeven
1 sibling, 2 replies; 14+ messages in thread
From: Wolfram Sang @ 2018-12-10 19:00 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: 436 bytes --]
Hi Geert,
> if (!port_lock_held)
> spin_unlock_irqrestore(&port->lock, flags);
> + return -1;
-Esomething here...
> } else {
> + if (sci_submit_rx(s, false))
> + goto handle_pio;
and 'sci_submit_rx() < 0' here? I think it makes it more readable that
this is an error case then. 'if sci_submit_rx() do_something' sounds a
bit like the good case if boolean logic was used.
Thanks for picking this up again,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-10 19:00 ` Wolfram Sang
@ 2018-12-10 19:00 ` Wolfram Sang
2018-12-11 8:16 ` Geert Uytterhoeven
1 sibling, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2018-12-10 19:00 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: 436 bytes --]
Hi Geert,
> if (!port_lock_held)
> spin_unlock_irqrestore(&port->lock, flags);
> + return -1;
-Esomething here...
> } else {
> + if (sci_submit_rx(s, false))
> + goto handle_pio;
and 'sci_submit_rx() < 0' here? I think it makes it more readable that
this is an error case then. 'if sci_submit_rx() do_something' sounds a
bit like the good case if boolean logic was used.
Thanks for picking this up again,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-10 19:00 ` Wolfram Sang
2018-12-10 19:00 ` Wolfram Sang
@ 2018-12-11 8:16 ` Geert Uytterhoeven
2018-12-11 8:16 ` Geert Uytterhoeven
1 sibling, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-11 8:16 UTC (permalink / raw)
To: Wolfram Sang
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 Wolfram,
On Mon, Dec 10, 2018 at 8:00 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > if (!port_lock_held)
> > spin_unlock_irqrestore(&port->lock, flags);
> > + return -1;
>
> -Esomething here...
>
> > } else {
> > + if (sci_submit_rx(s, false))
> > + goto handle_pio;
>
> and 'sci_submit_rx() < 0' here? I think it makes it more readable that
> this is an error case then. 'if sci_submit_rx() do_something' sounds a
> bit like the good case if boolean logic was used.
Thanks, will update for v4, using -EAGAIN.
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] 14+ messages in thread
* Re: [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure
2018-12-11 8:16 ` Geert Uytterhoeven
@ 2018-12-11 8:16 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2018-12-11 8:16 UTC (permalink / raw)
To: Wolfram Sang
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 Wolfram,
On Mon, Dec 10, 2018 at 8:00 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > if (!port_lock_held)
> > spin_unlock_irqrestore(&port->lock, flags);
> > + return -1;
>
> -Esomething here...
>
> > } else {
> > + if (sci_submit_rx(s, false))
> > + goto handle_pio;
>
> and 'sci_submit_rx() < 0' here? I think it makes it more readable that
> this is an error case then. 'if sci_submit_rx() do_something' sounds a
> bit like the good case if boolean logic was used.
Thanks, will update for v4, using -EAGAIN.
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] 14+ messages in thread
end of thread, other threads:[~2018-12-11 8:16 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 18:21 [PATCH v3 0/4] serial: sh-sci: Fix fallback to PIO on DMA failure Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 1/4] serial: sh-sci: Fix locking in sci_submit_rx() Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 2/4] serial: sh-sci: Fix crash in rx_timer_fn() on PIO fallback Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 3/4] serial: sh-sci: Resume PIO in sci_rx_interrupt() on DMA failure Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
2018-12-10 19:00 ` Wolfram Sang
2018-12-10 19:00 ` Wolfram Sang
2018-12-11 8:16 ` Geert Uytterhoeven
2018-12-11 8:16 ` Geert Uytterhoeven
2018-12-10 18:21 ` [PATCH v3 4/4] serial: sh-sci: Fix fallback to PIO in sci_dma_rx_complete() Geert Uytterhoeven
2018-12-10 18:21 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).