[PATCH 0/3] serial: sh-sci: Fix port shutdown DMA race conditions

[PATCH 0/3] serial: sh-sci: Fix port shutdown DMA race conditions

[Geert Uytterhoeven]

	Hi all,

This patch series fixes race conditions between DMA use and port
shutdown on Renesas "SCIF" serial ports, causing e.g.

    sh-sci e6550000.serial: Failed preparing Tx DMA descriptor
    Unable to handle kernel read from unreadable memory at virtual address 0000000000000000
    ...
    Call trace:
     sci_tx_dma_release+0x50/0xfc
     work_fn_tx+0x128/0x22c
     process_one_work+0x394/0x62c
     worker_thread+0x21c/0x324
     kthread+0x118/0x128
     ret_from_fork+0x10/0x18

I have no guaranteed way to reproducis this issue. I see it sometimes
when doing a partial login on a port running getty, and letting getty
time out.

The first two patches simplify DMA release handling, and make sure the
work function is not called after port shutdown.
The last patch switches the driver to the new
dmaengine_terminate_(a)sync() functions, now DMA release is done from a
single point.

Please review. Thanks!

Geert Uytterhoeven (3):
  serial: sh-sci: Postpone DMA release when falling back to PIO
  serial: sh-sci: Stop TX DMA workqueue during port shutdown
  serial: sh-sci: Stop using deprecated dmaengine_terminate_all()

 drivers/tty/serial/sh-sci.c | 93 +++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 46 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 1/3] serial: sh-sci: Postpone DMA release when falling back to PIO

[Geert Uytterhoeven]

When the sh-sci driver detects an issue with DMA during operation, it
falls backs to PIO, and releases all DMA resources.

As releasing DMA resources immediately has no advantages, but
complicates the code, and is susceptible to races, it is better to
postpone this to port shutdown.

This allows to remove the locking from sci_rx_dma_release() and
sci_tx_dma_release(), but requires keeping a copy of the DMA channel
pointers for release during port shutdown.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 81 +++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index c181eb37f98509e6..0ed91692f53ad859 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -135,6 +135,8 @@ struct sci_port {
 	struct dma_chan			*chan_rx;
 
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
+	struct dma_chan			*chan_tx_saved;
+	struct dma_chan			*chan_rx_saved;
 	dma_cookie_t			cookie_tx;
 	dma_cookie_t			cookie_rx[2];
 	dma_cookie_t			active_rx;
@@ -1212,25 +1214,17 @@ static int sci_dma_rx_find_active(struct sci_port *s)
 	return -1;
 }
 
-static void sci_rx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_rx_dma_release(struct sci_port *s)
 {
-	struct dma_chan *chan = s->chan_rx;
+	struct dma_chan *chan = s->chan_rx_saved;
 	struct uart_port *port = &s->port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
-	s->chan_rx = NULL;
+	s->chan_rx_saved = s->chan_rx = NULL;
 	s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
-	spin_unlock_irqrestore(&port->lock, flags);
 	dmaengine_terminate_all(chan);
 	dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
 			  sg_dma_address(&s->sg_rx[0]));
 	dma_release_channel(chan);
-	if (enable_pio) {
-		spin_lock_irqsave(&port->lock, flags);
-		sci_start_rx(port);
-		spin_unlock_irqrestore(&port->lock, flags);
-	}
 }
 
 static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
@@ -1289,33 +1283,31 @@ static void sci_dma_rx_complete(void *arg)
 fail:
 	spin_unlock_irqrestore(&port->lock, flags);
 	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
-	sci_rx_dma_release(s, true);
+	/* Switch to PIO */
+	spin_lock_irqsave(&port->lock, flags);
+	s->chan_rx = NULL;
+	sci_start_rx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
-static void sci_tx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_tx_dma_release(struct sci_port *s)
 {
-	struct dma_chan *chan = s->chan_tx;
+	struct dma_chan *chan = s->chan_tx_saved;
 	struct uart_port *port = &s->port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
-	s->chan_tx = NULL;
+	s->chan_tx_saved = s->chan_tx = NULL;
 	s->cookie_tx = -EINVAL;
-	spin_unlock_irqrestore(&port->lock, flags);
 	dmaengine_terminate_all(chan);
 	dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
 			 DMA_TO_DEVICE);
 	dma_release_channel(chan);
