Re: [PATCH] dmaengine: img-mdc: Fix runtime PM usage counter leak

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

Pre-existing issues:
- [High] Hardware register access before clock enablement during device probe.
- [High] NULL pointer dereference when calculating residue for cyclic transfers.
--

--- Patch [1]: [PATCH] dmaengine: img-mdc: Fix runtime PM usage counter leak ---
commit ab74c845981a8cdefe6ab65f16f64bbdc3a65623
Author: Ruoyu Wang <[email protected]>

dmaengine: img-mdc: Fix runtime PM usage counter leak

pm_runtime_get_sync() leaves the IMG MDC device's usage counter
incremented when runtime resume fails. This patch replaces it with
pm_runtime_resume_and_get() so a failed resume does not retain a usage
reference, preventing runtime PM usage counter leaks.

> diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
> index b3765ba158030..a3192d49b6c86 100644
> --- a/drivers/dma/img-mdc-dma.c
> +++ b/drivers/dma/img-mdc-dma.c
> @@ -738,7 +738,7 @@ static int mdc_alloc_chan_resources(struct dma_chan *chan)
>  	struct mdc_chan *mchan = to_mdc_chan(chan);
>  	struct device *dev = mdma2dev(mchan->mdma);
>  
> -	return pm_runtime_get_sync(dev);
> +	return pm_runtime_resume_and_get(dev);
>  }
>  
>  static void mdc_free_chan_resources(struct dma_chan *chan)

[Severity: High]
This is a pre-existing issue, but I noticed a potential hardware register
access before clock enablement during device probe:

drivers/dma/img-mdc-dma.c:mdc_dma_probe() {
    ...
	mdma->clk = devm_clk_get(&pdev->dev, "sys");
	if (IS_ERR(mdma->clk))
		return PTR_ERR(mdma->clk);

	dma_cap_zero(mdma->dma_dev.cap_mask);
	dma_cap_set(DMA_SLAVE, mdma->dma_dev.cap_mask);
	dma_cap_set(DMA_PRIVATE, mdma->dma_dev.cap_mask);
	dma_cap_set(DMA_CYCLIC, mdma->dma_dev.cap_mask);
	dma_cap_set(DMA_MEMCPY, mdma->dma_dev.cap_mask);

	val = mdc_readl(mdma, MDC_GLOBAL_CONFIG_A);
    ...
}

In mdc_dma_probe(), MDC_GLOBAL_CONFIG_A is read to determine hardware
capabilities immediately after fetching the clock, but before the clock is
ever enabled. The clock is explicitly enabled only later via
img_mdc_runtime_resume() or by PM runtime. 

If the bootloader has not left the clock enabled, could accessing the
unclocked peripheral's registers cause a system hang or synchronous external
abort?

[Severity: High]
This is also a pre-existing issue, but is it possible to encounter a NULL
pointer dereference when calculating the residue for cyclic transfers?

drivers/dma/img-mdc-dma.c:mdc_tx_status() {
    ...
		bytes = mdesc->list_xfer_size;
		ldesc = mdesc->list;
		for (i = 0; i < cmds; i++) {
			bytes -= ldesc->xfer_size + 1;
			ldesc = ldesc->next_desc;
		}
    ...
}

In mdc_tx_status(), the cmds variable is computed by adding unhandled hardware
completions to mdesc->list_cmds_done. In cyclic mode, the CPU-side descriptor
list is not circular, so the last descriptor's next_desc is NULL.

If the interrupt handler is delayed due to high system load, cmds might exceed
the list length. Under these conditions, wouldn't the loop traverse past the
end of the list, causing ldesc to become NULL and dereferencing it on the
subsequent loop iteration's ldesc->xfer_size read?

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