All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] dma: k3dma: Fix non-cyclic mode
@ 2017-07-10 20:53 John Stultz
  2017-07-11  1:16 ` zhangfei
  2017-07-18 15:58 ` Vinod Koul
  0 siblings, 2 replies; 11+ messages in thread
From: John Stultz @ 2017-07-10 20:53 UTC (permalink / raw)
  To: lkml
  Cc: Antonio Borneo, Vinod Koul, Dan Williams, Zhangfei Gao,
	dmaengine, John Stultz

From: Antonio Borneo <borneo.antonio@gmail.com>

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 <vinod.koul@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: dmaengine@vger.kernel.org
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
[jstultz: Expanded commit message a bit]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/dma/k3dma.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index 01e25c6..01d2a75 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -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);
 
-		WARN_ON_ONCE(c->phy->ds_run);
-		WARN_ON_ONCE(c->phy->ds_done);
 		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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] dma: k3dma: Fix non-cyclic mode
  2017-07-10 20:53 [PATCH] dma: k3dma: Fix non-cyclic mode John Stultz
@ 2017-07-11  1:16 ` zhangfei
  2017-07-18 15:58 ` Vinod Koul
  1 sibling, 0 replies; 11+ messages in thread
From: zhangfei @ 2017-07-11  1:16 UTC (permalink / raw)
  To: John Stultz, lkml; +Cc: Antonio Borneo, Vinod Koul, Dan Williams, dmaengine



On 2017年07月11日 04:53, John Stultz wrote:
> From: Antonio Borneo <borneo.antonio@gmail.com>
>
> 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 <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: dmaengine@vger.kernel.org
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> [jstultz: Expanded commit message a bit]
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>

Thanks

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dma: k3dma: Fix non-cyclic mode
  2017-07-10 20:53 [PATCH] dma: k3dma: Fix non-cyclic mode John Stultz
  2017-07-11  1:16 ` zhangfei
@ 2017-07-18 15:58 ` Vinod Koul
  2017-07-18 22:29   ` Antonio Borneo
  1 sibling, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2017-07-18 15:58 UTC (permalink / raw)
  To: John Stultz; +Cc: lkml, Antonio Borneo, Dan Williams, Zhangfei Gao, dmaengine

On Mon, Jul 10, 2017 at 01:53:43PM -0700, John Stultz wrote:
> From: Antonio Borneo <borneo.antonio@gmail.com>
> 
> 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.

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

> 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 <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: dmaengine@vger.kernel.org
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> [jstultz: Expanded commit message a bit]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/dma/k3dma.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
> index 01e25c6..01d2a75 100644
> --- a/drivers/dma/k3dma.c
> +++ b/drivers/dma/k3dma.c
> @@ -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);
>  
> -		WARN_ON_ONCE(c->phy->ds_run);
> -		WARN_ON_ONCE(c->phy->ds_done);

Not sure why WARN_ON should be removed?

>  		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);
> -- 
> 2.7.4
> 

-- 
~Vinod

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dma: k3dma: Fix non-cyclic mode
  2017-07-18 15:58 ` Vinod Koul
@ 2017-07-18 22:29   ` Antonio Borneo
  2017-07-19  3:47     ` Vinod Koul
  0 siblings, 1 reply; 11+ messages in thread
From: Antonio Borneo @ 2017-07-18 22:29 UTC (permalink / raw)
  To: Vinod Koul; +Cc: John Stultz, lkml, Dan Williams, Zhangfei Gao, dmaengine

On Tue, Jul 18, 2017 at 5:58 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Mon, Jul 10, 2017 at 01:53:43PM -0700, John Stultz wrote:
>> From: Antonio Borneo <borneo.antonio@gmail.com>
>>
>> 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.
>
> 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.

>> 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 <vinod.koul@intel.com>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
>> Cc: dmaengine@vger.kernel.org
>> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
>> [jstultz: Expanded commit message a bit]
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>  drivers/dma/k3dma.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
>> index 01e25c6..01d2a75 100644
>> --- a/drivers/dma/k3dma.c
>> +++ b/drivers/dma/k3dma.c
>> @@ -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);
>>
>> -             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.

Best Regards
Antonio

>
>>               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);
>> --
>> 2.7.4
>>
>
> --
> ~Vinod

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dma: k3dma: Fix non-cyclic mode
  2017-07-18 22:29   ` Antonio Borneo
@ 2017-07-19  3:47     ` Vinod Koul
  2017-07-19  6:00       ` Antonio Borneo
  0 siblings, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2017-07-19  3:47 UTC (permalink / raw)
  To: Antonio Borneo; +Cc: John Stultz, lkml, Dan Williams, Zhangfei Gao, dmaengine

On Wed, Jul 19, 2017 at 12:29:55AM +0200, Antonio Borneo wrote:
> On Tue, Jul 18, 2017 at 5:58 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Mon, Jul 10, 2017 at 01:53:43PM -0700, John Stultz wrote:
> >> From: Antonio Borneo <borneo.antonio@gmail.com>
> >>
> >> 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.
> >
> > 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.

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.

> >> 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 <vinod.koul@intel.com>
> >> Cc: Dan Williams <dan.j.williams@intel.com>
> >> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> >> Cc: dmaengine@vger.kernel.org
> >> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> >> [jstultz: Expanded commit message a bit]
> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >> ---
> >>  drivers/dma/k3dma.c | 12 ++++--------
> >>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
> >> index 01e25c6..01d2a75 100644
> >> --- a/drivers/dma/k3dma.c
> >> +++ b/drivers/dma/k3dma.c
> >> @@ -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);
> >>
> >> -             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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dma: k3dma: Fix non-cyclic mode
  2017-07-19  3:47     ` Vinod Koul