-	if (enable_pio) {
-		spin_lock_irqsave(&port->lock, flags);
-		sci_start_tx(port);
-		spin_unlock_irqrestore(&port->lock, flags);
-	}
 }
 
 static void sci_submit_rx(struct sci_port *s)
 {
 	struct dma_chan *chan = s->chan_rx;
+	struct uart_port *port = &s->port;
+	unsigned long flags;
 	int i;
 
 	for (i = 0; i < 2; i++) {
@@ -1347,7 +1339,11 @@ static void sci_submit_rx(struct sci_port *s)
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
 	s->active_rx = -EINVAL;
-	sci_rx_dma_release(s, true);
+	/* Switch to PIO */
+	spin_lock_irqsave(&port->lock, flags);
+	s->chan_rx = NULL;
+	sci_start_rx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static void work_fn_tx(struct work_struct *work)
@@ -1357,6 +1353,7 @@ static void work_fn_tx(struct work_struct *work)
 	struct dma_chan *chan = s->chan_tx;
 	struct uart_port *port = &s->port;
 	struct circ_buf *xmit = &port->state->xmit;
+	unsigned long flags;
 	dma_addr_t buf;
 
 	/*
@@ -1378,9 +1375,7 @@ static void work_fn_tx(struct work_struct *work)
 					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!desc) {
 		dev_warn(port->dev, "Failed preparing Tx DMA descriptor\n");
-		/* switch to PIO */
-		sci_tx_dma_release(s, true);
-		return;
+		goto switch_to_pio;
 	}
 
 	dma_sync_single_for_device(chan->device->dev, buf, s->tx_dma_len,
@@ -1393,15 +1388,21 @@ static void work_fn_tx(struct work_struct *work)
 	s->cookie_tx = dmaengine_submit(desc);
 	if (dma_submit_error(s->cookie_tx)) {
 		dev_warn(port->dev, "Failed submitting Tx DMA descriptor\n");
-		/* switch to PIO */
-		sci_tx_dma_release(s, true);
-		return;
+		goto switch_to_pio;
 	}
 
 	dev_dbg(port->dev, "%s: %p: %d...%d, cookie %d\n",
 		__func__, xmit->buf, xmit->tail, xmit->head, s->cookie_tx);
 
 	dma_async_issue_pending(chan);
+	return;
+
+switch_to_pio:
+	spin_lock_irqsave(&port->lock, flags);
+	s->chan_tx = NULL;
+	sci_start_tx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
+	return;
 }
 
 static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
@@ -1535,7 +1536,6 @@ static void sci_request_dma(struct uart_port *port)
 	chan = sci_request_dma_chan(port, DMA_MEM_TO_DEV);
 	dev_dbg(port->dev, "%s: TX: got channel %p\n", __func__, chan);
 	if (chan) {
-		s->chan_tx = chan;
 		/* UART circular tx buffer is an aligned page. */
 		s->tx_dma_addr = dma_map_single(chan->device->dev,
 						port->state->xmit.buf,
@@ -1544,11 +1544,13 @@ static void sci_request_dma(struct uart_port *port)
 		if (dma_mapping_error(chan->device->dev, s->tx_dma_addr)) {
 			dev_warn(port->dev, "Failed mapping Tx DMA descriptor\n");
 			dma_release_channel(chan);
-			s->chan_tx = NULL;
+			chan = NULL;
 		} else {
 			dev_dbg(port->dev, "%s: mapped %lu@%p to %pad\n",
 				__func__, UART_XMIT_SIZE,
 				port->state->xmit.buf, &s->tx_dma_addr);
+
+			s->chan_tx_saved = s->chan_tx = chan;
 		}
 
 		INIT_WORK(&s->work_tx, work_fn_tx);
@@ -1561,8 +1563,6 @@ static void sci_request_dma(struct uart_port *port)
 		dma_addr_t dma;
 		void *buf;
 
-		s->chan_rx = chan;
-
 		s->buf_len_rx = 2 * max_t(size_t, 16, port->fifosize);
 		buf = dma_alloc_coherent(chan->device->dev, s->buf_len_rx * 2,
 					 &dma, GFP_KERNEL);
@@ -1570,7 +1570,6 @@ static void sci_request_dma(struct uart_port *port)
 			dev_warn(port->dev,
 				 "Failed to allocate Rx dma buffer, using PIO\n");
 			dma_release_channel(chan);
-			s->chan_rx = NULL;
 			return;
 		}
 
@@ -1591,6 +1590,8 @@ static void sci_request_dma(struct uart_port *port)
 
 		if (port->type = PORT_SCIFA || port->type = PORT_SCIFB)
 			sci_submit_rx(s);
+
+		s->chan_rx_saved = s->chan_rx = chan;
 	}
 }
 
