On Sun, Jul 17, 2016 at 08:37:00PM +0200, Lars-Peter Clausen wrote:
> On 07/15/2016 10:17 PM, Dave Jiang wrote:
> > The following series implements a way for DMA drivers to report the result
> > of a transaction via callback in order to notify the upper layer. Thanks
> > Lars for providing guidance.
> >
> > I added usage of the new mechanism in the ioatdma driver and also updated
> > the NTB code that uses this mechanism to receive the error. Jon, I assume
> > you'll let Vinod push the two NTB patches since it's part of the series.
>
> This mostly looks good. A few things I've noticed in a couple of patches.
>
> The dmaengine_desc_callback fields should not be directly accessed, it
> should be fully transparent to drives. The whole idea behind the stuct and
> its helper functions is to separate functionality and implementation, so
> that the implementation can be changed without affecting functionality.
>
> E.g. a common pattern where this happens is
>
> if (cb.callback) {
> spin_unlock(...);
> dmaengine_desc_callback_invoke(&cb);
> spin_lock(...);
> }
>
> This will break if the client uses callback_result instead of callback. The
> easiest way to resolve this is probably to add a helper function that checks
> if the callback should be called.
>
> Another thing is the common occurrence of the following pattern
>
> struct dmaengine_desc_callback cb;
>
> dmaengine_desc_get_callback(desc, &cb);
> dmaengine_desc_callback_invoke(&cb);
So do we need these helpers? Why not fold these into
dmaengine_desc_callback_invoke() and callback details hidden to drivers..
>
> It makes sense to have a dedicated helper function that performs both steps
> and operates on only a descriptor.
Yup!
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
