Re: [PATCH v3] dmaengine: qcom: gpi: Fix resource leaks as part of channel clean up

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

New issues:
- [Medium] Inconsistent software state after channel rollback causes duplicate shutdown commands and timeouts during client teardown.

Pre-existing issues:
- [Critical] gpi_free_chan_resources() unconditionally frees ev_ring, causing a double-free crash when an initialization failure occurs.
- [Critical] Hardware event ring is not deallocated in the gpi_ch_init() error path, causing a permanent DMA Use-After-Free.
- [High] Event processing tasklet is not killed before freeing ev_ring in the error path, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v3] dmaengine: qcom: gpi: Fix resource leaks as part of channel clean up ---

commit f50a0a6a0fc075e17ad27e65c7651c6aa5bb6a12
Author: Aniket Randive [off-list ref]

dmaengine: qcom: gpi: Fix resource leaks as part of channel clean up

This commit fixes the gpi_ch_init() and gpi_alloc_chan_resources() error
handling paths to properly iterate over the sibling channels, release
the ch_ring when channel initialization fails, and restore the event ring
state after freeing the ring.

Link: https://patch.msgid.link/20260810-gpi_bug_fix-v2-1-60555394cbf9@oss.qualcomm.com (local)

[ ... ]

[Severity: Medium]
Does this sequence leave the sibling channel's software state inconsistent?

When gpi_start_chan() fails for the second channel, this loop correctly
rolls back the sibling channel in hardware by sending the STOP, RESET, and
DE_ALLOC commands.

However, it appears to leave the sibling's pm_state as ACTIVE_STATE.
During client teardown, gpi_free_chan_resources() observes ACTIVE_STATE
and incorrectly re-issues these commands to the hardware:

gpi_free_chan_resources() {
    ...
    if (cur_state == ACTIVE_STATE) {
        ...
        gpi_send_cmd(gpii, gchan, GPI_CH_CMD_STOP);
    }
}

Because the hardware is already deallocated, do the commands time out
in wait_for_completion_timeout() and cause a blocking delay regression?

[Severity: High]
This is a pre-existing issue, but does this error path need to kill the
event processing tasklet before proceeding to free the ring?

If an error occurs after gpi_alloc_ev_chan() succeeds, a hardware
interrupt could fire and schedule ev_task. A concurrently executing
tasklet might read from the freed ev_ring DMA memory before pm_state is
updated. 

Would calling tasklet_kill(&gpii->ev_task) before freeing the ring prevent
this regression where memory is used after being freed?

[Severity: Critical]
This is a pre-existing issue, but is the hardware event ring deallocated
before we free its memory here?

If the error path frees ev_ring without sending the GPI_EV_CMD_DEALLOC
command, the hardware might continue writing events to the freed memory.

Additionally, does setting pm_state to DISABLE_STATE bypass the fallback
cleanup in gpi_free_chan_resources()?

gpi_free_chan_resources() {
    ...
    if (cur_state == ACTIVE_STATE)
        gpi_send_cmd(gpii, NULL, GPI_EV_CMD_DEALLOC);
}

By forcing the state to DISABLE_STATE, it seems the fallback DEALLOC
is prevented when the sibling channel is cleaned up, potentially making what
was previously a race window into a permanent regression of the event ring.

[Severity: Critical]
This is another pre-existing issue, but does freeing the event ring here
cause a regression by freeing the resource twice later?

When gpi_free_ring() is called, it zeroes the ring structure:

gpi_free_ring() {
    dma_free_coherent(...);
    memset(ring, 0, sizeof(*ring));
}

Later, when the client handles the probe failure and releases the
previously initialized sibling channel, gpi_free_chan_resources()
observes no remaining active channels and unconditionally executes:

    gpi_free_ring(&gpii->ev_ring, gpii);

Since the ring properties are now 0 and NULL, does this cause the dma layer
to attempt to free an invalid page?

 exit_gpi_init:
 	return ret;
 }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-gpi_bug_fix-v3-1-4c97333e6f5b@oss.qualcomm.com?part=1