> -----Original Message-----
> From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Sent: 2018年9月3日 16:41
> To: Robin Gong <yibin.gong@xxxxxxx>; Vinod Koul <vkoul@xxxxxxxxxx>
> Cc: dmaengine@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> kernel@xxxxxxxxxxxxxx; patchwork-lst@xxxxxxxxxxxxxx
> Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel
> termination via worker
>
> 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
Yes, but how about other drivers which not call dmaengine_terminate_sync()?
>
>
> > > -----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
> >
> >