@@ -1598,10 +1599,10 @@ static void sci_free_dma(struct uart_port *port)
 {
 	struct sci_port *s = to_sci_port(port);
 
-	if (s->chan_tx)
-		sci_tx_dma_release(s, false);
-	if (s->chan_rx)
-		sci_rx_dma_release(s, false);
+	if (s->chan_tx_saved)
+		sci_tx_dma_release(s);
+	if (s->chan_rx_saved)
+		sci_rx_dma_release(s);
 }
 
 static void sci_flush_buffer(struct uart_port *port)
@@ -2092,7 +2093,7 @@ static void sci_shutdown(struct uart_port *port)
 	spin_unlock_irqrestore(&port->lock, flags);
 
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
-	if (s->chan_rx) {
+	if (s->chan_rx_saved) {
 		dev_dbg(port->dev, "%s(%d) deleting rx_timer\n", __func__,
 			port->line);
 		hrtimer_cancel(&s->rx_timer);
-- 
2.17.1

Re: [PATCH 1/3] serial: sh-sci: Postpone DMA release when falling back to PIO

[Geert Uytterhoeven]

On Fri, Jun 29, 2018 at 4:26 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> When the sh-sci driver detects an issue with DMA during operation, it
> falls backs to PIO, and releases all DMA resources.
>
> As releasing DMA resources immediately has no advantages, but
> complicates the code, and is susceptible to races, it is better to
> postpone this to port shutdown.
>
> This allows to remove the locking from sci_rx_dma_release() and
> sci_tx_dma_release(), but requires keeping a copy of the DMA channel
> pointers for release during port shutdown.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c

> @@ -1212,25 +1214,17 @@ static int sci_dma_rx_find_active(struct sci_port *s)
>         return -1;
>  }
>
> -static void sci_rx_dma_release(struct sci_port *s, bool enable_pio)
> +static void sci_rx_dma_release(struct sci_port *s)
>  {
> -       struct dma_chan *chan = s->chan_rx;
> +       struct dma_chan *chan = s->chan_rx_saved;
>         struct uart_port *port = &s->port;

Brown paper bag #2: unused variable port. Let's blame it on the warm Friday
afternoon.

So yes, there will be a v2, eventually...

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 2/3] serial: sh-sci: Stop TX DMA workqueue during port shutdown

[Geert Uytterhoeven]

The transmit DMA workqueue is never stopped, hence the work function may
be called after the port has been shut down.

Fix this race condition by cancelling queued work, if any, before DMA
release.  Don't initialize the work if DMA initialization failed, as it
won't be used anyway.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 0ed91692f53ad859..1e0b6fe6865dcff6 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1295,6 +1295,7 @@ static void sci_tx_dma_release(struct sci_port *s)
 	struct dma_chan *chan = s->chan_tx_saved;
 	struct uart_port *port = &s->port;
 
+	cancel_work_sync(&s->work_tx);
 	s->chan_tx_saved = s->chan_tx = NULL;
 	s->cookie_tx = -EINVAL;
 	dmaengine_terminate_all(chan);
@@ -1550,10 +1551,9 @@ static void sci_request_dma(struct uart_port *port)
 				__func__, UART_XMIT_SIZE,
 				port->state->xmit.buf, &s->tx_dma_addr);
 
+			INIT_WORK(&s->work_tx, work_fn_tx);
 			s->chan_tx_saved = s->chan_tx = chan;
 		}
-
-		INIT_WORK(&s->work_tx, work_fn_tx);
 	}
 
 	chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM);
