On 07/18/2016 06:52 PM, Jiang, Dave wrote:
> 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.
Some drivers have to unlock a spinlock before calling the callback and then
relock it once the callback finished and they don't want to go through this
if there is not callback.
- Lars
--
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
