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.
--
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