-- 
2.17.1

[PATCH 3/3] serial: sh-sci: Stop using deprecated dmaengine_terminate_all()

[Geert Uytterhoeven]

As of commit b36f09c3c441a6e5 ("dmaengine: Add transfer termination
synchronization support"), dmaengine_terminate_all() is deprecated.

Replace calls to dmaengine_terminate_all() in DMA release code by calls
to dmaengine_terminate_sync(), as the latter waits until all running
completion callbacks have finished.

Replace calls to dmaengine_terminate_all() in DMA failure paths by calls
to dmaengine_terminate_async(), as these are usually done in atomic
context.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 1e0b6fe6865dcff6..5c870ab80b98e25b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1221,7 +1221,7 @@ static void sci_rx_dma_release(struct sci_port *s)
 
 	s->chan_rx_saved = s->chan_rx = NULL;
 	s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
-	dmaengine_terminate_all(chan);
+	dmaengine_terminate_sync(chan);
 	dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
 			  sg_dma_address(&s->sg_rx[0]));
 	dma_release_channel(chan);
@@ -1298,7 +1298,7 @@ static void sci_tx_dma_release(struct sci_port *s)
 	cancel_work_sync(&s->work_tx);
 	s->chan_tx_saved = s->chan_tx = NULL;
 	s->cookie_tx = -EINVAL;
-	dmaengine_terminate_all(chan);
+	dmaengine_terminate_sync(chan);
 	dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
 			 DMA_TO_DEVICE);
 	dma_release_channel(chan);
@@ -1336,7 +1336,7 @@ static void sci_submit_rx(struct sci_port *s)
 
 fail:
 	if (i)
-		dmaengine_terminate_all(chan);
+		dmaengine_terminate_async(chan);
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
 	s->active_rx = -EINVAL;
@@ -1454,7 +1454,7 @@ static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
 	}
 
 	/* Handle incomplete DMA receive */
-	dmaengine_terminate_all(s->chan_rx);
+	dmaengine_terminate_async(s->chan_rx);
 	read = sg_dma_len(&s->sg_rx[active]) - state.residue;
 
 	if (read) {
-- 
2.17.1

[PATCH 1/4] serial: sh-sci: Postpone DMA release when falling back to PIO

[Geert Uytterhoeven]

When the sh-sci driver detects an issue with DMA during operation, it
falls backs to PIO, and releases all DMA resources.

As releasing DMA resources immediately has no advantages, but
complicates the code, and is susceptible to races, it is better to
postpone this to port shutdown.

This allows to remove the locking from sci_rx_dma_release() and
sci_tx_dma_release(), but requires keeping a copy of the DMA channel
pointers for release during port shutdown.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 81 +++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index cf8c394c6f185792..898c1034cad23a88 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -414,6 +414,8 @@ struct sci_port {
 	struct dma_chan			*chan_rx;
 
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
+	struct dma_chan			*chan_tx_saved;
+	struct dma_chan			*chan_rx_saved;
 	dma_cookie_t			cookie_tx;
 	dma_cookie_t			cookie_rx[2];
 	dma_cookie_t			active_rx;
@@ -1602,27 +1604,19 @@ static int sci_dma_rx_find_active(struct sci_port *s)
 	return -1;
 }
 
-static void sci_rx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_rx_dma_release(struct sci_port *s)
 {
-	struct dma_chan *chan = s->chan_rx;
+	struct dma_chan *chan = s->chan_rx_saved;
 	struct uart_port *port = &s->port;
-	unsigned long flags;
 
 dev_dbg_dma(port->dev, "%s\n", __func__);
-	spin_lock_irqsave(&port->lock, flags);
-	s->chan_rx = NULL;
+	s->chan_rx_saved = s->chan_rx = NULL;
 	s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
-	spin_unlock_irqrestore(&port->lock, flags);
 WARN(!chan, "RX DMA channel already released\n");
 	dmaengine_terminate_all(chan);
 	dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
 			  sg_dma_address(&s->sg_rx[0]));
 	dma_release_channel(chan);
-	if (enable_pio) {
-		spin_lock_irqsave(&port->lock, flags);
-		sci_start_rx(port);
-		spin_unlock_irqrestore(&port->lock, flags);
-	}
 }
 
 static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
