Re: [PATCH 2/4] dmaengine: ste_dma40: Fix probe runtime PM disable

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

New issues:
- [Medium] The commit message claims to fix a bug regarding a 'later successful deferred probe', but deferred probe is structurally impossible for this driver.

Pre-existing issues:
- [High] Calling `free_pages()` on an `ioremap` mapped address in the probe error path.
- [High] IRQ handler accesses hardware registers without checking runtime PM state.
- [High] Passing a raw page count instead of an allocation order to `__get_free_pages()` and `free_pages()`.
--

--- Patch [2]: [PATCH 2/4] dmaengine: ste_dma40: Fix probe runtime PM disable ---
commit 724a6133520581f191bdd536bd3123c1d0c353fb
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix probe runtime PM disable

d40_probe() can jump to destroy_cache before runtime PM has been enabled
for the DMA controller device. Calling pm_runtime_disable() on that path
increments disable_depth, which can leave runtime PM disabled after a later
successful deferred probe.

Reported-by: [email protected]
Closes: https://lore.kernel.org/dmaengine/[email protected]/
Assisted-by: Codex:gpt-5-5
Signed-off-by: Linus Walleij <[email protected]>

> d40_probe() can jump to destroy_cache before runtime PM has been enabled
> for the DMA controller device. Calling pm_runtime_disable() on that path
> increments disable_depth, which can leave runtime PM disabled after a later
> successful deferred probe.

[Severity: Medium]
Is this description accurate regarding deferred probe?

The stedma40 driver is registered using platform_driver_probe() via
stedma40_init(), which sets drv->prevent_deferred_probe = true and prevents
any future binds.

A later successful deferred probe appears to be impossible here, even though
fixing the unbalanced disable_depth is still semantically correct.

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index d5837aa0a789a..0c8ff299c2c2d 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c

[ ... ]

> @@ -3658,7 +3660,8 @@ static int __init d40_probe(struct platform_device *pdev)
>  		regulator_disable(base->lcpa_regulator);
>  		regulator_put(base->lcpa_regulator);
>  	}

[Severity: High]
This is a pre-existing issue, but does the error path in destroy_cache
call free_pages() on an ioremap address?

If base->plat_data->use_esram_lcla is true, memory is mapped using
devm_ioremap() and assigned to base->lcla_pool.base. Just above this
regulator cleanup in destroy_cache, the code does:

    if (!base->lcla_pool.base_unaligned && base->lcla_pool.base)
        free_pages((unsigned long)base->lcla_pool.base,
                   base->lcla_pool.pages);

Calling virt_to_page() on an ioremap address yields a garbage pointer,
which would cause a crash when __free_pages() dereferences it.

> -	pm_runtime_disable(base->dev);
> +	if (runtime_pm_enabled)
> +		pm_runtime_disable(base->dev);
>  
>   report_failure:
>  	d40_err(dev, "probe failed\n");

[Severity: High]
This is a pre-existing issue, but does the interrupt handler access
hardware registers without checking the runtime PM state?

A spurious interrupt could fire while the device is in runtime suspend.
The IRQ handler d40_handle_interrupt() unconditionally accesses the
PrimeCell registers:

drivers/dma/ste_dma40.c:d40_handle_interrupt() {
    ...
    regs[i] = readl(base->virtbase + il[i].src);
    ...
}

Since dma40_runtime_suspend() turns off the GCC clock, reading the unclocked
registers without calling pm_runtime_get_if_active() could cause a bus
fault (external abort).

[Severity: High]
This is a pre-existing issue, but does d40_lcla_allocate() pass a raw
page count instead of an allocation order?

During allocation, base->lcla_pool.pages is calculated as the absolute
number of pages needed:

drivers/dma/ste_dma40.c:d40_lcla_allocate() {
    ...
    base->lcla_pool.pages = SZ_1K * base->num_phy_chans / PAGE_SIZE;
    ...
    page_list[i] = __get_free_pages(GFP_KERNEL,
                                    base->lcla_pool.pages);
    ...
}

This raw count is then erroneously passed as the order parameter to
__get_free_pages(). For example, an order of 8 would allocate 256 pages
instead of the 8 pages actually needed, leading to wasted memory or
potential allocation failures.

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