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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
