From patchwork Tue Mar 31 05:20:27 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jassi Brar X-Patchwork-Id: 456491 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2001:1868:205::9]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4F69F1400B6 for ; Tue, 31 Mar 2015 16:23:32 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="verification failed; unprotected key" header.d=gmail.com header.i=@gmail.com header.b=XaIrnLKl; dkim-adsp=none (unprotected policy); dkim-atps=neutral Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Ycobd-0005HV-Af; Tue, 31 Mar 2015 05:20:53 +0000 Received: from mail-ie0-x22a.google.com ([2607:f8b0:4001:c03::22a]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1YcobZ-0005Bp-3E for linux-arm-kernel@lists.infradead.org; Tue, 31 Mar 2015 05:20:50 +0000 Received: by ierf6 with SMTP id f6so7251482ier.2 for ; Mon, 30 Mar 2015 22:20:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=zIiU5MsE6dMhKSSX5MTEP7hNM4EUSvoG3iWhe/gVWr4=; b=XaIrnLKl/JR414IJoftlVIeN8+n3zj2s39lpAfTtmB7xj0Czdx/4B85oMlLi1QSwfH /qFeJWNkzPEdfyMvXV6n8FLrNKotVV7gJkkSF9IpLafo/8yGPaV4UsKTJ7vLY+6j6+n5 79CG0LkFEXesi0HXrRrN7pYBRjmmx/4O2nWjdUcDi7jG9s5V94Xd8Bs/gHZaQbGvQRHp NNGdq4llOJnCz7vj0LJBgafG7wWASJMZJz7i1EvDNOKKQnwFAFsUycBLl9ziNo7hn5ea UanL409DSW9FHkMa6LE2sEywjNv2GohuZx7ODcqwP5kc9OCNqerMsI0WoQdq2Rj1NAg+ Ul3A== MIME-Version: 1.0 X-Received: by 10.42.30.139 with SMTP id v11mr62135240icc.76.1427779227383; Mon, 30 Mar 2015 22:20:27 -0700 (PDT) Received: by 10.64.135.225 with HTTP; Mon, 30 Mar 2015 22:20:27 -0700 (PDT) In-Reply-To: <551A173B.4010704@broadcom.com> References: <1427414105-3480-1-git-send-email-sbranden@broadcom.com> <20150330172546.GI7192@intel.com> <551A173B.4010704@broadcom.com> Date: Tue, 31 Mar 2015 10:50:27 +0530 Message-ID: Subject: Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. From: Jassi Brar To: Scott Branden X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150330_222049_256382_84702E13 X-CRM114-Status: GOOD ( 37.86 ) X-Spam-Score: -0.8 (/) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-0.8 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2607:f8b0:4001:c03:0:0:0:22a listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (jassisinghbrar[at]gmail.com) -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid Cc: Vinod Koul , Linux Kernel Mailing List , ismail , Anatol Pomazao , "linux-arm-kernel@lists.infradead.org" , bcm-kernel-feedback-list@broadcom.com, Dan Williams , Dmitry Torokhov X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org On Tue, Mar 31, 2015 at 9:10 AM, Scott Branden 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 >>> wrote: >>>> >>>> From: ismail >>>> >>>> Update the thread running index before issuing the >>>> GO command to the DMAC. >>>> >>>> Tested-by: Mohamed Ismail Abdul Packir Mohamed >>>> Reviewed-by: Ray Jui >>>> Reviewed-by: Arun Parameswaran >>>> Reviewed-by: Scott Branden >>>> Signed-off-by: Scott Branden >>>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed >>>> --- >>>> 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. From 2cd6bf6748f28008a1650dca57a8f14b27283803 Mon Sep 17 00:00:00 2001 From: Jassi Brar 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 Signed-off-by: Jassi Brar --- 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