Re: [PATCH v7 2/2] dmaengine: qcom-gpi: Keep GPI interrupt active during system resume

[sashiko-bot@kernel.org]

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

New issues:
- [High] Adding IRQF_NO_SUSPEND leaves the interrupt unmasked during system sleep, causing fatal unclocked register accesses.
- [Low] The IRQF_EARLY_RESUME flag is functionally dead code and redundant when combined with IRQF_NO_SUSPEND.

Pre-existing issues:
- [High] A 3-way AB-BA deadlock occurs because gpi_ev_tasklet() acquires a read lock on pm_lock with local hardware interrupts enabled.
- [High] The error cleanup paths in gpi_ch_init() operate on the wrong channel pointer, causing leaks and multiple resets of the same channel.
- [High] Unregistered DMA device on the probe error path leads to a Use-After-Free and global list corruption.
- [High] tasklet_kill is called while the interrupt is still unmasked and enabled, risking a Use-After-Free on the event ring.
- [High] The interrupt handler returns IRQ_HANDLED when bailing out due to an invalid PM state, causing a permanent interrupt storm.
--

--- Patch [2]: [PATCH v7 2/2] dmaengine: qcom-gpi: Keep GPI interrupt active during system resume ---
commit a7024226a78be6fe1a44f0ec7dd85e67623b43ab
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

dmaengine: qcom-gpi: Keep GPI interrupt active during system resume

GPI DMA clients may initiate transfers during the early stages of
system resume before the normal IRQ resume phase has completed.
Request the GPI interrupt with IRQF_NO_SUSPEND and IRQF_EARLY_RESUME
to keep it available across suspend/resume transitions.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..29872b6cb2c75 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -615,7 +615,8 @@ static int gpi_config_interrupts(struct gpii *gpii, enum gpii_irq_settings setti
>  
>  	if (!gpii->configured_irq) {
>  		ret = devm_request_irq(gpii->gpi_dev->dev, gpii->irq,
> -				       gpi_handle_irq, IRQF_TRIGGER_HIGH,
> +				       gpi_handle_irq,
> +				       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND | IRQF_EARLY_RESUME,

[Severity: High]
Adding IRQF_NO_SUSPEND prevents the DMA interrupt from being masked by the
core IRQ subsystem during system suspend. Does this allow spurious or
delayed hardware interrupts to trigger while consumer driver hardware clocks
are disabled via runtime PM?

Looking at gpi_handle_irq(), the pm_state check may remain in ACTIVE_STATE
or PAUSE_STATE during suspend:

drivers/dma/qcom/gpi.c:gpi_handle_irq() {
    if (!REG_ACCESS_VALID(gpii->pm_state)) {
    ...
}

If this check passes while clocks are off, will the subsequent register
reads cause a synchronous external abort and crash the kernel?

[Severity: Low]
Is IRQF_EARLY_RESUME redundant here? When combined with IRQF_NO_SUSPEND,
IRQS_SUSPENDED is never set on the interrupt descriptor during
suspend_device_irqs(). This means resume_irq() won't do anything for this
interrupt during the early resume phase, making the flag functionally dead
code.

>  				       "gpi-dma", gpii);
>  		if (ret < 0) {

[ ... ]

[Severity: High]
This is a pre-existing issue, but can a 3-way AB-BA deadlock occur with
pm_lock?

gpi_ev_tasklet() running in softirq acquires a read lock on pm_lock without
disabling local hardware interrupts:

drivers/dma/qcom/gpi.c:gpi_ev_tasklet() {
    ...
    read_lock(&gpii->pm_lock);
    ...
}

A concurrent thread calls write_lock_irq():

drivers/dma/qcom/gpi.c:gpi_resume() {
    ...
    write_lock_irq(&gpii->pm_lock);
    ...
}

Due to fair rwlock semantics, the writer blocks new readers. If a hard
interrupt then fires on the tasklet's CPU, gpi_handle_irq() preempts the
tasklet and acquires the read lock:

drivers/dma/qcom/gpi.c:gpi_handle_irq() {
    ...
    read_lock_irqsave(&gpii->pm_lock, flags);
    ...
}

Will the hardirq reader spin waiting for the writer, while the writer spins
waiting for the preempted tasklet, creating a fatal deadlock?

[Severity: High]
This is a pre-existing issue, but do the error cleanup paths in
gpi_ch_init() operate on the wrong channel pointer?

If channel allocation fails, the error handlers iterate backwards but pass
the function argument gchan instead of the iterated element &gpii->gchan[i]:

drivers/dma/qcom/gpi.c:gpi_ch_init() {
    ...
error_start_chan:
    gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
    ...
error_alloc_chan:
    gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
    ...
}

Does this repeatedly reset the same channel while failing to clean up the
newly allocated channels, leaking hardware resources and memory?

[Severity: High]
This is a pre-existing issue, but does gpi_probe() leak the DMA device on
failure?

If of_dma_controller_register() fails, the function returns directly:

drivers/dma/qcom/gpi.c:gpi_probe() {
    ...
    ret = of_dma_controller_register(...);
    if (ret)
        return ret;
    ...
}

Since dma_async_device_register() was already called earlier in the probe,
does this leave a dangling pointer in the global dma_device_list when
devres frees the gpi_dev structure?

[Severity: High]
This is a pre-existing issue, but is there a race condition in
gpi_free_chan_resources() that could cause a use-after-free?

The function kills the tasklet before disabling interrupts:

drivers/dma/qcom/gpi.c:gpi_free_chan_resources() {
    ...
    tasklet_kill(&gpii->ev_task);
    ...
    gpi_disable_interrupts(gpii);
    ...
}

If a pending or spurious IEOB interrupt arrives immediately after
tasklet_kill returns, could gpi_handle_irq() reschedule the tasklet,
allowing it to execute concurrently with or after
gpi_free_ring(&gpii->ev_ring, gpii), and access the freed event ring memory?

[Severity: High]
This is a pre-existing issue, but does returning IRQ_HANDLED when bailing
out cause a permanent interrupt storm?

If the pm_state check fails, gpi_handle_irq() jumps to exit_irq:

drivers/dma/qcom/gpi.c:gpi_handle_irq() {
    if (!REG_ACCESS_VALID(gpii->pm_state)) {
        ...
        goto exit_irq;
    }
    ...
exit_irq:
    return IRQ_HANDLED;
}

Since the interrupt is level-triggered, does falsely reporting IRQ_HANDLED
without servicing and clearing the device hardware condition cause the
interrupt controller to immediately re-assert the interrupt to the CPU,
creating an infinite loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709134623.1724212-1-mukesh.savaliya@oss.qualcomm.com?part=2