@ 2017-07-19  6:00       ` Antonio Borneo
  2017-08-01 20:09         ` [PATCH v2 0/3] dmaengine: " Antonio Borneo
                           ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Antonio Borneo @ 2017-07-19  6:00 UTC (permalink / raw)
  To: Vinod Koul; +Cc: John Stultz, lkml, Dan Williams, Zhangfei Gao, dmaengine

On Wed, Jul 19, 2017 at 5:47 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Wed, Jul 19, 2017 at 12:29:55AM +0200, Antonio Borneo wrote:
>> On Tue, Jul 18, 2017 at 5:58 PM, Vinod Koul <vinod.koul@intel.com> wrote:
>> > On Mon, Jul 10, 2017 at 01:53:43PM -0700, John Stultz wrote:
>> >> From: Antonio Borneo <borneo.antonio@gmail.com>
>> >>
>> >> 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.
>> >
>> > 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.
>
> 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.

Ok, I will make it clear in V2


>
>> >> 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 <vinod.koul@intel.com>
>> >> Cc: Dan Williams <dan.j.williams@intel.com>
>> >> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
>> >> Cc: dmaengine@vger.kernel.org
>> >> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
>> >> [jstultz: Expanded commit message a bit]
>> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> >> ---
>> >>  drivers/dma/k3dma.c | 12 ++++--------
>> >>  1 file changed, 4 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
>> >> index 01e25c6..01d2a75 100644
>> >> --- a/drivers/dma/k3dma.c
>> >> +++ b/drivers/dma/k3dma.c
>> >> @@ -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);
>> >>
>> >> -             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.
>
> 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: ....

Ok, I will split it and change the title.

Thanks
Antonio

>
> Thanks
> --
> ~Vinod

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 0/3] dmaengine: k3dma: Fix non-cyclic mode
  2017-07-19  6:00       ` Antonio Borneo
@ 2017-08-01 20:09         ` Antonio Borneo
  2017-08-25  6:46           ` Vinod Koul
  2017-08-01 20:09         ` [PATCH v2 1/3] dmaengine: k3dma: fix " Antonio Borneo
                           ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Antonio Borneo @ 2017-08-01 20:09 UTC (permalink / raw)
  To: dmaengine, Vinod Koul, Dan Williams
  Cc: Antonio Borneo, linux-kernel, John Stultz, Zhangfei Gao

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 v2 splits the initial patch in three parts:
- the real fix for non-cyclic mode
- another fix for a double free introduced in the same commit
- cosmetic removal of useless ON_WARN_ONCE()

Thread in https://patchwork.kernel.org/patch/9833791/

v1 -> v2
- split the patch
- change patch title to "dmaengine: ..."

Antonio Borneo (3):
  dmaengine: k3dma: fix non-cyclic mode
  dmaengine: k3dma: fix double free of descriptor
  dmaengine: k3dma: remove useless ON_WARN_ONCE()

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

-- 
1.9.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/3] dmaengine: k3dma: fix non-cyclic mode
  2017-07-19  6:00       ` Antonio Borneo
  2017-08-01 20:09         ` [PATCH v2 0/3] dmaengine: " Antonio Borneo
@ 2017-08-01 20:09         ` Antonio Borneo
  2017-08-01 20:09         ` [PATCH v2 2/3] dmaengine: k3dma: fix double free of descriptor Antonio Borneo
  2017-08-01 20:09         ` [PATCH v2 3/3] dmaengine: k3dma: remove useless ON_WARN_ONCE() Antonio Borneo
  3 siblings, 0 replies; 11+ messages in thread
From: Antonio Borneo @ 2017-08-01 20:09 UTC (permalink / raw)
  To: dmaengine, Vinod Koul, Dan Williams
  Cc: Antonio Borneo, linux-kernel, John Stultz, Zhangfei Gao

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 popped out.

The fix re-applies the initialization to ds_run/ds_done in
k3_dma_start_txd() that were removed by the commit above.

Also, one of the calls to k3_dma_start_txd() is triggered by
(ds_done != NULL), so remove the noisy and useless call to
WARN_ON_ONCE().

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---
To: dmaengine@vger.kernel.org
To: Vinod Koul <vinod.koul@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: John Stultz <john.stultz@linaro.org>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/dma/k3dma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index 01e25c6..c00eb12 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -275,12 +275,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);
 		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;
 }
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/3] dmaengine: k3dma: fix double free of descriptor
  2017-07-19  6:00       ` Antonio Borneo
  2017-08-01 20:09         ` [PATCH v2 0/3] dmaengine: " Antonio Borneo
  2017-08-01 20:09         ` [PATCH v2 1/3] dmaengine: k3dma: fix " Antonio Borneo
