| Message ID | 20180830132213.5958-4-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | imx-sdma fixes for 4.19-rc | expand |
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@pengutronix.de> > Sent: 2018年8月30日 21:22 > To: Vinod Koul <vkoul@kernel.org> > Cc: Robin Gong <yibin.gong@nxp.com>; dmaengine@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; > patchwork-lst@pengutronix.de > 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@pengutronix.de> > --- > 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
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@pengutronix.de> > > Sent: 2018年8月30日 21:22 > > > > To: Vinod Koul <vkoul@kernel.org> > > > > > > Cc: Robin Gong <yibin.gong@nxp.com>; dmaengine@vger.kernel.org; > > > > > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; > > patchwork-lst@pengutronix.de > > 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@pengutronix.de> > > --- > > 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 > >
> -----Original Message----- > From: Lucas Stach <l.stach@pengutronix.de> > Sent: 2018年9月3日 16:41 > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > kernel@pengutronix.de; patchwork-lst@pengutronix.de > 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@pengutronix.de> > > > Sent: 2018年8月30日 21:22 > > > > > To: Vinod Koul <vkoul@kernel.org> > > > > > > > Cc: Robin Gong <yibin.gong@nxp.com>; > > > > > > > dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > > > > > > kernel@pengutronix.de; > > > patchwork-lst@pengutronix.de > > > 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@pengutronix.de> > > > --- > > > 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 > > > >
Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > -----Original Message----- > > From: Lucas Stach <l.stach@pengutronix.de> > > Sent: 2018年9月3日 16:41 > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > 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()? Please read the dmaengine documentation. device_terminate_all has no requirement that the transfer is actually canceled when the call returns. If the caller needs a guarantee that the channel is stopped it _must_ call device_synchronize. For your convenience I'm copying the relevant part of the docs below (from dmaengine_terminate_async(), which is what calls device_terminate_all(): "Calling this function will terminate all active and pending descriptors that have previously been submitted to the channel. It is not guaranteed though that the transfer for the active descriptor has stopped when the function returns. Furthermore it is possible the complete callback of a submitted transfer is still running when this function returns. dmaengine_synchronize() needs to be called before it is safe to free any memory that is accessed by previously submitted descriptors or before freeing any resources accessed from within the completion callback of any previously submitted descriptors." Regards, Lucas
> -----Original Message----- > From: Lucas Stach <l.stach@pengutronix.de> > Sent: 2018年9月3日 21:12 > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > kernel@pengutronix.de; patchwork-lst@pengutronix.de > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > termination via worker > > Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > > -----Original Message----- > > > From: Lucas Stach <l.stach@pengutronix.de> > > > Sent: 2018年9月3日 16:41 > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > 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()? > > Please read the dmaengine documentation. device_terminate_all has no > requirement that the transfer is actually canceled when the call returns. If the > caller needs a guarantee that the channel is stopped it _must_ call > device_synchronize. I know that, but the fact is some driver still use dmaengine_terminate_all() such as Spi/uart driver. My concern is how to avoid to break their function. > > For your convenience I'm copying the relevant part of the docs below (from > dmaengine_terminate_async(), which is what calls > device_terminate_all(): > > "Calling this function will terminate all active and pending descriptors that have > previously been submitted to the channel. It is not guaranteed though that the > transfer for the active descriptor has stopped when the function returns. > Furthermore it is possible the complete callback of a submitted transfer is still > running when this function returns. > > dmaengine_synchronize() needs to be called before it is safe to free any > memory that is accessed by previously submitted descriptors or before freeing > any resources accessed from within the completion callback of any previously > submitted descriptors." > > Regards, > Lucas
Am Dienstag, den 04.09.2018, 02:36 +0000 schrieb Robin Gong: > > -----Original Message----- > > From: Lucas Stach <l.stach@pengutronix.de> > > Sent: 2018年9月3日 21:12 > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > termination via worker > > > > Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > > > -----Original Message----- > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > Sent: 2018年9月3日 16:41 > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.o > > > > rg> > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > > ; > > > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > 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()? > > > > Please read the dmaengine documentation. device_terminate_all has > > no > > requirement that the transfer is actually canceled when the call > > returns. If the > > caller needs a guarantee that the channel is stopped it _must_ call > > device_synchronize. > > I know that, but the fact is some driver still use > dmaengine_terminate_all() such as > Spi/uart driver. My concern is how to avoid to break their > function. They should simply be fixed to not use a deprecated function. Both of those are only using device_terminate_all in error or shutdown paths, so the risk of races is pretty minimal even with the current code. And I think the SPI driver is trivial to fix, as we can just use the terminate_sync variant there. The UART driver is a bit more tricky. Regards, Lucas
Hi Lucas, I have quick test for UART, seems this patch set broke uart function on i.mx7d, I believe it should be same on other i.mx6 family. Could you double check it? > -----Original Message----- > From: Lucas Stach <l.stach@pengutronix.de> > Sent: 2018年9月10日 17:59 > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > kernel@pengutronix.de; patchwork-lst@pengutronix.de > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > termination via worker > > Am Dienstag, den 04.09.2018, 02:36 +0000 schrieb Robin Gong: > > > -----Original Message----- > > > From: Lucas Stach <l.stach@pengutronix.de> > > > Sent: 2018年9月3日 21:12 > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > termination via worker > > > > > > Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > > > > -----Original Message----- > > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > > Sent: 2018年9月3日 16:41 > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.o > > > > > rg> > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > > > ; kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > > 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()? > > > > > > Please read the dmaengine documentation. device_terminate_all has no > > > requirement that the transfer is actually canceled when the call > > > returns. If the caller needs a guarantee that the channel is stopped > > > it _must_ call device_synchronize. > > > > I know that, but the fact is some driver still use > > dmaengine_terminate_all() such as > > Spi/uart driver. My concern is how to avoid to break their function. > > They should simply be fixed to not use a deprecated function. Both of those are > only using device_terminate_all in error or shutdown paths, so the risk of races > is pretty minimal even with the current code. And I think the SPI driver is trivial > to fix, as we can just use the terminate_sync variant there. The UART driver > is a bit more tricky. > > Regards, > Lucas
Hi Robin, Am Dienstag, den 11.09.2018, 08:18 +0000 schrieb Robin Gong: > Hi Lucas, > I have quick test for UART, seems this patch set broke uart function on i.mx7d, > I believe it should be same on other i.mx6 family. Could you double check it? Can you describe your testcase and the issue you've seen in more detail? I'm running this patch series on a system that does a lot of communication with an UART attached Co-processor and I'm not seeing any issues. Regards, Lucas > > -----Original Message----- > > From: Lucas Stach <l.stach@pengutronix.de> > > Sent: 2018年9月10日 17:59 > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > termination via worker > > > > Am Dienstag, den 04.09.2018, 02:36 +0000 schrieb Robin Gong: > > > > -----Original Message----- > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > Sent: 2018年9月3日 21:12 > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.o > > > > rg> > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > > ; > > > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > > termination via worker > > > > > > > > Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > > > > > -----Original Message----- > > > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > > > Sent: 2018年9月3日 16:41 > > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kern > > > > > > el.o > > > > > > rg> > > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp. > > > > > > com> > > > > > > ; kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > > > 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()? > > > > > > > > Please read the dmaengine documentation. device_terminate_all > > > > has no > > > > requirement that the transfer is actually canceled when the > > > > call > > > > returns. If the caller needs a guarantee that the channel is > > > > stopped > > > > it _must_ call device_synchronize. > > > > > > I know that, but the fact is some driver still use > > > dmaengine_terminate_all() such as > > > Spi/uart driver. My concern is how to avoid to break their > > > function. > > > > They should simply be fixed to not use a deprecated function. Both > > of those are > > only using device_terminate_all in error or shutdown paths, so the > > risk of races > > is pretty minimal even with the current code. And I think the SPI > > driver is trivial > > to fix, as we can just use the terminate_sync variant there. The > > UART driver > > is a bit more tricky. > > > > Regards, > > Lucas
My testcase is simple loopback test as attached. Are you sure your serial port running in DMA mode? By default, DMA not enabled on i.mx7d, you can enable it in uart device node of dts as below: + dmas = <&sdma 32 4 0>, <&sdma 33 4 0>; + dma-names = "rx", "tx" Please flow below command(ttymxc5 on i.mx7d-sdb board) to run loopback test: ./mxc_uart_stress_test.out /dev/ttymxc5 2000000 D L 100 100 L > -----Original Message----- > From: Lucas Stach <l.stach@pengutronix.de> > Sent: 2018年9月12日 17:22 > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > kernel@pengutronix.de; patchwork-lst@pengutronix.de > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > termination via worker > > Hi Robin, > > Am Dienstag, den 11.09.2018, 08:18 +0000 schrieb Robin Gong: > > Hi Lucas, > > I have quick test for UART, seems this patch set broke uart function > > on i.mx7d, I believe it should be same on other i.mx6 family. Could you double > check it? > > Can you describe your testcase and the issue you've seen in more detail? I'm > running this patch series on a system that does a lot of communication with an > UART attached Co-processor and I'm not seeing any issues. > > Regards, > Lucas > > > > -----Original Message----- > > > From: Lucas Stach <l.stach@pengutronix.de> > > > Sent: 2018年9月10日 17:59 > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > termination via worker > > > > > > Am Dienstag, den 04.09.2018, 02:36 +0000 schrieb Robin Gong: > > > > > -----Original Message----- > > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > > Sent: 2018年9月3日 21:12 > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.o > > > > > rg> > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > > > ; kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > > > termination via worker > > > > > > > > > > Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > > > > > > -----Original Message----- > > > > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > > > > Sent: 2018年9月3日 16:41 > > > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kern > > > > > > > el.o > > > > > > > rg> > > > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp. > > > > > > > com> > > > > > > > ; kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > > > > 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()? > > > > > > > > > > Please read the dmaengine documentation. device_terminate_all > > > > > has no requirement that the transfer is actually canceled when > > > > > the call returns. If the caller needs a guarantee that the > > > > > channel is stopped it _must_ call device_synchronize. > > > > > > > > I know that, but the fact is some driver still use > > > > dmaengine_terminate_all() such as > > > > Spi/uart driver. My concern is how to avoid to break their > > > > function. > > > > > > They should simply be fixed to not use a deprecated function. Both > > > of those are only using device_terminate_all in error or shutdown > > > paths, so the risk of races is pretty minimal even with the current > > > code. And I think the SPI driver is trivial to fix, as we can just > > > use the terminate_sync variant there. The UART driver is a bit more > > > tricky. > > > > > > Regards, > > > Lucas
Hi Robin, Am Mittwoch, den 12.09.2018, 09:47 +0000 schrieb Robin Gong: > My testcase is simple loopback test as attached. Are you sure your serial port running in DMA mode? > By default, DMA not enabled on i.mx7d, you can enable it in uart device node of dts as below: > + dmas = <&sdma 32 4 0>, <&sdma 33 4 0>; > + dma-names = "rx", "tx" > Please flow below command(ttymxc5 on i.mx7d-sdb board) to run loopback test: > ./mxc_uart_stress_test.out /dev/ttymxc5 2000000 D L 100 100 L Thanks. With this testcase I was able to reproduce the issue on i.MX6. The v2 patchset I just sent out fixed this, so should hopefully be good enough to go in now. Regards, Lucas > > -----Original Message----- > > > > From: Lucas Stach <l.stach@pengutronix.de> > > Sent: 2018年9月12日 17:22 > > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > termination via worker > > > > Hi Robin, > > > > Am Dienstag, den 11.09.2018, 08:18 +0000 schrieb Robin Gong: > > > Hi Lucas, > > > I have quick test for UART, seems this patch set broke uart function > > > on i.mx7d, I believe it should be same on other i.mx6 family. Could you double > > > > check it? > > > > Can you describe your testcase and the issue you've seen in more detail? I'm > > running this patch series on a system that does a lot of communication with an > > UART attached Co-processor and I'm not seeing any issues. > > > > Regards, > > Lucas > > > > > > -----Original Message----- > > > > > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > Sent: 2018年9月10日 17:59 > > > > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org> > > > > > > > > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > > > kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > > termination via worker > > > > > > > > Am Dienstag, den 04.09.2018, 02:36 +0000 schrieb Robin Gong: > > > > > > -----Original Message----- > > > > > > > > > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > > > Sent: 2018年9月3日 21:12 > > > > > > > > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.o > > > > > > rg> > > > > > > > > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > > > > > > > > > > ; kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > > > > termination via worker > > > > > > > > > > > > Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > > > > > > > -----Original Message----- > > > > > > > > > > > > > > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > > > > > Sent: 2018年9月3日 16:41 > > > > > > > > > > > > > > > > To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kern > > > > > > > > el.o > > > > > > > > rg> > > > > > > > > > > > > > > > > Cc: dmaengine@vger.kernel.org; dl-linux-imx <linux-imx@nxp. > > > > > > > > com> > > > > > > > > > > > > > > > > ; kernel@pengutronix.de; patchwork-lst@pengutronix.de > > > > > > > > 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()? > > > > > > > > > > > > Please read the dmaengine documentation. device_terminate_all > > > > > > has no requirement that the transfer is actually canceled when > > > > > > the call returns. If the caller needs a guarantee that the > > > > > > channel is stopped it _must_ call device_synchronize. > > > > > > > > > > I know that, but the fact is some driver still use > > > > > dmaengine_terminate_all() such as > > > > > Spi/uart driver. My concern is how to avoid to break their > > > > > function. > > > > > > > > They should simply be fixed to not use a deprecated function. Both > > > > of those are only using device_terminate_all in error or shutdown > > > > paths, so the risk of races is pretty minimal even with the current > > > > code. And I think the SPI driver is trivial to fix, as we can just > > > > use the terminate_sync variant there. The UART driver is a bit more > > > > tricky. > > > > > > > > Regards, > > > > Lucas
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;
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@pengutronix.de> --- drivers/dma/imx-sdma.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-)