Re: [PATCH v11 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] dmaengine: ti: missing of_dma_controller_free() in udma_v2_remove()
- [High] dmaengine: ti: use-after-free due to missing cancel_work_sync() for purge_work
- [High] dmaengine: ti: sleep in atomic context via udma_reset_rings() in udma_v2_start()
- [High] dmaengine: ti: invalid pointer assignment for BCDMA rflow->reg_rt
- [Medium] dmaengine: ti: missing dma_unmap_single() for rx_flush buffers
- [Medium] dmaengine: ti: missing k3_configure_chan_coherency() cleanup on error
- [Low] dmaengine: ti: unnecessary Kconfig dependency on TI_SCI_INTA_IRQCHIP
--

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

dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2

This commit introduces support for the K3 BCDMA_V2 driver. It handles
architectural changes like static channel mapping, autopairing without PSIL
pair, and directly connected GIC interrupts. It also unifies Tx and Rx channel
configurations and modifies the match_data structure.

> diff --git a/drivers/dma/ti/Kconfig b/drivers/dma/ti/Kconfig
> index a389ae04ec930..e6a2cf37062e7 100644
> --- a/drivers/dma/ti/Kconfig
> +++ b/drivers/dma/ti/Kconfig
> @@ -49,6 +49,19 @@ 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 || COMPILE_TEST
> +	depends on TI_SCI_INTA_IRQCHIP

[Severity: Low]
Is the dependency on TI_SCI_INTA_IRQCHIP required here? The driver appears
to rely exclusively on directly connected GIC interrupts accessed via
platform_get_irq_byname() without using the INT-A router, so keeping this
might prevent the driver from being enabled on K3 architectures that lack the
INT-A interrupt router.

> +	select DMA_ENGINE
> +	select DMA_VIRTUAL_CHANNELS
> +	select TI_K3_UDMA_COMMON
> +	select TI_K3_RINGACC
> +	select TI_K3_PSIL

[ ... ]

> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> new file mode 100644
> index 0000000000000..8465517e8d21a
> --- /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);
> +			/*
> +			 * udma_start_desc() above already pushed uc->desc to
> +			 * the hardware ring. Since the channel never actually
> +			 * started, no completion interrupt will ever arrive
> +			 * to free it - drain the ring and free it here to
> +			 * avoid leaking it.
> +			 */
> +			udma_reset_rings(uc);

[Severity: High]
Can this code sleep in atomic context? udma_v2_start() is called while
holding the uc->vc.lock raw spinlock (e.g. from udma_issue_pending()), but if
the autopair times out, it invokes udma_reset_rings(). This eventually calls
k3_ringacc_ring_reset_sci(), which uses the readl_poll_timeout macro that
attempts to sleep via usleep_range.

[ ... ]

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

[ ... ]

> +	k3_configure_chan_coherency(&uc->vc.chan, uc->config.asel);
> +	ring_cfg.asel = uc->config.asel;
> +	ring_cfg.dma_dev = dmaengine_get_dma_device(&uc->vc.chan);
> +
> +	ret = k3_ringacc_ring_cfg(tchan->t_ring, &ring_cfg);
> +	ret |= k3_ringacc_ring_cfg(tchan->tc_ring, &ring_cfg);
> +
> +	if (ret)
> +		goto err_ringcfg;
> +
> +	return 0;
> +
> +err_ringcfg:
> +	k3_ringacc_ring_free(uc->tchan->tc_ring);
> +	uc->tchan->tc_ring = NULL;
> +	k3_ringacc_ring_free(uc->tchan->t_ring);
> +	uc->tchan->t_ring = NULL;

[Severity: Medium]
Should k3_configure_chan_coherency(&uc->vc.chan, 0) be called here in the
error path? It appears the coherency state is configured earlier in the
function but isn't reverted on failure, potentially leaving the underlying
dma_parms structure incorrectly assigned for subsequent allocation attempts.

> +err_ring:
> +	udma_put_tchan(uc);
> +
> +	return ret;
> +}

[ ... ]

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

[ ... ]

> +	k3_configure_chan_coherency(&uc->vc.chan, uc->config.asel);
> +	ring_cfg.asel = uc->config.asel;
> +	ring_cfg.dma_dev = dmaengine_get_dma_device(&uc->vc.chan);
> +
> +	ret = k3_ringacc_ring_cfg(rflow->fd_ring, &ring_cfg);
> +
> +	ring_cfg.size = K3_UDMA_DEFAULT_RING_SIZE;
> +	ret |= k3_ringacc_ring_cfg(rflow->r_ring, &ring_cfg);
> +
> +	if (ret)
> +		goto err_ringcfg;
> +
> +	return 0;
> +
> +err_ringcfg:
> +	k3_ringacc_ring_free(rflow->r_ring);
> +	rflow->r_ring = NULL;
> +	k3_ringacc_ring_free(rflow->fd_ring);
> +	rflow->fd_ring = NULL;

[Severity: Medium]
Similarly, should k3_configure_chan_coherency(&uc->vc.chan, 0) be added to
this error path to revert the coherency configuration if k3_ringacc_ring_cfg()
fails?

> +err_ring:
> +	udma_put_rflow(uc);
> +err_rflow:
> +	udma_put_rchan(uc);
> +
> +	return ret;
> +}

[ ... ]

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

[ ... ]

> +	for (i = 0; i < ud->rflow_cnt; i++) {
> +		struct udma_rflow *rflow = &ud->rflows[i];
> +
> +		rflow->id = i;
> +		rflow->reg_rt = ud->rflow_rt + i * 0x2000;
> +	}

[Severity: High]
Can this result in an invalid pointer assignment? For the DMA_TYPE_BCDMA
device type, the base address ud->rflow_rt is never mapped or initialized,
remaining NULL due to the devm_kzalloc allocation. When the K3 UDMA glue
interface configures the RX flow (e.g. via xudma_rflowrt_write()), wouldn't
this dereference an invalid memory-mapped I/O address and cause a kernel panic?

[ ... ]

> +static void udma_v2_remove(struct platform_device *pdev)
> +{
> +	struct udma_dev *ud = platform_get_drvdata(pdev);
> +
> +	dma_async_device_unregister(&ud->ddev);
> +}

[Severity: High]
Does this fail to deregister the OF DMA controller?
of_dma_controller_register() dynamically allocates memory for the of_dma
structure and adds it to the global of_dma_list during probe. Since
of_dma_controller_free() is not called here, could this leak the allocation
and leave a dangling pointer in the global list, risking a use-after-free
if another device requests a DMA channel via the Device Tree after the driver
is unloaded?

[Severity: High]
Is it necessary to call cancel_work_sync(&ud->purge_work) here? The driver
uses an asynchronous workqueue (ud->purge_work) to free descriptors, and the
udma_dev structure is managed by devres (devm_kzalloc) and freed
automatically upon return. Any pending work item could continue to execute
after module unload and dereference the freed ud structure.

[Severity: Medium]
Should dma_unmap_single() be called for the rx_flush buffers?
udma_setup_rx_flush() maps memory buffers and descriptors during probe, but
this remove function does not release those mappings. This leaks IOMMU mapping
space and, because the memory itself is allocated via devm_kzalloc, it is
freed upon unload, leaving the DMA mapping active and pointing to freed memory.

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