Re: [PATCH 2/2] dmaengine: ma35d1-pdma: Add MA35D1 PDMA driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] dmaengine: ma35d1: premature register setup in ma35d1_desc_alloc() and prep
- [Critical] dmaengine: ma35d1: active DMA after timeout and double completion in IRQ
- [High] dmaengine: ma35d1: missing PDMA_TBINTDIS in ma35d1_prep_dma_memcpy()
- [High] dmaengine: ma35d1: unvalidated SG segment lengths in ma35d1_prep_slave_sg()
- [High] dmaengine: ma35d1: broken residue and status reporting in ma35d1_tx_status()
- [High] dmaengine: ma35d1: unlocked PDMA_TOUTEN RMW in ma35d1_pdma_interrupt()
- [High] dmaengine: ma35d1: state bugs in ma35d1_of_xlate() and free_chan_resources()
- [High] dmaengine: ma35d1-pdma: fix signed dma-channels pointer and zero bounds check
- [High] dmaengine: ma35d1-pdma: unsafe teardown order in ma35d1_pdma_remove()
- [Medium] dmaengine: ma35d1-pdma: disable channel timeout in ma35d1_terminate_all()
- [Medium] dmaengine: ma35d1-pdma: unreleased ch->desc and stale ch->cfg in ma35d1_free_chan_resources()
--

commit f0cfa08c80802c32fef4b715415e04126d30e654
Author: Shan-Chun Hung <shanchun1218@gmail.com>

dmaengine: ma35d1-pdma: Add MA35D1 PDMA driver

This commit adds a DMAEngine driver for the PDMA controller in the Nuvoton
MA35D1 SoC, supporting up to 10 channels with memory-to-memory, slave
scatter-gather, and cyclic transfers, as well as per-channel peripheral RX
timeouts.

> diff --git a/drivers/dma/ma35d1-pdma.c b/drivers/dma/ma35d1-pdma.c
> new file mode 100644
> index 0000000000000..176bc56c80639
> --- /dev/null
> +++ b/drivers/dma/ma35d1-pdma.c
> @@ -0,0 +1,780 @@

[ ... ]

> +#define PDMA_WIDTH(w)		FIELD_PREP(GENMASK(13, 12), (w) >> 1)
> +#define PDMA_TXCNT(cnt)		FIELD_PREP(GENMASK(31, 16), (cnt) - 1)
> +#define PDMA_GET_TXCNT(ctrl)	FIELD_GET(GENMASK(31, 16), readl(ctrl))
> +#define PDMA_CH_OFFSET(ch)	((ch) * 0x10)

[ ... ]

> +static int ma35d1_terminate_all(struct dma_chan *chan)
> +{
> +	LIST_HEAD(head);
> +	struct ma35d1_chan *ch = to_ma35d1_pdma_chan(chan);
> +	unsigned long flags, pdmadev_flags;
> +
> +	spin_lock_irqsave(&ch->vc.lock, flags);
> +
> +	spin_lock_irqsave(&ch->pdmadev->lock, pdmadev_flags);
> +	ma35d1_pdma_reg_clear(ch, PDMA_CHCTL, BIT(ch->id));
> +	spin_unlock_irqrestore(&ch->pdmadev->lock, pdmadev_flags);

[Severity: Medium]
When a channel configured with a peripheral timeout is terminated in
ma35d1_terminate_all(), only BIT(ch->id) in PDMA_CHCTL is cleared, leaving
BIT(ch->id) enabled in PDMA_TOUTEN and PDMA_TOUTIEN from
ma35d1_pdma_start_desc() -> ma35d1_pdma_timeout().

Could the hardware timeout counter fire after termination and cause
ma35d1_pdma_interrupt() to prematurely complete a subsequently started
descriptor on the same channel? Should PDMA_TOUTEN and PDMA_TOUTIEN also be
cleared here?

> +
> +	if (ch->desc) {
> +		vchan_terminate_vdesc(&ch->desc->vd);
> +		ch->desc = NULL;
> +	}

[ ... ]

> +static void ma35d1_pdma_setup_channel(struct ma35d1_chan *ch)
> +{
> +	u32 offset, reg, shift;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ch->pdmadev->lock, flags);
> +
> +	if ((readl(ch->base + PDMA_CH_OFFSET(ch->id)) & GENMASK(1, 0)) != 0) {
> +		reg = readl(ch->base + PDMA_CHCTL);
> +		writel(0, ch->base + PDMA_CH_OFFSET(ch->id));
> +		ma35d1_pdma_reg_set(ch, PDMA_CHRST, BIT(ch->id));
> +		writel(reg | BIT(ch->id), ch->base + PDMA_CHCTL);
> +	} else {
> +		writel(0, ch->base + PDMA_CH_OFFSET(ch->id));
> +		ma35d1_pdma_reg_set(ch, PDMA_CHCTL, BIT(ch->id));
> +	}
> +	ma35d1_pdma_reg_set(ch, PDMA_INTEN, BIT(ch->id));
> +
> +	offset = PDMA_REQSEL + (ch->id & ~GENMASK(1, 0));
> +	shift = (ch->id & GENMASK(1, 0)) * 8;
> +	ma35d1_pdma_reg_clear(ch, offset, GENMASK(7, 0) << shift);
> +	ma35d1_pdma_reg_set(ch, offset, ch->request << shift);
> +
> +	spin_unlock_irqrestore(&ch->pdmadev->lock, flags);
> +}

