From: Michael Tretter <m.tretter@pengutronix.de>
To: dmaengine@vger.kernel.org, vkoul@kernel.org
Cc: michal.simek@xilinx.com, appanad@xilinx.com,
linux-arm-kernel@lists.infradead.org
Subject: Re: dmaengine: zynqmp_dma: lockdep warning
Date: Thu, 24 Jun 2021 17:36:04 +0200 [thread overview]
Message-ID: <20210624153604.GA24339@pengutronix.de> (raw)
In-Reply-To: <20210601130108.GA12967@pengutronix.de>
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
WARNING: multiple messages have this Message-ID (diff)
From: Michael Tretter <m.tretter@pengutronix.de>
To: dmaengine@vger.kernel.org, vkoul@kernel.org
Cc: michal.simek@xilinx.com, appanad@xilinx.com,
linux-arm-kernel@lists.infradead.org
Subject: Re: dmaengine: zynqmp_dma: lockdep warning
Date: Thu, 24 Jun 2021 17:36:04 +0200 [thread overview]
Message-ID: <20210624153604.GA24339@pengutronix.de> (raw)
In-Reply-To: <20210601130108.GA12967@pengutronix.de>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-24 15:36 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-01 13:01 dmaengine: zynqmp_dma: lockdep warning Michael Tretter
2021-06-01 13:01 ` Michael Tretter
2021-06-24 15:36 ` Michael Tretter [this message]
2021-06-24 15:36 ` Michael Tretter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210624153604.GA24339@pengutronix.de \
--to=m.tretter@pengutronix.de \
--cc=appanad@xilinx.com \
--cc=dmaengine@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=michal.simek@xilinx.com \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.