Re: [PATCH v9 16/19] dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing NULL check for devm_kasprintf() return value in channel initialization
- [High] Dangling pointer uc->chan causes use-after-free register corruption
- [High] Missing driver remove function causes resource leaks and UAF on module unload
- [Medium] Kconfig unmet direct dependency for TI_K3_RINGACC
- [Medium] Ring IRQ handler erroneously returns IRQ_NONE after servicing TR interrupt
- [Low] Returning a negative errno (-ENOENT) from an irqreturn_t function
- [Low] Redundant consecutive MMIO writes to channel control register
--

commit 9332700324e4002eaf2d2700e1d64d1763905a6e
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>
Subject: dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2

This commit adds a new driver for the TI BCDMA_V2 controller. It supports
autopairing, static channel mapping, direct IRQs, and a unified channel
register space where Tx and Rx channels share indices.

> diff --git a/drivers/dma/ti/Kconfig b/drivers/dma/ti/Kconfig
> --- a/drivers/dma/ti/Kconfig
> +++ b/drivers/dma/ti/Kconfig

[ ... ]

> @@ -49,6 +49,18 @@ config TI_K3_UDMA
>  	  Enable support for the TI UDMA (Unified DMA) controller. This
>  	  DMA engine is used in AM65x and j721e.
>  
> +config TI_K3_UDMA_V2
> +	tristate "Texas Instruments K3 UDMA v2 support"
> +	depends on ARCH_K3
> +	select DMA_ENGINE
> +	select DMA_VIRTUAL_CHANNELS
> +	select TI_K3_UDMA_COMMON
> +	select TI_K3_RINGACC

[Severity: Medium]
Will selecting TI_K3_RINGACC without inheriting its direct dependency on
TI_SCI_INTA_IRQCHIP cause an unmet direct dependencies warning during the
build? It seems Kconfig could force TI_K3_RINGACC on without the required
IRQ chip infrastructure.

> +	select TI_K3_PSIL
> +        help
> +	  Enable support for the TI UDMA (Unified DMA) v2 controller. This
> +	  DMA engine is used in AM62L.

[ ... ]

> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> --- /dev/null
> +++ b/drivers/dma/ti/k3-udma-v2.c

[ ... ]

> +static int udma_v2_reset_chan(struct udma_chan *uc, bool hard)
> +{
> +	udma_chanrt_write(uc, UDMA_CHAN_RT_CTL_REG, 0);

[Severity: High]
Does the resource cleanup logic (like in bcdma_put_bchan) clear the newly
introduced V2 alias uc->chan? If the alias is left dangling after primary
pointers are set to NULL, would udma_chanrt_write() bypass the macro's safety
check and inadvertently write to a freed hardware register here?

> +
> +	/* Reset all counters */
> +	udma_v2_reset_counters(uc);

[ ... ]

> +static int udma_v2_start(struct udma_chan *uc)
> +{

[ ... ]

> +	case DMA_MEM_TO_MEM:
> +		udma_bchanrt_write(uc, UDMA_CHAN_RT_CTL_REG,
> +				   UDMA_CHAN_RT_CTL_EN);
> +		udma_bchanrt_write(uc, UDMA_CHAN_RT_CTL_REG,
> +				   UDMA_CHAN_RT_CTL_EN);
> +
> +		break;

[Severity: Low]
This isn't a bug, but is the duplicate write to UDMA_CHAN_RT_CTL_REG
intentional here? It looks like it might be a copy-paste artifact from the
V1 architecture where both tchan and rchan were separately enabled.

> +	default:
> +		return -EINVAL;
> +	}

[ ... ]

> +static irqreturn_t udma_v2_udma_irq_handler(int irq, void *data)
> +{
> +	struct udma_chan *uc = data;
> +	struct udma_dev *ud = uc->ud;
> +	struct udma_desc *d;
> +
> +	switch (uc->config.dir) {
> +	case DMA_DEV_TO_MEM:
> +		k3_ringacc_ring_clear_irq(uc->rflow->r_ring,
> +					  K3_RINGACC_RT_INT_STATUS_TR);
> +		break;
> +	case DMA_MEM_TO_DEV:
> +	case DMA_MEM_TO_MEM:
> +		k3_ringacc_ring_clear_irq(uc->tchan->tc_ring,
> +					  K3_RINGACC_RT_INT_STATUS_TR);
> +		break;
> +	default:
> +		return -ENOENT;

[Severity: Low]
This isn't a bug, but should this return IRQ_NONE instead of a negative errno?
Returning -ENOENT from a function returning irqreturn_t breaks kernel type
semantics.

> +	}

[ ... ]

> +static irqreturn_t udma_v2_ring_irq_handler(int irq, void *data)
> +{

[ ... ]

> +	if (intr_status & K3_RINGACC_RT_INT_STATUS_TR) {
> +		reg = udma_chanrt_read(uc, UDMA_CHAN_RT_CTL_REG);
> +
> +		/* check teardown status */
> +		if ((reg & UDMA_CHAN_RT_CTL_TDOWN) && !(reg & UDMA_CHAN_RT_CTL_EN))
> +			complete_all(&uc->teardown_completed);
> +		udma_v2_udma_irq_handler(irq, data);
> +
> +		if (!(intr_status & K3_RINGACC_RT_INT_STATUS_COMPLETE))
> +			return IRQ_HANDLED;
> +	}
> +
> +	if (udma_pop_from_ring(uc, &paddr) || !paddr)
> +		return IRQ_NONE;

[Severity: Medium]
If the handler successfully processes a TR interrupt in the block above
(because both TR and COMPLETE bits are set), could returning IRQ_NONE here
if the ring is empty violate IRQ subsystem semantics? Repeated occurrences
might cause the kernel to permanently disable the IRQ line as a spurious
interrupt.

> +
> +	spin_lock(&uc->vc.lock);

[ ... ]

> +static int udma_v2_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	for (i = 0; i < ch_count; i++) {
> +		struct udma_chan *uc = &ud->channels[i];
> +
> +		uc->ud = ud;
> +		uc->vc.desc_free = udma_desc_free;
> +		uc->id = i;
> +		uc->bchan = NULL;
> +		uc->tchan = NULL;
> +		uc->rchan = NULL;
> +		uc->config.remote_thread_id = -1;
> +		uc->config.mapped_channel_id = -1;
> +		uc->config.default_flow_id = -1;
> +		uc->config.dir = DMA_MEM_TO_MEM;
> +		uc->name = devm_kasprintf(dev, GFP_KERNEL, "%s chan%d",
> +					  dev_name(dev), i);

[Severity: High]
Does this devm_kasprintf() return value need validation? If the allocation
fails and returns NULL, this is later passed to dma_pool_create() via
bcdma_v2_alloc_chan_resources(), which will unconditionally dereference it and
cause a kernel panic.

> +
> +		vchan_init(&uc->vc, &ud->ddev);

[ ... ]

> +static struct platform_driver bcdma_v2_driver = {
> +	.driver = {
> +		.name	= "ti-udma-v2",
> +		.of_match_table = udma_of_match,
> +		.suppress_bind_attrs = true,
> +		.pm = &udma_pm_ops,
> +	},
> +	.probe		= udma_v2_probe,
> +};

[Severity: High]
Since this driver is registered via module_platform_driver(), could the lack
of a .remove callback lead to resource leaks and a use-after-free on module
unload? Subsystems like dma_async_device_register() and requested DMA rings
will not be unregistered, leaving dangling pointers in the global
dma_device_list.

> +
> +module_platform_driver(bcdma_v2_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922064902.2719979-1-s-adivi@ti.com?part=16