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: 62793 Delivered-To: patch@linaro.org Received: by 10.112.43.199 with SMTP id y7csp2396748lbl; Wed, 24 Feb 2016 05:15:22 -0800 (PST) X-Received: by 10.66.141.71 with SMTP id rm7mr54624694pab.106.1456319722366; Wed, 24 Feb 2016 05:15:22 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id u69si1758908pfa.253.2016.02.24.05.15.22; Wed, 24 Feb 2016 05:15:22 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754249AbcBXNPS (ORCPT + 30 others); Wed, 24 Feb 2016 08:15:18 -0500 Received: from queue01c.mail.zen.net.uk ([212.23.3.237]:60759 "EHLO queue01c.mail.zen.net.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751103AbcBXNPQ (ORCPT ); Wed, 24 Feb 2016 08:15:16 -0500 Received: from [212.23.1.1] (helo=smarthost01a.mail.zen.net.uk) by queue01c.mail.zen.net.uk with esmtp (Exim 4.72) (envelope-from ) id 1aYZHd-0007hc-J2; Wed, 24 Feb 2016 13:15:13 +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 Cc: Lukasz Czerwinski , Dan Williams , Jaswinder Singh , dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org 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] Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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(-) -- 2.1.4 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) {