dma: k3dma: Fix non-cyclic mode

From: Antonio Borneo <[email protected]>

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix memory handling in preparation for cyclic mode") broke the logic around ds_run/ds_done in case of non-cyclic DMA.

This went unnoticed as the only user of k3dma was the i2s audio driver, but with a patch set to enable dma on SPI, the issue cropped up.

This patch resolves the issue by reverting part of the problematic commit.

This patch has been tested to ensure both audio playback and SPI works fine using DMA and that no memory leak is present.

Cc Vinod Koul <[email protected]>

Cc Dan Williams <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc [email protected]

Signed-off-by Antonio Borneo <[email protected]>

[jstultz: Expanded commit message a bit]

Signed-off-by John Stultz <[email protected]>

From: Antonio Borneo <[email protected]>

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix memory handling in preparation for cyclic mode") broke the logic around ds_run/ds_done in case of non-cyclic DMA.

This went unnoticed as the only user of k3dma was the i2s audio driver, but with a patch set to enable dma on SPI, the issue cropped up.

This patch resolves the issue by reverting part of the problematic commit.

This patch has been tested to ensure both audio playback and SPI works fine using DMA and that no memory leak is present.

Cc Vinod Koul <[email protected]>

Cc Dan Williams <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc [email protected]

Signed-off-by Antonio Borneo <[email protected]>

[jstultz: Expanded commit message a bit]

Signed-off-by John Stultz <[email protected]>

---
 drivers/dma/k3dma.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/dma/k3dma.c +0 -1

index 01e25c6..01d2a75 100644
@@ -223,7 +223,6 @@ static irqreturn_t k3_dma_int_handler(int irq, void *dev_id)
 			if (c && (tc1 & BIT(i))) {
 				spin_lock_irqsave(&c->vc.lock, flags);
 				vchan_cookie_complete(&p->ds_run->vd);
-				WARN_ON_ONCE(p->ds_done);
 				p->ds_done = p->ds_run;
 				p->ds_run = NULL;
 				spin_unlock_irqrestore(&c->vc.lock, flags);

drivers/dma/k3dma.c +4 -5

 		c->phy->ds_run = ds;
+		c->phy->ds_done = NULL;
 		/* start dma */
 		k3_dma_set_desc(c->phy, &ds->desc_hw[0]);
 		return 0;
 	}
+	c->phy->ds_run = NULL;
+	c->phy->ds_done = NULL;
 	return -EAGAIN;
 }
 

@@ -722,11 +722,7 @@ static int k3_dma_terminate_all(struct dma_chan *chan)
 			k3_dma_free_desc(&p->ds_run->vd);
 			p->ds_run = NULL;
 		}
-		if (p->ds_done) {
-			k3_dma_free_desc(&p->ds_done->vd);
-			p->ds_done = NULL;
-		}
-
+		p->ds_done = NULL;
 	}
 	spin_unlock_irqrestore(&c->vc.lock, flags);
 	vchan_dma_desc_free_list(&c->vc, &head);

Signature · 1 line

2.7.4

Quoted 10 lines · John Stultz

From: Antonio Borneo <[email protected]>

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix memory handling in preparation for cyclic mode") broke the logic around ds_run/ds_done in case of non-cyclic DMA.

This went unnoticed as the only user of k3dma was the i2s audio driver, but with a patch set to enable dma on SPI, the issue cropped up.

Quoted 2 lines

This patch resolves the issue by reverting part of the problematic commit.

Can you explain the fix here, how reverting helps around with that? Right now am not able to comprehend the fix on this..

Quoted 30 lines

This patch has been tested to ensure both audio playback and SPI works fine using DMA and that no memory leak is present.

Cc Vinod Koul <[email protected]>

Cc Dan Williams <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc [email protected]

Signed-off-by Antonio Borneo <[email protected]>

[jstultz: Expanded commit message a bit]

Signed-off-by John Stultz <[email protected]>

---
 drivers/dma/k3dma.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/dma/k3dma.c +0 -1

