Hi Lucas,
Seems I miss your previous mail. Thanks for your patch, but if move most jobs
of sdma_disable_channel_with_delay() into worker, that will bring another race condition
that upper driver such as Audio terminate channel and free resource of dma channel without
really channel stop, if dma transfer done interrupt come after that, oops or kernel cash may
be caught. Leave 'sdmac->desc = NULL' in the sdma_disable_channel_with_delay() may fix
such potential issue.
> -----Original Message-----
> From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Sent: 2018年8月30日 21:22
> To: Vinod Koul <vkoul@xxxxxxxxxx>
> Cc: Robin Gong <yibin.gong@xxxxxxx>; dmaengine@xxxxxxxxxxxxxxx;
> dl-linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> patchwork-lst@xxxxxxxxxxxxxx
> Subject: [PATCH 3/4] dmaengine: imx-sdma: implement channel termination via
> worker
>
> The dmaengine documentation states that device_terminate_all may be
> asynchronous and need not wait for the active transfers to stop.
>
> This allows us to move most of the functionality currently implemented in the
> sdma channel termination function to run in a worker, outside of any atomic
> context. Moving this out of atomic context has two
> benefits: we can now sleep while waiting for the channel to terminate, instead
> of busy waiting and the freeing of the dma descriptors happens with IRQs
> enabled, getting rid of a warning in the dma mapping code.
>
> As the termination is now async, we need to implement the device_synchronize
> dma engine function which simply waits for the worker to finish its execution.
>
> Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> ---
> drivers/dma/imx-sdma.c | 40 +++++++++++++++++++++++++++++-----------
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> 8d2fec8b16cc..a3ac216ede37 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -32,6 +32,7 @@
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> #include <linux/of_dma.h>
> +#include <linux/workqueue.h>
>
> #include <asm/irq.h>
> #include <linux/platform_data/dma-imx-sdma.h>
> @@ -375,6 +376,7 @@ struct sdma_channel {
> u32 shp_addr, per_addr;
> enum dma_status status;
> struct imx_dma_data data;
> + struct work_struct terminate_worker;
> };
>
> #define IMX_DMA_SG_LOOP BIT(0)
> @@ -1025,31 +1027,45 @@ static int sdma_disable_channel(struct dma_chan
> *chan)
>
> return 0;
> }
> -
> -static int sdma_disable_channel_with_delay(struct dma_chan *chan)
> +static void sdma_channel_terminate(struct work_struct *work)
> {
> - struct sdma_channel *sdmac = to_sdma_chan(chan);
> + struct sdma_channel *sdmac = container_of(work, struct sdma_channel,
> + terminate_worker);
> unsigned long flags;
> LIST_HEAD(head);
>
> - sdma_disable_channel(chan);
> - spin_lock_irqsave(&sdmac->vc.lock, flags);
> - vchan_get_all_descriptors(&sdmac->vc, &head);
> - sdmac->desc = NULL;
> - spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> - vchan_dma_desc_free_list(&sdmac->vc, &head);
> -
> /*
> * According to NXP R&D team a delay of one BD SDMA cost time
> * (maximum is 1ms) should be added after disable of the channel
> * bit, to ensure SDMA core has really been stopped after SDMA
> * clients call .device_terminate_all.
> */
> - mdelay(1);
> + usleep_range(1000, 2000);
> +
> + spin_lock_irqsave(&sdmac->vc.lock, flags);
> + vchan_get_all_descriptors(&sdmac->vc, &head);
> + sdmac->desc = NULL;
> + spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> + vchan_dma_desc_free_list(&sdmac->vc, &head); }
> +
> +static int sdma_disable_channel_with_delay(struct dma_chan *chan) {
> + struct sdma_channel *sdmac = to_sdma_chan(chan);
> +
> + sdma_disable_channel(chan);
> + schedule_work(&sdmac->terminate_worker);
>
> return 0;
> }
>
> +static void sdma_channel_synchronize(struct dma_chan *chan) {
> + struct sdma_channel *sdmac = to_sdma_chan(chan);
> +
> + flush_work(&sdmac->terminate_worker);
> +}
> +
> static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
> {
> struct sdma_engine *sdma = sdmac->sdma; @@ -1993,6 +2009,7 @@
> static int sdma_probe(struct platform_device *pdev)
>
> sdmac->channel = i;
> sdmac->vc.desc_free = sdma_desc_free;
> + INIT_WORK(&sdmac->terminate_worker,
> sdma_channel_terminate);
> /*
> * Add the channel to the DMAC list. Do not add channel 0 though
> * because we need it internally in the SDMA driver. This also means
> @@ -2045,6 +2062,7 @@ static int sdma_probe(struct platform_device
> *pdev)
> sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
> sdma->dma_device.device_config = sdma_config;
> sdma->dma_device.device_terminate_all =
> sdma_disable_channel_with_delay;
> + sdma->dma_device.device_synchronize = sdma_channel_synchronize;
> sdma->dma_device.src_addr_widths = SDMA_DMA_BUSWIDTHS;
> sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
> sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
> --
> 2.18.0
