Re: [PATCH] DMA: extend documentation to provide more API details

From: Russell King - ARM Linux
Date: Sat Oct 05 2013 - 15:02:35 EST


On Sat, Oct 05, 2013 at 07:36:20PM +0200, Guennadi Liakhovetski wrote:
> This patch extends dmaengine documentation to provide more details
> on descriptor prepare stage, transaction completion requirements
> and DMA error processing.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> ---
>
> These extensions reflect my understanding of some aspects of the dmaengine
> API. If it is wrong, I'd be happy if someone could correct me. If or where
> I'm right, I think, those aspects might want an update. Once we understand
> exactly the situation we can think about improvements to the API.
>
> Documentation/dmaengine.txt | 58 ++++++++++++++++++++++++++++++++++++------
> 1 files changed, 49 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/dmaengine.txt b/Documentation/dmaengine.txt
> index 879b6e3..21bb72d 100644
> --- a/Documentation/dmaengine.txt
> +++ b/Documentation/dmaengine.txt
> @@ -133,8 +133,17 @@ The slave DMA usage consists of following steps:
> locks before calling the callback function which may cause a
> deadlock.
>
> - Note that callbacks will always be invoked from the DMA
> - engines tasklet, never from interrupt context.
> + Note that callbacks will always be invoked from the DMA engine's
> + interrupt processing bottom half, never from interrupt context.
> +
> + Note 2:
> + A DMA transaction descriptor, returned to the user by one of "prep"
> + methods is considered as belogning to the user until it is submitted
> + to the dmaengine driver for transfer. However, it is recommended, that
> + the dmaengine driver keeps references to prepared descriptors too,
> + because if dmaengine_terminate_all() is called at that time, the driver
> + will have to recycle all allocated descriptors for the respective
> + channel.

No. That's quite dangerous. think about what can happen.

Thread 1 Thread 2
Driver calls prepare
dmaengine_terminate_all()
dmaengine driver frees prepared descriptor
driver submits descriptor

You now have a descriptor which has been freed submitted to the DMA engine
queue. This will cause chaos.

> 4. Submit the transaction
>
> @@ -150,15 +159,27 @@ The slave DMA usage consists of following steps:
> dmaengine_submit() will not start the DMA operation, it merely adds
> it to the pending queue. For this, see step 5, dma_async_issue_pending.
>
> -5. Issue pending DMA requests and wait for callback notification
> +5. Issue pending DMA requests and (optionally) request callback notification
>
> - The transactions in the pending queue can be activated by calling the
> - issue_pending API. If channel is idle then the first transaction in
> - queue is started and subsequent ones queued up.
> + Dmaengine drivers accumulate submitted transactions on a pending queue.
> + After this call all such pending transactions are activated. Transactions,
> + submitted after this call will be queued again in a deactivated state until
> + this function is called again. If at the time of this call the channel is
> + idle then the first transaction in queue is started and subsequent ones are
> + queued up.
>
> - On completion of each DMA operation, the next in queue is started and
> - a tasklet triggered. The tasklet will then call the client driver
> - completion callback routine for notification, if set.
> + On completion of each DMA operation, the next active transaction in queue is
> + started and the ISR bottom half, e.g. a tasklet or a kernel thread is
> + triggered.

Or a kernel thread? I don't think that's right. It's always been
specified that the callback will happen from tasklet context.

> + The dmaengine driver also has to check the DMA_CTRL_ACK flag by calling
> + async_tx_test_ack() on the transaction. If the function returns true, i.e.
> + if the transaction either has been flagged from the very beginning, or
> + the user has flagged it later, then the transaction descriptor can be
> + recycled immediately by the dmaengine driver.

"has to check" I think is wrong. This is optional for slave only drivers,
because most slave stuff doesn't use the ACK stuff - that's more for the
async_tx APIs.

> If the function returns
> + false, the descriptor cannot be recycled yet and the dmaengine driver has
> + to keep polling the descriptor until it is acknowledged by the user.
>
> Interface:
> void dma_async_issue_pending(struct dma_chan *chan);
> @@ -171,6 +192,14 @@ Further APIs:
> discard data in the DMA FIFO which hasn't been fully transferred.
> No callback functions will be called for any incomplete transfers.
>
> + Note:
> + Transactions, aborted by this call are considered as failed. However,
> + if the user requests their status, using dma_async_is_complete() /
> + dma_async_is_complete(), as described below, one of DMA_IN_PROGRESS and
> + DMA_SUCCESS will be returned. So, drivers are advised not to use those
> + calls on transactions, submitted before a call to
> + dmaengine_terminate_all().

The last sentence doesn't make sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/