Hi Robin,
Am Freitag, den 31.08.2018, 09:49 +0000 schrieb Robin Gong:
> 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.
No, there is no such issue. The audio channel terminate will call
dmaengine_terminate_sync(), which internally calls
dmaengine_terminate_async() and then does a dmaengine_synchronize(). As this patchset implements the device_synchronize function in the sdma driver, this will wait for the worker to finish its execution, so there is no race condition to worry about here.
Regards,
Lucas
> > -----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
>
>
