[v2,0/9] DMA engine cookie handling cleanups

On Mon, 2012-03-12 at 21:53 +0530, Vinod Koul wrote:
> > > I applied the v2 on a branch and also rebased on top of
> slave-dma.next.
> > > There were few conflicts in imx-dma.c. Sacha, Javier, pls see that
> merge
> > > is right.
> > > 
> > > This branch (rmk_cookie_fixes2_rebased) is not yet pushed, as I am
> not
> > > able to connect to infradead.org, should be done when server is
> back.
> > 
> > Are you going to pick up Shawn Guo's tested-by for these patches, or
> are
> > we saying its too late for that now?
> Sure, his and others we have received on this series.
> Also, IIRC there was one fixup from Jassi on pl330?
> 
> I should be able to merge this tomorrow, if thats fine with you. 
I have merged this to my next, but haven't pushed out yet. This is
available on branch next_cookie2_merge.
Also I got few (more than expected build failures) and a merge issue on
imx driver. Sascha, Russell can you see if fix on imx is right

Please see if the below patch is the right fix for build failures in
addition to one suggested by Jassi.

-------------------x-------------------------x----------------------

>From 949ff5b8d46b5e3435d21b2651ce3a2599208d44 Mon Sep 17 00:00:00 2001
From: Vinod Koul <vinod.koul@linux.intel.com>
Date: Tue, 13 Mar 2012 11:58:12 +0530
Subject: [PATCH] dmaengine: fix for cookie changes and merge

Fixed trivial issues in drivers:
	drivers/dma/imx-sdma.c
	drivers/dma/intel_mid_dma.c
	drivers/dma/ioat/dma_v3.c
	drivers/dma/iop-adma.c
	drivers/dma/sirf-dma.c
	drivers/dma/timb_dma.c

Signed-off-by: Vinod Koul <vinod.koul@linux.intel.com>
---
 drivers/dma/imx-sdma.c      |    1 +
 drivers/dma/intel_mid_dma.c |    1 +
 drivers/dma/ioat/dma_v3.c   |    1 +
 drivers/dma/iop-adma.c      |    1 +
 drivers/dma/sirf-dma.c      |    2 ++
 drivers/dma/timb_dma.c      |    6 +-----
 6 files changed, 7 insertions(+), 5 deletions(-)

On Tue, Mar 13, 2012 at 02:10:36PM +0530, Vinod Koul wrote:
> Please see if the below patch is the right fix for build failures in
> addition to one suggested by Jassi.

I'm not sure that Jassi's solution is correct - and I'm wondering whether
any of the DMA engine drivers do the right thing when transfers are
terminated.  Is it right for the DMA status function to return IN_PROGRESS
for a previously submitted cookie which has been terminated?

I can see two answers to that, both equally valid:

1. It allows you to find out exactly where the DMA engine got to before
   the transfer was terminated, and therefore recover from the termination
   if you wish to.

2. Returning in-progress when a cookie will never be completed is
   misleading, and could be misinterpreted by users of the tx_status
   function, especially if they are waiting for a particular transaction
   to complete.

Maybe we need to introduce a DMA_TERMINATED status?

> -------------------x-------------------------x----------------------
> 
> >From 949ff5b8d46b5e3435d21b2651ce3a2599208d44 Mon Sep 17 00:00:00 2001
> From: Vinod Koul <vinod.koul@linux.intel.com>
> Date: Tue, 13 Mar 2012 11:58:12 +0530
> Subject: [PATCH] dmaengine: fix for cookie changes and merge
> 
> Fixed trivial issues in drivers:
> 	drivers/dma/imx-sdma.c
> 	drivers/dma/intel_mid_dma.c
> 	drivers/dma/ioat/dma_v3.c
> 	drivers/dma/iop-adma.c
> 	drivers/dma/sirf-dma.c
> 	drivers/dma/timb_dma.c
> 
> Signed-off-by: Vinod Koul <vinod.koul@linux.intel.com>
> ---
>  drivers/dma/imx-sdma.c      |    1 +
>  drivers/dma/intel_mid_dma.c |    1 +
>  drivers/dma/ioat/dma_v3.c   |    1 +
>  drivers/dma/iop-adma.c      |    1 +
>  drivers/dma/sirf-dma.c      |    2 ++
>  drivers/dma/timb_dma.c      |    6 +-----
>  6 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index ccfc7c4..81f9d57 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1127,6 +1127,7 @@ static void sdma_issue_pending(struct dma_chan *chan)
>  	struct sdma_engine *sdma = sdmac->sdma;
>  
>  	if (sdmac->status == DMA_IN_PROGRESS)
> +		sdma_enable_channel(sdma, sdmac->channel);
>  }
>  
>  #define SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V1	34

