sh-sci: fix a race of DMA submit_tx on transfer

Hi, Guennadi.

I believe I've found a small bug in DMA handling of sh-sci, and made a patch attached. This shows good result -- the issue can't be reproduced -- but, I'm afraid I am not confident of the correctness of the fix in logic/theory.
Would you please see it and give some comments or ack?

--
The test to reproduce the issue is simple as follows.

$ stty -F /dev/ttySC0 115200 # faster is better, I think.
$ while :; do dd if=/dev/zero bs  count00 of=/dev/ttySC0; done

in about half an hour on sh7757lcr board(with DMA enabled locally), (and, in few seconds ape5r SMP system(one of kota2's variation) here), it stops as

| Unable to handle kernel NULL pointer dereference at virtual address 00000004
| pc = 801cca66
| *pde = 8ee51000
| Oops: 0000 [#1]
| Modules linked in:
| 
| Pid : 232, Comm: 		kworker/0:1
| CPU : 0        		Not tainted  (3.3.0-rc4-00037-gc671a05-dirty #104)
| 
| PC is at sci_dma_tx_complete+0x46/0x120

...

Apparently, this is because async_tx_ack(s->desc_tx) at sci_dma_tx_complete() is called when s->desc_tx is NULL.
Detailed trace showed the mechanism of the issue as a description of attached patch.

Thank you.
/yoshii

--

Quoted 1 line

From 8fb99cf51248b9b2a73ea9e849823bd71ac81769 Mon Sep 17 00:00:00 2001

From: Takashi YOSHII <[email protected]>
Date: Fri, 10 Feb 2012 15:54:07 +0900
Subject: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

When DMA is enabled, sh-sci transfer begins with
uart_start()
sci_start_tx()
if (cookie_tx < 0) schedule_work()
Then, starts DMA when wq scheduled, -- (A)
process_one_work()
work_fn_rx()
cookie_tx = desc->submit_tx()
And finishes when DMA transfer ends, -- (B)
sci_dma_tx_complete()
async_tx_ack()
cookie_tx = -EINVAL
(possible another schedule_work())

This A to B sequence is not reentrant, since controlling variables (for example, cookie_tx above) are not queues nor lists. So, they must be invoked as A B A B..., otherwise results in kernel crash.

To ensure the sequence, sci_start_tx() seems to test if cookie_tx < 0 (represents "not used") to call schedule_work().
But cookie_tx will not be set (to a cookie, also means "used") until in the middle of work queue scheduled function work_fn_tx().

This gap between the test and set allows the breakage of the sequence under the very frequently call of uart_start().
Another gap between async_tx_ack() and another schedule_work() results in the same issue, too.

This patch introduces a new condition "cookie_tx = 0" just to mark it is "busy" and assign it within spin-locked region to fill the gaps.

Signed-off-by Takashi Yoshii <[email protected]>

---
 drivers/tty/serial/sh-sci.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

drivers/tty/serial/sh-sci.c +2 -1

index 7508579..107db7a 100644
@@ -1229,7 +1229,7 @@ static void sci_dma_tx_complete(void *arg)
 	port->icount.tx += sg_dma_len(&s->sg_tx);
 
 	async_tx_ack(s->desc_tx);
-	s->cookie_tx = -EINVAL;
+	s->cookie_tx = 0;
 	s->desc_tx = NULL;
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)

@@ -1240,6 +1240,7 @@ static void sci_dma_tx_complete(void *arg)
 	} else if (port->type = PORT_SCIFA || port->type = PORT_SCIFB) {
 		u16 ctrl = sci_in(port, SCSCR);
 		sci_out(port, SCSCR, ctrl & ~SCSCR_TIE);
+		s->cookie_tx = -EINVAL;
 	}
 
 	spin_unlock_irqrestore(&port->lock, flags);

@@ -1501,8 +1502,10 @@ static void sci_start_tx(struct uart_port *port)
 	}
 

It looks like sci_start_tx() can be called asynchronously - before the previous request has completed. In such a case we have to actually protect the whole function.

... Please, check whether this slightly simpler patch works for you too.

... If it does, we can then actually completely remove the cookie_tx member of struct sci_port:

Thank you for your reply.

Quoted 3 lines

It looks like sci_start_tx() can be called asynchronously - before the previous request has completed. In such a case we have to actually protect the whole function.

Perhaps.
Low level debugging shows sci_start_tx() called twice before work_fn_tx(), just before the issue occurs. I don't know if this frequent calls is an erroneous behavior or not, though.

Quoted 2 lines

... Please, check whether this slightly simpler patch works for you too.

Not working. (it crashes)

I applied only

Review

Quoted 1 line

(quoted diff) +1 -0

+	    !s->desc_tx)

