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

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

[Yoshii Takashi]

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  count\x1000 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

--
From 8fb99cf51248b9b2a73ea9e849823bd71ac81769 Mon Sep 17 00:00:00 2001
From: Takashi YOSHII <takashi.yoshii.zj@renesas.com>
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 <takashi.yoshii.zj@renesas.com>
---
 drivers/tty/serial/sh-sci.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 7508579..107db7a 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -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)
 	}
 
 	if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
-	    s->cookie_tx < 0)
+	    s->cookie_tx < 0) {
+		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
+	}
 #endif
 
 	if (!s->chan_tx || port->type = PORT_SCIFA || port->type = PORT_SCIFB) {
-- 1.7.3.4

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Guennadi Liakhovetski]

Hi Takashi-san

On Mon, 12 Mar 2012, Yoshii Takashi wrote:

> 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?

Thanks for reporting and investigating the problem and proposing a patch! 
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:

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 8b99aac..20ad83b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1488,6 +1488,9 @@ static void sci_start_tx(struct uart_port *port)
 {
 	struct sci_port *s = to_sci_port(port);
 	unsigned short ctrl;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
 
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
 	if (port->type = PORT_SCIFA || port->type = PORT_SCIFB) {
@@ -1501,7 +1504,7 @@ static void sci_start_tx(struct uart_port *port)
 	}
 
 	if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
-	    s->cookie_tx < 0)
+	    s->desc_tx)
 		schedule_work(&s->work_tx);
 #endif
 
@@ -1510,6 +1513,8 @@ static void sci_start_tx(struct uart_port *port)
 		ctrl = sci_in(port, SCSCR);
 		sci_out(port, SCSCR, ctrl | SCSCR_TIE);
 	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static void sci_stop_tx(struct uart_port *port)

Thanks
Guennadi

> 
> --
> 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  count\x1000 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
> 
> --
> >From 8fb99cf51248b9b2a73ea9e849823bd71ac81769 Mon Sep 17 00:00:00 2001
> From: Takashi YOSHII <takashi.yoshii.zj@renesas.com>
> 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 <takashi.yoshii.zj@renesas.com>
> ---
>  drivers/tty/serial/sh-sci.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 7508579..107db7a 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -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)
>  	}
>  
>  	if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
> -	    s->cookie_tx < 0)
> +	    s->cookie_tx < 0) {
> +		s->cookie_tx = 0;
>  		schedule_work(&s->work_tx);
> +	}
>  #endif
>  
>  	if (!s->chan_tx || port->type = PORT_SCIFA || port->type = PORT_SCIFB) {
> -- 1.7.3.4
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Takashi Yoshii]

Thank you for your reply.

> 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.

> ... Please, check whether this slightly simpler patch 
> works for you too. 
Not working. (it crashes)

I applied only
> +	    !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.

> ... 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?

Cheers,
/yoshii

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Guennadi Liakhovetski]

On Tue, 13 Mar 2012, Takashi Yoshii wrote:

> Thank you for your reply.
> 
> > 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.

It is not as such, but it is not currently correctly processed.

> > ... Please, check whether this slightly simpler patch 
> > works for you too. 
> Not working. (it crashes)
> 
> I applied only
> > +	    !s->desc_tx)
> (You mean *!*s->desc_tx, don't you? Otherwise no output)

Yes, right, thanks for catching.

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

Ok, good point, .start_tx() is called by serial_core.c under the same 
spinlock. But we also call sci_start_tx() internally from a couple of 
locations, so, there we should be taking the spinlock too.

> > ... 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 can certainly add linux-serial to CC if you like. But, I think, your 
patch looks good and is actually correct. You could slightly improve it in 
my subjective opinion by avoiding an extra assignment in its first part:

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 8b99aac..3be3dfa 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1229,17 +1229,18 @@ 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->desc_tx = NULL;
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
 
 	if (!uart_circ_empty(xmit)) {
+		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
 	} 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);

The bottom part remains the same. So, either with or without this change 
you can add my

Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Takashi Yoshii]

Hi,

