Re: [PATCH v10 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 4 potential issue(s) to consider:
- [Critical] dmaengine: ti: kernel panic due to double descriptor completion
- [High] dmaengine: ti: descriptor memory leak on autopair timeout
- [Medium] kconfig: TI_K3_UDMA_V2 selects TI_K3_RINGACC without inheriting dependencies
- [Medium] dmaengine: ti: code contradicts commit message regarding PEER registers
--

commit d73848739c5384bf6614c47a767d4ef7e89a8626
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>

    dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2
    
    Add support for BCDMA_V2.
    
    The BCDMA_V2 is different than the existing BCDMA supported by the
    k3-udma driver.

> 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]
Is this select missing a dependency? Enabling TI_K3_UDMA_V2 selects
TI_K3_RINGACC, but omits TI_SCI_INTA_IRQCHIP which is required by
TI_K3_RINGACC. Could this cause unmet dependency warnings or build failures
during configuration?

> +	select TI_K3_PSIL
> +        help
> +	  Enable support for the TI UDMA (Unified DMA) v2 controller. This
> +	  DMA engine is used in AM62L.
> +
>  config TI_K3_UDMA_COMMON
>  	tristate
>  
> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/dma/ti/k3-udma-v2.c
[ ... ]
> +static int udma_v2_start(struct udma_chan *uc)
> +{
[ ... ]
> +		udma_chanrt_write(uc, UDMA_CHAN_RT_CTL_REG,
> +				  UDMA_CHAN_RT_CTL_EN | UDMA_CHAN_RT_CTL_AUTOPAIR);
> +
> +		/* Poll for autopair completion */
> +		read_poll_timeout_atomic(udma_v2_check_chan_autopair_completion,
> +					 status, status != 0, 100, 500, false, uc);
> +
> +		if (status <= 0) {
> +			udma_chanrt_write(uc, UDMA_CHAN_RT_CTL_REG, 0);
> +			return -ETIMEDOUT;
> +		}

[Severity: High]
What happens to the DMA descriptor if udma_v2_start() times out and returns
-ETIMEDOUT here? The caller, udma_issue_pending(), ignores the return value,
leaving the descriptor assigned to uc->desc and pushed to the hardware ring.
Since the channel is disabled, no completion interrupt will fire to free
uc->terminated_desc. It looks like this might lead to a permanent memory
leak.

[ ... ]
> +static int udma_v2_stop(struct udma_chan *uc)
> +{
> +	uc->state = UDMA_CHAN_IS_TERMINATING;
> +	reinit_completion(&uc->teardown_completed);
> +
> +	if (uc->config.dir == DMA_DEV_TO_MEM) {
> +		if (!uc->cyclic && !uc->desc)
> +			udma_push_to_ring(uc, -1);
> +	}
> +
> +	if (uc->config.dir != DMA_MEM_TO_MEM)
> +		udma_chanrt_write(uc, UDMA_CHAN_RT_PEER_REG(8), UDMA_CHAN_RT_PEER_REG8_FLUSH);

[Severity: Medium]
The commit message states there is "no need to write to PEER registers",
but this code explicitly writes a flush command to UDMA_CHAN_RT_PEER_REG(8)
during teardown. Are there certain cases where writing to the PEER register
is still required for this hardware?

> +
> +	udma_chanrt_update_bits(uc, UDMA_CHAN_RT_CTL_REG,
> +				UDMA_CHAN_RT_CTL_EN | UDMA_CHAN_RT_CTL_TDOWN,
> +				UDMA_CHAN_RT_CTL_EN | UDMA_CHAN_RT_CTL_TDOWN);
> +
> +	return 0;
> +}
[ ... ]
> +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);
> +		tr_handled = true;
> +
> +		if (!(intr_status & K3_RINGACC_RT_INT_STATUS_COMPLETE))
> +			return IRQ_HANDLED;
> +	}
> +
> +	if (udma_pop_from_ring(uc, &paddr) || !paddr)
> +		return tr_handled ? IRQ_HANDLED : IRQ_NONE;
> +
> +	spin_lock(&uc->vc.lock);
> +
> +	/* Teardown completion message */
> +	if (cppi5_desc_is_tdcm(paddr)) {
[ ... ]
> +	d = udma_udma_desc_from_paddr(uc, paddr);
> +
> +	if (d) {
> +		dma_addr_t desc_paddr = udma_curr_cppi5_desc_paddr(d,
> +								   d->desc_idx);
[ ... ]
> +		if (d == uc->desc) {
[ ... ]
> +		} else {
> +			/*
> +			 * terminated descriptor, mark the descriptor as
> +			 * completed to update the channel's cookie marker
> +			 */
> +			dma_cookie_complete(&d->vd.tx);
> +		}

[Severity: Critical]
Is it possible for this interrupt handler to complete the same descriptor
twice and cause a kernel panic?

If intr_status has both the K3_RINGACC_RT_INT_STATUS_TR and
K3_RINGACC_RT_INT_STATUS_COMPLETE bits set, the handler processes the TR
event and completes the descriptor via udma_v2_udma_irq_handler(), which
zeroes its cookie.

Because K3_RINGACC_RT_INT_STATUS_COMPLETE is also set, the handler does
not return early. It falls through to the completion handling branch below,
completing the descriptor a second time and potentially triggering a BUG_ON
in dma_cookie_complete() because the cookie is now less than
DMA_MIN_COOKIE.

[ ... ]
> +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]
This is a pre-existing issue, but this platform driver lacks a .remove
callback. Because dma_async_device_register() is used without being managed
by devres, unloading the module leaves the DMA engine registered while the
underlying devm structures are freed. Might this lead to a use-after-free
if the module is unloaded?

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