On Sun, 2016-07-17 at 20:37 +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.
Ok I'll do that. Mainly I was trying to preserve the original code and
do the minimal alteration but I don't think the check is really
necessary since invoke checks as well.
>
> 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);
>
> It makes sense to have a dedicated helper function that performs both
> steps
> and operates on only a descriptor.
>
> - Lars��.n��������+%������w��{.n��������)�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
