[PATCH v2] dmaengine: qcom: gpi: Fix channel cleanup in unwind path

[PATCH v2] dmaengine: qcom: gpi: Fix channel cleanup in unwind path

[Aniket Randive]

The gpi_ch_init() error path has three bugs: sibling channels are
not fully reset and deallocated, the event ring pm_state is left
stale after being freed, and ch_ring leaks if gpi_ch_init() fails.

Fix the unwind loops in error_start_chan and error_alloc_chan to
iterate over gpii->gchan[i] instead of the original gchan pointer,
so each sibling channel is properly reset and deallocated.

Restore pm_state to DISABLE_STATE after freeing the event ring, so
gpi_free_chan_resources() does not attempt to free an already freed
ring or issue a redundant EV_CMD_DEALLOC.

Free ch_ring in gpi_alloc_chan_resources() if gpi_ch_init() fails,
since the ring is allocated before the call and would otherwise leak.

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---
Changes in v2:
- Updated the commit message as per Mukesh suggestion.
---
 drivers/dma/qcom/gpi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..c41bfac65ddf 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -1965,16 +1965,19 @@ static int gpi_ch_init(struct gchan *gchan)
 error_start_chan:
 	for (i = i - 1; i >= 0; i--) {
 		gpi_stop_chan(&gpii->gchan[i]);
-		gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
+		gpi_send_cmd(gpii, &gpii->gchan[i], GPI_CH_CMD_RESET);
 	}
 	i = 2;
 error_alloc_chan:
 	for (i = i - 1; i >= 0; i--)
-		gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
+		gpi_reset_chan(&gpii->gchan[i], GPI_CH_CMD_DE_ALLOC);
 error_alloc_ev_ring:
 	gpi_disable_interrupts(gpii);
 error_config_int:
 	gpi_free_ring(&gpii->ev_ring, gpii);
+	write_lock_irq(&gpii->pm_lock);
+	gpii->pm_state = DISABLE_STATE;
+	write_unlock_irq(&gpii->pm_lock);
 exit_gpi_init:
 	return ret;
 }
@@ -2065,6 +2068,8 @@ static int gpi_alloc_chan_resources(struct dma_chan *chan)
 		goto xfer_alloc_err;
 
 	ret = gpi_ch_init(gchan);
+	if (ret)
+		gpi_free_ring(&gchan->ch_ring, gpii);
 
 	mutex_unlock(&gpii->ctrl_lock);
 

---
base-commit: 415606a7be939835db9b0d6b711887586646346d
change-id: 20260803-gpi_bug_fix-b0b80ef315b5

Best regards,
--  
Aniket Randive <aniket.randive@oss.qualcomm.com>

Re: [PATCH v2] dmaengine: qcom: gpi: Fix channel cleanup in unwind path

[sashiko-bot]

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

New issues:
- [High] Setting `pm_state = DISABLE_STATE` actively suppresses the necessary hardware `GPI_EV_CMD_DEALLOC` command, leaking hardware state.

Pre-existing issues:
- [High] `ev_ring` is freed on the `gpi_ch_init()` error path without explicitly canceling the event tasklet, leading to a race condition and UAF.
- [High] `gpi_ch_init()` error path destroys hardware state and shared resources for an already-active channel, leaving the sibling channel silently broken.
- [High] `gpi_issue_pending()` writes to the channel doorbell register without verifying that the hardware `pm_state` is active.
--

--- Patch [1]: [PATCH v2] dmaengine: qcom: gpi: Fix channel cleanup in unwind path ---
commit 47c71a5c319e41aac4e85d0f59c76a1364b2c273
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

dmaengine: qcom: gpi: Fix channel cleanup in unwind path

This commit attempts to fix three bugs in the gpi_ch_init() error path.
It updates the unwind loops to properly reset and deallocate sibling
channels, restores pm_state to DISABLE_STATE, and frees ch_ring if
initialization fails.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..c41bfac65ddf3 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -1965,16 +1965,19 @@ static int gpi_ch_init(struct gchan *gchan)
>  error_start_chan:
>  	for (i = i - 1; i >= 0; i--) {
>  		gpi_stop_chan(&gpii->gchan[i]);
> -		gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
> +		gpi_send_cmd(gpii, &gpii->gchan[i], GPI_CH_CMD_RESET);
>  	}
>  	i = 2;
>  error_alloc_chan:
>  	for (i = i - 1; i >= 0; i--)
> -		gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
> +		gpi_reset_chan(&gpii->gchan[i], GPI_CH_CMD_DE_ALLOC);

[Severity: High]
This is a pre-existing issue, but does this error path destroy hardware state
for an already-active channel?