(You mean *!*s->desc_tx, don't you? Otherwise no output)

Spin-locks are not needed (and should not be) here, because this function is called with lock held.

Quoted 2 lines

... If it does, we can then actually completely remove the cookie_tx member of struct sci_port:

Doing same as my patch with desc_tx instead of cookie_tx, introducing other special value like desc_tx=1 or so, is OK. Because what is done by the patch is manual mutex by flag variable, choice of the variable is not a problem.

The problem seems to be that I don't know how the .start_tx() should be called. Shall we go to linux-serial ML?

... You could slightly improve it in
my subjective opinion by avoiding an extra assignment in its first part:

Reviewed-by Guennadi Liakhovetski <[email protected]>

serial: sh-sci: fix a race of DMA submit_tx on transfer

When DMA is enabled, sh-sci transfer begins with
uart_start()
sci_start_tx()
if (cookie_tx < 0) schedule_work()
Then, starts DMA when wq scheduled, -- (A)
process_one_work()
work_fn_rx()
cookie_tx = desc->submit_tx()
And finishes when DMA transfer ends, -- (B)
sci_dma_tx_complete()
async_tx_ack()
cookie_tx = -EINVAL
(possible another schedule_work())

This A to B sequence is not reentrant, since controlling variables (for example, cookie_tx above) are not queues nor lists. So, they must be invoked as A B A B..., otherwise results in kernel crash.

To ensure the sequence, sci_start_tx() seems to test if cookie_tx < 0 (represents "not used") to call schedule_work().
But cookie_tx will not be set (to a cookie, also means "used") until in the middle of work queue scheduled function work_fn_tx().

This gap between the test and set allows the breakage of the sequence under the very frequently call of uart_start().
Another gap between async_tx_ack() and another schedule_work() results in the same issue, too.

This patch introduces a new condition "cookie_tx = 0" just to mark it is "busy" and assign it within spin-locked region to fill the gaps.

Signed-off-by Takashi Yoshii <[email protected]>

Reviewed-by Guennadi Liakhovetski <[email protected]>

Quoted 30 lines · Takashi Yoshii

serial: sh-sci: fix a race of DMA submit_tx on transfer

When DMA is enabled, sh-sci transfer begins with
uart_start()
sci_start_tx()
if (cookie_tx < 0) schedule_work()
Then, starts DMA when wq scheduled, -- (A)
process_one_work()
work_fn_rx()
cookie_tx = desc->submit_tx()
And finishes when DMA transfer ends, -- (B)
sci_dma_tx_complete()
async_tx_ack()
cookie_tx = -EINVAL
(possible another schedule_work())

This A to B sequence is not reentrant, since controlling variables (for example, cookie_tx above) are not queues nor lists. So, they must be invoked as A B A B..., otherwise results in kernel crash.

To ensure the sequence, sci_start_tx() seems to test if cookie_tx < 0 (represents "not used") to call schedule_work().
But cookie_tx will not be set (to a cookie, also means "used") until in the middle of work queue scheduled function work_fn_tx().

This gap between the test and set allows the breakage of the sequence under the very frequently call of uart_start().
Another gap between async_tx_ack() and another schedule_work() results in the same issue, too.

Quoted 5 lines

This patch introduces a new condition "cookie_tx == 0" just to mark it is "busy" and assign it within spin-locked region to fill the gaps.

Signed-off-by Takashi Yoshii <[email protected]>

Reviewed-by Guennadi Liakhovetski <[email protected]>

Looks good to me. I'll apply it unless Guennadi has any other concerns.

Quoted 1 line · Paul Mundt

Looks good to me. I'll apply it unless Guennadi has any other concerns.

No, I don't - my ack holds:) However, I do have some concerns regarding a couple of other possible issues with SCI DMA:

1. As I mentioned earlier, I think, sci_start_tx() should always be called under the port spinlock to get consistent ->cookie_tx and ->chan_tx values. This is the case, when called from serial core as ->start_tx() from most locations, but not from uart_handle_cts_change(), which is an exported function. It is also called internally in the SCI driver itself several times - with no locking. This might need fixing, especially in sci_tx_dma_release().

2. The DMA Tx work might need to be cancelled from time to time... E.g., when DMA is disabled to switch to PIO, or when shutting down the port.

Looks good to me. I'll apply it unless Guennadi has any other concerns.