On Wed, 2016-07-13 at 08:00 +0000, Lars-Peter Clausen wrote:
> On 07/13/2016 12:24 AM, Dave Jiang wrote:
> > Lars,
> > Here's the reworked code per your comment. Let me know if this
> > matches
> > what you are thinking. Patch 2/4 shows how the driver provides the
> > error, and patch 3,4/4 show how the errors are received by upper
> > layer.
> > The callback is an addition so people should be able to transition
> > to the new one when convenient and the old callback still remains
> > for
> > compatibility. We should be able to expand on the new callback as
> > needed since I don't know much about the slave DMA side of things.
> > If
> > you are ok with it I'll clean it up and add the proper
> > comments/docs
> > etc and resubmit.
>
> Looks OK.
>
> For the dispatching between the two callback functions there should
> be a
> helper function in the core and all drivers should be update to use
> this
> helper, so that clients can rely on that the right callback will be
> called.
> If no result is provided by the driver the helper function should
> provide a
> dummy result with no errors and no residue.
>
> What makes the whole thing slightly tricky is that some drivers store
> the
> callback on the stack and free (or recycle) the descriptor before the
> callback is called. Since this is actually required for correct
> operation we
> have to work with it.
>
> Something like this might work.
Maybe I'm missing something. In the example below you proposed two
steps. Implement helper function and update drivers, and then change
the helper function to provide results and update the drivers again
since the new helper function require extra parameter. That would
require us to fix up all the drivers twice. Is there anything that
prevents us to go directly to step two and implement the the new helper
function with results and only touch all the drivers once?
>
> struct dma_desc_callback {
> dma_async_tx_callback callback;
> void *callback_param;
> };
>
> static inline void dmaengine_desc_get_callback(
> struct dma_async_tx_descriptor *tx,
> struct dma_desc_callback *cb)
> {
> cb->callback = tx->callback;
> cb->callback_param = tx->callback_param;
> }
>
> static inline void dmaengine_desc_callback_invoke(
> struct dma_desc_callback *cb, void *result)
> {
> if (cb->callback)
> cb->callback(cb->callback_param);
> }
>
> And then update the drivers to something like:
>
> struct dmaengine_desc_callback cb;
> ...
> dmaengine_desc_get_callback(tx, &cb);
> kfree(tx);
> dmaengine_desc_callback_invoke(&cb, NULL);
> ...
>
> This should result in the same generated code, but will allow to make
> changes.
>
> And then when implementing the result callback update those helper
> functions to.
>
> struct dma_desc_callback {
> dma_async_tx_callback callback;
> dma_async_tx_callback_result callback_result;
> void *callback_param;
> };
>
> static inline void dmaengine_desc_get_callback(
> struct dma_async_tx_descriptor *tx,
> struct dma_desc_callback *cb)
> {
> cb->callback = tx->callback;
> cb->callback_result = tx->callback_result;
> cb->callback_param = tx->callback_param;
> }
>
> static inline void dmaengine_desc_callback_invoke(
> struct dma_desc_callback *cb, struct dmaengine_result *result)
> {
> static const struct dmaengine_result dummy_result = {
> .result = DMA_TRANS_NOERROR,
> .residue = 0
> };
>
> if (cb->callback_result) {
> if (!result) {
> result = &dummy_result;
> cb->callback_result(cb->callback_param, result);
> } else if (cb->callback) {
> cb->callback(cb->callback_param);
> }
> }��.n��������+%������w��{.n��������)�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
