Re: [PATCH v2 38/64] dmaengine: img-mdc-dma: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Missing pm_runtime_set_suspended() in the driver removal path causes PM state mismatch on subsequent rebinds, leading to hardware faults.
- [High] The PM runtime usage counter is leaked in mdc_alloc_chan_resources() if pm_runtime_get_sync() fails.
- [High] Hardware registers are accessed in mdc_dma_probe() before the device clock is enabled.
- [High] The IRQ handler is registered before its associated virtual channel spinlock is initialized, allowing a spurious interrupt to lock an uninitialized spinlock.
--

--- Patch [38]: [PATCH v2 38/64] dmaengine: img-mdc-dma: kill vchan BH on teardown ---
commit b4d81ea29e678b96ef67f182eaf0b8fc7065fe66
Author: Allen Pais <[email protected]>

dmaengine: img-mdc-dma: kill vchan BH on teardown

Use dma_chan_kill_bh() for virt-dma cleanup.

> diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
> index b3765ba158030..374034c737c0e 100644
> --- a/drivers/dma/img-mdc-dma.c
> +++ b/drivers/dma/img-mdc-dma.c
[ ... ]
> @@ -1031,7 +1031,7 @@ static void mdc_dma_remove(struct platform_device *pdev)
>  
>  		devm_free_irq(&pdev->dev, mchan->irq, mchan);
>  
> -		tasklet_kill(&mchan->vc.task);
> +		dma_chan_kill_bh(&mchan->vc.chan);
>  	}
>  
>  	pm_runtime_disable(&pdev->dev);
>  	if (!pm_runtime_status_suspended(&pdev->dev))
>  		img_mdc_runtime_suspend(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but does mdc_dma_remove() need to call
pm_runtime_set_suspended() here?

When the driver is unbound and rebound via sysfs, the hardware is manually
suspended but the PM runtime state is left as RPM_ACTIVE. On rebind,
mdc_dma_probe() enables runtime PM but sees the device as already active.

When a channel is requested, won't pm_runtime_get_sync() return without
enabling the clock, leading to an imprecise external abort (kernel panic)
during subsequent register writes?


While reviewing this file, I noticed a few other pre-existing issues:

drivers/dma/img-mdc-dma.c:mdc_alloc_chan_resources()
> 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);
> }

[Severity: High]
This isn't a bug introduced by this patch, but does mdc_alloc_chan_resources()
leak the PM runtime usage counter if pm_runtime_get_sync() fails?

If pm_runtime_get_sync() returns an error (e.g., -EACCES), the usage counter
is incremented but the error is directly propagated. Since the allocation
aborts, the free routine is never called to decrement the counter.

Doesn't this permanently prevent the device (and potentially the parent power
domain) from entering low-power suspend states? Should this check the return
value and call pm_runtime_put_noidle() on failure?


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);
[ ... ]
> 	val = mdc_readl(mdma, MDC_GLOBAL_CONFIG_A);

[Severity: High]
This is a pre-existing issue, but are hardware registers accessed in
mdc_dma_probe() before the device clock is enabled?

mdc_readl() reads MDC_GLOBAL_CONFIG_A shortly after getting the "sys" clock,
but the clock appears to only be enabled later in img_mdc_runtime_resume().

On systems where the bootloader doesn't leave the clock enabled, wouldn't
reading from unclocked peripheral registers cause an imprecise external abort
(kernel panic) during device initialization?


drivers/dma/img-mdc-dma.c:mdc_dma_probe()
> 		ret = devm_request_irq(&pdev->dev, mchan->irq, mdc_chan_irq,
> 				       IRQ_TYPE_LEVEL_HIGH,
> 				       dev_name(&pdev->dev), mchan);
> 		if (ret < 0)
> 			return ret;
> 
> 		mchan->vc.desc_free = mdc_desc_free;
> 		vchan_init(&mchan->vc, &mdma->dma_dev);

[Severity: High]
This isn't a bug introduced by this patch, but is the IRQ handler registered
before its associated virtual channel spinlock is initialized?

devm_request_irq() is called before vchan_init() initializes mchan->vc.lock.
If the IRQ line is asserted immediately after registration (e.g. from a warm
boot or shared interrupt), mdc_chan_irq() will fire and attempt to acquire a
zero-initialized lock.

Could this trigger a kernel panic with lock debugging enabled?

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