Re: [PATCH v2 07/13] dmaengine: ste_dma40: Fix probe runtime PM disable

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

Pre-existing issues:
- [High] request_irq() is called before PM runtime is enabled, which can cause an IRQ storm if an interrupt is pending.
- [High] The driver systematically ignores the return value of pm_runtime_get_sync(), failing to handle resume errors.
--

--- Patch [7]: [PATCH v2 07/13] dmaengine: ste_dma40: Fix probe runtime PM disable ---
commit 95c55fe7afd321bc228d4b1a735de2f4eb7a79fc
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix probe runtime PM disable

This commit fixes an issue where some d40_probe() error paths disable
runtime PM even if it was never enabled for the device. It introduces a
boolean flag to track whether runtime PM was actually enabled, keeping the
disable_depth balanced during probe failures.

Closes: https://lore.kernel.org/dmaengine/[email protected]/

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 739e47941d0cc..0c9094b4c4ade 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
[ ... ]
> @@ -3633,6 +3634,7 @@ static int __init d40_probe(struct platform_device *pdev)
>  	pm_runtime_mark_last_busy(base->dev);
>  	pm_runtime_set_active(base->dev);
>  	pm_runtime_enable(base->dev);
> +	runtime_pm_enabled = true;

[Severity: High]
This is a pre-existing issue, but request_irq() is called before PM
runtime is enabled, which could cause an IRQ storm if an interrupt is
pending.

If an interrupt triggers before runtime PM is enabled, the ISR
d40_handle_interrupt() fails to clear it:

d40_handle_interrupt() {
    ...
    ret = pm_runtime_get_if_active(base->dev);
    if (ret <= 0)
        return IRQ_NONE;
    ...
}

Because pm_runtime_get_if_active() returns an error when runtime PM is
disabled, the handler returns IRQ_NONE without clearing the hardware
interrupt. This causes the interrupt to continually re-trigger.

Could the request_irq() call be moved after pm_runtime_enable() to
prevent this?

[Severity: High]
This is a pre-existing issue, but the driver systematically ignores the
return value of pm_runtime_get_sync().

If the device fails to resume due to PM subsystem issues or memory
pressure, does this cause the driver to access powered-off hardware
registers?

For example, in d40_queue_start():

d40_queue_start() {
    ...
    pm_runtime_get_sync(d40c->base->dev);
    /* hardware accesses follow */
    ...
}

Similar unchecked calls exist in d40_pause(), d40_resume(), and
d40_free_dma(). Could these be updated to check for negative error codes
and abort if the resume fails?

>  
>  	dma_set_max_seg_size(base->dev, STEDMA40_MAX_SEG_SIZE);
>  
> @@ -3678,7 +3680,8 @@ static int __init d40_probe(struct platform_device *pdev)
>  		regulator_disable(base->lcpa_regulator);
>  		regulator_put(base->lcpa_regulator);
>  	}
> -	pm_runtime_disable(base->dev);
> +	if (runtime_pm_enabled)
> +		pm_runtime_disable(base->dev);
>  
>   report_failure:
>  	d40_err(dev, "probe failed\n");

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