| Message ID | 20181116135628.19388-1-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: fix dmaengine_desc_callback_valid() doesn't check for callback_result | expand |
> -----Original Message----- > From: Andrea Merello <andrea.merello@gmail.com> > Sent: Friday, November 16, 2018 7:26 PM > To: vkoul@kernel.org; dan.j.williams@intel.com; dmaengine@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; Radhey Shyam Pandey > <radheys@xilinx.com>; Andrea Merello <andrea.merello@gmail.com> > Subject: [PATCH] dmaengine: fix dmaengine_desc_callback_valid() doesn't > check for callback_result > > There are two flavors of DMA completion callbacks: callback() and > callback_result(); the latter takes an additional parameter that carries > result information. > > Most dmaengine helper functions that work with callbacks take care of both > flavors i.e. dmaengine_desc_get_callback_invoke() first checks for > callback_result() to be not NULL, and eventually it calls this one; > otherwise it goes on checking for callback(). > > It seems however that dmaengine_desc_callback_valid() does not care about > callback_result(), and it returns false in case callback() is NULL but > callback_result() is not; unless there is a (hidden to me) reason for doing > so then I'd say this is wrong. > > I've hit this by using a DMA controller driver (xilinx_dma) that doesn't > trigger any callback invocation unless dmaengine_desc_callback_valid() > returns true, while I had only callback_result() implemented in my client > driver (which AFAICT is always fine since dmaengine documentation says that > callback() will be deprecated). Thanks for the patch. In xilinx_dma driver call to _desc_callback_valid can be safely removed as callback ptrs are anyway checked in invoke(). There is no much benefit in having redundant checks. Related to dmaengine_desc_callback_valid extension will let Vinod comment. > > This patch fixes this by making dmaengine_desc_callback_valid() to return > true in the said scenario. > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > --- > drivers/dma/dmaengine.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h > index 501c0b063f85..0ba2c1f3c55d 100644 > --- a/drivers/dma/dmaengine.h > +++ b/drivers/dma/dmaengine.h > @@ -168,7 +168,7 @@ dmaengine_desc_get_callback_invoke(struct > dma_async_tx_descriptor *tx, > static inline bool > dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) > { > - return (cb->callback) ? true : false; > + return (cb->callback || cb->callback_result); > } > > #endif > -- > 2.17.1
On 16-11-18, 14:56, Andrea Merello wrote: > There are two flavors of DMA completion callbacks: callback() and > callback_result(); the latter takes an additional parameter that carries > result information. > > Most dmaengine helper functions that work with callbacks take care of both > flavors i.e. dmaengine_desc_get_callback_invoke() first checks for > callback_result() to be not NULL, and eventually it calls this one; > otherwise it goes on checking for callback(). > > It seems however that dmaengine_desc_callback_valid() does not care about > callback_result(), and it returns false in case callback() is NULL but > callback_result() is not; unless there is a (hidden to me) reason for doing > so then I'd say this is wrong. > > I've hit this by using a DMA controller driver (xilinx_dma) that doesn't > trigger any callback invocation unless dmaengine_desc_callback_valid() > returns true, while I had only callback_result() implemented in my client > driver (which AFAICT is always fine since dmaengine documentation says that > callback() will be deprecated). > > This patch fixes this by making dmaengine_desc_callback_valid() to return > true in the said scenario. > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > --- > drivers/dma/dmaengine.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h > index 501c0b063f85..0ba2c1f3c55d 100644 > --- a/drivers/dma/dmaengine.h > +++ b/drivers/dma/dmaengine.h > @@ -168,7 +168,7 @@ dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx, > static inline bool > dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) > { > - return (cb->callback) ? true : false; > + return (cb->callback || cb->callback_result); So I do not think this one should take care of callback_result, it is supposed to check if the callback is valid or not.. Nothing more. Ofcourse usage of this maybe incorrect which should be fixed. We do have dmaengine_desc_callback_invoke() which propagates the callback_result to user
diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h index 501c0b063f85..0ba2c1f3c55d 100644 --- a/drivers/dma/dmaengine.h +++ b/drivers/dma/dmaengine.h @@ -168,7 +168,7 @@ dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx, static inline bool dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) { - return (cb->callback) ? true : false; + return (cb->callback || cb->callback_result); } #endif
There are two flavors of DMA completion callbacks: callback() and callback_result(); the latter takes an additional parameter that carries result information. Most dmaengine helper functions that work with callbacks take care of both flavors i.e. dmaengine_desc_get_callback_invoke() first checks for callback_result() to be not NULL, and eventually it calls this one; otherwise it goes on checking for callback(). It seems however that dmaengine_desc_callback_valid() does not care about callback_result(), and it returns false in case callback() is NULL but callback_result() is not; unless there is a (hidden to me) reason for doing so then I'd say this is wrong. I've hit this by using a DMA controller driver (xilinx_dma) that doesn't trigger any callback invocation unless dmaengine_desc_callback_valid() returns true, while I had only callback_result() implemented in my client driver (which AFAICT is always fine since dmaengine documentation says that callback() will be deprecated). This patch fixes this by making dmaengine_desc_callback_valid() to return true in the said scenario. Signed-off-by: Andrea Merello <andrea.merello@gmail.com> --- drivers/dma/dmaengine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)