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

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

[Aniket Randive]

Fix three issues in the gpi_ch_init() error path.

Use the indexed channel in the error_start_chan and
error_alloc_chan unwind loops instead of the original gchan
pointer. Otherwise, each iteration operates on the same channel,
leaving sibling channels unreset and undeallocated on failure.

Clear pm_state after freeing the event ring in the error path.
gpi_alloc_ev_chan() sets pm_state to ACTIVE_STATE, but the error
path frees ev_ring without restoring pm_state. As a result,
gpi_free_chan_resources() may attempt to deallocate an already
freed event ring and issue a redundant EV_CMD_DEALLOC.

Also free ch_ring in gpi_alloc_chan_resources() if gpi_ch_init()
fails. The ring is allocated before calling gpi_ch_init(), so a
failure leaves it leaked without this free.

Fix all three issues by unwinding the correct channels, restoring
pm_state to DISABLE_STATE after freeing the event ring, and freeing
ch_ring on gpi_ch_init() failure.

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---
 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);
 
-- 
2.34.1

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

[sashiko-bot]

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

New issues:
- [High] Incomplete Error Cleanup / Missing State Restoration causes 750ms kernel stalls per channel.
- [High] Missing state reversion combined with ring freeing causes IOMMU hardware faults (base address 0x0) on reallocation.
- [High] Hardware state leak and DMA Use-After-Free of the event ring.

Pre-existing issues:
- [High] Dangling pointer in `gchan->config` leads to Use-After-Free upon channel reallocation.
- [High] Deferred shared hardware initialization leaves a dangling channel vulnerable to MMIO bus faults.
--

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

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

This commit attempts to fix channel cleanup in the gpi_ch_init() error
path. It corrects the channel indices used in the unwind loops, restores
pm_state to DISABLE_STATE, and frees ch_ring on initialization failure.

> 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);

[Severity: High]
If gpi_start_chan() fails for channel 1, this error path stops and resets
channel 0, but does it leave gpii->gchan[0].pm_state as ACTIVE_STATE?

If gpii->pm_state is forced to DISABLE_STATE later in this unwind path, when
the client releases channel 0, gpi_free_chan_resources() might see
gchan[0].pm_state == ACTIVE_STATE and attempt to send STOP, RESET, and DE_ALLOC
commands.

Because gpii->pm_state is DISABLE_STATE, won't the interrupt handler drop
completion interrupts, causing these commands to time out and stall for 750ms?

>  	}
>  	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);

[Severity: High]
If initialization fails after the hardware event channel is allocated, we free
the event ring DMA memory here and force gpii->pm_state to DISABLE_STATE.

Later, when gpi_free_chan_resources() is called for the active channel, will
it skip sending GPI_EV_CMD_DEALLOC because cur_state is no longer ACTIVE_STATE?

Could this leave the hardware event channel active, leading to a DMA
use-after-free on the freed event ring memory?

>  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);

[Severity: High]
When gpi_ch_init() fails, it appears gchan->pm_state remains set to
CONFIG_STATE.

If we free and zero the ch_ring here without reverting gchan->pm_state, what
happens if a DMA client retries channel allocation?

Would a subsequent call to gpi_ch_init() bypass hardware initialization
because pm_state is already CONFIG_STATE, and program the hardware with a
zeroed physical address from the freed ring, leading to an IOMMU fault?

>  
>  	mutex_unlock(&gpii->ctrl_lock);
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential dangling pointer when
a DMA channel is released.

In drivers/dma/qcom/gpi.c:gpi_free_chan_resources():

        vchan_free_chan_resources(&gchan->vc);
        kfree(gchan->config);

Since gchan persists across clients, if a new client allocates the same channel
and calls dmaengine_slave_config(), this executes:

drivers/dma/qcom/gpi.c:gpi_peripheral_config():

        new_config = krealloc(gchan->config, config->peripheral_size, GFP_NOWAIT);

