| Message ID | 20131017020745.GA14013@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 17 Oct 2013, Vinod Koul wrote: > On Wed, Oct 16, 2013 at 11:45:48AM -0700, Dan Williams wrote: > > On Wed, Oct 16, 2013 at 11:29 AM, Guennadi Liakhovetski > > > > > > Doesn't this break kernel compilation for a total of 27 commits? Or am I > > > missing anything? > > > > Yes, I think at the start DMA_COMPLETE should just be a alias for > > DMA_SUCCESS, then after all the driver renames are in delete > > DMA_SUCCESS. > Oops, taht was bad of me. ffixes in v2 and sending patch 29 for removal case Ok, yes, this should work now. I'm wondering though - is DMA_COMPLTE really a better name? AFAICS, we can only differentiate between 2 possibilities with the current API: a transfer is "in progress" - between last used and last completed, and "unknown" - either completed, or aborted, or not yet submitted - if the cookie is larger, than last completed and we assume, that it has wrapped. Actually for a driver, that I'm currently working on, I implemented a cache of N last cookies (e.g. 128), which is a bitfield, where I just record a 1, if that descriptor has failed, and a 0, if completed successfully. That way I can report one of 4 states: cookie on queue, completed successfully, failed, unknown. I'm not sure, whether I'll keep this in the final version, this doesn't really fit the present dmaengine API concept. We could make this generic, if desired. Otherwise your proposed error callback should help too. But in either case I think with the current implementation we cannot find out whether a specific cookie completed successfully or failed. One more observation: I looked at a couple of drivers, using the DMA_ERROR state. E.g. mmp_tdma.c, mxs-dma.c. They store errors in a .status field in their private data. Then they return that status in their .device_tx_status() methods - independent on the cookie! This doesn't look right to me... at_hdmac.c does something similarly strange. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Thu, Oct 17, 2013 at 10:27:13AM +0200, Guennadi Liakhovetski wrote: > On Thu, 17 Oct 2013, Vinod Koul wrote: > > > On Wed, Oct 16, 2013 at 11:45:48AM -0700, Dan Williams wrote: > > > On Wed, Oct 16, 2013 at 11:29 AM, Guennadi Liakhovetski > > > > > > > > Doesn't this break kernel compilation for a total of 27 commits? Or am I > > > > missing anything? > > > > > > Yes, I think at the start DMA_COMPLETE should just be a alias for > > > DMA_SUCCESS, then after all the driver renames are in delete > > > DMA_SUCCESS. > > Oops, taht was bad of me. ffixes in v2 and sending patch 29 for removal case > > Ok, yes, this should work now. I'm wondering though - is DMA_COMPLTE > really a better name? AFAICS, we can only differentiate between 2 > possibilities with the current API: a transfer is "in progress" - between > last used and last completed, and "unknown" - either completed, or > aborted, or not yet submitted - if the cookie is larger, than last > completed and we assume, that it has wrapped. well, once you submit N, and chekcing status, if you get last > N, then you assume it completed. If last is M then M is completed and M + 1 running and rest in queue. You know which one is last submitted in client > Actually for a driver, that I'm currently working on, I implemented a > cache of N last cookies (e.g. 128), which is a bitfield, where I just > record a 1, if that descriptor has failed, and a 0, if completed > successfully. That way I can report one of 4 states: cookie on queue, > completed successfully, failed, unknown. I'm not sure, whether I'll keep > this in the final version, this doesn't really fit the present dmaengine > API concept. We could make this generic, if desired. Otherwise your > proposed error callback should help too. But in either case I think with > the current implementation we cannot find out whether a specific cookie > completed successfully or failed. The propsed error callback will tell you if dmaengine detected a failure or not. That should with above cover well > One more observation: I looked at a couple of drivers, using the DMA_ERROR > state. E.g. mmp_tdma.c, mxs-dma.c. They store errors in a .status field in > their private data. Then they return that status in their > .device_tx_status() methods - independent on the cookie! This doesn't look > right to me... at_hdmac.c does something similarly strange. Yup bunch of ones arent being good citizens.. ~Vinod
On Thu, Oct 17, 2013 at 04:27:55PM +0200, Sebastian Andrzej Siewior wrote: > On Thu, Oct 17, 2013 at 07:37:45AM +0530, Vinod Koul wrote: > > index 0bc7275..683c380 100644 > > --- a/include/linux/dmaengine.h > > +++ b/include/linux/dmaengine.h > > @@ -45,16 +45,17 @@ static inline int dma_submit_error(dma_cookie_t cookie) > > > > /** > > * enum dma_status - DMA transaction status > > - * @DMA_SUCCESS: transaction completed successfully > > + * @DMA_COMPLETE: transaction completed > > * @DMA_IN_PROGRESS: transaction not yet processed > > * @DMA_PAUSED: transaction is paused > > * @DMA_ERROR: transaction failed > > */ > > enum dma_status { > > - DMA_SUCCESS, > > + DMA_COMPLETE, > > DMA_IN_PROGRESS, > > DMA_PAUSED, > > DMA_ERROR, > > + DMA_SUCCESS, > > }; > > There are some drivers which compare against == or != DMA_SUCCESS. Shouldn't this > become > enum dma_status { > - DMA_SUCCESS, > + DMA_COMPLETE = 0, DMA_SUCCESS = 0, > DMA_IN_PROGRESS, > DMA_PAUSED, > DMA_ERROR, > }; > > so nothing breaks during the transition? Yes i missed it in first place update the patch to fix that ~Vinod
On Thu, Oct 17, 2013 at 07:37:45AM +0530, Vinod Koul wrote: > index 0bc7275..683c380 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -45,16 +45,17 @@ static inline int dma_submit_error(dma_cookie_t cookie) > > /** > * enum dma_status - DMA transaction status > - * @DMA_SUCCESS: transaction completed successfully > + * @DMA_COMPLETE: transaction completed > * @DMA_IN_PROGRESS: transaction not yet processed > * @DMA_PAUSED: transaction is paused > * @DMA_ERROR: transaction failed > */ > enum dma_status { > - DMA_SUCCESS, > + DMA_COMPLETE, > DMA_IN_PROGRESS, > DMA_PAUSED, > DMA_ERROR, > + DMA_SUCCESS, > }; There are some drivers which compare against == or != DMA_SUCCESS. Shouldn't this become enum dma_status { - DMA_SUCCESS, + DMA_COMPLETE = 0, DMA_SUCCESS = 0, DMA_IN_PROGRESS, DMA_PAUSED, DMA_ERROR, }; so nothing breaks during the transition? Sebastian
Hi Sebastian On Thu, 17 Oct 2013, Sebastian Andrzej Siewior wrote: > On Thu, Oct 17, 2013 at 07:37:45AM +0530, Vinod Koul wrote: > > index 0bc7275..683c380 100644 > > --- a/include/linux/dmaengine.h > > +++ b/include/linux/dmaengine.h > > @@ -45,16 +45,17 @@ static inline int dma_submit_error(dma_cookie_t cookie) > > > > /** > > * enum dma_status - DMA transaction status > > - * @DMA_SUCCESS: transaction completed successfully > > + * @DMA_COMPLETE: transaction completed > > * @DMA_IN_PROGRESS: transaction not yet processed > > * @DMA_PAUSED: transaction is paused > > * @DMA_ERROR: transaction failed > > */ > > enum dma_status { > > - DMA_SUCCESS, > > + DMA_COMPLETE, > > DMA_IN_PROGRESS, > > DMA_PAUSED, > > DMA_ERROR, > > + DMA_SUCCESS, > > }; > > There are some drivers which compare against == or != DMA_SUCCESS. Shouldn't this > become > enum dma_status { > - DMA_SUCCESS, > + DMA_COMPLETE = 0, DMA_SUCCESS = 0, > DMA_IN_PROGRESS, > DMA_PAUSED, > DMA_ERROR, > }; > > so nothing breaks during the transition? Good catch. I would do enum dma_status { - DMA_SUCCESS, + DMA_COMPLETE, DMA_IN_PROGRESS, DMA_PAUSED, DMA_ERROR, }; + #define DMA_SUCCESS DMA_COMPLETE and then just remove the last line again Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
Hi Vinod On Thu, 17 Oct 2013, Vinod Koul wrote: > On Thu, Oct 17, 2013 at 04:27:55PM +0200, Sebastian Andrzej Siewior wrote: > > On Thu, Oct 17, 2013 at 07:37:45AM +0530, Vinod Koul wrote: > > > index 0bc7275..683c380 100644 > > > --- a/include/linux/dmaengine.h > > > +++ b/include/linux/dmaengine.h > > > @@ -45,16 +45,17 @@ static inline int dma_submit_error(dma_cookie_t cookie) > > > > > > /** > > > * enum dma_status - DMA transaction status > > > - * @DMA_SUCCESS: transaction completed successfully > > > + * @DMA_COMPLETE: transaction completed > > > * @DMA_IN_PROGRESS: transaction not yet processed > > > * @DMA_PAUSED: transaction is paused > > > * @DMA_ERROR: transaction failed > > > */ > > > enum dma_status { > > > - DMA_SUCCESS, > > > + DMA_COMPLETE, > > > DMA_IN_PROGRESS, > > > DMA_PAUSED, > > > DMA_ERROR, > > > + DMA_SUCCESS, > > > }; > > > > There are some drivers which compare against == or != DMA_SUCCESS. Shouldn't this > > become > > enum dma_status { > > - DMA_SUCCESS, > > + DMA_COMPLETE = 0, DMA_SUCCESS = 0, > > DMA_IN_PROGRESS, > > DMA_PAUSED, > > DMA_ERROR, > > }; > > > > so nothing breaks during the transition? > Yes i missed it in first place update the patch to fix that Are you planning to post a fixed version of this patch or you just fix it internally? Would be good to have it posted to be able to ack it and other relevant patches. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Thu, Oct 24, 2013 at 11:28:29PM +0200, Guennadi Liakhovetski wrote: > Hi Vinod > > On Thu, 17 Oct 2013, Vinod Koul wrote: > > Yes i missed it in first place update the patch to fix that > > Are you planning to post a fixed version of this patch or you just fix it > internally? Would be good to have it posted to be able to ack it and other > relevant patches. looks like you missed it... I had posted updated patch [1] in this thread here and I posted 29th patch as removal one [2] Both were pushed to -next after few days [1]: http://marc.info/?l=linux-kernel&m=138197886521699&w=2 [2]: http://marc.info/?l=linux-kernel&m=138197936321767&w=2
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index 9162ac8..81d8765 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -1062,7 +1062,7 @@ dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) unsigned long dma_sync_wait_timeout = jiffies + msecs_to_jiffies(5000); if (!tx) - return DMA_SUCCESS; + return DMA_COMPLETE; while (tx->cookie == -EBUSY) { if (time_after_eq(jiffies, dma_sync_wait_timeout)) { diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 0bc7275..683c380 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -45,16 +45,17 @@ static inline int dma_submit_error(dma_cookie_t cookie) /** * enum dma_status - DMA transaction status - * @DMA_SUCCESS: transaction completed successfully + * @DMA_COMPLETE: transaction completed * @DMA_IN_PROGRESS: transaction not yet processed * @DMA_PAUSED: transaction is paused * @DMA_ERROR: transaction failed */ enum dma_status { - DMA_SUCCESS, + DMA_COMPLETE, DMA_IN_PROGRESS, DMA_PAUSED, DMA_ERROR, + DMA_SUCCESS, }; /** @@ -979,10 +980,10 @@ static inline enum dma_status dma_async_is_complete(dma_cookie_t cookie, { if (last_complete <= last_used) { if ((cookie <= last_complete) || (cookie > last_used)) - return DMA_SUCCESS; + return DMA_COMPLETE; } else { if ((cookie <= last_complete) && (cookie > last_used)) - return DMA_SUCCESS; + return DMA_COMPLETE; } return DMA_IN_PROGRESS; } @@ -1013,11 +1014,11 @@ static inline struct dma_chan *dma_find_channel(enum dma_transaction_type tx_typ } static inline enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie) { - return DMA_SUCCESS; + return DMA_COMPLETE; } static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) { - return DMA_SUCCESS; + return DMA_COMPLETE; } static inline void dma_issue_pending_all(void) {