[PATCH 0/4] serial: sh-sci: response to Dirk's bugreport

[PATCH 0/4] serial: sh-sci: response to Dirk's bugreport

[Wolfram Sang]

Investigating the issue Dirk reported [1], and after the RFC sent[2],
here is now the series with my conclusions. Patch 1 fixes a real issue
IMHO and is the minimal solution for backporting. Patch 2 adds some
documentation. Patch 3 reduces potential race windows by letting the
timer only run when we really want it. Patch 4 is a simplification
because locking should be as simple as possible, right?

I could trigger the code path meanwhile and did not encounter a
regression. But I could not reproduce the initial report from Dirk, so I
can't say this is a 100% fix. Yet, I think, we want these patches
nonetheless. Dirk metioned that his system could have been shutting
down. If so, patch 1 is probably the solution, but we don't know for
sure.

[1] https://lore.kernel.org/r/ee6c9e16-9f29-450e-81da-4a8dceaa8fc7@de.bosch.com
[2] https://lore.kernel.org/r/20240416123545.7098-4-wsa+renesas@sang-engineering.com


Wolfram Sang (4):
  serial: sh-sci: protect invalidating RXDMA on shutdown
  serial: sh-sci: describe locking requirements for invalidating RXDMA
  serial: sh-sci: let timeout timer only run when DMA is scheduled
  serial: sh-sci: simplify locking when re-issuing RXDMA fails

 drivers/tty/serial/sh-sci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH 1/4] serial: sh-sci: protect invalidating RXDMA on shutdown

[Wolfram Sang]

The to-be-fixed commit removed locking when invalidating the DMA RX
descriptors on shutdown. It overlooked that there is still a rx_timer
running which may still access the protected data. So, re-add the
locking.

Reported-by: Dirk Behme <dirk.behme@de.bosch.com>
Closes: https://lore.kernel.org/r/ee6c9e16-9f29-450e-81da-4a8dceaa8fc7@de.bosch.com
Fixes: 2c4ee23530ff ("serial: sh-sci: Postpone DMA release when falling back to PIO")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/tty/serial/sh-sci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index e512eaa57ed5..a6f3517dce74 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1271,9 +1271,14 @@ static void sci_dma_rx_chan_invalidate(struct sci_port *s)
 static void sci_dma_rx_release(struct sci_port *s)
 {
 	struct dma_chan *chan = s->chan_rx_saved;
+	struct uart_port *port = &s->port;
+	unsigned long flags;
 
+	uart_port_lock_irqsave(port, &flags);
 	s->chan_rx_saved = NULL;
 	sci_dma_rx_chan_invalidate(s);
+	uart_port_unlock_irqrestore(port, flags);
+
 	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]));
-- 
2.43.0

[PATCH 2/4] serial: sh-sci: describe locking requirements for invalidating RXDMA

[Wolfram Sang]

Make sure everyone knows that calling this function needs protection.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/tty/serial/sh-sci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index a6f3517dce74..09eb0c824b10 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1258,6 +1258,7 @@ static int sci_dma_rx_find_active(struct sci_port *s)
 	return -1;
 }
 
+/* Must only be called with uart_port_lock taken */
 static void sci_dma_rx_chan_invalidate(struct sci_port *s)
 {
 	unsigned int i;
-- 
2.43.0

[PATCH 3/4] serial: sh-sci: let timeout timer only run when DMA is scheduled

[Wolfram Sang]

The hrtimer for RXDMA timeout was unconditionally restarted in the RXDMA
complete handler ignoring the fact that setting up DMA may fail and PIO
is used instead. Explicitly stop the timer when DMA is completed and
only restart it when setting up DMA was successful. This makes the
intention of the timer much clearer, the driver easier to understand and
simplifies assumptions about the timer. The latter avoids race
conditions if these assumptions were not met or confused.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/tty/serial/sh-sci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 09eb0c824b10..7cc354ee6305 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1325,14 +1325,14 @@ static void sci_dma_rx_complete(void *arg)
 	dev_dbg(port->dev, "%s(%d) active cookie %d\n", __func__, port->line,
 		s->active_rx);
 
+	hrtimer_cancel(&s->rx_timer);
+
 	uart_port_lock_irqsave(port, &flags);
 
 	active = sci_dma_rx_find_active(s);
 	if (active >= 0)
 		count = sci_dma_rx_push(s, s->rx_buf[active], s->buf_len_rx);
 
-	start_hrtimer_us(&s->rx_timer, s->rx_timeout);
-
 	if (count)
 		tty_flip_buffer_push(&port->state->port);
 
@@ -1355,6 +1355,9 @@ static void sci_dma_rx_complete(void *arg)
 	uart_port_unlock_irqrestore(port, flags);
 	dev_dbg(port->dev, "%s: cookie %d #%d, new active cookie %d\n",
 		__func__, s->cookie_rx[active], active, s->active_rx);
+
+	start_hrtimer_us(&s->rx_timer, s->rx_timeout);
+
 	return;
 
 fail:
-- 
2.43.0

[PATCH 4/4] serial: sh-sci: simplify locking when re-issuing RXDMA fails

[Wolfram Sang]

Avoid a superfluous unlock/lock-pair by simply moving the printout to
the end of bailing out.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/tty/serial/sh-sci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 7cc354ee6305..4dc9c142690a 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1361,14 +1361,12 @@ static void sci_dma_rx_complete(void *arg)
 	return;
 
 fail:
-	uart_port_unlock_irqrestore(port, flags);
-	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
 	/* Switch to PIO */
-	uart_port_lock_irqsave(port, &flags);
 	dmaengine_terminate_async(chan);
 	sci_dma_rx_chan_invalidate(s);
 	sci_dma_rx_reenable_irq(s);
 	uart_port_unlock_irqrestore(port, flags);
+	dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
 }
 
 static void sci_dma_tx_release(struct sci_port *s)
-- 
2.43.0