On Mon, 2016-07-18 at 11:56 +0200, Lars-Peter Clausen wrote:
> On 07/18/2016 06:16 AM, Vinod Koul wrote:
> > 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..
>
> Typically for correct operation the driver needs to release the
> descriptor
> before calling the callback. Otherwise things will go wrong when
> terminate_all() is called from the callback.
>
> For those the pattern is
>
> dmaengine_desc_get_callback(desc, &cb);
> /* ... release descriptor */
> dmaengine_desc_callback_invoke(&cb);
>
> It just seems that there is a fair bunch of drivers which either
> don't care
> about this case or work correctly nevertheless.
>
I will add an additional helper to just perform both steps if the
driver doesn't care. ��.n��������+%������w��{.n��������)�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