[ ... ]

> +static struct ma35d1_desc *ma35d1_desc_alloc(struct ma35d1_chan *ch, u32 sg_len)
> +{
> +	struct ma35d1_desc *d;
> +
> +	d = kzalloc(struct_size(d, sg, sg_len), GFP_ATOMIC);
> +	if (!d)
> +		return NULL;
> +
> +	d->sg_len = sg_len;
> +	d->dma_len = sizeof(*d->sg) * sg_len;
> +	d->dma_addr = dma_map_single(ch->dev, d->sg, d->dma_len, DMA_TO_DEVICE);
> +	if (dma_mapping_error(ch->dev, d->dma_addr)) {
> +		dev_err(ch->dev, "dma mapping error for channel\n");
> +		kfree(d);
> +		return NULL;
> +	}
> +
> +	writel(d->dma_addr, ch->base + PDMA_CH_OFFSET(ch->id) + PDMA_DSCT_NEXT);
> +
> +	return d;
> +}
> +
> +static struct dma_async_tx_descriptor *
> +ma35d1_desc_submit(struct ma35d1_chan *ch, struct ma35d1_desc *d, bool cyclic,
> +		   unsigned long tx_flags)
> +{
> +	dma_sync_single_for_device(ch->dev, d->dma_addr, d->dma_len, DMA_TO_DEVICE);
> +	d->cyclic = cyclic;
> +	d->ch = ch;
> +	ch->error = false;
> +
> +	return vchan_tx_prep(&ch->vc, &d->vd, tx_flags);
> +}

[Severity: Critical]
Hardware registers and per-channel state are being modified during descriptor
preparation (ma35d1_prep_dma_memcpy(), ma35d1_prep_slave_sg(), and
ma35d1_prep_dma_cyclic()) rather than when the descriptor is actually started
in ma35d1_pdma_start_desc() -> ma35d1_pdma_start():

1. ma35d1_desc_alloc() writes d->dma_addr directly to PDMA_DSCT_NEXT before
d->sg is populated or synced with dma_sync_single_for_device(), and without
holding ch->vc.lock.

2. Immediately after ma35d1_desc_alloc(), the prep functions call
ma35d1_pdma_setup_channel(ch). If bits 1:0 of PDMA_DSCT_CTL are non-zero (for
example, while an active transfer is running, or after a timeout or
ma35d1_terminate_all()), ma35d1_pdma_setup_channel() writes 0 to PDMA_DSCT_CTL
and asserts BIT(ch->id) in PDMA_CHRST. Doesn't resetting the channel here
abort any in-flight transfer and wipe out the PDMA_DSCT_NEXT value that
ma35d1_desc_alloc() just programmed?