index 01e25c6..01d2a75 100644
@@ -223,7 +223,6 @@ static irqreturn_t k3_dma_int_handler(int irq, void *dev_id)
                      if (c && (tc1 & BIT(i))) {
                              spin_lock_irqsave(&c->vc.lock, flags);
                              vchan_cookie_complete(&p->ds_run->vd);
-                             WARN_ON_ONCE(p->ds_done);
                              p->ds_done = p->ds_run;
                              p->ds_run = NULL;
                              spin_unlock_irqrestore(&c->vc.lock, flags);

@@ -274,13 +273,14 @@ static int k3_dma_start_txd(struct k3_dma_chan *c)
               */
              list_del(&ds->vd.node);
 

drivers/dma/k3dma.c static int k3_dma_start_txd(struct k3_dma_chan *c) · line 278

Quoted 2 lines

drivers/dma/k3dma.c +0 -2

-             WARN_ON_ONCE(c->phy->ds_run);
-             WARN_ON_ONCE(c->phy->ds_done);

Not sure why WARN_ON should be removed?

Quoted 26 lines

drivers/dma/k3dma.c +4 -6

              c->phy->ds_run = ds;
+             c->phy->ds_done = NULL;
              /* start dma */
              k3_dma_set_desc(c->phy, &ds->desc_hw[0]);
              return 0;
      }
+     c->phy->ds_run = NULL;
+     c->phy->ds_done = NULL;
      return -EAGAIN;
 }
 

@@ -722,11 +722,7 @@ static int k3_dma_terminate_all(struct dma_chan *chan)
                      k3_dma_free_desc(&p->ds_run->vd);
                      p->ds_run = NULL;
              }
-             if (p->ds_done) {
-                     k3_dma_free_desc(&p->ds_done->vd);
-                     p->ds_done = NULL;
-             }
-
+             p->ds_done = NULL;
      }
      spin_unlock_irqrestore(&c->vc.lock, flags);
      vchan_dma_desc_free_list(&c->vc, &head);
--

Quoted 2 lines

2.7.4

--
~Vinod

Quoted 11 lines · Vinod Koul

Quoted 10 lines · John Stultz

From: Antonio Borneo <[email protected]>

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix memory handling in preparation for cyclic mode") broke the logic around ds_run/ds_done in case of non-cyclic DMA.

This went unnoticed as the only user of k3dma was the i2s audio driver, but with a patch set to enable dma on SPI, the issue cropped up.

Quoted 6 lines

Quoted 2 lines

This patch resolves the issue by reverting part of the problematic commit.

Can you explain the fix here, how reverting helps around with that? Right now am not able to comprehend the fix on this..

Hi Vinod,

thanks for your review.

The structure of the driver k3dma seams inspired to zx_dma that uses the same ds_run/ds_done method.
This driver k3dma was already working fine for non-cyclic use cases, like SPI. While preparing support for cyclic mode, the commit above removed some assignment to NULL for ds_run/ds_done at the end of the non-cyclic DMA transfer. As result, only the first non-cyclic DMA transfer completes successfully, the following non-cyclic DMA transfer hangs (plus it triggers one WARN_ON() because of the missing NULL initialization).

The main target of this patch is to re-add the NULL assignments.

Quoted 30 lines

Quoted 30 lines

This patch has been tested to ensure both audio playback and SPI works fine using DMA and that no memory leak is present.

Cc Vinod Koul <[email protected]>

Cc Dan Williams <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc [email protected]

Signed-off-by Antonio Borneo <[email protected]>

[jstultz: Expanded commit message a bit]

Signed-off-by John Stultz <[email protected]>

---
 drivers/dma/k3dma.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/dma/k3dma.c +0 -1

