[PATCH dmaengine-fixes 1/1] dmaengine: read completed cookie before used cookie

[PATCH dmaengine-fixes 1/1] dmaengine: read completed cookie before used cookie

[Siva Yerramreddy]

When running dmatest with my yet-to-be submitted driver for the Intel MIC DMA
engine, dmatest detected "dma0chan3-copy5: result #8096161:completion busy
status with src_off=0x0 dst_off=0x0 len=0x40 (0)". This is caused by reading
the used cookie before the completed cookie in dma_cookie_status(),  if a DMA
request is submitted in between the two reads, and completes,  the completed
cookie will be newer than the used cookie value read previously.  Reversing
the order of reads ensures that the completed cookie is for a DMA request
older than the used cookie.

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
Reviewed-by: Nikhil Rao <nikhil.rao@intel.com>
Signed-off-by: Siva Yerramreddy <siva.krishna.kumar.reddy.yerramreddy@intel.com>
---
 drivers/dma/dmaengine.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
index 17f983a..4c96892 100644
--- a/drivers/dma/dmaengine.h
+++ b/drivers/dma/dmaengine.h
@@ -7,6 +7,7 @@
 
 #include <linux/bug.h>
 #include <linux/dmaengine.h>
+#include <asm/barrier.h>
 
 /**
  * dma_cookie_init - initialize the cookies for a DMA channel
@@ -69,8 +70,13 @@ static inline enum dma_status dma_cookie_status(struct dma_chan *chan,
 {
 	dma_cookie_t used, complete;
 
-	used = chan->cookie;
 	complete = chan->completed_cookie;
+	/*
+	 * If this order is not maintained, used can end up being older than
+	 * complete
+	 */
+	smp_rmb();
+	used = chan->cookie;
 	barrier();
 	if (state) {
 		state->last = complete;
-- 
1.8.2.2

Re: [PATCH dmaengine-fixes 1/1] dmaengine: read completed cookie before used cookie

[Vinod Koul]

On Fri, Feb 14, 2014 at 03:32:57PM -0800, Siva Yerramreddy wrote:
> When running dmatest with my yet-to-be submitted driver for the Intel MIC DMA
> engine, dmatest detected "dma0chan3-copy5: result #8096161:completion busy
> status with src_off=0x0 dst_off=0x0 len=0x40 (0)". This is caused by reading
> the used cookie before the completed cookie in dma_cookie_status(),  if a DMA
> request is submitted in between the two reads, and completes,  the completed
> cookie will be newer than the used cookie value read previously.  Reversing
> the order of reads ensures that the completed cookie is for a DMA request
> older than the used cookie.
> 
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
> Reviewed-by: Nikhil Rao <nikhil.rao@intel.com>
> Signed-off-by: Siva Yerramreddy <siva.krishna.kumar.reddy.yerramreddy@intel.com>
> ---
>  drivers/dma/dmaengine.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
> index 17f983a..4c96892 100644
> --- a/drivers/dma/dmaengine.h
> +++ b/drivers/dma/dmaengine.h
> @@ -7,6 +7,7 @@
>  
>  #include <linux/bug.h>
>  #include <linux/dmaengine.h>
> +#include <asm/barrier.h>
>  
>  /**
>   * dma_cookie_init - initialize the cookies for a DMA channel
> @@ -69,8 +70,13 @@ static inline enum dma_status dma_cookie_status(struct dma_chan *chan,
>  {
>  	dma_cookie_t used, complete;
>  
> -	used = chan->cookie;
>  	complete = chan->completed_cookie;
> +	/*
> +	 * If this order is not maintained, used can end up being older than
> +	 * complete
> +	 */
> +	smp_rmb();
> +	used = chan->cookie;
>  	barrier();
>  	if (state) {
>  		state->last = complete;
The idea is right to grab the channle cookie after completed. But I dont see why
you need another barrier in between these two?

Dan,

This really is intresting case where the request completed raced with status.
I am sure for us slow slave users we may not see such issues :D but for these
and other memcpy cases do we really need to bother checking the status properly
and getting right data or delayed reporting is fine?

-- 
~Vinod

Re: [PATCH dmaengine-fixes 1/1] dmaengine: read completed cookie before used cookie

[Dan Williams]

On Fri, Feb 14, 2014 at 3:32 PM, Siva Yerramreddy
<siva.krishna.kumar.reddy.yerramreddy@intel.com> wrote:
> When running dmatest with my yet-to-be submitted driver for the Intel MIC DMA
> engine, dmatest detected "dma0chan3-copy5: result #8096161:completion busy
> status with src_off=0x0 dst_off=0x0 len=0x40 (0)". This is caused by reading
> the used cookie before the completed cookie in dma_cookie_status(),  if a DMA
> request is submitted in between the two reads, and completes,  the completed
> cookie will be newer than the used cookie value read previously.  Reversing
> the order of reads ensures that the completed cookie is for a DMA request
> older than the used cookie.
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
> Reviewed-by: Nikhil Rao <nikhil.rao@intel.com>
> Signed-off-by: Siva Yerramreddy <siva.krishna.kumar.reddy.yerramreddy@intel.com>
> ---
>  drivers/dma/dmaengine.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
> index 17f983a..4c96892 100644
> --- a/drivers/dma/dmaengine.h
> +++ b/drivers/dma/dmaengine.h
> @@ -7,6 +7,7 @@
>
>  #include <linux/bug.h>
>  #include <linux/dmaengine.h>
> +#include <asm/barrier.h>
>
>  /**
>   * dma_cookie_init - initialize the cookies for a DMA channel
> @@ -69,8 +70,13 @@ static inline enum dma_status dma_cookie_status(struct dma_chan *chan,
>  {
>         dma_cookie_t used, complete;
>
> -       used = chan->cookie;
>         complete = chan->completed_cookie;
> +       /*
> +        * If this order is not maintained, used can end up being older than
> +        * complete
> +        */
> +       smp_rmb();
> +       used = chan->cookie;
>         barrier();
>         if (state) {
>                 state->last = complete;

Hmm, this patch by itself is problematic because the rmb() needs to be
paired with a wmb() otherwise it really has no effect.  Is there a
wmb() in your driver?

I think this does need to be fixed, but we need to provide generic
helpers for other drivers.  I also think smb_rmb() and smb_wmb() are
overkill we should just be able to use smp_store_release() and
smp_load_acquire() for this.