This looks like a merge conflict resolution.  I don't see this being
caused by my patches as I haven't touched this function.

> diff --git a/drivers/dma/intel_mid_dma.c b/drivers/dma/intel_mid_dma.c
> index d599d96..2449812 100644
> --- a/drivers/dma/intel_mid_dma.c
> +++ b/drivers/dma/intel_mid_dma.c
> @@ -477,6 +477,7 @@ static enum dma_status intel_mid_dma_tx_status(struct dma_chan *chan,
>  						dma_cookie_t cookie,
>  						struct dma_tx_state *txstate)
>  {
> +	struct intel_mid_dma_chan *midc = to_intel_mid_dma_chan(chan);
>  	enum dma_status ret;
>  
>  	ret = dma_cookie_status(chan, cookie, txstate);

Ditto (my patches don't introduce new this new midc, nor do they remove
that line.)

> diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c
> index 145eda2..2c4476c 100644
> --- a/drivers/dma/ioat/dma_v3.c
> +++ b/drivers/dma/ioat/dma_v3.c
> @@ -61,6 +61,7 @@
>  #include <linux/dmaengine.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/prefetch.h>
> +#include "../dmaengine.h"
>  #include "registers.h"
>  #include "hw.h"
>  #include "dma.h"
> diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
> index 1f3a703..4499f88 100644
> --- a/drivers/dma/iop-adma.c
> +++ b/drivers/dma/iop-adma.c
> @@ -894,6 +894,7 @@ static enum dma_status iop_adma_status(struct dma_chan *chan,
>  					struct dma_tx_state *txstate)
>  {
>  	struct iop_adma_chan *iop_chan = to_iop_adma_chan(chan);
> +	int ret;

This was "enum dma_status ret;" before I accidentally removed it.  It
probably should be again, rather than an int.

>  
>  	ret = dma_cookie_status(chan, cookie, txstate);
>  	if (ret == DMA_SUCCESS)
> diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
> index a2cde85..45ba352 100644
> --- a/drivers/dma/sirf-dma.c
> +++ b/drivers/dma/sirf-dma.c
> @@ -18,6 +18,8 @@
>  #include <linux/of_platform.h>
>  #include <linux/sirfsoc_dma.h>
>  
> +#include "dmaengine.h"
> +
>  #define SIRFSOC_DMA_DESCRIPTORS                 16
>  #define SIRFSOC_DMA_CHANNELS                    16
>  

Hmm, guess that's what happens when old patches are brought forward and
things from the original series are forgotten...

> diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
> index 7805996..d408c22 100644
> --- a/drivers/dma/timb_dma.c
> +++ b/drivers/dma/timb_dma.c
> @@ -510,17 +510,13 @@ static void td_free_chan_resources(struct dma_chan *chan)
>  static enum dma_status td_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>  				    struct dma_tx_state *txstate)
>  {
> -	struct timb_dma_chan *td_chan =
> -		container_of(chan, struct timb_dma_chan, chan);
>  	enum dma_status ret;
>  
>  	dev_dbg(chan2dev(chan), "%s: Entry\n", __func__);
>  
>  	ret = dma_cookie_status(chan, cookie, txstate);
>  
> -	dev_dbg(chan2dev(chan),
> -		"%s: exit, ret: %d, last_complete: %d, last_used: %d\n",
> -		__func__, ret, last_complete, last_used);
> +	dev_dbg(chan2dev(chan), "%s: exit, ret: %d\n", 	__func__, ret);
>  
>  	return ret;
>  }
> -- 
> 1.7.0.4
> 
> 
> 
> 
> -- 
> ~Vinod
>

On Tue, 2012-03-13 at 12:31 +0000, Russell King - ARM Linux wrote:
> On Tue, Mar 13, 2012 at 02:10:36PM +0530, Vinod Koul wrote:
> > Please see if the below patch is the right fix for build failures in
> > addition to one suggested by Jassi.
> 
> I'm not sure that Jassi's solution is correct - and I'm wondering whether
> any of the DMA engine drivers do the right thing when transfers are
> terminated.  Is it right for the DMA status function to return IN_PROGRESS
> for a previously submitted cookie which has been terminated?
> 
> I can see two answers to that, both equally valid:
> 
> 1. It allows you to find out exactly where the DMA engine got to before
>    the transfer was terminated, and therefore recover from the termination
>    if you wish to.
> 
> 2. Returning in-progress when a cookie will never be completed is
>    misleading, and could be misinterpreted by users of the tx_status
>    function, especially if they are waiting for a particular transaction
>    to complete.
> 
> Maybe we need to introduce a DMA_TERMINATED status?
I would agree with you that DMA_TERMINATED seems to be correct option.
IN_PROGRESS would certainly confuse... 
I will drop Jassi's fix from this branch. Care to send the patch?
> 
> > -------------------x-------------------------x----------------------
> > 
> > >From 949ff5b8d46b5e3435d21b2651ce3a2599208d44 Mon Sep 17 00:00:00 2001
> > From: Vinod Koul <vinod.koul@linux.intel.com>
> > Date: Tue, 13 Mar 2012 11:58:12 +0530
> > Subject: [PATCH] dmaengine: fix for cookie changes and merge
> > 
> > Fixed trivial issues in drivers:
> > 	drivers/dma/imx-sdma.c
> > 	drivers/dma/intel_mid_dma.c
> > 	drivers/dma/ioat/dma_v3.c
> > 	drivers/dma/iop-adma.c
> > 	drivers/dma/sirf-dma.c
> > 	drivers/dma/timb_dma.c
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@linux.intel.com>
> > ---
> >  drivers/dma/imx-sdma.c      |    1 +
> >  drivers/dma/intel_mid_dma.c |    1 +
> >  drivers/dma/ioat/dma_v3.c   |    1 +
> >  drivers/dma/iop-adma.c      |    1 +
> >  drivers/dma/sirf-dma.c      |    2 ++
> >  drivers/dma/timb_dma.c      |    6 +-----
> >  6 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index ccfc7c4..81f9d57 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -1127,6 +1127,7 @@ static void sdma_issue_pending(struct dma_chan *chan)
> >  	struct sdma_engine *sdma = sdmac->sdma;
> >  
> >  	if (sdmac->status == DMA_IN_PROGRESS)
> > +		sdma_enable_channel(sdma, sdmac->channel);
> >  }
> >  
> >  #define SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V1	34
> 
> This looks like a merge conflict resolution.  I don't see this being
> caused by my patches as I haven't touched this function.
> 
> > diff --git a/drivers/dma/intel_mid_dma.c b/drivers/dma/intel_mid_dma.c
> > index d599d96..2449812 100644
> > --- a/drivers/dma/intel_mid_dma.c
> > +++ b/drivers/dma/intel_mid_dma.c
> > @@ -477,6 +477,7 @@ static enum dma_status intel_mid_dma_tx_status(struct dma_chan *chan,
> >  						dma_cookie_t cookie,
> >  						struct dma_tx_state *txstate)
> >  {
> > +	struct intel_mid_dma_chan *midc = to_intel_mid_dma_chan(chan);
> >  	enum dma_status ret;
> >  
> >  	ret = dma_cookie_status(chan, cookie, txstate);
> 
> Ditto (my patches don't introduce new this new midc, nor do they remove
> that line.)
> 
> > diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c
> > index 145eda2..2c4476c 100644
> > --- a/drivers/dma/ioat/dma_v3.c
> > +++ b/drivers/dma/ioat/dma_v3.c
> > @@ -61,6 +61,7 @@
> >  #include <linux/dmaengine.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/prefetch.h>
> > +#include "../dmaengine.h"
> >  #include "registers.h"
> >  #include "hw.h"
> >  #include "dma.h"
> > diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
> > index 1f3a703..4499f88 100644
> > --- a/drivers/dma/iop-adma.c
> > +++ b/drivers/dma/iop-adma.c
> > @@ -894,6 +894,7 @@ static enum dma_status iop_adma_status(struct dma_chan *chan,
> >  					struct dma_tx_state *txstate)
> >  {
> >  	struct iop_adma_chan *iop_chan = to_iop_adma_chan(chan);
> > +	int ret;
> 
> This was "enum dma_status ret;" before I accidentally removed it.  It
> probably should be again, rather than an int.
> 
> >  
> >  	ret = dma_cookie_status(chan, cookie, txstate);
> >  	if (ret == DMA_SUCCESS)
> > diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
> > index a2cde85..45ba352 100644
> > --- a/drivers/dma/sirf-dma.c
> > +++ b/drivers/dma/sirf-dma.c
> > @@ -18,6 +18,8 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/sirfsoc_dma.h>
> >  
> > +#include "dmaengine.h"
> > +
> >  #define SIRFSOC_DMA_DESCRIPTORS                 16
> >  #define SIRFSOC_DMA_CHANNELS                    16
> >  
> 
> Hmm, guess that's what happens when old patches are brought forward and
> things from the original series are forgotten...
> 
> > diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
> > index 7805996..d408c22 100644
> > --- a/drivers/dma/timb_dma.c
> > +++ b/drivers/dma/timb_dma.c
> > @@ -510,17 +510,13 @@ static void td_free_chan_resources(struct dma_chan *chan)
> >  static enum dma_status td_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> >  				    struct dma_tx_state *txstate)
> >  {
> > -	struct timb_dma_chan *td_chan =
> > -		container_of(chan, struct timb_dma_chan, chan);
> >  	enum dma_status ret;
> >  
> >  	dev_dbg(chan2dev(chan), "%s: Entry\n", __func__);
> >  
> >  	ret = dma_cookie_status(chan, cookie, txstate);
> >  
> > -	dev_dbg(chan2dev(chan),
> > -		"%s: exit, ret: %d, last_complete: %d, last_used: %d\n",
> > -		__func__, ret, last_complete, last_used);
> > +	dev_dbg(chan2dev(chan), "%s: exit, ret: %d\n", 	__func__, ret);
> >  
> >  	return ret;
> >  }
> > -- 
> > 1.7.0.4
> > 
> > 
> > 
> > 
> > -- 
> > ~Vinod
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, 2012-03-13 at 20:08 +0530, Vinod Koul wrote:
> On Tue, 2012-03-13 at 12:31 +0000, Russell King - ARM Linux wrote:
> > On Tue, Mar 13, 2012 at 02:10:36PM +0530, Vinod Koul wrote:
> > > Please see if the below patch is the right fix for build failures in
> > > addition to one suggested by Jassi.
> > 
> > I'm not sure that Jassi's solution is correct - and I'm wondering whether
> > any of the DMA engine drivers do the right thing when transfers are
> > terminated.  Is it right for the DMA status function to return IN_PROGRESS
> > for a previously submitted cookie which has been terminated?
> > 
> > I can see two answers to that, both equally valid:
> > 
> > 1. It allows you to find out exactly where the DMA engine got to before
> >    the transfer was terminated, and therefore recover from the termination
> >    if you wish to.
> > 
> > 2. Returning in-progress when a cookie will never be completed is
> >    misleading, and could be misinterpreted by users of the tx_status
> >    function, especially if they are waiting for a particular transaction
> >    to complete.
> > 
> > Maybe we need to introduce a DMA_TERMINATED status?
> I would agree with you that DMA_TERMINATED seems to be correct option.
> IN_PROGRESS would certainly confuse... 
> I will drop Jassi's fix from this branch. Care to send the patch? 

Even after adding such a state in dmaengine for DMA_TERMINATED, it
doesn't make much sense. In TERMINATE_ALL we do not update the
chan->complete value. So after client has terminated the channel it can
easily test to see if cookie is completed (before terminate will return
SUCCESS) or aborted (will return DMA_IN_PROGRESS)
So at present I am leaning on 1 :)

If I don't hear any opposition I will merge this (with Jassi's fix) and
then send pull to Linus