On Tue, 01 Jun 2021 15:01:08 +0200, Michael Tretter wrote:
> I get a lockdep warning in the zynqmp dma driver and I am not entirely sure
> how to fix it.
>
> The code in drivers/dma/xilinx/zynqmp_dma.c looks as follows:
>
> 604 static void zynqmp_dma_chan_desc_cleanup(struct zynqmp_dma_chan *chan)
> 605 {
> [...]
> 612 callback = desc->async_tx.callback;
> 613 callback_param = desc->async_tx.callback_param;
> 614 if (callback) {
> 615 spin_unlock(&chan->lock);
> 616 callback(callback_param);
> 617 spin_lock(&chan->lock);
> 618 }
> [...]
> 626 }
> [...]
> 747 static void zynqmp_dma_do_tasklet(struct tasklet_struct *t)
> 748 {
> [...]
> 753 spin_lock_irqsave(&chan->lock, irqflags);
> [...]
> 763 while (count) {
> 764 zynqmp_dma_complete_descriptor(chan);
> 765 zynqmp_dma_chan_desc_cleanup(chan);
> 766 count--;
> 767 }
> [...]
> 773 spin_unlock_irqrestore(&chan->lock, irqflags);
> 774 }
>
> Lockdep reports that in line 617 spin_lock() is called from a non-hardirq
> context, while the same lock is used from a hardirq context. During runtime,
> the sequence is as follows:
>
> line 753: acquire lock and disable interrupts
> line 615: release lock without enabling interrupts
> line 617: re-acquire lock with still disabled interrupts
> line 773: released lock and re-enable interrupts
>
> Is this a false positive of lockdep, because it does not know that the irqs
> are still disabled in line 617? Is it actually OK to leave interrupts disabled
> over a spin_unlock() -> spin_lock() sequence or is this a problem?
>
> Additionally, the lock is held for the entire tasklet that handles the
> finished dma transfer. This is conflict to the rule that spin locks should be
> held only for a short time. Is it necessary to hold the lock that long? I
> understand that the lock is only used to protect the descriptor lists and it
> would be better to only get the lock when descriptors are moved between lists.
>
> Any guidance would be helpful.
Gentle ping.
Michael