> ... You could slightly improve it in 
> my subjective opinion by avoiding an extra assignment in its first part:
...
>  	if (!uart_circ_empty(xmit)) {
> +		s->cookie_tx = 0;
That looks tidier.

>  	} 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;
I found the assignment should have been in "else" block, but "else if".
(my mistake). Fixed this time as follows, without run-away case.

 	if (!uart_circ_empty(xmit)) {
		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
	} else {
		s->cookie_tx = -EINVAL;
		if (port->type = PORT_SCIFA || port->type = PORT_SCIFB) {

> Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Thank you!
I will post the patch attached with it, if new modification above seems
to breaks nothing, after successful one day run (not a smart at all ;)

Thank you,
/yoshii

commit 3e0c3f7537318392123cacbc1264cc8180ad893d
Author: Yoshii Takashi <takashi.yoshii.zj@renesas.com>
Date:   Mon Mar 12 18:40:21 2012 +0900

    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 <takashi.yoshii.zj@renesas.com>
    Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 899bbfe..a09d142 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1229,17 +1229,20 @@ 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->desc_tx = NULL;
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
 
 	if (!uart_circ_empty(xmit)) {
+		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
-	} else if (port->type = PORT_SCIFA || port->type = PORT_SCIFB) {
-		u16 ctrl = sci_in(port, SCSCR);
-		sci_out(port, SCSCR, ctrl & ~SCSCR_TIE);
+	} else {
+		s->cookie_tx = -EINVAL;
+		if (port->type = PORT_SCIFA || port->type = PORT_SCIFB) {
+			u16 ctrl = sci_in(port, SCSCR);
+			sci_out(port, SCSCR, ctrl & ~SCSCR_TIE);
+		}
 	}
 
 	spin_unlock_irqrestore(&port->lock, flags);
@@ -1501,8 +1504,10 @@ static void sci_start_tx(struct uart_port *port)
 	}
 
 	if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
-	    s->cookie_tx < 0)
+	    s->cookie_tx < 0) {
+		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
+	}
 #endif
 
 	if (!s->chan_tx || port->type = PORT_SCIFA || port->type = PORT_SCIFB) {

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Paul Mundt]

On Wed, Mar 14, 2012 at 04:14:43PM +0900, Takashi Yoshii wrote:
>     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 <takashi.yoshii.zj@renesas.com>
>     Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

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

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Takashi Yoshii]

Hi,

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

Thank you. I was just wondering who to To: and Cc:
I should have Cc:ed linux-serial, sorry. Will do next time.

Test run for 1-day has finished without problem, this side is OK.

Cheers,
/yoshii

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Guennadi Liakhovetski]

Hi Paul

On Thu, 15 Mar 2012, Paul Mundt wrote:

> On Wed, Mar 14, 2012 at 04:14:43PM +0900, Takashi Yoshii wrote:
> >     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 <takashi.yoshii.zj@renesas.com>
> >     Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> 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.

Both of these might require a bit more thinking and testing.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] sh-sci: fix a race of DMA submit_tx on transfer

[Paul Mundt]

On Thu, Mar 15, 2012 at 11:50:28AM +0100, Guennadi Liakhovetski wrote:
> On Thu, 15 Mar 2012, Paul Mundt wrote:
> > 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:
> 
Ok, I've queued it up now (it was too late for -final, so we'll have to
rely on the stable backport later).

> 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().
> 
The bulk of the uart_handle_cts_change() callers do so with the lock
held, the only exception seems to be a few drivers that call it directly
from their interrupt handlers.

At first glance, the sci_tx/rx_dma_release() cases will probably need a
bit of reordering given that dma_release_channel() is taking a list
mutex, but I don't see too many issues otherwise. Having said that,
the whole DMA enable/disable path could probably be split up a bit with
individual toggle logic pushed down in to ->start/stop_tx as well as
->stop_rx for some finer-grained control. This would at least help make
the locking a bit more obvious.

> 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.
> 
Yes, this is something that needs to be done. I've tried to use the
driver as a module before in the past, and it does blow up rather
spectacularly in the remove case, this is hardly limited to the DMA
case though (and is also not a configuration most people are going to
ever really use, which is why we've largely ignored it thus far). The
PIO<->DMA transition on the other hand we're far more likely to hit,
especially if we end up exposing something like a userspace knob for
enabling/disabling arbitrarily for testing.

Most of the DMA cancelling looks it should be pretty easy to do via
->flush_buffer, unless I'm missing something. The amba-pl011.c driver
does just this for the dmaengine case.