[RFC,1/2] serial: sh-sci: start hrtimer after setting up DMA

Commit Message

Wolfram Sang April 16, 2024, 12:35 p.m. UTC

In the RX DMA completion handler, the hrtimer was restarted before DMA
was set up. If DMA failed, for some reason, it would clean up and the
hrtimer would run into a NULL-pointer. Restart the timer after DMA was
successfully set up.

Reported-by: Dirk Behme <[e-mail]>
Closes: https://lore.kernel.org/r/[e-mail]
Fixes: 67f462b069e9 ("serial: sh-sci: Get rid of the workqueue to handle receive DMA requests")
Signed-off-by: Wolfram Sang <[e-mail]>
---
 drivers/tty/serial/sh-sci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven April 24, 2024, 9:41 a.m. UTC | #1

Hi Wolfram,

Thanks for your patch!

On Tue, Apr 16, 2024 at 2:35 PM Wolfram Sang
<[e-mail]> wrote:
> In the RX DMA completion handler, the hrtimer was restarted before DMA
> was set up. If DMA failed, for some reason, it would clean up and the
> hrtimer would run into a NULL-pointer. Restart the timer after DMA was

... into a NULL-pointer dereference of s->chan_rx.

> successfully set up.
>
> Reported-by: Dirk Behme <[e-mail]>
> Closes: https://lore.kernel.org/r/[e-mail]
> Fixes: 67f462b069e9 ("serial: sh-sci: Get rid of the workqueue to handle receive DMA requests")
> Signed-off-by: Wolfram Sang <[e-mail]>

This is definitely a step in the right direction, so
Reviewed-by: Geert Uytterhoeven <[e-mail]>

Gr{oetje,eeting}s,

                        Geert

Wolfram Sang May 2, 2024, 6:01 p.m. UTC | #2

On Tue, Apr 16, 2024 at 02:35:47PM +0200, Wolfram Sang wrote:
> In the RX DMA completion handler, the hrtimer was restarted before DMA
> was set up. If DMA failed, for some reason, it would clean up and the
> hrtimer would run into a NULL-pointer. Restart the timer after DMA was
> successfully set up.

Further investigations, please review:

Originally, I thought this was the race condition Dirk encountered. But
I didn't take locking into account. sci_dma_rx_timer_fn() is protected
by the uart_port_lock. sci_dma_rx_complete() is also protected by the
uart_port_lock. So, the position of restarting the hrtimer should not
matter.

However, I still think it is good coding practice (and easier to
understand) if we cancel the hrtimer at the begin of
sci_dma_rx_complete() and reenable it only if setting DMA was
successful. Because that basically means the timer only runs when DMA
was scheduled and has not finished yet.

There is some unnecessary unlock/lock in the error handling of
sci_dma_rx_complete(). I'll simplify this by moving the dev_err
downwards.

Patch

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index e512eaa57ed5..1e3c26c11c49 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1325,8 +1325,6 @@  static void sci_dma_rx_complete(void *arg)
 	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);
 
@@ -1346,6 +1344,8 @@  static void sci_dma_rx_complete(void *arg)
 
 	dma_async_issue_pending(chan);
 
+	start_hrtimer_us(&s->rx_timer, s->rx_timeout);
+
 	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);