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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dmaengine: ti: k3-udma-v2: IRQ name mismatch for PKTDMA mapped channels
- [Low] dmaengine: ti: k3-udma-v2: uninitialized pktdma_v2_driver check
- [Low] dmaengine: ti: k3-udma-glue: enum passed as boolean parameter
--

commit 0e040285780e7048563c2c5482644c2f9d9fa82b
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>
Subject: dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2

This commit adds support for PKTDMA V2, which differs from V1 in that it
uses unified channel spaces, direct IRQs, and static channel mapping instead
of PSIL pairs. It also implements the allocation of TX and RX resources for
the new PKTDMA V2 hardware.

> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
> --- a/drivers/dma/ti/k3-udma-glue.c
> +++ b/drivers/dma/ti/k3-udma-glue.c
[ ... ]
> @@ -166,6 +166,29 @@ static int of_k3_udma_glue_parse_chn(struct device_node *chn_np,
>  		goto out_put_spec;
>  
>  	thread_id = dma_spec.args[0];
> +
> +	/*
> +	 * V2 PKTDMA DT specifiers encode the hardware flow/channel ID in
> +	 * args[0] rather than the PSI-L thread ID used by V1. Resolve the
> +	 * PSI-L thread ID before the common path checks the direction bit.
> +	 */
> +	if (common->udmax->match_data->version == K3_UDMA_V2 &&
> +	    xudma_is_pktdma(common->udmax)) {
> +		struct psil_endpoint_config *ep_cfg;
> +		u32 psil_thread_id;
> +		bool dev_to_mem;
> +
> +		ep_cfg = psil_get_ep_config_by_id(thread_id, DMA_TYPE_PKTDMA,

[Severity: Low]
Is it intentional to pass the DMA_TYPE_PKTDMA enum as the boolean is_pktdma
argument here? While the enum value evaluates to true and behaves correctly
by accident, this appears to be a type mismatch. Should this use
xudma_is_pktdma(common->udmax) or a boolean literal instead?

> +						  &psil_thread_id, &dev_to_mem);
> +		if (IS_ERR(ep_cfg)) {
> +			dev_err(common->dev,
> +				"No PSI-L config for flow %u\n", thread_id);
> +			ret = PTR_ERR(ep_cfg);
> +			goto out_put_spec;
> +		}
> +		thread_id = psil_thread_id;
> +	}
> +
>  	if (dma_spec.args_count == 2) {
>  		if (dma_spec.args[1] > 2 && !xudma_is_pktdma(common->udmax)) {
>  			dev_err(common->dev, "Invalid channel atype: %u\n",

> diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
> --- a/drivers/dma/ti/k3-udma-private.c
> +++ b/drivers/dma/ti/k3-udma-private.c
[ ... ]
> @@ -174,16 +174,30 @@ 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;
> +	if (ud->match_data->version == K3_UDMA_V1) {
> +		const struct udma_oes_offsets *oes = &ud->soc_data->oes;
>  
> -	return msi_get_virq(ud->dev, udma_tflow_id + oes->pktdma_tchan_flow);
> +		return msi_get_virq(ud->dev, udma_tflow_id + oes->pktdma_tchan_flow);
> +	}
> +	struct platform_device *pdev = to_platform_device(ud->dev);
> +	char irq_name[10];
> +
> +	snprintf(irq_name, sizeof(irq_name), "tx-%u", udma_tflow_id);

[Severity: Medium]
Does this cause an IRQ name mismatch for PKTDMA mapped channels?

In pktdma_v2_alloc_chan_resources() in k3-udma-v2.c, the DMA engine channel
allocation uses mapped_channel_id (via irq_ring_idx) to construct the "tx-%u"
IRQ name:

    if (uc->config.dir == DMA_MEM_TO_DEV)
        snprintf(irq_name, sizeof(irq_name), "tx-%u", irq_ring_idx);

However, this exported accessor xudma_pktdma_tflow_get_irq() uses the flow ID
to construct the exact same prefix.

If mapped_channel_id differs from default_flow_id, will this cause
initialization to fail for one of the consumers because it cannot lookup the
expected IRQ name from the device tree?

> +	return platform_get_irq_byname(pdev, irq_name);
>  }

> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> --- a/drivers/dma/ti/k3-udma-v2.c
> +++ b/drivers/dma/ti/k3-udma-v2.c
[ ... ]
> @@ -870,6 +1012,7 @@ static int udma_v2_resume(struct dma_chan *chan)
>  }
>  
>  static struct platform_driver bcdma_v2_driver;
> +static struct platform_driver pktdma_v2_driver;
>  
>  static bool udma_v2_dma_filter_fn(struct dma_chan *chan, void *param)
>  {
> @@ -879,7 +1022,8 @@ static bool udma_v2_dma_filter_fn(struct dma_chan *chan, void *param)
>  	struct udma_chan *uc;
>  	struct udma_dev *ud;
>  
> -	if (chan->device->dev->driver != &bcdma_v2_driver.driver)
> +	if (chan->device->dev->driver != &bcdma_v2_driver.driver &&
> +	    chan->device->dev->driver != &pktdma_v2_driver.driver)

[Severity: Low]
Is pktdma_v2_driver intentionally left uninitialized here?

Since the device is probed by bcdma_v2_driver (which matches both the BCDMA
and PKTDMA compatible strings), this check against the unregistered driver
struct seems like it would always evaluate to false, leaving pktdma_v2_driver
entirely unused in memory. Does this structure need to be registered
somewhere, or can this check be safely removed?

>  		return false;
>  
>  	uc = to_udma_chan(chan);

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