From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754149AbeAIKMO (ORCPT + 1 other); Tue, 9 Jan 2018 05:12:14 -0500 Received: from mail5.windriver.com ([192.103.53.11]:51406 "EHLO mail5.wrs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751518AbeAIKMJ (ORCPT ); Tue, 9 Jan 2018 05:12:09 -0500 Subject: Re: [RFC] dmaengine: pl330: fix a race condition in case of threaded irqs To: Jassi Brar CC: Vinod Koul , Dan Williams , , , Russell King , , Linux Kernel Mailing List References: <1514168416-4285-1-git-send-email-qi.hou@windriver.com> From: qhou Message-ID: <4da55eeb-3a81-b5a0-b8c2-73f657caaf9e@windriver.com> Date: Tue, 9 Jan 2018 18:12:09 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Originating-IP: [128.224.162.179] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi, Jassi Brar On 2018年01月09日 16:29, Jassi Brar wrote: > On Mon, Dec 25, 2017 at 7:50 AM, Qi Hou wrote: > > I found this problem below, and I now understand why it happens, but I'm not > > 100% sure what is the best way to fix it. > > > > When booting up with "threadirqs" in command line, all irq handlers of the DMA > > controller pl330 will be threaded forcedly. These threads will race for the same > > list, pl330->req_done. > > > > Before the callback, the spinlock was released. And after it, the spinlock was > > taken. This opened an race window where another threaded irq handler could steal > > the spinlock and be permitted to delete entries of the list, pl330->req_done. > > > The locking has been recently modified beyond recognition, so I can't > tell why that part of code is the way it is. > The safest and cleanest solution seems to be to not drop and re-aquire the lock. The terrible thing is that the lock can not be held all the time during doing the callbacks. If that, there will be a hidden deadlock. As to irq handler of pl330, get pl330->lock first, then pch->lock. As to tasklet of PL330, pch->lockfirst, then pl330->lock. It have been verified when I enabled the kernel lock debug option. And after tracing the changelogs of the driver of the DMA controller PL330, I have to stay back. The pair of unlock/lock staffs is there since the original commit.  It seems that the author intent to do things like that. If it's hard to trace the author's intention, maybe the advised fix below could be taken, since there is no side effect.  And it achieves the same goal as the original wants to. diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index d7327fd..de1fd59 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -1510,7 +1510,7 @@ static void pl330_dotask(unsigned long data)  /* Returns 1 if state was updated, 0 otherwise */  static int pl330_update(struct pl330_dmac *pl330)  { -       struct dma_pl330_desc *descdone, *tmp; +       struct dma_pl330_desc *descdone;         unsigned long flags;         void __iomem *regs;         u32 val; @@ -1588,7 +1588,9 @@ static int pl330_update(struct pl330_dmac *pl330)         }         /* Now that we are in no hurry, do the callbacks */ -       list_for_each_entry_safe(descdone, tmp, &pl330->req_done, rqd) { +       while (!list_empty(&pl330->req_done)) { +               descdone = list_first_entry(&pl330->req_done, +                                           struct dma_pl330_desc, rqd);                 list_del(&descdone->rqd);                 spin_unlock_irqrestore(&pl330->lock, flags);                 dma_pl330_rqcb(descdone, PL330_ERR_NONE); -- best regards, Qi Hou > > Cheers! -- Best regards, Qi Hou Phone number: +86-10-8477-8608 Address: Floor 15, Building B, Wangjing Plaza, No.9 Zhong-Huan Nanlu, Chaoyang District