[PATCH] dmaengine: qcom: gpi: correct channel name in error path

[PATCH] dmaengine: qcom: gpi: correct channel name in error path

[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. Let's fix this by passing
the correct gchan.

Fixes: 5d0c3533a19f ("dmaengine: qcom: Add GPI dma driver")
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.
---
 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] dmaengine: qcom: gpi: correct channel name in error path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The error path in `gpi_ch_init()` frees the event ring DMA memory without sending the required hardware deallocation command (`GPI_EV_CMD_DEALLOC`), resulting in DMA Use-After-Free.
- [High] The software state machine (`gchan->pm_state`) is not reverted when `gpi_ch_init()` fails, leading to a double-teardown hardware timeout when the client later releases the successfully allocated sibling channel.
- [High] The event processing tasklet is not killed before freeing the event ring memory in the error path of `gpi_ch_init()`, creating a Use-After-Free vulnerability.
--

commit 40116dd1267be01863d674224210915823326daa
Author: Brian Masney <bmasney@redhat.com>

dmaengine: qcom: gpi: correct channel name in error path

This commit fixes an issue where the wrong channel was passed to
gpi_send_cmd() during the error handling path of gpi_ch_init(). Correcting
this resolves command completion timeouts when hardware cleanup occurs.

> 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)
>  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 could this error path leave the software
pm_state in an incorrect state?

If gpi_ch_init() fails, the hardware channels are deallocated, but their
pm_state remains as ACTIVE_STATE or CONFIG_STATE. When the client handles the
failure by releasing the previously successful sibling channel via
gpi_free_chan_resources(), it reads the stale state:

drivers/dma/qcom/gpi.c:gpi_free_chan_resources() {
        ...
        if (cur_state == ACTIVE_STATE) {
                gpi_stop_chan(gchan);
                ret = gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
        ...
}

Would this cause the driver to attempt a graceful shutdown on a channel that
has already been deallocated in hardware, resulting in the same completion
timeouts this patch aims to fix?

>  error_alloc_ev_ring:
>  	gpi_disable_interrupts(gpii);
>  error_config_int:

[Severity: High]
This is a pre-existing issue, but should the event processing tasklet be
canceled in this error path?

gpi_config_interrupts() registers the IRQ, which can fire and schedule
gpii->ev_task. If initialization fails here, gpi_disable_interrupts() frees
the IRQ but does not wait for or cancel scheduled tasklets.

Could this allow the asynchronous tasklet to execute against the freed ev_ring
memory when gpi_process_events() runs?

[Severity: Critical]
This is a pre-existing issue, but is the event ring DMA memory freed without
sending the GPI_EV_CMD_DEALLOC command to the hardware?

Just below this in gpi_ch_init(), gpi_free_ring(&gpii->ev_ring, gpii) is
called. If the hardware retains the physical address of the event ring after
the driver has freed its backing DMA memory, could it silently write into
freed memory if an event is generated later, causing DMA memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-qcom-gpi-err-fix-v1-1-5ca3f00fe2e3@redhat.com?part=1

Re: [PATCH] dmaengine: qcom: gpi: correct channel name in error path

[Frank Li]

On Thu, Jun 25, 2026 at 10:21:36AM -0400, Brian Masney wrote:
> 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. Let's fix this by passing
> the correct gchan.

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")
> 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.
> ---
>  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]);

Not related this patch, but code logic is strange, why init one specific
gchan failure, need stop previous all chans.

gchan should be one of gpii->gchan[i].

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