[PATCH] dmaengine: pl330: fix autosuspend cleanup during removal

[PATCH] dmaengine: pl330: fix autosuspend cleanup during removal

[Guangshuo Li]

pl330_probe() calls pm_runtime_use_autosuspend(), but pl330_remove()
does not call the matching pm_runtime_dont_use_autosuspend() when
removing the device.

If the autosuspend delay is set to a negative value while autosuspend
is enabled, the runtime PM core increments usage_count to prevent
runtime suspend. Without calling pm_runtime_dont_use_autosuspend()
during teardown, this reference is not dropped and usage_count remains
unbalanced.

Add the missing pm_runtime_dont_use_autosuspend() call before restoring
the runtime PM usage reference in the remove path.

This issue was found by manual code inspection.

Fixes: ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/dma/pl330.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 25ba84b18704..8baadd104719 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3204,6 +3204,7 @@ static void pl330_remove(struct amba_device *adev)
 	struct dma_pl330_chan *pch, *_p;
 	int i, irq;
 
+	pm_runtime_dont_use_autosuspend(&adev->dev);
 	pm_runtime_get_noresume(pl330->ddma.dev);
 
 	if (adev->dev.of_node)
-- 
2.43.0

Re: [PATCH] dmaengine: pl330: fix autosuspend cleanup during removal

[sashiko-bot]

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

Pre-existing issues:
- [High] The IRQ handler accesses hardware registers unconditionally without checking if the device is active, risking a kernel panic during suspend.
- [High] Dynamically allocated `dma_pl330_desc` descriptors in the `desc_pool` are leaked when the driver unbinds.
- [High] The `peripherals` array allocated in `pl330_probe()` is never freed during device removal, causing a memory leak.
- [High] A debugfs file is created in the debugfs root but never removed, leading to a Use-After-Free upon unbind.
- [High] The IRQ handler is registered before the driver's synchronization primitives and tasklets are initialized, risking a kernel panic.
- [High] Casting an offset byte array pointer to `__le32 *` and dereferencing it causes an unaligned memory access.
--

--- Patch [1]: [PATCH] dmaengine: pl330: fix autosuspend cleanup during removal ---
commit e8b4012da1e87845c5bb71ff8b0d6b5e60fae048
Author: Guangshuo Li <lgs201920130244@gmail.com>

dmaengine: pl330: fix autosuspend cleanup during removal

pl330_probe() calls pm_runtime_use_autosuspend(), but pl330_remove()
does not call the matching pm_runtime_dont_use_autosuspend() when
removing the device. If the autosuspend delay is set to a negative value
while autosuspend is enabled, the runtime PM core increments usage_count
to prevent runtime suspend.

Cc: stable@vger.kernel.org

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 25ba84b187041..8baadd1047193 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -3204,6 +3204,7 @@ static void pl330_remove(struct amba_device *adev)
>  	struct dma_pl330_chan *pch, *_p;
>  	int i, irq;
>  
> +	pm_runtime_dont_use_autosuspend(&adev->dev);
>  	pm_runtime_get_noresume(pl330->ddma.dev);
>  
>  	if (adev->dev.of_node)

[Severity: High]
This isn't a bug introduced by this patch, but does the IRQ handler access
hardware registers unconditionally without checking if the device is active?

drivers/dma/pl330.c:pl330_irq_handler() {
    if (pl330_update(data))
        return IRQ_HANDLED;
    ...
}

drivers/dma/pl330.c:pl330_update() {
    ...
    val = readl(regs + FSM) & 0x1;
    ...
}

If a spurious interrupt occurs or an interrupt is in-flight while the device is
runtime suspended, pl330_update() unconditionally reads the FSM register. Since
the device clock is disabled during suspend, could accessing unclocked registers
result in an asynchronous bus fault on ARM architectures, leading to a system
crash?

[Severity: High]
This is a pre-existing issue, but are the dynamically allocated dma_pl330_desc
descriptors in the desc_pool leaked when the driver unbinds?

drivers/dma/pl330.c:add_desc() {
    ...
    desc = kzalloc_objs(*desc, count, flg);
    ...
}

During probe and normal operation, add_desc() dynamically allocates batches
of descriptors. However, the driver's teardown functions pl330_remove() and
pl330_del() completely fail to iterate over the desc_pool or the channel lists
to free these dynamically allocated descriptors:

drivers/dma/pl330.c:pl330_del() {
    ...
    dmac_free_threads(pl330);
    dma_free_attrs(pl330->ddma.dev, ...);
}

Does this cause a permanent memory leak of all descriptors each time the device
is unbound?

[Severity: High]
This is a pre-existing issue, but is the peripherals array leaked during
device removal?

drivers/dma/pl330.c:pl330_probe() {
    ...
    pl330->peripherals = kzalloc_objs(*pch, num_chan);
    if (!pl330->peripherals) {
    ...
}

In pl330_probe(), memory for the pl330->peripherals array is dynamically
allocated using kzalloc_objs(). However, neither the teardown loop in
pl330_remove() nor the cleanup function pl330_del() calls
kfree(pl330->peripherals).

Since the allocation is not managed via devres, is the array permanently leaked
upon driver unbind?

[Severity: High]
This is a pre-existing issue, but does a debugfs file creation lead to a
Use-After-Free upon unbind?

drivers/dma/pl330.c:init_pl330_debugfs() {
    debugfs_create_file(dev_name(pl330->ddma.dev),
                        S_IFREG | 0444, NULL, pl330,
                        &pl330_debugfs_fops);
}

The debugfs file is created in the debugfs root but never removed during
pl330_remove(). When the device unbinds, devres frees the pl330 struct, but
the debugfs file remains.

If userspace reads the file afterwards, does it trigger a Use-After-Free kernel
panic?

[Severity: High]
This is a pre-existing issue, but is the IRQ handler registered before the
driver's synchronization primitives and tasklets are initialized?

drivers/dma/pl330.c:pl330_probe() {
    ...
    ret = devm_request_irq(&adev->dev, irq, pl330_irq_handler, 0,
                           dev_name(&adev->dev), pl330);
    ...
    ret = pl330_add(pl330);
    ...
}

The hardware is taken out of reset and its IRQ is immediately requested.
However, the driver's primary spinlock and tasklet are not initialized until
pl330_add() runs later in the probe sequence:

drivers/dma/pl330.c:pl330_add() {
    ...
    spin_lock_init(&pl330->lock);
    ...
}

If an interrupt fires during this window (e.g., a pending interrupt from before
kexec), pl330_irq_handler() will lock an uninitialized spinlock and schedule an
uninitialized tasklet. Does this risk a kernel panic?

[Severity: High]
This is a pre-existing issue, but does casting an offset byte array pointer
to __le32 * and dereferencing it cause an unaligned memory access?

drivers/dma/pl330.c:_execute_DBGINSN() {
    ...
    val = le32_to_cpu(*((__le32 *)&insn[2]));
    ...
}

The code accesses the 3rd byte of the insn byte array. Because insn is a 6-byte
array allocated on the stack (e.g., in _stop()), &insn[2] is offset by 2 bytes
from the base pointer.

Does dereferencing this unaligned pointer as a 32-bit value violate C alignment
rules and trigger an alignment fault on architectures that do not support
unaligned accesses in hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808094452.2752443-1-lgs201920130244@gmail.com?part=1