Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752869AbdGRWaC (ORCPT ); Tue, 18 Jul 2017 18:30:02 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:36844 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752883AbdGRW35 (ORCPT ); Tue, 18 Jul 2017 18:29:57 -0400 MIME-Version: 1.0 In-Reply-To: <20170718155824.GT3053@localhost> References: <1499720023-6187-1-git-send-email-john.stultz@linaro.org> <20170718155824.GT3053@localhost> From: Antonio Borneo Date: Wed, 19 Jul 2017 00:29:55 +0200 Message-ID: Subject: Re: [PATCH] dma: k3dma: Fix non-cyclic mode To: Vinod Koul Cc: John Stultz , lkml , Dan Williams , Zhangfei Gao , dmaengine@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3878 Lines: 106 On Tue, Jul 18, 2017 at 5:58 PM, Vinod Koul wrote: > On Mon, Jul 10, 2017 at 01:53:43PM -0700, John Stultz wrote: >> From: Antonio Borneo >> >> 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 >> Cc: Dan Williams >> Cc: Zhangfei Gao >> Cc: dmaengine@vger.kernel.org >> Signed-off-by: Antonio Borneo >> [jstultz: Expanded commit message a bit] >> Signed-off-by: John Stultz >> --- >> 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