index 01e25c6..01d2a75 100644
@@ -223,7 +223,6 @@ static irqreturn_t k3_dma_int_handler(int irq, void *dev_id)
                      if (c && (tc1 & BIT(i))) {
                              spin_lock_irqsave(&c->vc.lock, flags);
                              vchan_cookie_complete(&p->ds_run->vd);
-                             WARN_ON_ONCE(p->ds_done);
                              p->ds_done = p->ds_run;
                              p->ds_run = NULL;
                              spin_unlock_irqrestore(&c->vc.lock, flags);

@@ -274,13 +273,14 @@ static int k3_dma_start_txd(struct k3_dma_chan *c)
               */
              list_del(&ds->vd.node);
 

Quoted 4 lines

drivers/dma/k3dma.c static int k3_dma_start_txd(struct k3_dma_chan *c) · line 278

Quoted 2 lines

drivers/dma/k3dma.c +0 -2

-             WARN_ON_ONCE(c->phy->ds_run);
-             WARN_ON_ONCE(c->phy->ds_done);

Not sure why WARN_ON should be removed?

The commit above added them. Now that the logic on ds_run/ds_done is fixed, I do not see reason for them anymore.

Quoted 32 lines · Antonio Borneo

Quoted 11 lines · Vinod Koul

Quoted 10 lines · John Stultz

From: Antonio Borneo <[email protected]>

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix memory handling in preparation for cyclic mode") broke the logic around ds_run/ds_done in case of non-cyclic DMA.

This went unnoticed as the only user of k3dma was the i2s audio driver, but with a patch set to enable dma on SPI, the issue cropped up.

Quoted 6 lines

Quoted 2 lines

This patch resolves the issue by reverting part of the problematic commit.

Can you explain the fix here, how reverting helps around with that? Right now am not able to comprehend the fix on this..

Hi Vinod,

thanks for your review.

The structure of the driver k3dma seams inspired to zx_dma that uses the same ds_run/ds_done method.
This driver k3dma was already working fine for non-cyclic use cases, like SPI. While preparing support for cyclic mode, the commit above removed some assignment to NULL for ds_run/ds_done at the end of the non-cyclic DMA transfer. As result, only the first non-cyclic DMA transfer completes successfully, the following non-cyclic DMA transfer hangs (plus it triggers one WARN_ON() because of the missing NULL initialization).

Quoted 1 line

The main target of this patch is to re-add the NULL assignments.

Okay so fix is only adding NULL assignments, and which was not at all clear from the changelog. The changelog need to give reviewers and maintainers and idea on why a change was done and the details of the fix.

Quoted 36 lines

Quoted 35 lines

Quoted 28 lines

This patch has been tested to ensure both audio playback and SPI works fine using DMA and that no memory leak is present.

Cc Vinod Koul <[email protected]>

Cc Dan Williams <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc [email protected]

Signed-off-by Antonio Borneo <[email protected]>

[jstultz: Expanded commit message a bit]

Signed-off-by John Stultz <[email protected]>

---
 drivers/dma/k3dma.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/dma/k3dma.c +0 -1

index 01e25c6..01d2a75 100644
@@ -223,7 +223,6 @@ static irqreturn_t k3_dma_int_handler(int irq, void *dev_id)
                      if (c && (tc1 & BIT(i))) {
                              spin_lock_irqsave(&c->vc.lock, flags);
                              vchan_cookie_complete(&p->ds_run->vd);
-                             WARN_ON_ONCE(p->ds_done);
                              p->ds_done = p->ds_run;
                              p->ds_run = NULL;
                              spin_unlock_irqrestore(&c->vc.lock, flags);

drivers/dma/k3dma.c static int k3_dma_start_txd(struct k3_dma_chan *c) · line 278

Quoted 5 lines

drivers/dma/k3dma.c +0 -2

@@ -274,13 +273,14 @@ static int k3_dma_start_txd(struct k3_dma_chan *c)
               */
              list_del(&ds->vd.node);
 
-             WARN_ON_ONCE(c->phy->ds_run);
-             WARN_ON_ONCE(c->phy->ds_done);

Not sure why WARN_ON should be removed?

Quoted 2 lines

The commit above added them. Now that the logic on ds_run/ds_done is fixed, I do not see reason for them anymore.

Since this is not a revert and only fix, I would prefer this to be removed from fix, if you want to remove these please do send a second patch as this has nothing to do with the fix

While at it, also change the patch title to dmaengine: ....

Thanks
--
~Vinod

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix memory handling in preparation for cyclic mode") broke the logic around ds_run/ds_done in case of non-cyclic DMA.