From patchwork Wed Feb 24 13:14:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Jon Medhurst (Tixy)" X-Patchwork-Id: 8406021 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 496C79F52D for ; Wed, 24 Feb 2016 13:17:36 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 6DB7F202E9 for ; Wed, 24 Feb 2016 13:17:35 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6F8D7202AE for ; Wed, 24 Feb 2016 13:17:34 +0000 (UTC) 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 1aYZHZ-0001PA-HA; Wed, 24 Feb 2016 13:15:09 +0000 Received: from smarthost01a.mail.zen.net.uk ([212.23.1.1]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aYZHU-0000JW-Nx for linux-arm-kernel@lists.infradead.org; Wed, 24 Feb 2016 13:15:06 +0000 Received: from [82.69.122.217] (helo=[192.168.2.110]) by smarthost01a.mail.zen.net.uk with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1aYZH0-000Axv-TA; Wed, 24 Feb 2016 13:14:35 +0000 Message-ID: <1456319674.2867.15.camel@linaro.org> Subject: [PATCH] dmaengine: pl330: Fix some race conditions in residue calculation From: "Jon Medhurst (Tixy)" To: Vinod Koul , Robert Baldyga Date: Wed, 24 Feb 2016 13:14:34 +0000 X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 X-Originating-smarthost01a-IP: [82.69.122.217] X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160224_051504_977527_A378D9D6 X-CRM114-Status: GOOD ( 17.28 ) X-Spam-Score: -2.0 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jaswinder Singh , linux-kernel@vger.kernel.org, Lukasz Czerwinski , dmaengine@vger.kernel.org, Dan Williams , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP The residue calculation in pl330_tx_status doesn't handle transitional states that occur at the time one descriptor (A) is completed and the next (B) is started. Specifically, both A and B can simultaneously be in the BUSY state and at this time the thread's 'req_running' may (or may not) be -1. To cope with this situation we change the code to ensure A is treated as complete and B as having not yet started. Prior to the change, the code would calculate a transferred byte count as if both A and B had completed. Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function") Signed-off-by: Jon Medhurst --- I discovered this issue when trying to work out why audio stopped working on ARM's Juno platform and bisected it to commit aee4d1fac887. Whilst this patch seems to fix the problems I was seeing, I can't help but think there are more race conditions with this code. E.g. if the running descriptor changes under us, pl330_get_current_xferred_count can end up reading values from hardware that relate to a different descriptor. And if we're really unlucky, the reading of the 'val' and 'addr' values in pl330_get_current_xferred_count can come from different descriptors. I don't know if there is any locks we can use to prevent such races or if we need to try and detect when things have changed and redo/abort the residue calculation... drivers/dma/pl330.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 17ee758..55e3c5f 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -2240,6 +2240,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, struct dma_pl330_desc *desc, *running = NULL; struct dma_pl330_chan *pch = to_pchan(chan); unsigned int transferred, residual = 0; + bool first_busy; ret = dma_cookie_status(chan, cookie, txstate); @@ -2253,16 +2254,31 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, if (pch->thread->req_running != -1) running = pch->thread->req[pch->thread->req_running].desc; + first_busy = true; /* Check in pending list */ list_for_each_entry(desc, &pch->work_list, node) { if (desc->status == DONE) transferred = desc->bytes_requested; - else if (running && desc == running) - transferred = - pl330_get_current_xferred_count(pch, desc); - else + else if (desc->status == BUSY && first_busy) { + first_busy = false; + if (running && desc == running) { + transferred = + pl330_get_current_xferred_count(pch, desc); + } else { + /* BUSY but not running means it's just completed */ + transferred = desc->bytes_requested; + } + } else { + /* + * Descriptor is either in PREP state queued for future + * transfer or it is the second BUSY descriptor we have + * seen. The latter case means it has just, or is about + * to be, started, so treat it as having not yet + * transferred any bytes, the same as PREP. + */ transferred = 0; + } residual += desc->bytes_requested - transferred; if (desc->txd.cookie == cookie) { switch (desc->status) {