3. Conversely, when a descriptor is dequeued in ma35d1_pdma_start_desc() ->
ma35d1_pdma_start(), neither ma35d1_pdma_setup_channel(ch) nor the write of
d->dma_addr to PDMA_DSCT_NEXT is performed:

    static void ma35d1_pdma_start(struct ma35d1_chan *ch)
    {
        struct ma35d1_desc *d = ch->desc;

        if (d->sg_len == 1 && !d->cyclic) {
            ...
        } else {
            /* Trigger scatter-gather mode. */
            ma35d1_pdma_reg_clear(ch, PDMA_CH_OFFSET(ch->id) + PDMA_DSCT_CTL, PDMA_OP_MSK);
            ma35d1_pdma_reg_set(ch, PDMA_CH_OFFSET(ch->id) + PDMA_DSCT_CTL, PDMA_OP_SCATTER);
        }

If multiple descriptors are prepared before dma_async_issue_pending(), won't
PDMA_DSCT_NEXT be overwritten by the last prepared descriptor, and subsequent
descriptors started from ma35d1_pdma_complete() never program PDMA_DSCT_NEXT
or re-enable BIT(ch->id) in PDMA_CHCTL?

4. In ma35d1_desc_submit(), ch->error = false is written without holding
ch->vc.lock, which races with ma35d1_pdma_interrupt() and ma35d1_tx_status().

Should channel setup and writing PDMA_DSCT_NEXT be moved into
ma35d1_pdma_start() under ch->vc.lock?

> +
> +static struct dma_async_tx_descriptor *
> +ma35d1_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
> +		       size_t len, unsigned long tx_flags)
> +{

[ ... ]

> +	for (i = 0; i < sg_len; i++) {
> +		tlen = min_t(size_t, len, max_chunk);
> +		d->sg[i].src = src;
> +		d->sg[i].dst = dst;
> +		d->sg[i].ctl = PDMA_WIDTH(width) | PDMA_TXCNT(tlen / width) | PDMA_OP_SCATTER;
> +		d->sg[i].next = d->dma_addr + PDMA_CH_OFFSET(i + 1);
> +		src += tlen;
> +		dst += tlen;
> +		len -= tlen;
> +	}
> +	d->sg[sg_len - 1].ctl &= ~(PDMA_OP_MSK | PDMA_TBINTDIS);
> +	d->sg[sg_len - 1].ctl |= PDMA_OP_BASIC;

[Severity: High]
In ma35d1_prep_dma_memcpy(), the final SG entry clears PDMA_TBINTDIS, but
intermediate entries in the loop never set PDMA_TBINTDIS in d->sg[i].ctl
(unlike ma35d1_prep_slave_sg()).

When len > max_chunk (so sg_len > 1), won't the controller raise a transfer
done interrupt (PDMA_TDSTS) as soon as the first chunk (d->sg[0]) finishes,
causing ma35d1_pdma_interrupt() -> ma35d1_pdma_complete() to complete and
free the descriptor while the remaining chunks are still in flight?

> +
> +	return ma35d1_desc_submit(ch, d, false, tx_flags);
> +}
> +
> +static struct dma_async_tx_descriptor *
> +ma35d1_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +		     u32 sg_len, enum dma_transfer_direction dir,
> +		     unsigned long tx_flags, void *context)
> +{

[ ... ]

> +	for_each_sg(sgl, sgent, sg_len, i) {
> +		if (dir == DMA_MEM_TO_DEV) {
> +			d->sg[i].ctl = PDMA_WIDTH(width) | PDMA_DAFIX;
> +			d->sg[i].src = sg_dma_address(sgent);
> +			d->sg[i].dst = ch->cfg.dst_addr;
> +		} else {
> +			d->sg[i].ctl = PDMA_WIDTH(width) | PDMA_SAFIX;
> +			d->sg[i].src = ch->cfg.src_addr;
> +			d->sg[i].dst = sg_dma_address(sgent);
> +		}
> +		d->sg[i].ctl |= PDMA_TXCNT(sg_dma_len(sgent) / width) | PDMA_TBINTDIS |
> +				PDMA_TXTYPE | PDMA_OP_SCATTER;
> +		d->sg[i].next = d->dma_addr + PDMA_CH_OFFSET(i + 1);
> +	}

[Severity: High]
In ma35d1_prep_slave_sg(), sg_dma_len(sgent) / width is passed directly to
PDMA_TXCNT(cnt), which encodes FIELD_PREP(GENMASK(31, 16), (cnt) - 1), without
validating the segment length (unlike ma35d1_prep_dma_cyclic() and
ma35d1_prep_dma_memcpy()).

If sg_dma_len(sgent) < width (or 0), sg_dma_len(sgent) / width is 0 and
(cnt) - 1 underflows to 0xffff (65536 beats), causing an out-of-bounds DMA
transfer. Conversely, if sg_dma_len(sgent) / width > PDMA_MAX_CHAN_BEATS,
the 16-bit field overflows and truncates the transfer, and if
sg_dma_len(sgent) % width != 0, trailing bytes are dropped.

Should ma35d1_prep_slave_sg() validate (or split) each SG entry's length and
alignment, and should ma35d1_pdma_probe() also advertise the maximum segment
size via dma_set_max_seg_size()?

[ ... ]

> +static struct dma_chan *ma35d1_of_xlate(struct of_phandle_args *dma_spec, struct of_dma *of)
> +{
> +	struct ma35d1_pdmadev *pdmadev = of->of_dma_data;
> +	unsigned int chan_id, reqsel;
> +
> +	if (dma_spec->args_count != 2)
> +		return NULL;
> +
> +	chan_id = dma_spec->args[0];
> +	reqsel = dma_spec->args[1];
> +	if (chan_id >= pdmadev->nr_chans || reqsel > GENMASK(7, 0))
> +		return NULL;
> +
> +	pdmadev->channels[chan_id].request = reqsel;
> +
> +	return dma_get_slave_channel(&pdmadev->channels[chan_id].vc.chan);
> +}

[Severity: High]
In ma35d1_of_xlate(), pdmadev->channels[chan_id].request = reqsel is written
before calling dma_get_slave_channel().

If chan_id is already in use by another consumer, dma_get_slave_channel()
will return NULL, but the active channel's request field will already have
been overwritten with reqsel. Should request only be assigned after
dma_get_slave_channel() succeeds?

> +
> +static void ma35d1_free_chan_resources(struct dma_chan *chan)
> +{
> +	struct ma35d1_chan *ch = to_ma35d1_pdma_chan(chan);
> +
> +	vchan_free_chan_resources(&ch->vc);
> +	ch->request = 0;
> +}

[Severity: Medium]
When a channel is released in ma35d1_free_chan_resources(), ch->request is
reset to 0, but ch->cfg (including ch->cfg.peripheral_config) is not cleared,
and any active descriptor in ch->desc (which ma35d1_pdma_start_desc() unlinked
from vc.desc_issued via list_del(&vd->node)) is not terminated or freed.

If a slave consumer configures peripheral_config via ma35d1_slave_config() and
later releases the channel, won't a subsequent DMA_MEMCPY user (or a slave
consumer that does not set peripheral_config) inherit the stale
ch->cfg.peripheral_config pointer in ma35d1_pdma_start_desc() ->
ma35d1_pdma_timeout() and erroneously enable peripheral timeout interrupts?

