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);
It makes sense to have a dedicated helper function that performs both steps
and operates on only a descriptor.
- 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
