On Thu, Jun 09, 2016 at 03:16:30PM +0300, Nandor Han wrote:
> Having the SDMA driver use a tasklet for running the clients
> callback introduce some issues:
> - probability to have desynchronized data because of the
> race condition created since the DMA transaction status
> is retrieved only when the callback is executed, leaving
> plenty of time for transaction status to get altered.
> - inter-transfer latency which can leave channels idle.
>
> Move the callback execution, for cyclic channels, to SDMA
> interrupt (as advised in `Documentation/dmaengine/provider.txt`)
> to (a)reduce the inter-transfer latency and (b) eliminate the
> race condition possibility where DMA transaction status might
> be changed by the time is read.
>
> The responsibility of the SDMA interrupt latency
> is moved to the SDMA clients which case by case should defer
> the work to bottom-halves when needed.
Both of these look fine. Please change the patch titles to dmaengine: xxxx
Are these going to be merged thru dmaengine tree or serial one?
>
> Signed-off-by: Nandor Han <nandor.han@xxxxxx>
> ---
> drivers/dma/imx-sdma.c | 36 ++++++++++++++++--------------------
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 0f6fd42..e497847 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -654,12 +654,6 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event)
> writel_relaxed(val, sdma->regs + chnenbl);
> }
>
> -static void sdma_handle_channel_loop(struct sdma_channel *sdmac)
> -{
> - if (sdmac->desc.callback)
> - sdmac->desc.callback(sdmac->desc.callback_param);
> -}
> -
> static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> {
> struct sdma_buffer_descriptor *bd;
> @@ -678,13 +672,25 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> sdmac->status = DMA_ERROR;
>
> bd->mode.status |= BD_DONE;
> +
> + /*
> + * The callback is called from the interrupt context in order
> + * to reduce latency and to avoid the risk of altering the
> + * SDMA transaction status by the time the client tasklet is
> + * executed.
> + */
> +
> + if (sdmac->desc.callback)
> + sdmac->desc.callback(sdmac->desc.callback_param);
> +
> sdmac->buf_tail++;
> sdmac->buf_tail %= sdmac->num_bd;
> }
> }
>
> -static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac)
> +static void mxc_sdma_handle_channel_normal(unsigned long data)
> {
> + struct sdma_channel *sdmac = (struct sdma_channel *) data;
> struct sdma_buffer_descriptor *bd;
> int i, error = 0;
>
> @@ -711,16 +717,6 @@ static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac)
> sdmac->desc.callback(sdmac->desc.callback_param);
> }
>
> -static void sdma_tasklet(unsigned long data)
> -{
> - struct sdma_channel *sdmac = (struct sdma_channel *) data;
> -
> - if (sdmac->flags & IMX_DMA_SG_LOOP)
> - sdma_handle_channel_loop(sdmac);
> - else
> - mxc_sdma_handle_channel_normal(sdmac);
> -}
> -
> static irqreturn_t sdma_int_handler(int irq, void *dev_id)
> {
> struct sdma_engine *sdma = dev_id;
> @@ -737,8 +733,8 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id)
>
> if (sdmac->flags & IMX_DMA_SG_LOOP)
> sdma_update_channel_loop(sdmac);
> -
> - tasklet_schedule(&sdmac->tasklet);
> + else
> + tasklet_schedule(&sdmac->tasklet);
>
> __clear_bit(channel, &stat);
> }
> @@ -1739,7 +1735,7 @@ static int sdma_probe(struct platform_device *pdev)
> dma_cookie_init(&sdmac->chan);
> sdmac->channel = i;
>
> - tasklet_init(&sdmac->tasklet, sdma_tasklet,
> + tasklet_init(&sdmac->tasklet, mxc_sdma_handle_channel_normal,
> (unsigned long) sdmac);
> /*
> * Add the channel to the DMAC list. Do not add channel 0 though
> --
> 2.8.3
>
--
~Vinod
--
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
