Re: [PATCH 14/26] dmaengine: sh-shdmac: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Possible use-after-free of `sdev->schan` in `sh_dmae_err` during device probe failure or driver removal.
- [High] Unsafe hardware register access in a shared interrupt handler while the device is runtime suspended.
- [High] Failure of pm_runtime_get_sync() is logged but execution continues, potentially causing hardware faults.
--

--- Patch [14]: [PATCH 14/26] dmaengine: sh-shdmac: Remove redundant dev_err()/dev_err_probe() ---
commit c205bd1feae8c44a14e3a6ace262ed40dc6c1ba6
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: sh-shdmac: Remove redundant dev_err()/dev_err_probe()

The devm_request_irq() now automatically logs detailed error messages on
failure. This eliminates the need for driver-specific dev_err() and
dev_err_probe() calls that previously printed generic messages.

> diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c
> index 603e15102e45e..e0391f72384d3 100644
> --- a/drivers/dma/sh/shdmac.c
> +++ b/drivers/dma/sh/shdmac.c
> @@ -800,12 +800,8 @@ static int sh_dmae_probe(struct platform_device *pdev)
>  
>  		err = devm_request_irq(&pdev->dev, errirq, sh_dmae_err,
>  				       irqflags, "DMAC Address Error", shdev);

[Severity: High]
This is a pre-existing issue, but since we are requesting a potentially
shared interrupt here, is there a risk of accessing memory-mapped registers
of an unclocked or powered-down device?

If the device is runtime suspended and a shared interrupt fires from another
device, the handlers sh_dmae_err() and sh_dmae_chan_irq() unconditionally
read registers like DMAOR and CHCR.

Could this result in a synchronous external abort or system hang if the
module clock is disabled?

> -		if (err) {
> -			dev_err(&pdev->dev,
> -				"DMA failed requesting irq #%d, error %d\n",
> -				errirq, err);
> +		if (err)
>  			goto eirq_err;

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling path
have a use-after-free risk?

If a subsequent probe step fails and we take the eirq_err path, it calls
shdma_cleanup() which frees the sdev->schan array. However, the devm-managed
interrupt requested above is not freed until the probe function returns and
devres cleanup runs.

If the shared interrupt fires during this window, sh_dmae_err() executes. If
it reads a value with DMAOR_AE set, it calls sh_dmae_reset() -> shdma_reset()
which iterates over the freed sdev->schan array.

The same vulnerability appears to exist in sh_dmae_remove() where
shdma_cleanup() is called before the devm-managed interrupt is freed. Might
we need to explicitly free or disable the interrupt before cleaning up?

[Severity: High]
This is also a pre-existing issue, but I noticed earlier in sh_dmae_probe()
that we log a pm_runtime_get_sync() failure but continue execution:

drivers/dma/sh/shdmac.c:sh_dmae_probe() {
    ...
	pm_runtime_enable(&pdev->dev);
	err = pm_runtime_get_sync(&pdev->dev);
	if (err < 0)
		dev_err(&pdev->dev, "%s(): GET = %d\n", __func__, err);

	spin_lock_irq(&sh_dmae_lock);
	list_add_tail_rcu(&shdev->node, &sh_dmae_devices);
	spin_unlock_irq(&sh_dmae_lock);

	/* reset dma controller - only needed as a test */
	err = sh_dmae_rst(shdev);
    ...
}

Will this cause a fatal bus error? If the PM domain failed to power on the
device, accessing the memory-mapped registers in sh_dmae_rst() could cause a
crash. Should the probe process be aborted if pm_runtime_get_sync() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709135846.97972-1-panchuang@vivo.com?part=14