| Message ID | 146853360943.12248.15088959265126359043.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Jul 14, 2016 at 03:00:09PM -0700, Dave Jiang wrote: > Adding a new callback that will provide the error result for a transaction. > The result is allocated on the stack and the callback should create a copy > if it wishes to retain the information after exiting. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > include/linux/dmaengine.h | 35 ++++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index b601f23..ab8c498 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -441,6 +441,21 @@ typedef bool (*dma_filter_fn)(struct dma_chan *chan, void *filter_param); > > typedef void (*dma_async_tx_callback)(void *dma_async_param); > > +enum dma_trans_result { dmaengine_tx_result? > + DMA_TRANS_NOERROR = 0, > + DMA_TRANS_READ_FAILED, > + DMA_TRANS_WRITE_FAILED, > + DMA_TRANS_ABORTED, > +}; > + > +struct dmaengine_result { > + enum dma_trans_result result; > + u32 residue; > +}; > + > +typedef void (*dma_async_tx_callback_result)(void *dma_async_param, > + struct dmaengine_result *result); > + > struct dmaengine_unmap_data { > u8 map_cnt; > u8 to_cnt; > @@ -478,6 +493,7 @@ struct dma_async_tx_descriptor { > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); > int (*desc_free)(struct dma_async_tx_descriptor *tx); > dma_async_tx_callback callback; > + dma_async_tx_callback_result callback_result; > void *callback_param; > struct dmaengine_unmap_data *unmap; > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > @@ -1374,6 +1390,7 @@ static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc) > > struct dma_desc_callback { > dma_async_tx_callback callback; > + dma_async_tx_callback_result callback_result; > void *callback_param; > }; > > @@ -1382,14 +1399,26 @@ dmaengine_desc_get_callback(struct dma_async_tx_descriptor *tx, > struct dma_desc_callback *cb) > { > cb->callback = tx->callback; > + cb->callback_result = tx->callback_result; > cb->callback_param = tx->callback_param; > } > > static inline void > -dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, void *result) > -{ > - if (cb->callback) > +dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, > + struct dmaengine_result *result) > +{ > + struct dmaengine_result dummy_result = { > + .result = DMA_TRANS_NOERROR, > + .residue = 0 > + }; > + > + if (cb->callback_result) { > + if (!result) > + result = &dummy_result; > + cb->callback_result(cb->callback_param, result); > + } else if (cb->callback) { > cb->callback(cb->callback_param); > + } > } > > /* --- DMA device --- */ >
Hi Dave, Thank you for the patch. On Thursday 14 Jul 2016 15:00:09 Dave Jiang wrote: > Adding a new callback that will provide the error result for a transaction. > The result is allocated on the stack and the callback should create a copy > if it wishes to retain the information after exiting. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > include/linux/dmaengine.h | 35 ++++++++++++++++++++++++++++++++--- Missing change for Documentation/dmaengine/* > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index b601f23..ab8c498 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -441,6 +441,21 @@ typedef bool (*dma_filter_fn)(struct dma_chan *chan, > void *filter_param); > > typedef void (*dma_async_tx_callback)(void *dma_async_param); > > +enum dma_trans_result { > + DMA_TRANS_NOERROR = 0, > + DMA_TRANS_READ_FAILED, > + DMA_TRANS_WRITE_FAILED, > + DMA_TRANS_ABORTED, > +}; > + > +struct dmaengine_result { > + enum dma_trans_result result; > + u32 residue; > +}; > + > +typedef void (*dma_async_tx_callback_result)(void *dma_async_param, > + struct dmaengine_result *result); How about making the result const ? Is there any reason to allow the callback to modify the result ? > + > struct dmaengine_unmap_data { > u8 map_cnt; > u8 to_cnt; > @@ -478,6 +493,7 @@ struct dma_async_tx_descriptor { > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); > int (*desc_free)(struct dma_async_tx_descriptor *tx); > dma_async_tx_callback callback; > + dma_async_tx_callback_result callback_result; So we now have two APIs. On the DMA engine drivers side, the transition path is quite clear. Drivers should be ported to the new API to report results. The results will be ignored by older client drivers, and newer client drivers that interact with older DMA engine drivers will get dummy results that always report success. There's no regression compared to the current API as client drivers have no way to detect failures today. On the client drivers side however, what's your plan to transition to the new API ? We don't want to keep two APIs forever, so we should get all client drivers converted in the not too distant future. There needs to be an incentive for that to happen, unless you plan to do the work yourself. The latter option would certainly be appreciated :-) > void *callback_param; > struct dmaengine_unmap_data *unmap; > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > @@ -1374,6 +1390,7 @@ static inline int dmaengine_desc_free(struct > dma_async_tx_descriptor *desc) > > struct dma_desc_callback { > dma_async_tx_callback callback; > + dma_async_tx_callback_result callback_result; > void *callback_param; > }; > > @@ -1382,14 +1399,26 @@ dmaengine_desc_get_callback(struct > dma_async_tx_descriptor *tx, struct dma_desc_callback *cb) > { > cb->callback = tx->callback; > + cb->callback_result = tx->callback_result; > cb->callback_param = tx->callback_param; > } > > static inline void > -dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, void *result) > -{ > - if (cb->callback) > +dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, > + struct dmaengine_result *result) > +{ > + struct dmaengine_result dummy_result = { > + .result = DMA_TRANS_NOERROR, > + .residue = 0 > + }; > + > + if (cb->callback_result) { > + if (!result) > + result = &dummy_result; > + cb->callback_result(cb->callback_param, result); > + } else if (cb->callback) { > cb->callback(cb->callback_param); > + } > } > > /* --- DMA device --- */
On Fri, 2016-07-15 at 09:29 +0300, Laurent Pinchart wrote: > Hi Dave, > > Thank you for the patch. > > On Thursday 14 Jul 2016 15:00:09 Dave Jiang wrote: > > Adding a new callback that will provide the error result for a > > transaction. > > The result is allocated on the stack and the callback should create > > a copy > > if it wishes to retain the information after exiting. > > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > > include/linux/dmaengine.h | 35 ++++++++++++++++++++++++++++++++- > > -- > > Missing change for Documentation/dmaengine/* Yes I realized that after the patches went out. I'll update. > > > 1 file changed, 32 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > > index b601f23..ab8c498 100644 > > --- a/include/linux/dmaengine.h > > +++ b/include/linux/dmaengine.h > > @@ -441,6 +441,21 @@ typedef bool (*dma_filter_fn)(struct dma_chan > > *chan, > > void *filter_param); > > > > typedef void (*dma_async_tx_callback)(void *dma_async_param); > > > > +enum dma_trans_result { > > + DMA_TRANS_NOERROR = 0, > > + DMA_TRANS_READ_FAILED, > > + DMA_TRANS_WRITE_FAILED, > > + DMA_TRANS_ABORTED, > > +}; > > + > > +struct dmaengine_result { > > + enum dma_trans_result result; > > + u32 residue; > > +}; > > + > > +typedef void (*dma_async_tx_callback_result)(void > > *dma_async_param, > > + struct > > dmaengine_result *result); > > How about making the result const ? Is there any reason to allow the > callback > to modify the result ? I'll make the change. > > > + > > struct dmaengine_unmap_data { > > u8 map_cnt; > > u8 to_cnt; > > @@ -478,6 +493,7 @@ struct dma_async_tx_descriptor { > > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor > > *tx); > > int (*desc_free)(struct dma_async_tx_descriptor *tx); > > dma_async_tx_callback callback; > > + dma_async_tx_callback_result callback_result; > > So we now have two APIs. On the DMA engine drivers side, the > transition path > is quite clear. Drivers should be ported to the new API to report > results. The > results will be ignored by older client drivers, and newer client > drivers that > interact with older DMA engine drivers will get dummy results that > always > report success. There's no regression compared to the current API as > client > drivers have no way to detect failures today. > > On the client drivers side however, what's your plan to transition to > the new > API ? We don't want to keep two APIs forever, so we should get all > client > drivers converted in the not too distant future. There needs to be > an > incentive for that to happen, unless you plan to do the work > yourself. The > latter option would certainly be appreciated :-) Honestly I would prefer the driver owners cared enough to move over to the new API. Can we mark the old callback as deprecated in documentation? We could add a check and print out a warning saying it's deprecated? > > > void *callback_param; > > struct dmaengine_unmap_data *unmap; > > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > > @@ -1374,6 +1390,7 @@ static inline int dmaengine_desc_free(struct > > dma_async_tx_descriptor *desc) > > > > struct dma_desc_callback { > > dma_async_tx_callback callback; > > + dma_async_tx_callback_result callback_result; > > void *callback_param; > > }; > > > > @@ -1382,14 +1399,26 @@ dmaengine_desc_get_callback(struct > > dma_async_tx_descriptor *tx, struct dma_desc_callback *cb) > > { > > cb->callback = tx->callback; > > + cb->callback_result = tx->callback_result; > > cb->callback_param = tx->callback_param; > > } > > > > static inline void > > -dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, void > > *result) > > -{ > > - if (cb->callback) > > +dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, > > + struct dmaengine_result *result) > > +{ > > + struct dmaengine_result dummy_result = { > > + .result = DMA_TRANS_NOERROR, > > + .residue = 0 > > + }; > > + > > + if (cb->callback_result) { > > + if (!result) > > + result = &dummy_result; > > + cb->callback_result(cb->callback_param, result); > > + } else if (cb->callback) { > > cb->callback(cb->callback_param); > > + } > > } > > > > /* --- DMA device --- */ >
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index b601f23..ab8c498 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -441,6 +441,21 @@ typedef bool (*dma_filter_fn)(struct dma_chan *chan, void *filter_param); typedef void (*dma_async_tx_callback)(void *dma_async_param); +enum dma_trans_result { + DMA_TRANS_NOERROR = 0, + DMA_TRANS_READ_FAILED, + DMA_TRANS_WRITE_FAILED, + DMA_TRANS_ABORTED, +}; + +struct dmaengine_result { + enum dma_trans_result result; + u32 residue; +}; + +typedef void (*dma_async_tx_callback_result)(void *dma_async_param, + struct dmaengine_result *result); + struct dmaengine_unmap_data { u8 map_cnt; u8 to_cnt; @@ -478,6 +493,7 @@ struct dma_async_tx_descriptor { dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); int (*desc_free)(struct dma_async_tx_descriptor *tx); dma_async_tx_callback callback; + dma_async_tx_callback_result callback_result; void *callback_param; struct dmaengine_unmap_data *unmap; #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH @@ -1374,6 +1390,7 @@ static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc) struct dma_desc_callback { dma_async_tx_callback callback; + dma_async_tx_callback_result callback_result; void *callback_param; }; @@ -1382,14 +1399,26 @@ dmaengine_desc_get_callback(struct dma_async_tx_descriptor *tx, struct dma_desc_callback *cb) { cb->callback = tx->callback; + cb->callback_result = tx->callback_result; cb->callback_param = tx->callback_param; } static inline void -dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, void *result) -{ - if (cb->callback) +dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, + struct dmaengine_result *result) +{ + struct dmaengine_result dummy_result = { + .result = DMA_TRANS_NOERROR, + .residue = 0 + }; + + if (cb->callback_result) { + if (!result) + result = &dummy_result; + cb->callback_result(cb->callback_param, result); + } else if (cb->callback) { cb->callback(cb->callback_param); + } } /* --- DMA device --- */
Adding a new callback that will provide the error result for a transaction. The result is allocated on the stack and the callback should create a copy if it wishes to retain the information after exiting. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- include/linux/dmaengine.h | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) -- 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