Re: [PATCH v8 07/19] dmaengine: ti: k3-udma: Add variant-specific function pointers to udma_dev

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

New issues:
- [Medium] Unconditional invocation of optional psil_pair and psil_unpair function pointers leads to potential NULL pointer dereferences.

Pre-existing issues:
- [High] Debug objects leak due to misuse of INIT_DELAYED_WORK_ONSTACK for heap-allocated work items.
- [High] Unconditional destruction of hdesc_pool in the error path of pktdma_alloc_chan_resources causes a double-free.
--

--- Patch [7]: [PATCH v8 07/19] dmaengine: ti: k3-udma: Add variant-specific function pointers to udma_dev ---
commit e169d39794002b63060719c1aeef8f8eb77d45c6
Author: Sai Sree Kartheek Adivi <[email protected]>

dmaengine: ti: k3-udma: Add variant-specific function pointers to udma_dev

Introduce function pointers in the udma_dev structure to allow
variant-specific implementations for certain operations.
This prepares the driver for supporting multiple K3 UDMA variants,
such as UDMA v2, with minimal code duplication.

> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 40ba114ac81dd..c44a12157e65f 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
[ ... ]
> @@ -1663,7 +1663,7 @@ static int udma_alloc_chan_resources(struct dma_chan *chan)
>  	}
>  
>  	/* PSI-L pairing */
> -	ret = navss_psil_pair(ud, uc->config.src_thread, uc->config.dst_thread);
> +	ret = ud->psil_pair(ud, uc->config.src_thread, uc->config.dst_thread);

[Severity: Medium]
Will this crash if a future variant does not support PSI-L pairing?

The wrapper function xudma_navss_psil_pair() checks if ud->psil_pair is set
before calling it, but this direct call does not. If a future variant leaves
this pointer unassigned, couldn't this result in a NULL pointer dereference?

[ ... ]
> @@ -1849,8 +1849,8 @@ static int bcdma_alloc_chan_resources(struct dma_chan *chan)
>  		uc->use_dma_pool = true;
>  	} else if (uc->config.dir != DMA_MEM_TO_MEM) {
>  		/* PSI-L pairing */
> -		ret = navss_psil_pair(ud, uc->config.src_thread,
> -				      uc->config.dst_thread);
> +		ret = ud->psil_pair(ud, uc->config.src_thread,
> +				    uc->config.dst_thread);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing issue
in bcdma_alloc_chan_resources() and pktdma_alloc_chan_resources(). It looks
like the work item is initialized as a stack object but it's embedded within
the heap-allocated struct udma_chan:

	INIT_DELAYED_WORK_ONSTACK(&uc->tx_drain.work,
				  udma_check_tx_completion);

Since destroy_delayed_work_on_stack() is never called when the channel is
freed, wouldn't every allocation/free cycle leak a debug object, eventually
leading to ODEBUG OOM warnings?

[ ... ]
> @@ -2083,7 +2083,7 @@ static int pktdma_alloc_chan_resources(struct dma_chan *chan)
>  err_irq_free:
>  	uc->irq_num_ring = 0;
>  err_psi_free:
> -	navss_psil_unpair(ud, uc->config.src_thread, uc->config.dst_thread);
> +	ud->psil_unpair(ud, uc->config.src_thread, uc->config.dst_thread);
>  	uc->psil_paired = false;
>  err_res_free:
>  	udma_free_tx_resources(uc);

[Severity: High]
This is a pre-existing issue, but the error path in 
pktdma_alloc_chan_resources() unconditionally destroys the dma pool:

err_res_free:
	udma_free_tx_resources(uc);
	udma_free_rx_resources(uc);
	udma_reset_uchan(uc);
	dma_pool_destroy(uc->hdesc_pool);

If a channel is freed (leaving uc->hdesc_pool as a dangling pointer), and
later re-requested but fails early in the allocation process, wouldn't this
jump to err_res_free and cause a double-free of the pool?

Unlike other paths, there's no check for uc->use_dma_pool here.

[ ... ]
> @@ -2374,8 +2374,8 @@ static void udma_free_chan_resources(struct dma_chan *chan)
>  
>  	/* Release PSI-L pairing */
>  	if (uc->psil_paired) {
> -		navss_psil_unpair(ud, uc->config.src_thread,
> -				  uc->config.dst_thread);
> +		ud->psil_unpair(ud, uc->config.src_thread,
> +				uc->config.dst_thread);

[Severity: Medium]
As mentioned earlier, does this need a check for ud->psil_unpair being
non-NULL to avoid a potential panic if a future variant lacks PSI-L pairing
support?

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