[ ... ]

> +static enum dma_status ma35d1_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> +					struct dma_tx_state *txstate)
> +{
> +	enum dma_status ret;
> +	struct ma35d1_chan *ch = to_ma35d1_pdma_chan(chan);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ch->vc.lock, flags);
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +	if (ret == DMA_COMPLETE) {
> +		if (ch->error)
> +			ret = DMA_ERROR;
> +		else
> +			dma_set_residue(txstate, ch->remain);
> +	}
> +
> +	spin_unlock_irqrestore(&ch->vc.lock, flags);
> +
> +	return ret;
> +}

[Severity: High]
There appear to be several issues with status and residue reporting in
ma35d1_tx_status() and ma35d1_pdma_interrupt():

1. ma35d1_tx_status() only calls dma_set_residue() when ret == DMA_COMPLETE.
For DMA_IN_PROGRESS transfers (including cyclic transfers, where cookie status
is always DMA_IN_PROGRESS), residue is never computed and remains 0.

2. Error and residue state are stored in per-channel fields (ch->error and
ch->remain) rather than per-descriptor, and ch->error is cleared in
ma35d1_desc_submit() whenever a new descriptor is prepared. Won't preparing a
new descriptor clear ch->error before a client queries the failed transfer's
cookie, or cause an abort to report DMA_ERROR for previously completed cookies
on that channel? Also, ch->desc->vd.tx_result is never updated on abort or
timeout, so callback_result clients will always see DMA_TRANS_NOERROR with 0
residue.

