[PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Ilya Yanok]

From: Yuri Tikhonov <yur@emcraft.com>

Should clear the next pointer of the TX if we are sure that the
next TX (say NXT) will be submitted to the channel too. Overwise,
we break the chain of descriptors, because we lose the information
about the next descriptor to run. So next time, when invoke
async_tx_run_dependencies() with TX, it's TX->next will be NULL, and
NXT will be never submitted.

Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
---
 crypto/async_tx/async_tx.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/crypto/async_tx/async_tx.c b/crypto/async_tx/async_tx.c
index 85eaf7b..e8362c1 100644
--- a/crypto/async_tx/async_tx.c
+++ b/crypto/async_tx/async_tx.c
@@ -137,7 +137,8 @@ async_tx_run_dependencies(struct dma_async_tx_descriptor *tx)
 		spin_lock_bh(&next->lock);
 		next->parent = NULL;
 		_next = next->next;
-		next->next = NULL;
+		if (_next && _next->chan == chan)
+			next->next = NULL;
 		spin_unlock_bh(&next->lock);
 
 		next->tx_submit(next);
-- 
1.5.6.3

Re: [PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Andrew Morton]

On Thu,  4 Sep 2008 01:43:51 +0400
Ilya Yanok <yanok@emcraft.com> wrote:

> From: Yuri Tikhonov <yur@emcraft.com>
> 
> Should clear the next pointer of the TX if we are sure that the
> next TX (say NXT) will be submitted to the channel too. Overwise,
> we break the chain of descriptors, because we lose the information
> about the next descriptor to run. So next time, when invoke
> async_tx_run_dependencies() with TX, it's TX->next will be NULL, and
> NXT will be never submitted.
> 
> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>

This patch should include your signed-off-by: as well.  Because you
were on the delivery path, as described in
Documentation/SubmittingPatches, section 12.  

> 
> diff --git a/crypto/async_tx/async_tx.c b/crypto/async_tx/async_tx.c
> index 85eaf7b..e8362c1 100644
> --- a/crypto/async_tx/async_tx.c
> +++ b/crypto/async_tx/async_tx.c
> @@ -137,7 +137,8 @@ async_tx_run_dependencies(struct dma_async_tx_descriptor *tx)
>  		spin_lock_bh(&next->lock);
>  		next->parent = NULL;
>  		_next = next->next;
> -		next->next = NULL;
> +		if (_next && _next->chan == chan)
> +			next->next = NULL;
>  		spin_unlock_bh(&next->lock);
>  
>  		next->tx_submit(next);

Dan, please review asap?

Re: [PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Dan Williams]

On Thu, Sep 4, 2008 at 2:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu,  4 Sep 2008 01:43:51 +0400
> Ilya Yanok <yanok@emcraft.com> wrote:
>
>> From: Yuri Tikhonov <yur@emcraft.com>
>>
>> Should clear the next pointer of the TX if we are sure that the
>> next TX (say NXT) will be submitted to the channel too. Overwise,
>> we break the chain of descriptors, because we lose the information
>> about the next descriptor to run. So next time, when invoke
>> async_tx_run_dependencies() with TX, it's TX->next will be NULL, and
>> NXT will be never submitted.
>>
>> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
>
> This patch should include your signed-off-by: as well.  Because you
> were on the delivery path, as described in
> Documentation/SubmittingPatches, section 12.
>

Yes, Ilya once I have your signed-off-by I will push this to Linus and
-stable.  As far as impact for in-tree drivers: iop13xx by is immune,
iop3xx can hit this, and it looks like the orion5x implementation is
immune since copy and xor are available on the same channel.

--
Dan

Re: [PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Dan Williams]

On Thu, Sep 4, 2008 at 4:24 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Sep 4, 2008 at 2:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Thu,  4 Sep 2008 01:43:51 +0400
>> Ilya Yanok <yanok@emcraft.com> wrote:
>>
>>> From: Yuri Tikhonov <yur@emcraft.com>
>>>
>>> Should clear the next pointer of the TX if we are sure that the
>>> next TX (say NXT) will be submitted to the channel too. Overwise,
>>> we break the chain of descriptors, because we lose the information
>>> about the next descriptor to run. So next time, when invoke
>>> async_tx_run_dependencies() with TX, it's TX->next will be NULL, and
>>> NXT will be never submitted.
>>>
>>> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
>>
>> This patch should include your signed-off-by: as well.  Because you
>> were on the delivery path, as described in
>> Documentation/SubmittingPatches, section 12.
>>
>
> Yes, Ilya once I have your signed-off-by I will push this to Linus and
> -stable.  As far as impact for in-tree drivers: iop13xx by is immune,
> iop3xx can hit this, and it looks like the orion5x implementation is
> immune since copy and xor are available on the same channel.
>

Please also take a look at this cleanup patch that I will queue for
.28.  I think it makes things easier to read let me know if you
disagree.

---gmail mangled patch follows--->
async_tx: make async_tx_run_dependencies() easier to read

From: Dan Williams <dan.j.williams@intel.com>

