Message ID | 1331628036.1727.22.camel@vkoul-udesk3 (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
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
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 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); 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; 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 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; }