Re: [PATCH v7 17/19] dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unconditional replacement of msi_get_virq() with platform_get_irq_byname() breaks PKTDMA V1 support.
- [High] udma_v2_stop unconditionally writes to UDMA_CHAN_RT_PEER_REG(8), violating PKTDMA V2 constraints.
- [High] Incorrect num_rings initialization and fd_ring_id calculation for PKTDMA V2 causes out-of-bounds array access and RX ring RT register aliasing.
- [High] The IRQ handler is registered with IRQF_SHARED before the channel's work structure is initialized, creating a race condition.
- [Medium] Missing cleanup on error path when platform_get_irq_byname() fails in pktdma_v2_alloc_chan_resources().
- [Medium] Invalid use of INIT_DELAYED_WORK_ONSTACK for heap-allocated delayed work object.
--

--- Patch [17]: [PATCH v7 17/19] dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2 ---
commit f4058832544fa5fe72bb0caead7472593369f2f4
Author: Sai Sree Kartheek Adivi <[email protected]>

dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2

The PKTDMA V2 is different than the existing PKTDMA supported by the
k3-udma driver. It introduces autopairing, static channel mapping,
direct IRQs, and unified channel space.

>     - Remote side configuration handled by DMA. So no need to write to
>       PEER registers to START / STOP / PAUSE / TEARDOWN.

[Severity: High]
The commit message notes that there is no need to write to PEER registers
for operations like STOP. However, does the existing udma_v2_stop() function
still unconditionally write to UDMA_CHAN_RT_PEER_REG(8)?

If PKTDMA V2 hardware does not implement these registers, could this trigger
an asynchronous external abort or bus fault when stopping the channel?

> diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
> index 44c097fff5ee6..41299d3c3d91a 100644
> --- a/drivers/dma/ti/k3-udma-private.c
> +++ b/drivers/dma/ti/k3-udma-private.c
> @@ -174,16 +174,20 @@ EXPORT_SYMBOL(xudma_is_pktdma);
>  
>  int xudma_pktdma_tflow_get_irq(struct udma_dev *ud, int udma_tflow_id)
>  {
> -	const struct udma_oes_offsets *oes = &ud->soc_data->oes;
> +	struct platform_device *pdev = to_platform_device(ud->dev);
> +	char irq_name[10];
>  
> -	return msi_get_virq(ud->dev, udma_tflow_id + oes->pktdma_tchan_flow);
> +	snprintf(irq_name, sizeof(irq_name), "tflow%u", udma_tflow_id);
> +	return platform_get_irq_byname(pdev, irq_name);
>  }

[Severity: High]
Does this unconditional change to platform_get_irq_byname() break PKTDMA V1
hardware?

V1 devices rely on MSI interrupts and their device tree nodes might not
contain named interrupt properties like "tflowX", causing interrupt lookups
to fail for all V1 platforms.

>  int xudma_pktdma_rflow_get_irq(struct udma_dev *ud, int udma_rflow_id)
>  {
> -	const struct udma_oes_offsets *oes = &ud->soc_data->oes;
> +	struct platform_device *pdev = to_platform_device(ud->dev);
> +	char irq_name[10];
>  
> -	return msi_get_virq(ud->dev, udma_rflow_id + oes->pktdma_rchan_flow);
> +	snprintf(irq_name, sizeof(irq_name), "rflow%u", udma_rflow_id);
> +	return platform_get_irq_byname(pdev, irq_name);
>  }

[Severity: High]
As with xudma_pktdma_tflow_get_irq() above, will this also break V1 hardware
that depends on msi_get_virq() for interrupt lookups?

> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> index e7989b0b34cac..879042bff79a6 100644
> --- a/drivers/dma/ti/k3-udma-v2.c
> +++ b/drivers/dma/ti/k3-udma-v2.c
[ ... ]
> +static int pktdma_v2_alloc_chan_resources(struct dma_chan *chan)
> +{
[ ... ]
> +	if (uc->config.dir == DMA_MEM_TO_DEV)
> +		snprintf(irq_name, sizeof(irq_name), "tflow%u", irq_ring_idx);
> +	else
> +		snprintf(irq_name, sizeof(irq_name), "rflow%u", irq_ring_idx);
> +	uc->irq_num_ring = platform_get_irq_byname(pdev, irq_name);
> +	if (uc->irq_num_ring < 0)
> +		return uc->irq_num_ring;

[Severity: Medium]
If platform_get_irq_byname() fails here, the direct return bypasses the
err_res_free cleanup block. Does this leak the descriptor pool
(uc->hdesc_pool) and the DMA rings allocated earlier in this function?

> +
> +	ret = request_irq(uc->irq_num_ring, udma_v2_ring_irq_handler,
> +			  IRQF_SHARED | IRQF_TRIGGER_HIGH, uc->name, uc);
> +
> +	if (ret) {
> +		dev_err(ud->dev, "chan%d: ring irq request failed\n", uc->id);
> +		goto err_irq_free;
> +	}
> +
> +	uc->irq_num_udma = 0;
> +
> +	udma_reset_rings(uc);
> +
> +	INIT_DELAYED_WORK_ONSTACK(&uc->tx_drain.work,
> +				  udma_check_tx_completion);

[Severity: High]
Because request_irq() is called with IRQF_SHARED prior to initializing
the delayed work structure, could a pending spurious interrupt or shared
IRQ cause udma_v2_ring_irq_handler() to run concurrently?

If the handler processes a descriptor and calls schedule_delayed_work(),
it would operate on uninitialized memory before INIT_DELAYED_WORK_ONSTACK()
is executed.

[Severity: Medium]
Additionally, is INIT_DELAYED_WORK_ONSTACK() the correct macro to use here?

The uc object is part of ud->channels which is allocated on the heap via
devm_kcalloc() during setup. Using the _ONSTACK variant registers the
object with lockdep's object tracker as a stack object, which could corrupt
lockdep state tracking and leak memory in the tracker since there is no
matching destroy_delayed_work_on_stack() on teardown. This pattern also
appears in bcdma_v2_alloc_chan_resources().

[ ... ]
> @@ -1101,7 +1274,17 @@ static int udma_v2_probe(struct platform_device *pdev)
>  
>  	struct k3_ringacc_init_data ring_init_data = {0};
>  
> -	ring_init_data.num_rings = ud->bchan_cnt + ud->chan_cnt;
> +	if (ud->match_data->type == DMA_TYPE_BCDMA) {
> +		ring_init_data.num_rings = ud->bchan_cnt + ud->chan_cnt;
> +	} else if (ud->match_data->type == DMA_TYPE_PKTDMA) {
> +		ring_init_data.num_rings = ud->rflow_cnt;
> +
> +		ud->rflow_rt = devm_platform_ioremap_resource_byname(pdev, "ringrt");
> +		if (IS_ERR(ud->rflow_rt))
> +			return PTR_ERR(ud->rflow_rt);
> +
> +		ring_init_data.base_rt = ud->rflow_rt;
> +	}

[Severity: High]
Is ring_init_data.num_rings incorrectly sized here for PKTDMA V2?

It is currently set to only ud->rflow_cnt. If this should be
ud->tflow_cnt + ud->rflow_cnt, later calculations for fd_ring_id
in udma_v2_alloc_rx_resources() (which computes ud->tflow_cnt + rflow->id)
will generate IDs that exceed the allocated rings. Passing these
out-of-bounds IDs to k3_ringacc_request_rings_pair() could read and write
past the end of the ring arrays.

Furthermore, does initializing ud->rflow_rt at offset 0 cause the RX flows
to alias exactly over the TX flows' register space in hardware when
rflow->reg_rt is configured during probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17