Re: [RFC] dmaengine: pl330: fix a race condition in case of threaded irqs

[qhou]

Hi, Jassi Brar

On 2018å01æ09æ 16:29, Jassi Brar wrote:
> On Mon, Dec 25, 2017 at 7:50 AM, Qi Hou <qi.hou@xxxxxxxxxxxxx> 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