Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752901AbdGSGAM (ORCPT ); Wed, 19 Jul 2017 02:00:12 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:32834 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752072AbdGSGAK (ORCPT ); Wed, 19 Jul 2017 02:00:10 -0400 MIME-Version: 1.0 In-Reply-To: <20170719034747.GZ3053@localhost> References: <1499720023-6187-1-git-send-email-john.stultz@linaro.org> <20170718155824.GT3053@localhost> <20170719034747.GZ3053@localhost> From: Antonio Borneo Date: Wed, 19 Jul 2017 08:00:07 +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: 3936 Lines: 97 On Wed, Jul 19, 2017 at 5:47 AM, Vinod Koul 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 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. > > 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 >> >> 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. > > 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