3. In ma35d1_pdma_interrupt(), timeout residue is computed as:

    ch->remain = PDMA_GET_TXCNT(ch->base + PDMA_CH_OFFSET(ch->id)) *
                 ch->desc->width;

Because PDMA_TXCNT(cnt) programs the hardware with (cnt) - 1, doesn't reading
PDMA_GET_TXCNT() without adding 1 under-report the remaining bytes in the
current segment by one beat (ch->desc->width bytes), and ignore any remaining
segments when sg_len > 1?

[ ... ]

> +static irqreturn_t ma35d1_pdma_interrupt(int irq, void *devid)
> +{
> +	int i;
> +	struct ma35d1_pdmadev *pdmadev = devid;
> +	struct ma35d1_chan *ch = &pdmadev->channels[pdmadev->nr_chans - 1];
> +	unsigned int timeout_sts, done_sts, abort_sts;
> +	unsigned long pending;
> +
> +	/*
> +	 * Read the interrupt flags. Normally, only one is set per channel.
> +	 * Check and clear each flag separately.
> +	 */
> +	done_sts = readl(ch->base + PDMA_TDSTS);
> +	timeout_sts = readl(ch->base + PDMA_INTSTS);
> +	abort_sts = readl(ch->base + PDMA_ABTSTS);
> +
> +	pending = (done_sts | abort_sts | (timeout_sts >> 8)) &
> +		  GENMASK(pdmadev->nr_chans - 1, 0);
> +
> +	for_each_set_bit(i, &pending, pdmadev->nr_chans) {
> +		unsigned long flags;
> +
> +		ch = &pdmadev->channels[i];
> +
> +		spin_lock_irqsave(&ch->vc.lock, flags);
> +
> +		if (done_sts & BIT(i)) {
> +			writel(BIT(i), ch->base + PDMA_TDSTS);
> +			if (ch->desc) {
> +				ch->remain = 0;
> +				if (ch->desc->cyclic)
> +					vchan_cyclic_callback(&ch->desc->vd);
> +				else
> +					ma35d1_pdma_complete(ch);
> +			}
> +		}
> +
> +		if (timeout_sts & BIT(i + 8)) {
> +			if (ch->desc)
> +				ch->remain = PDMA_GET_TXCNT(ch->base + PDMA_CH_OFFSET(ch->id)) *
> +					     ch->desc->width;
> +			ma35d1_pdma_reg_clear(ch, PDMA_TOUTEN, BIT(i));

[Severity: High]
PDMA_TOUTEN is a controller-wide register shared across all channels, and
ma35d1_pdma_timeout() serializes read-modify-write updates to PDMA_TOUTEN
using ch->pdmadev->lock.

Here in ma35d1_pdma_interrupt(), ma35d1_pdma_reg_clear(ch, PDMA_TOUTEN, BIT(i))
performs a read-modify-write while holding only the per-channel ch->vc.lock.
Can this race with another CPU starting a descriptor on a different channel in
ma35d1_issue_pending() -> ma35d1_pdma_start_desc() -> ma35d1_pdma_timeout()
and clobber PDMA_TOUTEN?

> +			writel(BIT(i + 8), ch->base + PDMA_INTSTS);
> +			ma35d1_pdma_complete(ch);
> +		}
> +
> +		if (abort_sts & BIT(i)) {
> +			writel(BIT(i), ch->base + PDMA_ABTSTS);
> +			ch->error = true;
> +			ma35d1_pdma_complete(ch);
> +		}
> +
> +		spin_unlock_irqrestore(&ch->vc.lock, flags);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Critical]
In ma35d1_pdma_interrupt(), there are a few issues with how channel completion
and interrupt status are handled:

1. When a timeout interrupt fires (timeout_sts & BIT(i + 8)), the transfer has
not completed all beats and the hardware channel remains enabled in PDMA_CHCTL
with an active operating mode in PDMA_DSCT_CTL. Calling ma35d1_pdma_complete()
without disabling or resetting the hardware channel will complete and free
ch->desc (via vchan_complete() -> ma35d1_pdma_desc_free()) while the hardware
channel is still active. If another peripheral request arrives when no next
descriptor was queued, won't the controller DMA into the unmapped/freed client
buffer or fetch freed SG descriptors?

2. Because done_sts, timeout_sts, and abort_sts are handled in three sequential
if blocks that each call ma35d1_pdma_complete(ch) (which completes ch->desc and
immediately starts the next queued descriptor in ch->desc), if more than one
status bit is set for channel i in the same interrupt, won't the second block
immediately complete the newly started next descriptor (or free an active
cyclic descriptor)?

3. If pending is 0, should ma35d1_pdma_interrupt() return IRQ_NONE instead of
unconditionally returning IRQ_HANDLED?

> +
> +static int ma35d1_pdma_probe(struct platform_device *pdev)
> +{
> +	int i, irq, nr_chans, ret;
> +	struct clk *pdma_clk;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct ma35d1_chan *ch;
> +	struct ma35d1_pdmadev *pdmadev;
> +	void __iomem *reg_base;

[ ... ]

> +	ret = of_property_read_u32(node, "dma-channels", &nr_chans);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get dma-channels\n");
> +
> +	if (nr_chans > PDMA_MAX_CHANS) {
> +		dev_warn(dev, "dma-channels %d exceeds max %d, clamping\n",
> +			 nr_chans, PDMA_MAX_CHANS);
> +		nr_chans = PDMA_MAX_CHANS;
> +	}

[Severity: High]
In ma35d1_pdma_probe(), nr_chans is declared as a signed int and passed as
&nr_chans to of_property_read_u32(), which expects a u32 * (triggering a
-Wpointer-sign warning), and the bounds check only tests nr_chans >
PDMA_MAX_CHANS without rejecting nr_chans <= 0.

If dma-channels in the Device Tree is 0 (or has bit 31 set, making nr_chans
negative), devm_kzalloc() will allocate 0 elements for pdmadev->channels[],
causing out-of-bounds accesses in ma35d1_pdma_interrupt() at
&pdmadev->channels[pdmadev->nr_chans - 1] and in ma35d1_pdma_remove() at
pdmadev->channels[0].base. Should nr_chans be a u32 and checked for
nr_chans == 0?

[ ... ]

> +	ret = devm_request_irq(dev, irq, ma35d1_pdma_interrupt, 0, pdev->name, pdmadev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> +
> +	ret = dmaenginem_async_device_register(&pdmadev->ddev);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_dma_controller_register(node, ma35d1_of_xlate, pdmadev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "of_dma_controller_register failed\n");
> +
> +	platform_set_drvdata(pdev, pdmadev);
> +
> +	return 0;
> +}
> +
> +static void ma35d1_pdma_remove(struct platform_device *pdev)
> +{
> +	struct ma35d1_pdmadev *pdmadev = platform_get_drvdata(pdev);
> +	int i;
> +
> +	of_dma_controller_free(pdev->dev.of_node);
> +	writel(0, pdmadev->channels[0].base + PDMA_INTEN);
> +
> +	for (i = 0; i < pdmadev->nr_chans; i++)
> +		tasklet_kill(&pdmadev->channels[i].vc.task);
> +}

[Severity: High]
In ma35d1_pdma_probe(), the interrupt handler and dma_device are registered via
devm_request_irq() and dmaenginem_async_device_register(), so devres only
releases them after ma35d1_pdma_remove() returns.

During ma35d1_pdma_remove():
1. Active channels in PDMA_CHCTL and timeout interrupts in PDMA_TOUTIEN /
PDMA_TOUTEN are not disabled.
2. The IRQ is not freed or synchronized before tasklet_kill(), so an in-flight
ma35d1_pdma_interrupt() on another CPU (or a timeout interrupt) can call
vchan_cookie_complete() or vchan_cyclic_callback() and reschedule vc.task
after tasklet_kill() finishes.
3. Because pdmadev->ddev is still registered during ma35d1_pdma_remove(), a
concurrent client can still prepare descriptors and re-enable PDMA_INTEN in
ma35d1_pdma_setup_channel().

Could this allow vc.task to run after devres frees pdmadev? Also, if
of_dma_controller_register() fails in ma35d1_pdma_probe(), the channel
tasklets are not killed on error unwind.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103022202.2139674-1-shanchun1218@gmail.com?part=2