* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-26 23:55 ` Scott Branden 0 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-26 23:55 UTC (permalink / raw) To: Dan Williams, Vinod Koul Cc: bcm-kernel-feedback-list, Dmitry Torokhov, Anatol Pomazao, linux-kernel, linux-arm-kernel, ismail, Scott Branden From: ismail <ismail@broadcom.com> Update the thread running index before issuing the GO command to the DMAC. Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> Reviewed-by: Ray Jui <rjui@broadcom.com> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> Reviewed-by: Scott Branden <sbranden@broadcom.com> Signed-off-by: Scott Branden <sbranden@broadcom.com> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> --- drivers/dma/pl330.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 0e1f567..631642d 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) /* Set to generate interrupts for SEV */ writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); + thrd->req_running = idx; + /* Only manager can execute GO */ _execute_DBGINSN(thrd, insn, true); - thrd->req_running = idx; - return true; } -- 2.3.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-26 23:55 ` Scott Branden 0 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-26 23:55 UTC (permalink / raw) To: linux-arm-kernel From: ismail <ismail@broadcom.com> Update the thread running index before issuing the GO command to the DMAC. Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> Reviewed-by: Ray Jui <rjui@broadcom.com> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> Reviewed-by: Scott Branden <sbranden@broadcom.com> Signed-off-by: Scott Branden <sbranden@broadcom.com> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> --- drivers/dma/pl330.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 0e1f567..631642d 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) /* Set to generate interrupts for SEV */ writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); + thrd->req_running = idx; + /* Only manager can execute GO */ _execute_DBGINSN(thrd, insn, true); - thrd->req_running = idx; - return true; } -- 2.3.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. 2015-03-26 23:55 ` Scott Branden @ 2015-03-29 21:55 ` Scott Branden -1 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-29 21:55 UTC (permalink / raw) To: Dan Williams, Vinod Koul, Jaswinder Singh, Jassi Brar, Jassi Brar Cc: bcm-kernel-feedback-list, Dmitry Torokhov, Anatol Pomazao, linux-kernel, linux-arm-kernel, ismail Adding original author Jaswinder Singh (3 email addresses) to email list. On 15-03-26 04:55 PM, Scott Branden wrote: > From: ismail <ismail@broadcom.com> > > Update the thread running index before issuing the > GO command to the DMAC. > > Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Arun Parameswaran <aparames@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > --- > drivers/dma/pl330.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 0e1f567..631642d 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) > /* Set to generate interrupts for SEV */ > writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); > > + thrd->req_running = idx; > + > /* Only manager can execute GO */ > _execute_DBGINSN(thrd, insn, true); > > - thrd->req_running = idx; > - > return true; > } > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-29 21:55 ` Scott Branden 0 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-29 21:55 UTC (permalink / raw) To: linux-arm-kernel Adding original author Jaswinder Singh (3 email addresses) to email list. On 15-03-26 04:55 PM, Scott Branden wrote: > From: ismail <ismail@broadcom.com> > > Update the thread running index before issuing the > GO command to the DMAC. > > Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Arun Parameswaran <aparames@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > --- > drivers/dma/pl330.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 0e1f567..631642d 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) > /* Set to generate interrupts for SEV */ > writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); > > + thrd->req_running = idx; > + > /* Only manager can execute GO */ > _execute_DBGINSN(thrd, insn, true); > > - thrd->req_running = idx; > - > return true; > } > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. 2015-03-26 23:55 ` Scott Branden @ 2015-03-30 16:47 ` Jassi Brar -1 siblings, 0 replies; 14+ messages in thread From: Jassi Brar @ 2015-03-30 16:47 UTC (permalink / raw) To: Scott Branden Cc: Dan Williams, Vinod Koul, Linux Kernel Mailing List, ismail, Anatol Pomazao, Dmitry Torokhov, bcm-kernel-feedback-list, linux-arm-kernel On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> wrote: > From: ismail <ismail@broadcom.com> > > Update the thread running index before issuing the > GO command to the DMAC. > > Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Arun Parameswaran <aparames@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > --- > drivers/dma/pl330.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 0e1f567..631642d 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) > /* Set to generate interrupts for SEV */ > writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); > > + thrd->req_running = idx; > + > /* Only manager can execute GO */ > _execute_DBGINSN(thrd, insn, true); > > - thrd->req_running = idx; > - It would help to know what the behavior looks like before and after the patch. If anything we should look at locking rather the reordering. Thanks ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-30 16:47 ` Jassi Brar 0 siblings, 0 replies; 14+ messages in thread From: Jassi Brar @ 2015-03-30 16:47 UTC (permalink / raw) To: linux-arm-kernel On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> wrote: > From: ismail <ismail@broadcom.com> > > Update the thread running index before issuing the > GO command to the DMAC. > > Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Arun Parameswaran <aparames@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > --- > drivers/dma/pl330.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 0e1f567..631642d 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) > /* Set to generate interrupts for SEV */ > writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); > > + thrd->req_running = idx; > + > /* Only manager can execute GO */ > _execute_DBGINSN(thrd, insn, true); > > - thrd->req_running = idx; > - It would help to know what the behavior looks like before and after the patch. If anything we should look at locking rather the reordering. Thanks ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. 2015-03-30 16:47 ` Jassi Brar @ 2015-03-30 17:25 ` Vinod Koul -1 siblings, 0 replies; 14+ messages in thread From: Vinod Koul @ 2015-03-30 17:25 UTC (permalink / raw) To: Jassi Brar Cc: Scott Branden, Dan Williams, Linux Kernel Mailing List, ismail, Anatol Pomazao, Dmitry Torokhov, bcm-kernel-feedback-list, linux-arm-kernel On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: > On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> wrote: > > From: ismail <ismail@broadcom.com> > > > > Update the thread running index before issuing the > > GO command to the DMAC. > > > > Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > > Reviewed-by: Ray Jui <rjui@broadcom.com> > > Reviewed-by: Arun Parameswaran <aparames@broadcom.com> > > Reviewed-by: Scott Branden <sbranden@broadcom.com> > > Signed-off-by: Scott Branden <sbranden@broadcom.com> > > Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > > --- > > drivers/dma/pl330.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index 0e1f567..631642d 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) > > /* Set to generate interrupts for SEV */ > > writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); > > > > + thrd->req_running = idx; > > + > > /* Only manager can execute GO */ > > _execute_DBGINSN(thrd, insn, true); > > > > - thrd->req_running = idx; > > - > It would help to know what the behavior looks like before and after > the patch. If anything we should look at locking rather the > reordering. Yes that ia fair request, looking at changelog it is hard to understand the issue seen? -- ~Vinod ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-30 17:25 ` Vinod Koul 0 siblings, 0 replies; 14+ messages in thread From: Vinod Koul @ 2015-03-30 17:25 UTC (permalink / raw) To: linux-arm-kernel On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: > On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> wrote: > > From: ismail <ismail@broadcom.com> > > > > Update the thread running index before issuing the > > GO command to the DMAC. > > > > Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > > Reviewed-by: Ray Jui <rjui@broadcom.com> > > Reviewed-by: Arun Parameswaran <aparames@broadcom.com> > > Reviewed-by: Scott Branden <sbranden@broadcom.com> > > Signed-off-by: Scott Branden <sbranden@broadcom.com> > > Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> > > --- > > drivers/dma/pl330.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index 0e1f567..631642d 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) > > /* Set to generate interrupts for SEV */ > > writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); > > > > + thrd->req_running = idx; > > + > > /* Only manager can execute GO */ > > _execute_DBGINSN(thrd, insn, true); > > > > - thrd->req_running = idx; > > - > It would help to know what the behavior looks like before and after > the patch. If anything we should look at locking rather the > reordering. Yes that ia fair request, looking at changelog it is hard to understand the issue seen? -- ~Vinod ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. 2015-03-30 17:25 ` Vinod Koul @ 2015-03-31 3:40 ` Scott Branden -1 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-31 3:40 UTC (permalink / raw) To: Vinod Koul, Jassi Brar Cc: Dan Williams, Linux Kernel Mailing List, ismail, Anatol Pomazao, Dmitry Torokhov, bcm-kernel-feedback-list, linux-arm-kernel Hi Vinod, Jassi, Some details on the problem encountered. On 15-03-30 10:25 AM, Vinod Koul wrote: > On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> wrote: >>> From: ismail <ismail@broadcom.com> >>> >>> Update the thread running index before issuing the >>> GO command to the DMAC. >>> >>> Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> >>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>> Signed-off-by: Scott Branden <sbranden@broadcom.com> >>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>> --- >>> drivers/dma/pl330.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>> index 0e1f567..631642d 100644 >>> --- a/drivers/dma/pl330.c >>> +++ b/drivers/dma/pl330.c >>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>> /* Set to generate interrupts for SEV */ >>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>> >>> + thrd->req_running = idx; >>> + >>> /* Only manager can execute GO */ >>> _execute_DBGINSN(thrd, insn, true); >>> >>> - thrd->req_running = idx; >>> - >> It would help to know what the behavior looks like before and after >> the patch. If anything we should look at locking rather the >> reordering. > Yes that ia fair request, looking at changelog it is hard to understand the > issue seen? > We encountered this problem as we modified the driver to make SMC calls to a TZ handler. This slowed down the driver to the point where DMA transactions easily failed. I believe the same could be accomplished by adding a delay between the GOCMD and update of the req_running and running the built in dmatest. The DMA transaction is broken if the interrupt occurs before the thrd->req_running is updated. The pl330 issues a GOCMD (in _trigger function) to start a new transfer. The issue of GOCMD generates an interrupt and the IRQ handler will call the pl330_update function to process the interrupt. The pl330_update function will verify the thread running index and break the transaction, if the thread running index is not set. Regards, Scott ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-31 3:40 ` Scott Branden 0 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-31 3:40 UTC (permalink / raw) To: linux-arm-kernel Hi Vinod, Jassi, Some details on the problem encountered. On 15-03-30 10:25 AM, Vinod Koul wrote: > On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> wrote: >>> From: ismail <ismail@broadcom.com> >>> >>> Update the thread running index before issuing the >>> GO command to the DMAC. >>> >>> Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> >>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>> Signed-off-by: Scott Branden <sbranden@broadcom.com> >>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>> --- >>> drivers/dma/pl330.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>> index 0e1f567..631642d 100644 >>> --- a/drivers/dma/pl330.c >>> +++ b/drivers/dma/pl330.c >>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>> /* Set to generate interrupts for SEV */ >>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>> >>> + thrd->req_running = idx; >>> + >>> /* Only manager can execute GO */ >>> _execute_DBGINSN(thrd, insn, true); >>> >>> - thrd->req_running = idx; >>> - >> It would help to know what the behavior looks like before and after >> the patch. If anything we should look at locking rather the >> reordering. > Yes that ia fair request, looking at changelog it is hard to understand the > issue seen? > We encountered this problem as we modified the driver to make SMC calls to a TZ handler. This slowed down the driver to the point where DMA transactions easily failed. I believe the same could be accomplished by adding a delay between the GOCMD and update of the req_running and running the built in dmatest. The DMA transaction is broken if the interrupt occurs before the thrd->req_running is updated. The pl330 issues a GOCMD (in _trigger function) to start a new transfer. The issue of GOCMD generates an interrupt and the IRQ handler will call the pl330_update function to process the interrupt. The pl330_update function will verify the thread running index and break the transaction, if the thread running index is not set. Regards, Scott ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. 2015-03-31 3:40 ` Scott Branden @ 2015-03-31 5:20 ` Jassi Brar -1 siblings, 0 replies; 14+ messages in thread From: Jassi Brar @ 2015-03-31 5:20 UTC (permalink / raw) To: Scott Branden Cc: Vinod Koul, Dan Williams, Linux Kernel Mailing List, ismail, Anatol Pomazao, Dmitry Torokhov, bcm-kernel-feedback-list, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 2703 bytes --] On Tue, Mar 31, 2015 at 9:10 AM, Scott Branden <sbranden@broadcom.com> wrote: > Hi Vinod, Jassi, > > Some details on the problem encountered. > > > On 15-03-30 10:25 AM, Vinod Koul wrote: >> >> On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >>> >>> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> >>> wrote: >>>> >>>> From: ismail <ismail@broadcom.com> >>>> >>>> Update the thread running index before issuing the >>>> GO command to the DMAC. >>>> >>>> Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>>> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> >>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>> Signed-off-by: Scott Branden <sbranden@broadcom.com> >>>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>> --- >>>> drivers/dma/pl330.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>> index 0e1f567..631642d 100644 >>>> --- a/drivers/dma/pl330.c >>>> +++ b/drivers/dma/pl330.c >>>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>>> /* Set to generate interrupts for SEV */ >>>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>>> >>>> + thrd->req_running = idx; >>>> + >>>> /* Only manager can execute GO */ >>>> _execute_DBGINSN(thrd, insn, true); >>>> >>>> - thrd->req_running = idx; >>>> - >>> >>> It would help to know what the behavior looks like before and after >>> the patch. If anything we should look at locking rather the >>> reordering. >> >> Yes that ia fair request, looking at changelog it is hard to understand >> the >> issue seen? >> > We encountered this problem as we modified the driver to make SMC calls to a > TZ handler. This slowed down the driver to the point where DMA transactions > easily failed. I believe the same could be accomplished by adding a delay > between the GOCMD and update of the req_running and running the built in > dmatest. > > The DMA transaction is broken if the interrupt occurs before the > thrd->req_running is updated. > > The pl330 issues a GOCMD (in _trigger function) to start a new transfer. > > The issue of GOCMD generates an interrupt and the IRQ handler will call the > pl330_update function to process the interrupt. > > The pl330_update function will verify the thread running index and break the > transaction, if the thread running index is not set. > As I suspected the locking seems screwed up. The following patch should fix the race properly. Can you please test the attached patches instead? Thanks. [-- Attachment #2: 0001-dma-pl330-change-busy-marker-for-threads.patch --] [-- Type: text/x-patch, Size: 3065 bytes --] From 1c44fc936d05fef3259354da1574c536ed1691c7 Mon Sep 17 00:00:00 2001 From: Jassi Brar <jaswinder.singh@linaro.org> Date: Tue, 31 Mar 2015 10:16:46 +0530 Subject: [PATCH 1/2] dma: pl330: change busy marker for threads Instead of a boolean flag to mark a thread busy, use the owner of the thread as the marker. For free/available threads, the owner is NULL. This will be useful in finding which channel owns a given thread. Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> --- drivers/dma/pl330.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 0e1f567..d1f777e 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -369,8 +369,7 @@ struct _pl330_tbd { struct pl330_thread { u8 id; int ev; - /* If the channel is not yet acquired by any client */ - bool free; + struct dma_pl330_chan *pch; /* Parent DMAC */ struct pl330_dmac *dmac; /* Only two at a time */ @@ -1648,7 +1647,8 @@ static bool _chan_ns(const struct pl330_dmac *pl330, int i) /* Upon success, returns IdentityToken for the * allocated channel, NULL otherwise. */ -static struct pl330_thread *pl330_request_channel(struct pl330_dmac *pl330) +static struct pl330_thread *pl330_request_channel(struct pl330_dmac *pl330, + struct dma_pl330_chan *pch) { struct pl330_thread *thrd = NULL; unsigned long flags; @@ -1663,11 +1663,11 @@ static struct pl330_thread *pl330_request_channel(struct pl330_dmac *pl330) for (i = 0; i < chans; i++) { thrd = &pl330->channels[i]; - if ((thrd->free) && (!_manager_ns(thrd) || + if (!thrd->pch && (!_manager_ns(thrd) || _chan_ns(pl330, i))) { thrd->ev = _alloc_event(thrd); if (thrd->ev >= 0) { - thrd->free = false; + thrd->pch = pch; thrd->lstenq = 1; thrd->req[0].desc = NULL; thrd->req[1].desc = NULL; @@ -1699,7 +1699,7 @@ static void pl330_release_channel(struct pl330_thread *thrd) struct pl330_dmac *pl330; unsigned long flags; - if (!thrd || thrd->free) + if (!thrd || !thrd->pch) return; _stop(thrd); @@ -1711,7 +1711,7 @@ static void pl330_release_channel(struct pl330_thread *thrd) spin_lock_irqsave(&pl330->lock, flags); _free_event(thrd, thrd->ev); - thrd->free = true; + thrd->pch = NULL; spin_unlock_irqrestore(&pl330->lock, flags); } @@ -1797,14 +1797,14 @@ static int dmac_alloc_threads(struct pl330_dmac *pl330) thrd->id = i; thrd->dmac = pl330; _reset_thread(thrd); - thrd->free = true; + thrd->pch = NULL; } /* MANAGER is indexed at the end */ thrd = &pl330->channels[chans]; thrd->id = chans; thrd->dmac = pl330; - thrd->free = false; + thrd->pch = NULL; pl330->manager = thrd; return 0; @@ -2082,7 +2082,7 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan) dma_cookie_init(chan); pch->cyclic = false; - pch->thread = pl330_request_channel(pl330); + pch->thread = pl330_request_channel(pl330, pch); if (!pch->thread) { spin_unlock_irqrestore(&pch->lock, flags); return -ENOMEM; -- 1.9.1 [-- Attachment #3: 0002-dma-pl330-fix-race-between-trigger-and-completion.patch --] [-- Type: text/x-patch, Size: 1612 bytes --] From 2cd6bf6748f28008a1650dca57a8f14b27283803 Mon Sep 17 00:00:00 2001 From: Jassi Brar <jaswinder.singh@linaro.org> Date: Tue, 31 Mar 2015 10:21:14 +0530 Subject: [PATCH 2/2] dma: pl330: fix race between trigger and completion We need to hold the lock on channel in ISR to prevent it racing against the trigger call on the channel. Reported-by: Scott Branden <sbranden@broadcom.com> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> --- drivers/dma/pl330.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index d1f777e..ce40677 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -1573,6 +1573,7 @@ static int pl330_update(struct pl330_dmac *pl330) if (val & (1 << ev)) { /* Event occurred */ struct pl330_thread *thrd; u32 inten = readl(regs + INTEN); + unsigned long flag; int active; /* Clear the event */ @@ -1584,10 +1585,13 @@ static int pl330_update(struct pl330_dmac *pl330) id = pl330->events[ev]; thrd = &pl330->channels[id]; + spin_lock_irqsave(&thrd->pch->lock, flag); active = thrd->req_running; - if (active == -1) /* Aborted */ + if (active == -1) { /* Aborted */ + spin_unlock_irqrestore(&thrd->pch->lock, flag); continue; + } /* Detach the req */ descdone = thrd->req[active].desc; @@ -1600,6 +1604,7 @@ static int pl330_update(struct pl330_dmac *pl330) /* For now, just make a list of callbacks to be done */ list_add_tail(&descdone->rqd, &pl330->req_done); + spin_unlock_irqrestore(&thrd->pch->lock, flag); } } -- 1.9.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-31 5:20 ` Jassi Brar 0 siblings, 0 replies; 14+ messages in thread From: Jassi Brar @ 2015-03-31 5:20 UTC (permalink / raw) To: linux-arm-kernel On Tue, Mar 31, 2015 at 9:10 AM, Scott Branden <sbranden@broadcom.com> wrote: > Hi Vinod, Jassi, > > Some details on the problem encountered. > > > On 15-03-30 10:25 AM, Vinod Koul wrote: >> >> On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >>> >>> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> >>> wrote: >>>> >>>> From: ismail <ismail@broadcom.com> >>>> >>>> Update the thread running index before issuing the >>>> GO command to the DMAC. >>>> >>>> Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>>> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> >>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>> Signed-off-by: Scott Branden <sbranden@broadcom.com> >>>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>> --- >>>> drivers/dma/pl330.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>> index 0e1f567..631642d 100644 >>>> --- a/drivers/dma/pl330.c >>>> +++ b/drivers/dma/pl330.c >>>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>>> /* Set to generate interrupts for SEV */ >>>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>>> >>>> + thrd->req_running = idx; >>>> + >>>> /* Only manager can execute GO */ >>>> _execute_DBGINSN(thrd, insn, true); >>>> >>>> - thrd->req_running = idx; >>>> - >>> >>> It would help to know what the behavior looks like before and after >>> the patch. If anything we should look at locking rather the >>> reordering. >> >> Yes that ia fair request, looking at changelog it is hard to understand >> the >> issue seen? >> > We encountered this problem as we modified the driver to make SMC calls to a > TZ handler. This slowed down the driver to the point where DMA transactions > easily failed. I believe the same could be accomplished by adding a delay > between the GOCMD and update of the req_running and running the built in > dmatest. > > The DMA transaction is broken if the interrupt occurs before the > thrd->req_running is updated. > > The pl330 issues a GOCMD (in _trigger function) to start a new transfer. > > The issue of GOCMD generates an interrupt and the IRQ handler will call the > pl330_update function to process the interrupt. > > The pl330_update function will verify the thread running index and break the > transaction, if the thread running index is not set. > As I suspected the locking seems screwed up. The following patch should fix the race properly. Can you please test the attached patches instead? Thanks. -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-dma-pl330-change-busy-marker-for-threads.patch Type: text/x-patch Size: 3065 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150331/6f9af47d/attachment-0002.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-dma-pl330-fix-race-between-trigger-and-completion.patch Type: text/x-patch Size: 1612 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150331/6f9af47d/attachment-0003.bin> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. 2015-03-31 5:20 ` Jassi Brar @ 2015-03-31 22:04 ` Scott Branden -1 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-31 22:04 UTC (permalink / raw) To: Jassi Brar Cc: Vinod Koul, Dan Williams, Linux Kernel Mailing List, ismail, Anatol Pomazao, Dmitry Torokhov, bcm-kernel-feedback-list, linux-arm-kernel Hi Jassi, Thanks for taking the time to comment on this patch and provide additional solution. We have went back to reproduce the problem using the dmatest. I am glad you asked for more info as we discovered the problem does not happen in the current code. The problem only happens when we make additional modifications to the existing driver to perform some SMC calls. Somehow the SMC must reenable interrupts without checking the IRQ context. And, looking at the pl330 code further there are spinlock's protecting large chunks of code. You have to trace up a number of functions to find this. As such, this patch is not required with the current codebase. Do you still think the new code you provided is needed to solve another problem? For reference, we run in the 3.10 kernel and modify dmatest.c as follows: --- a/drivers/dma/dmatest.c +++ b/drivers/dma/dmatest.c @@ -615,6 +615,8 @@ static int dmatest_func(void *data) else if (thread->type == DMA_PQ) align = dev->pq_align; + align = 8; + if (1 << align > params->buf_size) { pr_err("%u-byte buffer too small for %d-byte alignment\n", params->buf_size, 1 << align); And then launch dmatest: insmod /tmp/dmatest.ko echo 10 > /sys/kernel/debug/dmatest/iterations echo 1 > /sys/kernel/debug/dmatest/max_channels echo 1 > /sys/kernel/debug/dmatest/run Next, in pl330.c add an mdelay in the _trigger function. /* Only manager can execute GO */ _execute_DBGINSN(thrd, insn, true); + mdelay(1000); thrd->req_running = idx; On 15-03-30 10:20 PM, Jassi Brar wrote: > On Tue, Mar 31, 2015 at 9:10 AM, Scott Branden <sbranden@broadcom.com> wrote: >> Hi Vinod, Jassi, >> >> Some details on the problem encountered. >> >> >> On 15-03-30 10:25 AM, Vinod Koul wrote: >>> >>> On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >>>> >>>> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> >>>> wrote: >>>>> >>>>> From: ismail <ismail@broadcom.com> >>>>> >>>>> Update the thread running index before issuing the >>>>> GO command to the DMAC. >>>>> >>>>> Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>>>> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> >>>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>>> Signed-off-by: Scott Branden <sbranden@broadcom.com> >>>>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>>> --- >>>>> drivers/dma/pl330.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>>> index 0e1f567..631642d 100644 >>>>> --- a/drivers/dma/pl330.c >>>>> +++ b/drivers/dma/pl330.c >>>>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>>>> /* Set to generate interrupts for SEV */ >>>>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>>>> >>>>> + thrd->req_running = idx; >>>>> + >>>>> /* Only manager can execute GO */ >>>>> _execute_DBGINSN(thrd, insn, true); >>>>> >>>>> - thrd->req_running = idx; >>>>> - >>>> >>>> It would help to know what the behavior looks like before and after >>>> the patch. If anything we should look at locking rather the >>>> reordering. >>> >>> Yes that ia fair request, looking at changelog it is hard to understand >>> the >>> issue seen? >>> >> We encountered this problem as we modified the driver to make SMC calls to a >> TZ handler. This slowed down the driver to the point where DMA transactions >> easily failed. I believe the same could be accomplished by adding a delay >> between the GOCMD and update of the req_running and running the built in >> dmatest. >> >> The DMA transaction is broken if the interrupt occurs before the >> thrd->req_running is updated. >> >> The pl330 issues a GOCMD (in _trigger function) to start a new transfer. >> >> The issue of GOCMD generates an interrupt and the IRQ handler will call the >> pl330_update function to process the interrupt. >> >> The pl330_update function will verify the thread running index and break the >> transaction, if the thread running index is not set. >> > As I suspected the locking seems screwed up. The following patch > should fix the race properly. Can you please test the attached patches > instead? > > Thanks. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. @ 2015-03-31 22:04 ` Scott Branden 0 siblings, 0 replies; 14+ messages in thread From: Scott Branden @ 2015-03-31 22:04 UTC (permalink / raw) To: linux-arm-kernel Hi Jassi, Thanks for taking the time to comment on this patch and provide additional solution. We have went back to reproduce the problem using the dmatest. I am glad you asked for more info as we discovered the problem does not happen in the current code. The problem only happens when we make additional modifications to the existing driver to perform some SMC calls. Somehow the SMC must reenable interrupts without checking the IRQ context. And, looking at the pl330 code further there are spinlock's protecting large chunks of code. You have to trace up a number of functions to find this. As such, this patch is not required with the current codebase. Do you still think the new code you provided is needed to solve another problem? For reference, we run in the 3.10 kernel and modify dmatest.c as follows: --- a/drivers/dma/dmatest.c +++ b/drivers/dma/dmatest.c @@ -615,6 +615,8 @@ static int dmatest_func(void *data) else if (thread->type == DMA_PQ) align = dev->pq_align; + align = 8; + if (1 << align > params->buf_size) { pr_err("%u-byte buffer too small for %d-byte alignment\n", params->buf_size, 1 << align); And then launch dmatest: insmod /tmp/dmatest.ko echo 10 > /sys/kernel/debug/dmatest/iterations echo 1 > /sys/kernel/debug/dmatest/max_channels echo 1 > /sys/kernel/debug/dmatest/run Next, in pl330.c add an mdelay in the _trigger function. /* Only manager can execute GO */ _execute_DBGINSN(thrd, insn, true); + mdelay(1000); thrd->req_running = idx; On 15-03-30 10:20 PM, Jassi Brar wrote: > On Tue, Mar 31, 2015 at 9:10 AM, Scott Branden <sbranden@broadcom.com> wrote: >> Hi Vinod, Jassi, >> >> Some details on the problem encountered. >> >> >> On 15-03-30 10:25 AM, Vinod Koul wrote: >>> >>> On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >>>> >>>> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden <sbranden@broadcom.com> >>>> wrote: >>>>> >>>>> From: ismail <ismail@broadcom.com> >>>>> >>>>> Update the thread running index before issuing the >>>>> GO command to the DMAC. >>>>> >>>>> Tested-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>>>> Reviewed-by: Arun Parameswaran <aparames@broadcom.com> >>>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>>> Signed-off-by: Scott Branden <sbranden@broadcom.com> >>>>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed <ismail@broadcom.com> >>>>> --- >>>>> drivers/dma/pl330.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>>> index 0e1f567..631642d 100644 >>>>> --- a/drivers/dma/pl330.c >>>>> +++ b/drivers/dma/pl330.c >>>>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>>>> /* Set to generate interrupts for SEV */ >>>>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>>>> >>>>> + thrd->req_running = idx; >>>>> + >>>>> /* Only manager can execute GO */ >>>>> _execute_DBGINSN(thrd, insn, true); >>>>> >>>>> - thrd->req_running = idx; >>>>> - >>>> >>>> It would help to know what the behavior looks like before and after >>>> the patch. If anything we should look at locking rather the >>>> reordering. >>> >>> Yes that ia fair request, looking at changelog it is hard to understand >>> the >>> issue seen? >>> >> We encountered this problem as we modified the driver to make SMC calls to a >> TZ handler. This slowed down the driver to the point where DMA transactions >> easily failed. I believe the same could be accomplished by adding a delay >> between the GOCMD and update of the req_running and running the built in >> dmatest. >> >> The DMA transaction is broken if the interrupt occurs before the >> thrd->req_running is updated. >> >> The pl330 issues a GOCMD (in _trigger function) to start a new transfer. >> >> The issue of GOCMD generates an interrupt and the IRQ handler will call the >> pl330_update function to process the interrupt. >> >> The pl330_update function will verify the thread running index and break the >> transaction, if the thread running index is not set. >> > As I suspected the locking seems screwed up. The following patch > should fix the race properly. Can you please test the attached patches > instead? > > Thanks. > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-03-31 22:04 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-26 23:55 [PATCH] dmaengine: pl330: fix the race condition in pl330 driver Scott Branden 2015-03-26 23:55 ` Scott Branden 2015-03-29 21:55 ` Scott Branden 2015-03-29 21:55 ` Scott Branden 2015-03-30 16:47 ` Jassi Brar 2015-03-30 16:47 ` Jassi Brar 2015-03-30 17:25 ` Vinod Koul 2015-03-30 17:25 ` Vinod Koul 2015-03-31 3:40 ` Scott Branden 2015-03-31 3:40 ` Scott Branden 2015-03-31 5:20 ` Jassi Brar 2015-03-31 5:20 ` Jassi Brar 2015-03-31 22:04 ` Scott Branden 2015-03-31 22:04 ` Scott Branden
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.