@@ -1698,35 +1692,33 @@ dev_dbg_dma(port->dev, "  submit new desc #%u\n", active);
 fail:
 	spin_unlock_irqrestore(&port->lock, flags);
 	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
-	sci_rx_dma_release(s, true);
+	/* Switch to PIO */
+	spin_lock_irqsave(&port->lock, flags);
+	s->chan_rx = NULL;
+	sci_start_rx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
-static void sci_tx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_tx_dma_release(struct sci_port *s)
 {
-	struct dma_chan *chan = s->chan_tx;
+	struct dma_chan *chan = s->chan_tx_saved;
 	struct uart_port *port = &s->port;
-	unsigned long flags;
 
 dev_dbg_dma(port->dev, "%s\n", __func__);
-	spin_lock_irqsave(&port->lock, flags);
-	s->chan_tx = NULL;
+	s->chan_tx_saved = s->chan_tx = NULL;
 	s->cookie_tx = -EINVAL;
-	spin_unlock_irqrestore(&port->lock, flags);
 WARN(!chan, "TX DMA channel already released\n");
 	dmaengine_terminate_all(chan);
 	dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
 			 DMA_TO_DEVICE);
 	dma_release_channel(chan);
-	if (enable_pio) {
-		spin_lock_irqsave(&port->lock, flags);
-		sci_start_tx(port);
-		spin_unlock_irqrestore(&port->lock, flags);
-	}
 }
 
 static void sci_submit_rx(struct sci_port *s)
 {
 	struct dma_chan *chan = s->chan_rx;
+	struct uart_port *port = &s->port;
+	unsigned long flags;
 	int i;
 
 dev_dbg_dma(s->port.dev, "  %s\n", __func__);
@@ -1760,7 +1752,11 @@ dev_dbg_dma(s->port.dev, "  %s\n", __func__);
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
 	s->active_rx = -EINVAL;
-	sci_rx_dma_release(s, true);
+	/* Switch to PIO */
+	spin_lock_irqsave(&port->lock, flags);
+	s->chan_rx = NULL;
+	sci_start_rx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static void work_fn_tx(struct work_struct *work)
@@ -1770,6 +1766,7 @@ static void work_fn_tx(struct work_struct *work)
 	struct dma_chan *chan = s->chan_tx;
 	struct uart_port *port = &s->port;
 	struct circ_buf *xmit = &port->state->xmit;
+	unsigned long flags;
 	dma_addr_t buf;
 
 dev_dbg_dma(port->dev, "WORK %s\n", __func__);
@@ -1792,9 +1789,7 @@ dev_dbg_dma(port->dev, "WORK %s\n", __func__);
 					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!desc) {
 		dev_warn(port->dev, "Failed preparing Tx DMA descriptor\n");
-		/* switch to PIO */
-		sci_tx_dma_release(s, true);
-		return;
+		goto switch_to_pio;
 	}
 
 	dma_sync_single_for_device(chan->device->dev, buf, s->tx_dma_len,
@@ -1807,15 +1802,21 @@ dev_dbg_dma(port->dev, "WORK %s\n", __func__);
 	s->cookie_tx = dmaengine_submit(desc);
 	if (dma_submit_error(s->cookie_tx)) {
 		dev_warn(port->dev, "Failed submitting Tx DMA descriptor\n");
-		/* switch to PIO */
-		sci_tx_dma_release(s, true);
-		return;
+		goto switch_to_pio;
 	}
 
 	dev_dbg_dma(port->dev, "  %p: %d...%d, cookie %d\n",
 		xmit->buf, xmit->tail, xmit->head, s->cookie_tx);
 
 	dma_async_issue_pending(chan);
+	return;
+
+switch_to_pio:
+	spin_lock_irqsave(&port->lock, flags);
+	s->chan_tx = NULL;
+	sci_start_tx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
+	return;
 }
 
 static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
