From patchwork Mon Sep 19 18:07:07 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jassi Brar X-Patchwork-Id: 115380 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id BA014B70B7 for ; Tue, 20 Sep 2011 04:07:20 +1000 (EST) Received: from canuck.infradead.org ([2001:4978:20e::1]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1R5iFN-0008BE-OT; Mon, 19 Sep 2011 18:07:13 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1R5iFN-0001yX-Ab; Mon, 19 Sep 2011 18:07:13 +0000 Received: from mail-bw0-f49.google.com ([209.85.214.49]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1R5iFJ-0001yE-T5 for linux-arm-kernel@lists.infradead.org; Mon, 19 Sep 2011 18:07:11 +0000 Received: by bkat2 with SMTP id t2so6266748bka.36 for ; Mon, 19 Sep 2011 11:07:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=gR5qgyRPF6FroizZBE4Gzb78B7fgye+VhEEf/RUVNFI=; b=ONqCxKhxT/8DCJZ2kOW57rJQNyT6vgY9F5b478A8bYqv7hsN/X/lYuE7RqnFzzhaKm eXxZiKZGw/0I2ukPdYbbDYotiAJ1Cd4fXxmeUTLByXU4wbiKvydncxqFBQVMd8/CrU2b SmsHzHrsiClesghLs+ui+H4hhT0U6Yf6jNqBk= MIME-Version: 1.0 Received: by 10.204.146.90 with SMTP id g26mr1860971bkv.301.1316455627495; Mon, 19 Sep 2011 11:07:07 -0700 (PDT) Received: by 10.204.122.19 with HTTP; Mon, 19 Sep 2011 11:07:07 -0700 (PDT) In-Reply-To: <1316452291-19632-1-git-send-email-javi.merino@arm.com> References: <1316452291-19632-1-git-send-email-javi.merino@arm.com> Date: Mon, 19 Sep 2011 23:37:07 +0530 Message-ID: Subject: Re: [PATCH] ARM: pl330: Fix a race condition From: Jassi Brar To: Javi Merino X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110919_140710_276508_F14EF702 X-CRM114-Status: GOOD ( 24.58 ) X-Spam-Score: -0.8 (/) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (-0.8 points) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (jassisinghbrar[at]gmail.com) -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.214.49 listed in list.dnswl.org] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: Jassi Brar , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org On Mon, Sep 19, 2011 at 10:41 PM, Javi Merino wrote: > If two requests have been submitted and one of them is running, if you > call pl330_chan_ctrl(ch_id, PL330_OP_START), there's a window of time > between the spin_lock_irqsave() and the _state() check in which the > running transaction may finish.  In that case, we don't receive the > interrupt (because they are disabled), but _start() sees that the DMA > is stopped, so it starts it.  The problem is that it sends the > transaction that has just finished again, because pl330_update() > hasn't mark it as done yet. > > This patch moves the _state() check out of the critical section, which > removes the race condition.  It also treats PL330_STATE_COMPLETING as > still executing, because that introduces another race condition now > that we call _state() with interrupts enabled.  Namely, if we read the > state as "completing" and the DMA sends the interrupt before we > disable interrupts, pl330_update() starts the next transaction and > returns.  Then the _start() in pl330_chan_ctrl() will patiently wait > until the just issued transaction finishes (because the state we read > was PL330_STATE_COMPLETING) and when it does, it _trigger()s the same > transaction again. > > Signed-off-by: Javi Merino > Cc: Jassi Brar > --- >  arch/arm/common/pl330.c |   12 +++++++----- >  1 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c > index 97912fa..26b5615 100644 > --- a/arch/arm/common/pl330.c > +++ b/arch/arm/common/pl330.c > @@ -936,9 +936,9 @@ static bool _trigger(struct pl330_thread *thrd) >        return true; >  } > > -static bool _start(struct pl330_thread *thrd) > +static bool _start(struct pl330_thread *thrd, u32 state) >  { > -       switch (_state(thrd)) { > +       switch (state) { >        case PL330_STATE_FAULT_COMPLETING: >                UNTIL(thrd, PL330_STATE_FAULTING | PL330_STATE_KILLING); > > @@ -949,7 +949,6 @@ static bool _start(struct pl330_thread *thrd) >                _stop(thrd); > >        case PL330_STATE_KILLING: > -       case PL330_STATE_COMPLETING: >                UNTIL(thrd, PL330_STATE_STOPPED) > >        case PL330_STATE_STOPPED: > @@ -961,6 +960,7 @@ static bool _start(struct pl330_thread *thrd) >        case PL330_STATE_UPDTPC: >        case PL330_STATE_CACHEMISS: >        case PL330_STATE_EXECUTING: > +       case PL330_STATE_COMPLETING: >                return true; > >        case PL330_STATE_WFE: /* For RESUME, nothing yet */ > @@ -1471,7 +1471,7 @@ int pl330_update(const struct pl330_info *pi) >                        MARK_FREE(rqdone); > >                        /* Get going again ASAP */ > -                       _start(thrd); > +                       _start(thrd, _state(thrd)); > >                        /* For now, just make a list of callbacks to be done */ >                        list_add_tail(&rqdone->rqd, &pl330->req_done); > @@ -1510,12 +1510,14 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) >        struct pl330_dmac *pl330; >        unsigned long flags; >        int ret = 0, active; > +       u32 dma_state; > >        if (!thrd || thrd->free || thrd->dmac->state == DYING) >                return -EINVAL; > >        pl330 = thrd->dmac; > > +       dma_state = _state(thrd); >        spin_lock_irqsave(&pl330->lock, flags); > >        switch (op) { > @@ -1546,7 +1548,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) > >                /* Start the next */ >        case PL330_OP_START: > -               if (!_start(thrd)) > +               if (!_start(thrd, dma_state)) >                        ret = -EIO; >                break; > > -- IIUIC your race scenario should be taken care of simply by doing... Could you please test if it fixes the issue? Thanks. Acked-by: Jassi Brar diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index 97912fa..7129cfb 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) /* Start the next */ case PL330_OP_START: - if (!_start(thrd)) + if (!_thrd_active(thrd) && !_start(thrd)) ret = -EIO; break;