From: Frank Li <Frank.li@oss.nxp.com>
To: James Hilliard <james.hilliard1@gmail.com>
Cc: Vinod Koul <vkoul@kernel.org>, Frank Li <Frank.Li@kernel.org>,
Chen-Yu Tsai <wens@kernel.org>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Samuel Holland <samuel@sholland.org>,
Arnd Bergmann <arnd@arndb.de>, Maxime Ripard <mripard@kernel.org>,
Jens Kuske <jenskuske@gmail.com>,
Jean-Francois Moine <moinejf@free.fr>,
Hongling Zeng <zenghongling@kylinos.cn>,
Chengfeng Ye <cyeaa@connect.ust.hk>,
Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog>,
dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
Sashiko <sashiko-bot@kernel.org>
Subject: Re: [PATCH v3 4/9] dmaengine: sun6i: serialize physical channel ownership
Date: Mon, 26 Oct 2026 16:14:13 -0500 [thread overview]
Message-ID: <at_CpZrmQ986UEqv@SMW015318> (raw)
In-Reply-To: <20261023-submit-sun6i-dma-lifetime-next-v1-v3-4-ef1acb0be768@gmail.com>
On Fri, Oct 23, 2026 at 05:38:23PM -0600, James Hilliard wrote:
> The allocator changes vchan->phy and pchan->vchan under sdev->lock, but
> termination reads the physical channel before taking either lock. The IRQ
> handler also takes an unprotected owner snapshot. Termination can miss a
> new assignment or detach an owner while an interrupt is about to complete
> its descriptor. A sampled IRQ can also be applied after reassignment.
>
> Protect physical-channel ownership with sdev->lock across allocation,
> interrupt handling, pause, termination and residue queries. Read and
> acknowledge IRQ status under that lock, and allocate and start each pending
> channel in the same critical section. This also serializes updates to the
> shared interrupt-enable registers.
>
> Nest vc.lock inside the device lock when accessing descriptor lists or
> updating cookies, including cyclic notifications. virt-dma takes vc.lock
> independently when preparing, submitting and completing descriptors, so
> the device lock does not replace it. Client callbacks run outside both
> locks in the virtual-channel tasklet.
>
> Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/20260706065650.34B131F00A3A@smtp.kernel.org
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
> drivers/dma/sun6i-dma.c | 67 ++++++++++++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 435d7daa1c0f..d3a63c866645 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -217,6 +217,10 @@ struct sun6i_dma_dev {
> struct clk *clk;
> struct clk *clk_mbus;
> int irq;
> + /*
> + * Physical channels, pending list and registers; nests outside vc.lock.
> + * virt-dma also takes vc.lock independently for its lists and cookies.
> + */
> spinlock_t lock;
> struct reset_control *rstc;
> struct tasklet_struct task;
> @@ -524,11 +528,11 @@ static void sun6i_dma_tasklet(struct tasklet_struct *t)
> struct sun6i_dma_dev *sdev = from_tasklet(sdev, t, task);
> struct sun6i_vchan *vchan;
> struct sun6i_pchan *pchan;
> - unsigned int pchan_alloc = 0;
> unsigned int pchan_idx;
>
> + spin_lock_irq(&sdev->lock);
> list_for_each_entry(vchan, &sdev->slave.channels, vc.chan.device_node) {
> - spin_lock_irq(&vchan->vc.lock);
> + spin_lock(&vchan->vc.lock);
>
> pchan = vchan->phy;
>
> @@ -545,10 +549,9 @@ static void sun6i_dma_tasklet(struct tasklet_struct *t)
> pchan->vchan = NULL;
> }
> }
> - spin_unlock_irq(&vchan->vc.lock);
> + spin_unlock(&vchan->vc.lock);
> }
>
> - spin_lock_irq(&sdev->lock);
> for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) {
> pchan = &sdev->pchans[pchan_idx];
>
> @@ -557,31 +560,20 @@ static void sun6i_dma_tasklet(struct tasklet_struct *t)
>
> vchan = list_first_entry(&sdev->pending,
> struct sun6i_vchan, node);
> + spin_lock(&vchan->vc.lock);
>
> /* Remove from pending channels */
> list_del_init(&vchan->node);
> - pchan_alloc |= BIT(pchan_idx);
>
> /* Mark this channel allocated */
> pchan->vchan = vchan;
> vchan->phy = pchan;
> dev_dbg(sdev->slave.dev, "pchan %u: alloc vchan %p\n",
> pchan->idx, &vchan->vc);
> + sun6i_dma_start_desc(vchan);
> + spin_unlock(&vchan->vc.lock);
> }
> spin_unlock_irq(&sdev->lock);
> -
> - for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) {
> - if (!(pchan_alloc & BIT(pchan_idx)))
> - continue;
> -
> - pchan = sdev->pchans + pchan_idx;
> - vchan = pchan->vchan;
> - if (vchan) {
> - spin_lock_irq(&vchan->vc.lock);
> - sun6i_dma_start_desc(vchan);
> - spin_unlock_irq(&vchan->vc.lock);
> - }
> - }
> }
>
> static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> @@ -590,9 +582,11 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> u32 channels_per_reg = sdev->cfg->num_channels_per_reg;
> struct sun6i_vchan *vchan;
> struct sun6i_pchan *pchan;
> + unsigned long flags;
> int i, j, ret = IRQ_NONE;
> u32 status;
>
> + spin_lock_irqsave(&sdev->lock, flags);
> for (i = 0; i < DIV_ROUND_UP(sdev->num_pchans, channels_per_reg); i++) {
> status = sun6i_read_irq_stat(sdev, i);
> if (!status)
> @@ -608,14 +602,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> pchan = sdev->pchans + i * channels_per_reg + j;
> vchan = pchan->vchan;
> if (vchan && (status & vchan->irq_type)) {
> + spin_lock(&vchan->vc.lock);
> if (vchan->cyclic) {
> vchan_cyclic_callback(&pchan->desc->vd);
> } else {
> - spin_lock(&vchan->vc.lock);
> vchan_cookie_complete(&pchan->desc->vd);
> pchan->done = pchan->desc;
> - spin_unlock(&vchan->vc.lock);
> }
> + spin_unlock(&vchan->vc.lock);
> }
>
> status = status >> DMA_IRQ_CHAN_WIDTH;
> @@ -625,6 +619,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> tasklet_schedule(&sdev->task);
> ret = IRQ_HANDLED;
> }
> + spin_unlock_irqrestore(&sdev->lock, flags);
>
> return ret;
> }
> @@ -931,19 +926,20 @@ static int sun6i_dma_pause(struct dma_chan *chan)
> {
> struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
> struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> - struct sun6i_pchan *pchan = vchan->phy;
> + struct sun6i_pchan *pchan;
> unsigned long flags;
>
> dev_dbg(dmaengine_chan_dev(chan), "vchan %p: pause\n", &vchan->vc);
>
> + spin_lock_irqsave(&sdev->lock, flags);
> + pchan = vchan->phy;
> if (pchan) {
> writel(DMA_CHAN_PAUSE_PAUSE,
> pchan->base + DMA_CHAN_PAUSE);
> } else {
> - spin_lock_irqsave(&sdev->lock, flags);
> list_del_init(&vchan->node);
suppose need vchan lock here?
Frank
> - spin_unlock_irqrestore(&sdev->lock, flags);
> }
> + spin_unlock_irqrestore(&sdev->lock, flags);
>
> return 0;
> }
> @@ -978,15 +974,14 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
> {
> struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
> struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> - struct sun6i_pchan *pchan = vchan->phy;
> + struct sun6i_pchan *pchan;
> unsigned long flags;
> LIST_HEAD(head);
>
> spin_lock_irqsave(&sdev->lock, flags);
> + spin_lock(&vchan->vc.lock);
> + pchan = vchan->phy;
> list_del_init(&vchan->node);
> - spin_unlock_irqrestore(&sdev->lock, flags);
> -
> - spin_lock_irqsave(&vchan->vc.lock, flags);
>
> if (pchan && pchan->desc && pchan->desc != pchan->done) {
> struct virt_dma_desc *vd = &pchan->desc->vd;
> @@ -1007,7 +1002,8 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
> pchan->done = NULL;
> }
>
> - spin_unlock_irqrestore(&vchan->vc.lock, flags);
> + spin_unlock(&vchan->vc.lock);
> + spin_unlock_irqrestore(&sdev->lock, flags);
>
> vchan_dma_desc_free_list(&vchan->vc, &head);
>
> @@ -1018,8 +1014,9 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
> dma_cookie_t cookie,
> struct dma_tx_state *state)
> {
> + struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
> struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> - struct sun6i_pchan *pchan = vchan->phy;
> + struct sun6i_pchan *pchan;
> struct sun6i_dma_lli *lli;
> struct virt_dma_desc *vd;
> struct sun6i_desc *txd;
> @@ -1027,11 +1024,12 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
> unsigned long flags;
> size_t bytes = 0;
>
> + spin_lock_irqsave(&sdev->lock, flags);
> + spin_lock(&vchan->vc.lock);
> ret = dma_cookie_status(chan, cookie, state);
> if (ret == DMA_COMPLETE || !state)
> - return ret;
> -
> - spin_lock_irqsave(&vchan->vc.lock, flags);
> + goto out;
> + pchan = vchan->phy;
>
> vd = vchan_find_desc(&vchan->vc, cookie);
> txd = to_sun6i_desc(&vd->tx);
> @@ -1045,9 +1043,10 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
> bytes = sun6i_get_chan_size(pchan);
> }
>
> - spin_unlock_irqrestore(&vchan->vc.lock, flags);
> -
> dma_set_residue(state, bytes);
> +out:
> + spin_unlock(&vchan->vc.lock);
> + spin_unlock_irqrestore(&sdev->lock, flags);
>
> return ret;
> }
>
> --
> 2.53.0
>
next prev parent reply other threads:[~2026-10-26 21:14 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-23 23:38 [PATCH v3 0/9] dmaengine: sun6i: fix interrupt routing and channel lifetime James Hilliard
2026-10-23 23:38 ` [PATCH v3 1/9] dmaengine: sun6i: fix physical channel index in interrupt handler James Hilliard
2026-10-23 23:38 ` [PATCH v3 2/9] dmaengine: sun6i: handle the last partial interrupt bank James Hilliard
2026-10-26 20:57 ` Frank Li
2026-10-23 23:38 ` [PATCH v3 3/9] dmaengine: sun6i: take the device lock before the virtual channel lock James Hilliard
2026-10-26 21:05 ` Frank Li
2026-10-23 23:38 ` [PATCH v3 4/9] dmaengine: sun6i: serialize physical channel ownership James Hilliard
2026-10-26 21:14 ` Frank Li [this message]
2026-10-26 21:42 ` James Hilliard
2026-10-23 23:38 ` [PATCH v3 5/9] dmaengine: sun6i: detach completed descriptors before invoking callbacks James Hilliard
2026-10-23 23:38 ` [PATCH v3 6/9] dmaengine: sun6i: retire interrupt state before reusing physical channels James Hilliard
2026-10-23 23:38 ` [PATCH v3 7/9] dmaengine: sun6i: track paused virtual channels James Hilliard
2026-10-23 23:38 ` [PATCH v3 8/9] dmaengine: sun6i: synchronize callbacks before releasing channel resources James Hilliard
2026-10-23 23:38 ` [PATCH v3 9/9] dmaengine: sun6i: keep cyclic state with the prepared descriptor James Hilliard
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=at_CpZrmQ986UEqv@SMW015318 \
--to=frank.li@oss.nxp.com \
--cc=Frank.Li@kernel.org \
--cc=arnd@arndb.de \
--cc=cyeaa@connect.ust.hk \
--cc=dmaengine@vger.kernel.org \
--cc=james.hilliard1@gmail.com \
--cc=jenskuske@gmail.com \
--cc=jernej.skrabec@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=moinejf@free.fr \
--cc=mripard@kernel.org \
--cc=samuel@sholland.org \
--cc=sashiko-bot@kernel.org \
--cc=tien.nguyenminh@embeddedlinux.blog \
--cc=vkoul@kernel.org \
--cc=wens@kernel.org \
--cc=zenghongling@kylinos.cn \
/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.