@@ -1963,7 +1964,6 @@ static void sci_request_dma(struct uart_port *port)
 	chan = sci_request_dma_chan(port, DMA_MEM_TO_DEV);
 	dev_dbg_dma(port->dev, "  TX: got channel %p\n", chan);
 	if (chan) {
-		s->chan_tx = chan;
 		/* UART circular tx buffer is an aligned page. */
 		s->tx_dma_addr = dma_map_single(chan->device->dev,
 						port->state->xmit.buf,
@@ -1972,11 +1972,13 @@ static void sci_request_dma(struct uart_port *port)
 		if (dma_mapping_error(chan->device->dev, s->tx_dma_addr)) {
 			dev_warn(port->dev, "Failed mapping Tx DMA descriptor\n");
 			dma_release_channel(chan);
-			s->chan_tx = NULL;
+			chan = NULL;
 		} else {
 			dev_dbg_dma(port->dev, "  mapped %lu@%p to %pad\n",
 				UART_XMIT_SIZE,
 				port->state->xmit.buf, &s->tx_dma_addr);
+
+			s->chan_tx_saved = s->chan_tx = chan;
 		}
 
 		INIT_WORK(&s->work_tx, work_fn_tx);
@@ -1989,8 +1991,6 @@ static void sci_request_dma(struct uart_port *port)
 		dma_addr_t dma;
 		void *buf;
 
-		s->chan_rx = chan;
-
 		s->buf_len_rx = 2 * max_t(size_t, 16, port->fifosize);
 		buf = dma_alloc_coherent(chan->device->dev, s->buf_len_rx * 2,
 					 &dma, GFP_KERNEL);
@@ -1998,7 +1998,6 @@ static void sci_request_dma(struct uart_port *port)
 			dev_warn(port->dev,
 				 "Failed to allocate Rx dma buffer, using PIO\n");
 			dma_release_channel(chan);
-			s->chan_rx = NULL;
 			return;
 		}
 
@@ -2019,6 +2018,8 @@ static void sci_request_dma(struct uart_port *port)
 
 		if (port->type = PORT_SCIFA || port->type = PORT_SCIFB)
 			sci_submit_rx(s);
+
+		s->chan_rx_saved = s->chan_rx = chan;
 	}
 }
 
@@ -2026,10 +2027,10 @@ static void sci_free_dma(struct uart_port *port)
 {
 	struct sci_port *s = to_sci_port(port);
 
-	if (s->chan_tx)
-		sci_tx_dma_release(s, false);
-	if (s->chan_rx)
-		sci_rx_dma_release(s, false);
+	if (s->chan_tx_saved)
+		sci_tx_dma_release(s);
+	if (s->chan_rx_saved)
+		sci_rx_dma_release(s);
 }
 
 static void sci_flush_buffer(struct uart_port *port)