* Rename 'next' to 'dep'
* Move the channel switch check inside the loop to simplify
  termination

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 crypto/async_tx/async_tx.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)


diff --git a/crypto/async_tx/async_tx.c b/crypto/async_tx/async_tx.c
index e8362c1..b833cca 100644
--- a/crypto/async_tx/async_tx.c
+++ b/crypto/async_tx/async_tx.c
@@ -115,34 +115,32 @@ EXPORT_SYMBOL_GPL(dma_wait_for_async_tx);
  *	(start) dependent operations on their target channel
  * @tx: transaction with dependencies
  */
-void
-async_tx_run_dependencies(struct dma_async_tx_descriptor *tx)
+void async_tx_run_dependencies(struct dma_async_tx_descriptor *tx)
 {
-	struct dma_async_tx_descriptor *next = tx->next;
+	struct dma_async_tx_descriptor *dep = tx->next;
+	struct dma_async_tx_descriptor *dep_next;
 	struct dma_chan *chan;

-	if (!next)
+	if (dep)
+		chan = dep->chan;
+	else
 		return;

-	tx->next = NULL;
-	chan = next->chan;
-
 	/* keep submitting up until a channel switch is detected
 	 * in that case we will be called again as a result of
 	 * processing the interrupt from async_tx_channel_switch
 	 */
-	while (next && next->chan == chan) {
-		struct dma_async_tx_descriptor *_next;
-
-		spin_lock_bh(&next->lock);
-		next->parent = NULL;
-		_next = next->next;
-		if (_next && _next->chan == chan)
-			next->next = NULL;
-		spin_unlock_bh(&next->lock);
-
-		next->tx_submit(next);
-		next = _next;
+	for (; dep; dep = dep_next) {
+		spin_lock_bh(&dep->lock);
+		dep->parent = NULL;
+		dep_next = dep->next;
+		if (dep_next && dep_next->chan == chan)
+			dep->next = NULL; /* ->next will be submitted */
+		else
+			dep_next = NULL; /* submit current dep and terminate */
+		spin_unlock_bh(&dep->lock);
+
+		dep->tx_submit(dep);
 	}

 	chan->device->device_issue_pending(chan);

Re: [PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Ilya Yanok]

Hello,

Dan Williams wrote:
> Please also take a look at this cleanup patch that I will queue for
> .28.  I think it makes things easier to read let me know if you
> disagree.
>   

Look good for me. Hmm, actually I don't think that current version is so
hard to read...

Just one comment:

> -	if (!next)
> +	if (dep)
> +		chan = dep->chan;
> +	else
>  		return;
>   

I don't like 'return' inside 'else'.

Regards, Ilya.

Re: [PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Ilya Yanok]

Andrew Morton wrote:
>> From: Yuri Tikhonov <yur@emcraft.com>
>>
>> Should clear the next pointer of the TX if we are sure that the
>> next TX (say NXT) will be submitted to the channel too. Overwise,
>> we break the chain of descriptors, because we lose the information
>> about the next descriptor to run. So next time, when invoke
>> async_tx_run_dependencies() with TX, it's TX->next will be NULL, and
>> NXT will be never submitted.
>>
>> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
>>     
Signed-off-by: Ilya Yanok <yanok@emcraft.com>

> This patch should include your signed-off-by: as well.  Because you
> were on the delivery path, as described in
> Documentation/SubmittingPatches, section 12.  
>   

Excuse me for being inattentive.

Regards, Ilya.

Re: [PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Wolfgang Denk]

Dear Ilya &Yrui,

In message <1220478231-8725-1-git-send-email-yanok@emcraft.com> you wrote:
> 
> Should clear the next pointer of the TX if we are sure that the
> next TX (say NXT) will be submitted to the channel too. Overwise,
> we break the chain of descriptors, because we lose the information
> about the next descriptor to run. So next time, when invoke
> async_tx_run_dependencies() with TX, it's TX->next will be NULL, and
> NXT will be never submitted.
> 
> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
> ---
>  crypto/async_tx/async_tx.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

What about this patch? SHould it be applied to our repository?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The use of Microsoft crippleware systems is a sin that  carries  with
it its own punishment.
         -- Tom Christiansen in <6bo3fr$pj8$5@csnews.cs.colorado.edu>

Re: [PATCH] ASYNC_TX: fix the bug in async_tx_run_dependencies

[Ilya Yanok]

Dear Wolfgang,

this patch is already applied (d2029eba in linux-2.6-denx repo) I just
sent it upstream.

Regards, Ilya.

Wolfgang Denk wrote:
>> Should clear the next pointer of the TX if we are sure that the
>> next TX (say NXT) will be submitted to the channel too. Overwise,
>> we break the chain of descriptors, because we lose the information
>> about the next descriptor to run. So next time, when invoke
>> async_tx_run_dependencies() with TX, it's TX->next will be NULL, and
>> NXT will be never submitted.
>>
>> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
>> ---
>>  crypto/async_tx/async_tx.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>     
>
> What about this patch? SHould it be applied to our repository?
>
> Best regards,
>
> Wolfgang Denk
>
>