[PATCH v3 0/4] Correct dma pool for imx-sdma

[PATCH v3 0/4] Correct dma pool for imx-sdma

[Robin Gong]

This patch set to correct dma pool used in the commit fe5b85c656bc
("dmaengine: imx-sdma: alloclate bd memory from dma pool")
because all bds should be contiguous in memory in multi bds case
for sdma, but dma pool can't statify it and cause transfer failure
then.Thank Lucas for the making things on the right way.

This patch set is v3 based on Lucas's V1/V2, the main refine:
  --fix the issue I reported in V1 but Lucas workaround it by improper
    way in V2.
  --add vchan_synchronize() to kill tasklet in virt-dma internal.
  --refine name: use sdma_disable_channel_async instead of
    sdma_disable_channel_with_delay since no delay indeed for
    device_terminate_all.
Please refer to below link for more information:
https://patchwork.kernel.org/patch/10601111


Lucas Stach (4):
  Revert "dmaengine: imx-sdma: Use GFP_NOWAIT for dma allocations"
  Revert "dmaengine: imx-sdma: alloclate bd memory from dma pool"
  dmaengine: imx-sdma: implement channel termination via worker
  dmaengine: imx-sdma: use GFP_NOWAIT for dma descriptor allocations

 drivers/dma/imx-sdma.c | 69 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 25 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] Revert "dmaengine: imx-sdma: Use GFP_NOWAIT for dma allocations"

[Robin Gong]

From: Lucas Stach <l.stach@pengutronix.de>