@@ -2563,7 +2564,7 @@ static void sci_shutdown(struct uart_port *port)
 	spin_unlock_irqrestore(&port->lock, flags);
 
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
-	if (s->chan_rx) {
+	if (s->chan_rx_saved) {
 		dev_dbg(port->dev, "%s(%d) deleting rx_timer\n", __func__,
 			port->line);
 		hrtimer_cancel(&s->rx_timer);
-- 
2.17.1

[PATCH 2/4] serial: sh-sci: Stop using deprecated dmaengine_terminate_all()

[Geert Uytterhoeven]

As of commit b36f09c3c441a6e5 ("dmaengine: Add transfer termination
synchronization support"), dmaengine_terminate_all() is deprecated.

Replace calls to dmaengine_terminate_all() in DMA release code by calls
to dmaengine_terminate_sync(), as the latter waits until all running
completion callbacks have finished.

Replace calls to dmaengine_terminate_all() in DMA failure paths by calls
to dmaengine_terminate_async(), as these are usually done in atomic
context.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 898c1034cad23a88..061660ccf9442d02 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1613,7 +1613,7 @@ dev_dbg_dma(port->dev, "%s\n", __func__);
 	s->chan_rx_saved = s->chan_rx = NULL;
 	s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
 WARN(!chan, "RX DMA channel already released\n");
-	dmaengine_terminate_all(chan);
+	dmaengine_terminate_sync(chan);
 	dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
 			  sg_dma_address(&s->sg_rx[0]));
 	dma_release_channel(chan);
@@ -1708,7 +1708,7 @@ dev_dbg_dma(port->dev, "%s\n", __func__);
 	s->chan_tx_saved = s->chan_tx = NULL;
 	s->cookie_tx = -EINVAL;
 WARN(!chan, "TX DMA channel already released\n");
-	dmaengine_terminate_all(chan);
+	dmaengine_terminate_sync(chan);
 	dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
 			 DMA_TO_DEVICE);
 	dma_release_channel(chan);
@@ -1747,7 +1747,7 @@ dev_dbg_dma(s->port.dev, "  %s\n", __func__);
 
 fail:
 	if (i)
-		dmaengine_terminate_all(chan);
+		dmaengine_terminate_async(chan);
 	// FIXME Not needed?
 	for (i = 0; i < 2; i++)
 		s->cookie_rx[i] = -EINVAL;
@@ -1877,9 +1877,9 @@ dev_dbg_dma(port->dev, "  %s active %d\n", __func__, active);
 
 	/* Handle incomplete DMA receive */
 dev_dbg_dma(port->dev, "  %s:%u dmaengine_terminate_all()\n", __func__, __LINE__);
-	dmaengine_terminate_all(s->chan_rx);
+	dmaengine_terminate_async(s->chan_rx);
 	// FIXME More data may have been transfered in between
-	// FIXME dmaengine_tx_status() and dmaengine_terminate_all()
+	// FIXME dmaengine_tx_status() and dmaengine_terminate_async()
 	// FIXME Call dmaengine_pause() first
 dev_dbg_dma(port->dev, "  %s:%u terminated\n", __func__, __LINE__);
 	read = sg_dma_len(&s->sg_rx[active]) - state.residue;
-- 
2.17.1

Re: [PATCH 2/4] serial: sh-sci: Stop using deprecated dmaengine_terminate_all()

[Geert Uytterhoeven]

On Fri, Jun 29, 2018 at 4:25 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> As of commit b36f09c3c441a6e5 ("dmaengine: Add transfer termination
> synchronization support"), dmaengine_terminate_all() is deprecated.
>
> Replace calls to dmaengine_terminate_all() in DMA release code by calls
> to dmaengine_terminate_sync(), as the latter waits until all running
> completion callbacks have finished.
>
> Replace calls to dmaengine_terminate_all() in DMA failure paths by calls
> to dmaengine_terminate_async(), as these are usually done in atomic
> context.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/tty/serial/sh-sci.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 898c1034cad23a88..061660ccf9442d02 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1613,7 +1613,7 @@ dev_dbg_dma(port->dev, "%s\n", __func__);
>         s->chan_rx_saved = s->chan_rx = NULL;
>         s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
>  WARN(!chan, "RX DMA channel already released\n");

Woops, I had some old versions of the patches lying around, and accidentally
sent them with the real series.

Sorry for that, the "[x/4]" patches should be ignored.

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 3/4] serial: sh-sci: Stop TX DMA workqueue during port shutdown

[Geert Uytterhoeven]

The transmit DMA workqueue is never stopped, hence the work function may
be called after the port has been shut down.

Fix this race condition by cancelling queued work, if any, before DMA
release.  Don't initialize the work if DMA initialization failed, as it
won't be used anyway.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 061660ccf9442d02..58016386558c6789 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1704,6 +1704,7 @@ static void sci_tx_dma_release(struct sci_port *s)
 	struct dma_chan *chan = s->chan_tx_saved;
 	struct uart_port *port = &s->port;
 
+	cancel_work_sync(&s->work_tx);
 dev_dbg_dma(port->dev, "%s\n", __func__);
 	s->chan_tx_saved = s->chan_tx = NULL;
 	s->cookie_tx = -EINVAL;
@@ -1978,10 +1979,9 @@ static void sci_request_dma(struct uart_port *port)
 				UART_XMIT_SIZE,
 				port->state->xmit.buf, &s->tx_dma_addr);
 
+			INIT_WORK(&s->work_tx, work_fn_tx);
 			s->chan_tx_saved = s->chan_tx = chan;
 		}
-
-		INIT_WORK(&s->work_tx, work_fn_tx);
 	}
 
 	chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM);
-- 
2.17.1

[PATCH 4/4] serial: sh-sci: Stop RX FIFO timer during port shutdown

