| Message ID | d0106d723f7455208f83e50522007fee7887e67e.1392419479.git.siva.krishna.kumar.reddy.yerramreddy@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Dan Williams |
| Headers | show |
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?
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;