gpi_ch_init() initializes both channels when the second channel is allocated.
If the second channel's initialization fails, this error path iterates
backwards, stopping and deallocating both channels and freeing the shared
ev_ring.

Since the first channel's client successfully completed
gpi_alloc_chan_resources() previously, could it remain unaware that its
hardware channel and event ring have just been destroyed?

[Severity: High]
This is also a pre-existing issue, but does this lead to a bus fault or
system stall if the first channel's client submits a transfer after the
sibling channel allocation fails?

If the first channel's client is unaware of the failure and calls
dma_issue_pending(), gpi_issue_pending() will execute:

drivers/dma/qcom/gpi.c:gpi_issue_pending() {
    ...
    gpi_desc->db = ch_ring->wp;
    gpi_write_ch_db(gchan, &gchan->ch_ring, gpi_desc->db);
    ...
}

Could this write directly to gchan->ch_cntxt_db_reg without checking if
REG_ACCESS_VALID(gpii->pm_state) is active, accessing unclocked or
deallocated hardware?

>  error_alloc_ev_ring:
>  	gpi_disable_interrupts(gpii);
>  error_config_int:
>  	gpi_free_ring(&gpii->ev_ring, gpii);

[Severity: High]
This is a pre-existing issue, but does this error path create a race condition
and use-after-free with the event tasklet?

If a channel fails gpi_ch_init(), gpi_disable_interrupts() frees the IRQ but
does not kill scheduled tasklets before gpi_free_ring() frees the memory.

If ev_task is already scheduled, it will bypass the state check because
pm_state remains ACTIVE_STATE during this time:

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

Will the tasklet then process events using the freed and zeroed ev_ring?

> +	write_lock_irq(&gpii->pm_lock);
> +	gpii->pm_state = DISABLE_STATE;
> +	write_unlock_irq(&gpii->pm_lock);

[Severity: High]
Does this regression actively prevent the necessary hardware GPI_EV_CMD_DEALLOC
command?

If the initialization fails, gpi_free_chan_resources() is called, but it
checks for ACTIVE_STATE before deallocating:

drivers/dma/qcom/gpi.c:gpi_free_chan_resources() {
    ...
    if (cur_state == ACTIVE_STATE)
        gpi_send_cmd(gpii, NULL, GPI_EV_CMD_DEALLOC);
    ...
}

Could this leak the hardware state because the hardware is never told to
release the event ring context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-gpi_bug_fix-v2-1-60555394cbf9@oss.qualcomm.com?part=1

Re: [PATCH v2] dmaengine: qcom: gpi: Fix channel cleanup in unwind path

[Mukesh Savaliya]

On 8/10/2026 12:21 PM, Aniket Randive wrote:
> The gpi_ch_init() error path has three bugs: sibling channels are
May be instead of three bugs, can actually mention issues
"start here mentioning  the three problems first in generic way."
is what i mentioned, not like write numbers :) .

May be misunderstood.

> not fully reset and deallocated, the event ring pm_state is left
> stale after being freed, and ch_ring leaks if gpi_ch_init() fails.
> 
> Fix the unwind loops in error_start_chan and error_alloc_chan to
> iterate over gpii->gchan[i] instead of the original gchan pointer,
> so each sibling channel is properly reset and deallocated.
> 
> Restore pm_state to DISABLE_STATE after freeing the event ring, so
> gpi_free_chan_resources() does not attempt to free an already freed
> ring or issue a redundant EV_CMD_DEALLOC.
> 
> Free ch_ring in gpi_alloc_chan_resources() if gpi_ch_init() fails,
> since the ring is allocated before the call and would otherwise leak.
> 
> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
> ---

Review if below looks fine, you may modify/change if anything wrong.

you may wait for other's review and make changes together. Do not upload 
v3 only for this immediately.

Title: Fix resource leaks in gpi_ch_init() error paths

The gpi_ch_init() unwind paths do not clean up resources correctly when
channel initialization fails.

The error_start_chan and error_alloc_chan labels iterate over the
original channel pointer instead of the channels stored in gpii->gchan[].
As a result, previously initialized sibling channels are not properly
reset and deallocated.

The event ring PM state is also left unchanged after the ring is freed.
Subsequent cleanup through gpi_free_chan_resources() may therefore
attempt to free the already released ring and issue a redundant
EV_CMD_DEALLOC command.

Additionally, gpi_alloc_chan_resources() allocates ch_ring before
calling gpi_ch_init(), but does not release it when gpi_ch_init() fails,
resulting in a memory leak.

Fix the unwind paths to operate on the correct channels, restore the
event ring PM state after freeing the ring, and release ch_ring when
channel initialization fails.