[Geert Uytterhoeven]

The RX FIFO timer may be armed when the port is shut down, hence the
timer function may still be called afterwards.

Fix this race condition by deleting the timer during port shutdown.

Fixes: 039403765e5da3c6 ("serial: sh-sci: SCIFA/B RX FIFO software timeout")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested with scifa0 on r8a7740/armadillo, after

    echo 2000 > /sys/devices/platform/e6c40000.serial/rx_fifo_timeout
---
 drivers/tty/serial/sh-sci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 58016386558c6789..cca3bbc62a6b1110 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2571,6 +2571,8 @@ static void sci_shutdown(struct uart_port *port)
 	}
 #endif
 
+	if (s->rx_trigger > 1 && s->rx_fifo_timeout > 0)
+		del_timer_sync(&s->rx_fifo_timer);
 	sci_free_irq(s);
 	sci_free_dma(port);
 }
-- 
2.17.1

Re: [PATCH 4/4] serial: sh-sci: Stop RX FIFO timer during port shutdown

[Rob Landley]

On 06/29/2018 09:25 AM, Geert Uytterhoeven wrote:
> The RX FIFO timer may be armed when the port is shut down, hence the
> timer function may still be called afterwards.
> 
> Fix this race condition by deleting the timer during port shutdown.

Will any of this make the qemu-system-sh4 serial console work again?

Rob

Re: [PATCH 4/4] serial: sh-sci: Stop RX FIFO timer during port shutdown

[Geert Uytterhoeven]

Hi Rob,

On Sun, Jul 1, 2018 at 7:27 PM Rob Landley <rob@landley.net> wrote:
> On 06/29/2018 09:25 AM, Geert Uytterhoeven wrote:
> > The RX FIFO timer may be armed when the port is shut down, hence the
> > timer function may still be called afterwards.
> >
> > Fix this race condition by deleting the timer during port shutdown.
>
> Will any of this make the qemu-system-sh4 serial console work again?

Probably not, as none of these patches touch the qemu source tree.

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 4/4] serial: sh-sci: Stop RX FIFO timer during port shutdown

[Rob Landley]

On 07/02/2018 04:50 AM, Geert Uytterhoeven wrote:
> Hi Rob,
> 
> On Sun, Jul 1, 2018 at 7:27 PM Rob Landley <rob@landley.net> wrote:
>> On 06/29/2018 09:25 AM, Geert Uytterhoeven wrote:
>>> The RX FIFO timer may be armed when the port is shut down, hence the
>>> timer function may still be called afterwards.
>>>
>>> Fix this race condition by deleting the timer during port shutdown.
>>
>> Will any of this make the qemu-system-sh4 serial console work again?
> 
> Probably not, as none of these patches touch the qemu source tree.

So we remain well into the second year of each project pointing the finger at
the other on this, and nothing getting fixed. And the workaround remains "run a
kernel from before February 2017 if you want a working serial console under qemu.

Good to know,

Rob

Re: [PATCH 4/4] serial: sh-sci: Stop RX FIFO timer during port shutdown

[Geert Uytterhoeven]

On Mon, Jul 2, 2018 at 9:01 PM Rob Landley <rob@landley.net> wrote:
> On 07/02/2018 04:50 AM, Geert Uytterhoeven wrote:
> > On Sun, Jul 1, 2018 at 7:27 PM Rob Landley <rob@landley.net> wrote:
> >> On 06/29/2018 09:25 AM, Geert Uytterhoeven wrote:
> >>> The RX FIFO timer may be armed when the port is shut down, hence the
> >>> timer function may still be called afterwards.
> >>>
> >>> Fix this race condition by deleting the timer during port shutdown.
> >>
> >> Will any of this make the qemu-system-sh4 serial console work again?
> >
> > Probably not, as none of these patches touch the qemu source tree.
>
> So we remain well into the second year of each project pointing the finger at
> the other on this, and nothing getting fixed. And the workaround remains "run a
> kernel from before February 2017 if you want a working serial console under qemu.
>
> Good to know,

FTR, patch for qemu is "hw/char/sh_serial: Add timeout handling to unbreak
serial input"
(http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg05579.html)

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