From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753222AbbLEIkH (ORCPT ); Sat, 5 Dec 2015 03:40:07 -0500 Received: from mga01.intel.com ([192.55.52.88]:5297 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752870AbbLEIkE (ORCPT ); Sat, 5 Dec 2015 03:40:04 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,384,1444719600"; d="scan'208";a="867157574" Date: Sat, 5 Dec 2015 14:13:05 +0530 From: Vinod Koul To: Damien Horsley Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, Dan Williams Subject: Re: [PATCH] dma: mdc: Correct terminate_all handling Message-ID: <20151205084305.GV1854@localhost> References: <1448288524-21572-1-git-send-email-Damien.Horsley@imgtec.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1448288524-21572-1-git-send-email-Damien.Horsley@imgtec.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote: > From: "Damien.Horsley" > > Use of the CANCEL bit in mdc_terminate_all creates an > additional 'command done' to appear in the registers (in > addition to an interrupt). > > In addition, there is a potential race between > mdc_terminate_all and the irq handler if a transfer > completes at the same time as the terminate all (presently > this results in an inappropriate warning). > > To handle these issues, any outstanding 'command done' > events are cleared during mdc_terminate_all and the irq > handler takes no action when there are no new 'command done' > events. SO what does 'command done' event represent, and what would be the behaviour of ignoring those > > Signed-off-by: Damien.Horsley > --- > drivers/dma/img-mdc-dma.c | 71 ++++++++++++++++++++++++++++++----------------- > 1 file changed, 45 insertions(+), 26 deletions(-) > > diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c > index 9ca5683..aa09590 100644 > --- a/drivers/dma/img-mdc-dma.c > +++ b/drivers/dma/img-mdc-dma.c > @@ -651,6 +651,42 @@ static enum dma_status mdc_tx_status(struct dma_chan *chan, > return ret; > } > > +static unsigned int mdc_get_new_events(struct mdc_chan *mchan) > +{ > + u32 val, processed, done1, done2; > + unsigned int ret; > + > + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); > + processed = (val >> MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) & > + MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK; This should be right justfied > + /* > + * CMDS_DONE may have incremented between reading CMDS_PROCESSED > + * and clearing INT_ACTIVE. Re-read CMDS_PROCESSED to ensure we > + * didn't miss a command completion. > + */ > + do { > + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); > + done1 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) & > + MDC_CMDS_PROCESSED_CMDS_DONE_MASK; > + val &= ~((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK << > + MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) | > + MDC_CMDS_PROCESSED_INT_ACTIVE); > + val |= done1 << MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT; > + mdc_chan_writel(mchan, val, MDC_CMDS_PROCESSED); > + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); > + done2 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) & > + MDC_CMDS_PROCESSED_CMDS_DONE_MASK; this is very hard to read, can you give some empty lines on logical blocks > + } while (done1 != done2); > + > + if (done1 >= processed) > + ret = done1 - processed; > + else > + ret = ((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK + 1) - > + processed) + done1; > + > + return ret; > +} > + > static int mdc_terminate_all(struct dma_chan *chan) > { > struct mdc_chan *mchan = to_mdc_chan(chan); > @@ -667,6 +703,8 @@ static int mdc_terminate_all(struct dma_chan *chan) > mchan->desc = NULL; > vchan_get_all_descriptors(&mchan->vc, &head); > > + mdc_get_new_events(mchan); > + Only getting? -- ~Vinod