@ 2017-08-01 20:09         ` Antonio Borneo
  2017-08-01 20:09         ` [PATCH v2 3/3] dmaengine: k3dma: remove useless ON_WARN_ONCE() Antonio Borneo
  3 siblings, 0 replies; 11+ messages in thread
From: Antonio Borneo @ 2017-08-01 20:09 UTC (permalink / raw)
  To: dmaengine, Vinod Koul, Dan Williams
  Cc: Antonio Borneo, linux-kernel, John Stultz, Zhangfei Gao

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix
memory handling in preparation for cyclic mode") adds code
to free the descriptor in ds_done.

In cyclic mode, ds_done is never used and it's always NULL,
so the added code is not executed.

In non-cyclic mode, ds_done is used as a flag: when not NULL
it signals that the descriptor has been consumed. No need to
free it because it would be free by vchan_complete().

The fix takes back the code changed by the commit above:
- remove the free on the descriptor;
- initialize ds_done to NULL for the next run.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---
To: dmaengine@vger.kernel.org
To: Vinod Koul <vinod.koul@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: John Stultz <john.stultz@linaro.org>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/dma/k3dma.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index c00eb12..b769623 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -724,11 +724,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);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 3/3] dmaengine: k3dma: remove useless ON_WARN_ONCE()
  2017-07-19  6:00       ` Antonio Borneo
                           ` (2 preceding siblings ...)
  2017-08-01 20:09         ` [PATCH v2 2/3] dmaengine: k3dma: fix double free of descriptor Antonio Borneo
@ 2017-08-01 20:09         ` Antonio Borneo
  3 siblings, 0 replies; 11+ messages in thread
From: Antonio Borneo @ 2017-08-01 20:09 UTC (permalink / raw)
  To: dmaengine, Vinod Koul, Dan Williams
  Cc: Antonio Borneo, linux-kernel, John Stultz, Zhangfei Gao

Commit 36387a2b1f62b5c087c5fe6f0f7b23b94f722ad7 ("k3dma: Fix
memory handling in preparation for cyclic mode") adds few
calls to ON_WARN_ONCE() to track the use of ds_run/ds_done.

After the two fixes:
- dmaengine: k3dma: fix non-cyclic mode
- dmaengine: k3dma: fix re-free of the same descriptor
the behaviour of ds_run/ds_done is properly fixed.
The remaining ON_WARN_ONCE() are never triggered and can be
removed.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---
To: dmaengine@vger.kernel.org
To: Vinod Koul <vinod.koul@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: John Stultz <john.stultz@linaro.org>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/dma/k3dma.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index b769623..01d2a75 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -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,7 +273,6 @@ static int k3_dma_start_txd(struct k3_dma_chan *c)
 		 */
 		list_del(&ds->vd.node);
 
-		WARN_ON_ONCE(c->phy->ds_run);
 		c->phy->ds_run = ds;
 		c->phy->ds_done = NULL;
 		/* start dma */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/3] dmaengine: k3dma: Fix non-cyclic mode
  2017-08-01 20:09         ` [PATCH v2 0/3] dmaengine: " Antonio Borneo
@ 2017-08-25  6:46           ` Vinod Koul
  0 siblings, 0 replies; 11+ messages in thread
From: Vinod Koul @ 2017-08-25  6:46 UTC (permalink / raw)
  To: Antonio Borneo
  Cc: dmaengine, Dan Williams, linux-kernel, John Stultz, Zhangfei Gao

On Tue, Aug 01, 2017 at 10:09:24PM +0200, Antonio Borneo wrote:
> 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.

Applied, thanks

-- 
~Vinod

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-08-25  6:42 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-10 20:53 [PATCH] dma: k3dma: Fix non-cyclic mode John Stultz
2017-07-11  1:16 ` zhangfei
2017-07-18 15:58 ` Vinod Koul
2017-07-18 22:29   ` Antonio Borneo
2017-07-19  3:47     ` Vinod Koul
2017-07-19  6:00       ` Antonio Borneo
2017-08-01 20:09         ` [PATCH v2 0/3] dmaengine: " Antonio Borneo
2017-08-25  6:46           ` Vinod Koul
2017-08-01 20:09         ` [PATCH v2 1/3] dmaengine: k3dma: fix " Antonio Borneo
2017-08-01 20:09         ` [PATCH v2 2/3] dmaengine: k3dma: fix double free of descriptor Antonio Borneo
2017-08-01 20:09         ` [PATCH v2 3/3] dmaengine: k3dma: remove useless ON_WARN_ONCE() Antonio Borneo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.