| Message ID | 1382634027-22243-1-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Vinod Koul |
| Headers | show |
Hi Vinod On Thu, 24 Oct 2013, Vinod Koul wrote: > for dma controllers which can inform the client that a transaction resulted in > an error > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> Looks good to me Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Thanks Guennadi > --- > include/linux/dmaengine.h | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 0bc7275..ed48615 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -425,6 +425,9 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param); > * @tx_submit: set the prepared descriptor(s) to be executed by the engine > * @callback: routine to call after this operation is complete > * @callback_param: general parameter to pass to the callback routine > + * @err_callback: routine to call if dmaengine encounter error during this > + * operation > + * @err_callback_param: general parameter to pass to the err_callback routine > * ---async_tx api specific fields--- > * @next: at completion submit this descriptor > * @parent: pointer to the next level up in the dependency chain > @@ -438,6 +441,8 @@ struct dma_async_tx_descriptor { > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); > dma_async_tx_callback callback; > void *callback_param; > + dma_async_tx_callback err_callback; > + void *err_callback_param; > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > struct dma_async_tx_descriptor *next; > struct dma_async_tx_descriptor *parent; > -- > 1.7.0.4 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
On Thu, Oct 31, 2013 at 10:19:46AM -0700, Dan Williams wrote: > On Thu, Oct 24, 2013 at 10:00 AM, Vinod Koul <vinod.koul@intel.com> wrote: > > for dma controllers which can inform the client that a transaction resulted in > > an error > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > --- > > include/linux/dmaengine.h | 5 +++++ > > 1 files changed, 5 insertions(+), 0 deletions(-) > > > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > > index 0bc7275..ed48615 100644 > > --- a/include/linux/dmaengine.h > > +++ b/include/linux/dmaengine.h > > @@ -425,6 +425,9 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param); > > * @tx_submit: set the prepared descriptor(s) to be executed by the engine > > * @callback: routine to call after this operation is complete > > * @callback_param: general parameter to pass to the callback routine > > + * @err_callback: routine to call if dmaengine encounter error during this > > + * operation > > + * @err_callback_param: general parameter to pass to the err_callback routine > > * ---async_tx api specific fields--- > > * @next: at completion submit this descriptor > > * @parent: pointer to the next level up in the dependency chain > > @@ -438,6 +441,8 @@ struct dma_async_tx_descriptor { > > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); > > dma_async_tx_callback callback; > > void *callback_param; > > + dma_async_tx_callback err_callback; > > + void *err_callback_param; > > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > > struct dma_async_tx_descriptor *next; > > struct dma_async_tx_descriptor *parent; > > Shouldn't this patch include changes to the driver(s) that will > actually use this feature? It does not make sense in isolation. Guennadi will use this API in sh drivers as he has the ability to get the errors from dma controller > In general this feels like a bandaid for the failings of the lifetime > of struct dma_async_tx_descriptor. A wider re-work is in order and > the question is can this wait for that re-work, or do we need a > temporary solution for 3.14? This was added so that the engines which have capablity to inform the driver about a failure can inform the drivers and then thru the callback the clients will know which transacation failed. Many controllers like ours cant do so we wont use this mechanism. And will need to abort and redo the transfers, at which point we know where we restarted. -- ~Vinod -- 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
On Thu, Oct 24, 2013 at 10:00 AM, Vinod Koul <vinod.koul@intel.com> wrote: > for dma controllers which can inform the client that a transaction resulted in > an error > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > include/linux/dmaengine.h | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 0bc7275..ed48615 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -425,6 +425,9 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param); > * @tx_submit: set the prepared descriptor(s) to be executed by the engine > * @callback: routine to call after this operation is complete > * @callback_param: general parameter to pass to the callback routine > + * @err_callback: routine to call if dmaengine encounter error during this > + * operation > + * @err_callback_param: general parameter to pass to the err_callback routine > * ---async_tx api specific fields--- > * @next: at completion submit this descriptor > * @parent: pointer to the next level up in the dependency chain > @@ -438,6 +441,8 @@ struct dma_async_tx_descriptor { > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); > dma_async_tx_callback callback; > void *callback_param; > + dma_async_tx_callback err_callback; > + void *err_callback_param; > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > struct dma_async_tx_descriptor *next; > struct dma_async_tx_descriptor *parent; Shouldn't this patch include changes to the driver(s) that will actually use this feature? It does not make sense in isolation. In general this feels like a bandaid for the failings of the lifetime of struct dma_async_tx_descriptor. A wider re-work is in order and the question is can this wait for that re-work, or do we need a temporary solution for 3.14? -- Dan -- 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
On Thu, Oct 31, 2013 at 10:05 AM, Vinod Koul <vinod.koul@intel.com> wrote: > On Thu, Oct 31, 2013 at 10:19:46AM -0700, Dan Williams wrote: >> On Thu, Oct 24, 2013 at 10:00 AM, Vinod Koul <vinod.koul@intel.com> wrote: >> > for dma controllers which can inform the client that a transaction resulted in >> > an error >> > >> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> >> > --- >> > include/linux/dmaengine.h | 5 +++++ >> > 1 files changed, 5 insertions(+), 0 deletions(-) >> > >> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h >> > index 0bc7275..ed48615 100644 >> > --- a/include/linux/dmaengine.h >> > +++ b/include/linux/dmaengine.h >> > @@ -425,6 +425,9 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param); >> > * @tx_submit: set the prepared descriptor(s) to be executed by the engine >> > * @callback: routine to call after this operation is complete >> > * @callback_param: general parameter to pass to the callback routine >> > + * @err_callback: routine to call if dmaengine encounter error during this >> > + * operation >> > + * @err_callback_param: general parameter to pass to the err_callback routine >> > * ---async_tx api specific fields--- >> > * @next: at completion submit this descriptor >> > * @parent: pointer to the next level up in the dependency chain >> > @@ -438,6 +441,8 @@ struct dma_async_tx_descriptor { >> > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); >> > dma_async_tx_callback callback; >> > void *callback_param; >> > + dma_async_tx_callback err_callback; >> > + void *err_callback_param; >> > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH >> > struct dma_async_tx_descriptor *next; >> > struct dma_async_tx_descriptor *parent; >> >> Shouldn't this patch include changes to the driver(s) that will >> actually use this feature? It does not make sense in isolation. > Guennadi will use this API in sh drivers as he has the ability to get the errors > from dma controller So then this patch can be folded into the sh driver update series in the first patch that calls err_callback... > >> In general this feels like a bandaid for the failings of the lifetime >> of struct dma_async_tx_descriptor. A wider re-work is in order and >> the question is can this wait for that re-work, or do we need a >> temporary solution for 3.14? > This was added so that the engines which have capablity to inform the driver > about a failure can inform the drivers and then thru the callback the clients > will know which transacation failed. Many controllers like ours cant do so we > wont use this mechanism. And will need to abort and redo the transfers, at which > point we know where we restarted. I understand, but the reason we need a separate callback is because dmaengine does not make any context guarantees beyond cookie value at callback time. The callback should be something like txd_end(txd, status), and the txd should have a lifetime determined by the client not the core. Once that is in place we only need one callback, but that's not 3.13 material. -- 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
On Thu, 2013-10-31 at 22:35 +0530, Vinod Koul wrote: > On Thu, Oct 31, 2013 at 10:19:46AM -0700, Dan Williams wrote: > > On Thu, Oct 24, 2013 at 10:00 AM, Vinod Koul <vinod.koul@intel.com> wrote: > > > for dma controllers which can inform the client that a transaction resulted in > > > an error > > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > --- > > > include/linux/dmaengine.h | 5 +++++ > > > 1 files changed, 5 insertions(+), 0 deletions(-) > > > > > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > > > index 0bc7275..ed48615 100644 > > > --- a/include/linux/dmaengine.h > > > +++ b/include/linux/dmaengine.h > > > @@ -425,6 +425,9 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param); > > > * @tx_submit: set the prepared descriptor(s) to be executed by the engine > > > * @callback: routine to call after this operation is complete > > > * @callback_param: general parameter to pass to the callback routine > > > + * @err_callback: routine to call if dmaengine encounter error during this > > > + * operation > > > + * @err_callback_param: general parameter to pass to the err_callback routine > > > * ---async_tx api specific fields--- > > > * @next: at completion submit this descriptor > > > * @parent: pointer to the next level up in the dependency chain > > > @@ -438,6 +441,8 @@ struct dma_async_tx_descriptor { > > > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); > > > dma_async_tx_callback callback; > > > void *callback_param; > > > + dma_async_tx_callback err_callback; > > > + void *err_callback_param; > > > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > > > struct dma_async_tx_descriptor *next; > > > struct dma_async_tx_descriptor *parent; > > > > Shouldn't this patch include changes to the driver(s) that will > > actually use this feature? It does not make sense in isolation. > Guennadi will use this API in sh drivers as he has the ability to get the errors > from dma controller > > > In general this feels like a bandaid for the failings of the lifetime > > of struct dma_async_tx_descriptor. A wider re-work is in order and > > the question is can this wait for that re-work, or do we need a > > temporary solution for 3.14? > This was added so that the engines which have capablity to inform the driver > about a failure can inform the drivers and then thru the callback the clients > will know which transacation failed. Many controllers like ours cant do so we > wont use this mechanism. And will need to abort and redo the transfers, at which > point we know where we restarted. ioatdma can use better error handling instead of just die. It can be done through the original callback, although separating out the error handling can probably make things cleaner.
On Thu, Oct 31, 2013 at 11:46:11PM +0530, Jiang, Dave wrote: > On Thu, 2013-10-31 at 22:35 +0530, Vinod Koul wrote: > > On Thu, Oct 31, 2013 at 10:19:46AM -0700, Dan Williams wrote: > > > On Thu, Oct 24, 2013 at 10:00 AM, Vinod Koul <vinod.koul@intel.com> wrote: > > > > for dma controllers which can inform the client that a transaction resulted in > > > > an error > > > > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > --- > > > > include/linux/dmaengine.h | 5 +++++ > > > > 1 files changed, 5 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > > > > index 0bc7275..ed48615 100644 > > > > --- a/include/linux/dmaengine.h > > > > +++ b/include/linux/dmaengine.h > > > > @@ -425,6 +425,9 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param); > > > > * @tx_submit: set the prepared descriptor(s) to be executed by the engine > > > > * @callback: routine to call after this operation is complete > > > > * @callback_param: general parameter to pass to the callback routine > > > > + * @err_callback: routine to call if dmaengine encounter error during this > > > > + * operation > > > > + * @err_callback_param: general parameter to pass to the err_callback routine > > > > * ---async_tx api specific fields--- > > > > * @next: at completion submit this descriptor > > > > * @parent: pointer to the next level up in the dependency chain > > > > @@ -438,6 +441,8 @@ struct dma_async_tx_descriptor { > > > > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); > > > > dma_async_tx_callback callback; > > > > void *callback_param; > > > > + dma_async_tx_callback err_callback; > > > > + void *err_callback_param; > > > > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > > > > struct dma_async_tx_descriptor *next; > > > > struct dma_async_tx_descriptor *parent; > > > > > > Shouldn't this patch include changes to the driver(s) that will > > > actually use this feature? It does not make sense in isolation. > > Guennadi will use this API in sh drivers as he has the ability to get the errors > > from dma controller > > > > > In general this feels like a bandaid for the failings of the lifetime > > > of struct dma_async_tx_descriptor. A wider re-work is in order and > > > the question is can this wait for that re-work, or do we need a > > > temporary solution for 3.14? > > This was added so that the engines which have capablity to inform the driver > > about a failure can inform the drivers and then thru the callback the clients > > will know which transacation failed. Many controllers like ours cant do so we > > wont use this mechanism. And will need to abort and redo the transfers, at which > > point we know where we restarted. > > ioatdma can use better error handling instead of just die. It can be > done through the original callback, although separating out the error > handling can probably make things cleaner. Okay, i am going to hold this patch for now and restart this discussion after merge window :)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 0bc7275..ed48615 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -425,6 +425,9 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param); * @tx_submit: set the prepared descriptor(s) to be executed by the engine * @callback: routine to call after this operation is complete * @callback_param: general parameter to pass to the callback routine + * @err_callback: routine to call if dmaengine encounter error during this + * operation + * @err_callback_param: general parameter to pass to the err_callback routine * ---async_tx api specific fields--- * @next: at completion submit this descriptor * @parent: pointer to the next level up in the dependency chain @@ -438,6 +441,8 @@ struct dma_async_tx_descriptor { dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); dma_async_tx_callback callback; void *callback_param; + dma_async_tx_callback err_callback; + void *err_callback_param; #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH struct dma_async_tx_descriptor *next; struct dma_async_tx_descriptor *parent;
for dma controllers which can inform the client that a transaction resulted in an error Signed-off-by: Vinod Koul <vinod.koul@intel.com> --- include/linux/dmaengine.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)