This reverts commit c1199875d327, as this depends on another commit
that is going to be reverted.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/dma/imx-sdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index b4ec2d2..3bca5e0 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1194,8 +1194,8 @@ static int sdma_alloc_bd(struct sdma_desc *desc)
 {
 	int ret = 0;
 
-	desc->bd = dma_pool_alloc(desc->sdmac->bd_pool, GFP_NOWAIT,
-				  &desc->bd_phys);
+	desc->bd = dma_pool_alloc(desc->sdmac->bd_pool, GFP_ATOMIC,
+					&desc->bd_phys);
 	if (!desc->bd) {
 		ret = -ENOMEM;
 		goto out;
-- 
2.7.4

[PATCH v3 2/4] Revert "dmaengine: imx-sdma: alloclate bd memory from dma pool"

[Robin Gong]

From: Lucas Stach <l.stach@pengutronix.de>

This reverts commit fe5b85c656bc. The SDMA engine needs the descriptors to
be contiguous in memory. As the dma pool API is only able to provide a
single descriptor per alloc invocation there is no guarantee that multiple
descriptors satisfy this requirement. Also the code in question is broken
as it only allocates memory for a single descriptor, without looking at the
number of descriptors required for the transfer, leading to out-of-bounds
accesses when the descriptors are written.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/dma/imx-sdma.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 3bca5e0..8d2fec8 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -24,7 +24,6 @@
 #include <linux/spinlock.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
-#include <linux/dmapool.h>
 #include <linux/firmware.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
@@ -376,7 +375,6 @@ struct sdma_channel {
 	u32				shp_addr, per_addr;
 	enum dma_status			status;
 	struct imx_dma_data		data;
-	struct dma_pool			*bd_pool;
 };
 
 #define IMX_DMA_SG_LOOP		BIT(0)
@@ -1192,10 +1190,11 @@ static int sdma_request_channel0(struct sdma_engine *sdma)
 
 static int sdma_alloc_bd(struct sdma_desc *desc)
 {
+	u32 bd_size = desc->num_bd * sizeof(struct sdma_buffer_descriptor);
 	int ret = 0;
 
-	desc->bd = dma_pool_alloc(desc->sdmac->bd_pool, GFP_ATOMIC,
-					&desc->bd_phys);
+	desc->bd = dma_zalloc_coherent(NULL, bd_size, &desc->bd_phys,
+					GFP_ATOMIC);
 	if (!desc->bd) {
 		ret = -ENOMEM;
 		goto out;
@@ -1206,7 +1205,9 @@ static int sdma_alloc_bd(struct sdma_desc *desc)
 
 static void sdma_free_bd(struct sdma_desc *desc)
 {
-	dma_pool_free(desc->sdmac->bd_pool, desc->bd, desc->bd_phys);
+	u32 bd_size = desc->num_bd * sizeof(struct sdma_buffer_descriptor);
+
+	dma_free_coherent(NULL, bd_size, desc->bd, desc->bd_phys);
 }
 
 static void sdma_desc_free(struct virt_dma_desc *vd)
@@ -1272,10 +1273,6 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 	if (ret)
 		goto disable_clk_ahb;
 
-	sdmac->bd_pool = dma_pool_create("bd_pool", chan->device->dev,
-				sizeof(struct sdma_buffer_descriptor),
-				32, 0);
-
 	return 0;
 
 disable_clk_ahb:
@@ -1304,9 +1301,6 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 
 	clk_disable(sdma->clk_ipg);
 	clk_disable(sdma->clk_ahb);
-
-	dma_pool_destroy(sdmac->bd_pool);
-	sdmac->bd_pool = NULL;
 }
 
 static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
-- 
2.7.4

[PATCH v3 3/4] dmaengine: imx-sdma: implement channel termination via worker

[Robin Gong]

From: Lucas Stach <l.stach@pengutronix.de>

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>
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/dma/imx-sdma.c | 51 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 8d2fec8..03d46f1 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,49 @@ 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_work(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_async(struct dma_chan *chan)
+{
+	struct sdma_channel *sdmac = to_sdma_chan(chan);
+
+	sdma_disable_channel(chan);
+
+	if (sdmac->desc)
+		schedule_work(&sdmac->terminate_worker);
 
 	return 0;
 }
 
+static void sdma_channel_synchronize(struct dma_chan *chan)
+{
+	struct sdma_channel *sdmac = to_sdma_chan(chan);
+
+	vchan_synchronize(&sdmac->vc);
+
+	flush_work(&sdmac->terminate_worker);
+}
+
 static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
 {
 	struct sdma_engine *sdma = sdmac->sdma;
@@ -1287,7 +1307,9 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
 	struct sdma_engine *sdma = sdmac->sdma;
 
-	sdma_disable_channel_with_delay(chan);
+	sdma_disable_channel_async(chan);
+
+	sdma_channel_synchronize(chan);
 
 	if (sdmac->event_id0)
 		sdma_event_disable(sdmac, sdmac->event_id0);
@@ -1993,6 +2015,8 @@ 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_work);
 		/*
 		 * 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
@@ -2044,7 +2068,8 @@ static int sdma_probe(struct platform_device *pdev)
 	sdma->dma_device.device_prep_slave_sg = sdma_prep_slave_sg;
 	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_terminate_all = sdma_disable_channel_async;
+	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.7.4

[PATCH v3 4/4] dmaengine: imx-sdma: use GFP_NOWAIT for dma descriptor allocations

[Robin Gong]

From: Lucas Stach <l.stach@pengutronix.de>

DMA buffer descriptors aren't allocated from atomic context, so they
can use the less heavyweigth GFP_NOWAIT.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/dma/imx-sdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 03d46f1..cb1b44d 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1214,7 +1214,7 @@ static int sdma_alloc_bd(struct sdma_desc *desc)
 	int ret = 0;
 
 	desc->bd = dma_zalloc_coherent(NULL, bd_size, &desc->bd_phys,
-					GFP_ATOMIC);
+					GFP_NOWAIT);
 	if (!desc->bd) {
 		ret = -ENOMEM;
 		goto out;
-- 
2.7.4

Re: [PATCH v3 0/4] Correct dma pool for imx-sdma

[Lucas Stach]

Hi Robin,

Am Dienstag, den 06.11.2018, 03:40 +0000 schrieb Robin Gong:
> This patch set to correct dma pool used in the commit fe5b85c656bc
> ("dmaengine: imx-sdma: alloclate bd memory from dma pool")
> because all bds should be contiguous in memory in multi bds case
> for sdma, but dma pool can't statify it and cause transfer failure
> then.Thank Lucas for the making things on the right way.
> 
> This patch set is v3 based on Lucas's V1/V2, the main refine:
>   --fix the issue I reported in V1 but Lucas workaround it by improper
>     way in V2.
>   --add vchan_synchronize() to kill tasklet in virt-dma internal.
>   --refine name: use sdma_disable_channel_async instead of
>     sdma_disable_channel_with_delay since no delay indeed for
>     device_terminate_all.
> Please refer to below link for more information:
> https://patchwork.kernel.org/patch/10601111

Thanks for taking care of those. I've tested them on my board and
things seem to work fine for all my use-cases.

I think those patches should get a CC to stable 4.19. I guess Vinod can
add this when applying the patchset.

Regards,
Lucas

RE: [PATCH v3 0/4] Correct dma pool for imx-sdma

[Robin Gong]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2018年11月12日 23:56
> To: Robin Gong <yibin.gong@nxp.com>; vkoul@kernel.org
> Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 0/4] Correct dma pool for imx-sdma
> 
> Hi Robin,
> 
> Am Dienstag, den 06.11.2018, 03:40 +0000 schrieb Robin Gong:
> > This patch set to correct dma pool used in the commit fe5b85c656bc
> > ("dmaengine: imx-sdma: alloclate bd memory from dma pool") because all
> > bds should be contiguous in memory in multi bds case for sdma, but dma
> > pool can't statify it and cause transfer failure then.Thank Lucas for
> > the making things on the right way.
> >
> > This patch set is v3 based on Lucas's V1/V2, the main refine:
> >   --fix the issue I reported in V1 but Lucas workaround it by improper
> >     way in V2.
> >   --add vchan_synchronize() to kill tasklet in virt-dma internal.
> >   --refine name: use sdma_disable_channel_async instead of
> >     sdma_disable_channel_with_delay since no delay indeed for
> >     device_terminate_all.
> > Please refer to below link for more information:
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.kernel.org%2Fpatch%2F10601111&amp;data=02%7C01%7Cyibin.gong
> %40n
> >
> xp.com%7Ce8faea984db24513bca208d648b766fc%7C686ea1d3bc2b4c6fa92cd
> 99c5c
> >
> 301635%7C0%7C0%7C636776349873940077&amp;sdata=U6y3CMUmMfiIzBc
> Zm7jZmXTf
> > 4JBgOnBEiHhkY6ejzl8%3D&amp;reserved=0
> 
> Thanks for taking care of those. I've tested them on my board and things seem
> to work fine for all my use-cases.
> 
> I think those patches should get a CC to stable 4.19. I guess Vinod can add this
> when applying the patchset.
Yes, we'd better fix on 4.19. Hope Vinod could help us.
> 
> Regards,
> Lucas

RE: [PATCH v3 0/4] Correct dma pool for imx-sdma

[Robin Gong]

Hello Vinod,
	 This patch set test by Lucas and no any other comment received yet, do you have
comments and help to push it on 4.19 stable tree?
> -----Original Message-----
> From: Robin Gong
> Sent: 2018年11月20日 17:09
> To: 'Lucas Stach' <l.stach@pengutronix.de>; vkoul@kernel.org
> Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH v3 0/4] Correct dma pool for imx-sdma
> 
> 
> 
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2018年11月12日 23:56
> > To: Robin Gong <yibin.gong@nxp.com>; vkoul@kernel.org
> > Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v3 0/4] Correct dma pool for imx-sdma
> >
> > Hi Robin,
> >
> > Am Dienstag, den 06.11.2018, 03:40 +0000 schrieb Robin Gong:
> > > This patch set to correct dma pool used in the commit fe5b85c656bc
> > > ("dmaengine: imx-sdma: alloclate bd memory from dma pool") because
> > > all bds should be contiguous in memory in multi bds case for sdma,
> > > but dma pool can't statify it and cause transfer failure then.Thank
> > > Lucas for the making things on the right way.
> > >
> > > This patch set is v3 based on Lucas's V1/V2, the main refine:
> > >   --fix the issue I reported in V1 but Lucas workaround it by
> > > improper
> > >     way in V2.
> > >   --add vchan_synchronize() to kill tasklet in virt-dma internal.
> > >   --refine name: use sdma_disable_channel_async instead of
> > >     sdma_disable_channel_with_delay since no delay indeed for
> > >     device_terminate_all.
> > > Please refer to below link for more information:
> > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fp
> > > at
> > >
> >
> chwork.kernel.org%2Fpatch%2F10601111&amp;data=02%7C01%7Cyibin.gong
> > %40n
> > >
> >
> xp.com%7Ce8faea984db24513bca208d648b766fc%7C686ea1d3bc2b4c6fa92cd
> > 99c5c
> > >
> >
> 301635%7C0%7C0%7C636776349873940077&amp;sdata=U6y3CMUmMfiIzBc
> > Zm7jZmXTf
> > > 4JBgOnBEiHhkY6ejzl8%3D&amp;reserved=0
> >
> > Thanks for taking care of those. I've tested them on my board and
> > things seem to work fine for all my use-cases.
> >
> > I think those patches should get a CC to stable 4.19. I guess Vinod
> > can add this when applying the patchset.
> Yes, we'd better fix on 4.19. Hope Vinod could help us.
> >
> > Regards,
> > Lucas

Re: [PATCH v3 0/4] Correct dma pool for imx-sdma

[Vinod Koul]

On 06-11-18, 03:40, Robin Gong wrote:
> This patch set to correct dma pool used in the commit fe5b85c656bc
> ("dmaengine: imx-sdma: alloclate bd memory from dma pool")
> because all bds should be contiguous in memory in multi bds case
> for sdma, but dma pool can't statify it and cause transfer failure
> then.Thank Lucas for the making things on the right way.
> 
> This patch set is v3 based on Lucas's V1/V2, the main refine:
>   --fix the issue I reported in V1 but Lucas workaround it by improper
>     way in V2.
>   --add vchan_synchronize() to kill tasklet in virt-dma internal.
>   --refine name: use sdma_disable_channel_async instead of
>     sdma_disable_channel_with_delay since no delay indeed for
>     device_terminate_all.
> Please refer to below link for more information:
> https://patchwork.kernel.org/patch/10601111

Applied all, thanks and CCed stable

-- 
~Vinod