[PATCH v2] dmaengine: qcom: gpi: correct gchan in error paths

[PATCH v2] dmaengine: qcom: gpi: correct gchan in error paths

[Brian Masney]

When attempting to start the Fedora graphical installer from a USB
thumbdrive on the Lenovo Thinkpad x13s laptop, the following errors are
shown in dmesg multiple times:

    kernel: gpi 800000.dma-controller: cmd: CH START completion timeout:0
    kernel: gpi 800000.dma-controller: Error with cmd:CH START ret:-5
    kernel: gpi 800000.dma-controller: Error start chan:-5

Looking through the error path, gpi_send_cmd() sends the wrong gchan to
gpi_send_cmd() in gpi_ch_init()'s error path. Fix this by passing the
correct gchan in gpi_ch_init()'s error path of gpi_send_cmd().

Fixes: 5d0c3533a19f ("dmaengine: qcom: Add GPI dma driver")
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Brian Masney <bmasney@redhat.com>
Assisted-by: Claude:claude-opus-4-6
---
There's a separate issue with the graphical Fedora installer not
working that I haven't had time to dig into further. I can work
around it by using the text installer.

Changes in v2:
- Commit message cleanup as suggested by Frank
- Commit title cleanup as suggested Konrad
- Add Konrad's Reviewed-by
- Link to v1: https://lore.kernel.org/r/20260625-qcom-gpi-err-fix-v1-1-5ca3f00fe2e3@redhat.com
---
 drivers/dma/qcom/gpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..3f390b5821ab 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -1965,12 +1965,12 @@ 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:

---
base-commit: 6c94b38b83a04c43ea49004275f0391404051093
change-id: 20260625-qcom-gpi-err-fix-06ef18453608

Best regards,
-- 
Brian Masney <bmasney@redhat.com>

Re: [PATCH v2] dmaengine: qcom: gpi: correct gchan in error paths

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `GPI_EV_CMD_DEALLOC` command in the `gpi_ch_init()` error path allows the hardware to write to freed event ring memory.
- [High] The transfer ring memory (`gchan->ch_ring`) is permanently leaked on the error path of `gpi_alloc_chan_resources()`.
- [High] The `gpi_ch_init` error path fails to kill the pending `ev_task` tasklet and reset `gpii->pm_state`, enabling a Use-After-Free of `ev_ring`.
- [High] Failure to reset `gchan->pm_state` on the error path corrupts the channel synchronization state machine, leading to unintended behavior.
--

--- Patch [1]: [PATCH v2] dmaengine: qcom: gpi: correct gchan in error paths ---
commit a95016733426dc2a9c83a8d6ed2c5dccd118b6bb
Author: Brian Masney <bmasney@redhat.com>

dmaengine: qcom: gpi: correct gchan in error paths

This commit corrects the gchan pointer passed in the error paths of
gpi_ch_init(). It ensures that the correct channel from the gpii array
is reset and deallocated, rather than reusing the initial gchan argument.

Link: https://lore.kernel.org/r/20260625-qcom-gpi-err-fix-v1-1-5ca3f00fe2e3@redhat.com

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..3f390b5821abb 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -1965,12 +1965,12 @@ static int gpi_ch_init(struct gchan *gchan)

[Severity: High]
This is a pre-existing issue, but since we are fixing the error path here,
should gchan->pm_state be reset on failure?

At the beginning of gpi_ch_init(), we set:
    gchan->pm_state = CONFIG_STATE;

If initialization fails and we take these error paths, gchan->pm_state is never
reverted back to DISABLE_STATE. If a client later requests this channel, could
gpi_ch_init() incorrectly assume the channel is already configured and break the
synchronization logic?

>  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:

[Severity: High]
This is a pre-existing issue, but does this error path leave the event ring
memory accessible to the hardware?

gpi_alloc_ev_chan() configures the hardware with the event ring's physical
address via GPI_EV_CMD_ALLOCATE. Since a GPI_EV_CMD_DEALLOC command is not sent
here before freeing the ring, can the device continue performing DMA writes to
the now-freed memory?

[Severity: High]
This is another pre-existing issue, but does this error path also leave a
tasklet potentially running after freeing the event ring?

If an error occurs after interrupts are enabled, an interrupt could fire and
schedule gpii->ev_task. The call to gpi_disable_interrupts() uses
devm_free_irq() which waits for IRQ handlers, but it does not cancel pending
tasklets.

Since tasklet_kill() is not called and gpii->pm_state is not reverted, could a
scheduled tasklet access the freed gpii->ev_ring?

[Severity: High]
This is a pre-existing issue in the caller of gpi_ch_init(), but does a failure
here cause a memory leak of the transfer ring?

Looking at gpi_alloc_chan_resources():

drivers/dma/qcom/gpi.c:gpi_alloc_chan_resources() {
    ...
    ret = gpi_alloc_ring(&gchan->ch_ring, elements,
                         sizeof(struct gpi_tre), gpii);
    if (ret)
            return ret;

    ret = gpi_ch_init(gchan);

    mutex_unlock(&gpii->ctrl_lock);

    return ret;
}

If gpi_ch_init() fails, it returns the error code without freeing gchan->ch_ring.

Since the DMA engine core does not automatically invoke the
device_free_chan_resources callback when device_alloc_chan_resources fails,
is this DMA-coherent transfer ring memory permanently leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-qcom-gpi-err-fix-v2-1-49c9be055501@redhat.com?part=1