Re: [PATCH] dma: k3dma: Fix non-cyclic mode

[Antonio Borneo]

On Wed, Jul 19, 2017 at 5:47 AM, Vinod Koul <vinod.koul@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>> > On Mon, Jul 10, 2017 at 01:53:43PM -0700, John Stultz wrote:
>> >> From: Antonio Borneo <borneo.antonio@xxxxxxxxx>
>> >>
>> >> 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@xxxxxxxxx>
>> >> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> >> Cc: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
>> >> Cc: dmaengine@xxxxxxxxxxxxxxx
>> >> Signed-off-by: Antonio Borneo <borneo.antonio@xxxxxxxxx>
>> >> [jstultz: Expanded commit message a bit]
>> >> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>> >> ---
>> >>  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