Does this pass a freed, non-null pointer into krealloc() if gchan->config wasn't
set to NULL after kfree()?

[Severity: High]
This is a pre-existing issue, but could deferred hardware initialization crash
the system if a client only requests one channel?

In drivers/dma/qcom/gpi.c:gpi_ch_init():

        for (i = 0; i < MAX_CHANNELS_PER_GPII; i++)
                if (gpii->gchan[i].pm_state != CONFIG_STATE)
                        goto exit_gpi_init;

If the first channel reaches CONFIG_STATE, it returns success immediately.
If a client then issues a transfer without initializing a second channel:

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);

Won't writing to the MMIO doorbell of an uninitialized (and potentially
unclocked) block cause a bus fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803120316.2703956-1-aniket.randive@oss.qualcomm.com?part=1

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

[Mukesh Savaliya]

On 8/3/2026 5:33 PM, Aniket Randive wrote:
> Fix three issues in the gpi_ch_init() error path.
> 
start here mentioning  the three problems first in generic way.
> Use the indexed channel in the error_start_chan and
> error_alloc_chan unwind loops instead of the original gchan
> pointer. Otherwise, each iteration operates on the same channel,
> leaving sibling channels unreset and undeallocated on failure.

meaning, channel cleanup not happened ? make it simpler anyway.
> 
> Clear pm_state after freeing the event ring in the error path.
> gpi_alloc_ev_chan() sets pm_state to ACTIVE_STATE, but the error
> path frees ev_ring without restoring pm_state. As a result,
> gpi_free_chan_resources() may attempt to deallocate an already
> freed event ring and issue a redundant EV_CMD_DEALLOC.
> 
Looking at your change, it looks we are cleaning up gpii iterating over 
each channel along with setting proper state.  Code seems simpler than 
the commit log, so request you to simplify.

> Also free ch_ring in gpi_alloc_chan_resources() if gpi_ch_init()
> fails. The ring is allocated before calling gpi_ch_init(), so a
> failure leaves it leaked without this free.
> 
> Fix all three issues by unwinding the correct channels, restoring
> pm_state to DISABLE_STATE after freeing the event ring, and freeing
> ch_ring on gpi_ch_init() failure.
> 
> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
> ---
>   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

[...]

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

[Aniket RANDIVE]

Thanks Mukesh for the review.

I will update the commit message as per your suggestion.

Thanks,
Aniket

On 8/7/2026 5:10 PM, Mukesh Savaliya wrote:
> 
> 
> On 8/3/2026 5:33 PM, Aniket Randive wrote:
>> Fix three issues in the gpi_ch_init() error path.
>>
> start here mentioning  the three problems first in generic way.
>> Use the indexed channel in the error_start_chan and
>> error_alloc_chan unwind loops instead of the original gchan
>> pointer. Otherwise, each iteration operates on the same channel,
>> leaving sibling channels unreset and undeallocated on failure.
> 
> meaning, channel cleanup not happened ? make it simpler anyway.
>>
>> Clear pm_state after freeing the event ring in the error path.
>> gpi_alloc_ev_chan() sets pm_state to ACTIVE_STATE, but the error
>> path frees ev_ring without restoring pm_state. As a result,
>> gpi_free_chan_resources() may attempt to deallocate an already
>> freed event ring and issue a redundant EV_CMD_DEALLOC.
>>
> Looking at your change, it looks we are cleaning up gpii iterating over 
> each channel along with setting proper state.  Code seems simpler than 
> the commit log, so request you to simplify.
> 
>> Also free ch_ring in gpi_alloc_chan_resources() if gpi_ch_init()
>> fails. The ring is allocated before calling gpi_ch_init(), so a
>> failure leaves it leaked without this free.
>>
>> Fix all three issues by unwinding the correct channels, restoring
>> pm_state to DISABLE_STATE after freeing the event ring, and freeing
>> ch_ring on gpi_ch_init() failure.
>>
>> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
>> ---
>>   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
> 
> [...]
>