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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Unconditional replacement of MSI interrupt fetching breaks existing V1 PKTDMA devices.
- [High] Unconditional dma_pool_destroy in error path causes a double-free of a dangling pointer.
- [High] udma_v2_stop writes to PEER registers on PKTDMA V2, violating hardware spec.
- [Low] Uninitialized static platform_driver used in DMA filter function.

Pre-existing issues:
- [High] DMA Use-After-Free if channel refuses to stop during allocation.
--

--- Patch [17]: [PATCH v8 17/19] dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2 ---
commit 97b7da552f385a68cda5edae3dfd22d57863e91d
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.

The changes in PKTDMA V2 are:
[ ... ]
> - Direct IRQs: There is no INT-A and interrupt lines from DMA are
>   directly connected to GIC.
> - Remote side configuration handled by DMA. So no need to write to
>   PEER registers to START / STOP / PAUSE / TEARDOWN.

[Severity: High]
Does the implementation match this description? udma_v2_stop() is used by
PKTDMA V2 through the udma_v2_probe() assignment, but it unconditionally
writes to UDMA_CHAN_RT_PEER_REG for all non-MEM_TO_MEM transfers. 

Will writing to these PEER registers on PKTDMA V2 hardware cause any
unexpected hardware exceptions?

> diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
> index 44c097fff5ee6..92d44ef9593d1 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), "tx-%u", udma_tflow_id);
> +	return platform_get_irq_byname(pdev, irq_name);
>  }

[Severity: Critical]
Does this unconditional replacement break existing V1 PKTDMA devices?

Client drivers for V1 devices that share this exported function will now call
platform_get_irq_byname() instead of msi_get_virq().

Since V1 device tree nodes do not define named "tx-%u" interrupts, will this
cause backward compatibility breakage and failures for V1 hardware?

>  EXPORT_SYMBOL(xudma_pktdma_tflow_get_irq);
>  
>  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), "rx-%u", udma_rflow_id);
> +	return platform_get_irq_byname(pdev, irq_name);
>  }

[Severity: Critical]
Similarly, does this break the RX flow interrupt fetching for V1 devices
that rely on the older MSI approach?

> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> index b9ce58d1934a9..c46341e768cdc 100644
> --- a/drivers/dma/ti/k3-udma-v2.c
> +++ b/drivers/dma/ti/k3-udma-v2.c
> @@ -764,6 +764,146 @@ static int bcdma_v2_alloc_chan_resources(struct dma_chan *chan)
>  	return ret;
>  }
>  
> +static int pktdma_v2_alloc_chan_resources(struct dma_chan *chan)
> +{
[ ... ]
> +	if (udma_is_chan_running(uc)) {
> +		dev_warn(ud->dev, "chan%d: is running!\n", uc->id);
> +		ud->reset_chan(uc, false);
> +		if (udma_is_chan_running(uc)) {
> +			dev_err(ud->dev, "chan%d: won't stop!\n", uc->id);
> +			ret = -EBUSY;
> +			goto err_res_free;
> +		}
> +	}

[Severity: High]
This is a pre-existing issue, but can this lead to a DMA use-after-free
if the channel refuses to stop? 

The error path frees the newly allocated DMA rings, but the hardware ring
was already configured to point to this memory earlier in the function via
udma_v2_alloc_tx_resources() or udma_v2_alloc_rx_resources(). 

If the hardware is still running, could it continue writing to the freed
memory?

[ ... ]
> +err_res_free:
> +	udma_free_tx_resources(uc);
> +	udma_free_rx_resources(uc);
> +
> +	udma_reset_uchan(uc);
> +
> +	dma_pool_destroy(uc->hdesc_pool);
> +	uc->use_dma_pool = false;
> +
> +	return ret;
> +}

[Severity: High]
Can this cause a double-free of a dangling pointer? 

Unlike the BCDMA equivalent, the dma_pool_destroy() call here is
unconditional. If the error path is taken early (e.g., from the
udma_is_chan_running() check above) before a new pool is allocated,
uc->hdesc_pool might still point to an already destroyed pool.

Should this be guarded with a check for uc->use_dma_pool?

> @@ -864,6 +1004,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)
>  {
> @@ -873,7 +1014,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)
>  		return false;

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

It is declared as a static struct platform_driver, but doesn't appear to be
defined anywhere in this patch, making this pointer comparison potentially
meaningless.

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