From patchwork Tue Apr 26 16:30:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Robin Murphy X-Patchwork-Id: 8940861 Return-Path: X-Original-To: patchwork-dmaengine@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 84BCD9F1D3 for ; Tue, 26 Apr 2016 16:30:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 7EA2620121 for ; Tue, 26 Apr 2016 16:30:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4022A2010C for ; Tue, 26 Apr 2016 16:30:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752207AbcDZQau (ORCPT ); Tue, 26 Apr 2016 12:30:50 -0400 Received: from foss.arm.com ([217.140.101.70]:56149 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751953AbcDZQat (ORCPT ); Tue, 26 Apr 2016 12:30:49 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3E63F3A; Tue, 26 Apr 2016 09:29:29 -0700 (PDT) Received: from e104324-lin.cambridge.arm.com (e104324-lin.cambridge.arm.com [10.1.205.154]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 83E693F42B; Tue, 26 Apr 2016 09:30:47 -0700 (PDT) From: Robin Murphy To: vinod.koul@intel.com, dan.j.williams@intel.com Cc: jassisinghbrar@gmail.com, dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: [PATCH] dmaengine: pl330: Fix race in pl330_get_desc() Date: Tue, 26 Apr 2016 17:30:37 +0100 Message-Id: X-Mailer: git-send-email 2.8.1.dirty Sender: dmaengine-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: dmaengine@vger.kernel.org X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, 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 current logic in pl330_get_desc() contains a clear race condition, whereby if the descriptor pool is empty, we will create a new descriptor, add it to the pool with the lock held, *release the lock*, then try to remove it from the pool again. Furthermore, if that second attempt then finds the pool empty because someone else got there first, we conclude that something must have gone terribly wrong and it's the absolute end of the world. In practice, however, this can be hit relatively easily by dmatest with a suitably aggressive number of threads and channels on a multi-core system, and the fallout is a veritable storm of this sort of thing: ... [ 709.328891] dma-pl330 7ff00000.dma: pl330_get_desc:2459 ALERT! ** 10 printk messages dropped ** [ 709.352280] dma-pl330 7ff00000.dma: __pl330_prep_dma_memcpy:2493 Unable to fetch desc ** 11 printk messages dropped ** [ 709.372930] dma-pl330 7ff00000.dma: pl330_get_desc:2459 ALERT! ** 2 printk messages dropped ** [ 709.372953] dma-pl330 7ff00000.dma: pl330_get_desc:2459 ALERT! ** 8 printk messages dropped ** [ 709.374157] dma-pl330 7ff00000.dma: __pl330_prep_dma_memcpy:2493 Unable to fetch desc ** 41 printk messages dropped ** [ 709.393095] dmatest: dma0chan4-copy1: result #4545: 'prep error' with src_off=0x3a32 dst_off=0x46dd len=0xc5c3 (0) ... Down with this sort of thing! The most sensible thing to do is avoid the allocate-add-remove dance entirely and simply use the freshly-allocated descriptor straight away. We can then further simplify the code and save unnecessary work by also inlining the initial pool allocation and removing add_desc() entirely. Signed-off-by: Robin Murphy --- I'm also seeing what looks like another occasional race under the same conditions where pl330_tx_submit() blows up from dma_cookie_assign() dereferencing a bogus tx->chan, but I think that's beyond my ability to figure out right now. Similarly the storm of WARNs from pl330_issue_pending() when using a large number of small buffers and dmatest.noverify=1. This one was some obvious low-hanging fruit. Robin. drivers/dma/pl330.c | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 372b4359da97..a196adb358f1 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -2393,29 +2393,6 @@ static inline void _init_desc(struct dma_pl330_desc *desc) INIT_LIST_HEAD(&desc->node); } -/* Returns the number of descriptors added to the DMAC pool */ -static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count) -{ - struct dma_pl330_desc *desc; - unsigned long flags; - int i; - - desc = kcalloc(count, sizeof(*desc), flg); - if (!desc) - return 0; - - spin_lock_irqsave(&pl330->pool_lock, flags); - - for (i = 0; i < count; i++) { - _init_desc(&desc[i]); - list_add_tail(&desc[i].node, &pl330->desc_pool); - } - - spin_unlock_irqrestore(&pl330->pool_lock, flags); - - return count; -} - static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330) { struct dma_pl330_desc *desc = NULL; @@ -2428,9 +2405,6 @@ static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330) struct dma_pl330_desc, node); list_del_init(&desc->node); - - desc->status = PREP; - desc->txd.callback = NULL; } spin_unlock_irqrestore(&pl330->pool_lock, flags); @@ -2449,19 +2423,18 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch) /* If the DMAC pool is empty, alloc new */ if (!desc) { - if (!add_desc(pl330, GFP_ATOMIC, 1)) - return NULL; - - /* Try again */ - desc = pluck_desc(pl330); + desc = kzalloc(sizeof(*desc), GFP_ATOMIC); if (!desc) { dev_err(pch->dmac->ddma.dev, "%s:%d ALERT!\n", __func__, __LINE__); return NULL; } + _init_desc(desc); } /* Initialize the descriptor */ + desc->status = PREP; + desc->txd.callback = NULL; desc->pchan = pch; desc->txd.cookie = 0; async_tx_ack(&desc->txd); @@ -2814,6 +2787,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) struct pl330_config *pcfg; struct pl330_dmac *pl330; struct dma_pl330_chan *pch, *_p; + struct dma_pl330_desc *desc; struct dma_device *pd; struct resource *res; int i, ret, irq; @@ -2874,8 +2848,15 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) spin_lock_init(&pl330->pool_lock); /* Create a descriptor pool of default size */ - if (!add_desc(pl330, GFP_KERNEL, NR_DEFAULT_DESC)) + desc = kcalloc(NR_DEFAULT_DESC, sizeof(*desc), GFP_KERNEL); + if (desc) { + for (i = 0; i < NR_DEFAULT_DESC; i++) { + _init_desc(&desc[i]); + list_add_tail(&desc[i].node, &pl330->desc_pool); + } + } else { dev_warn(&adev->dev, "unable to allocate desc\n"); + } INIT_LIST_HEAD(&pd->channels);