Re: [PATCH 10/26] dmaengine: qcom-gpi: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error cleanup paths in `gpi_ch_init()` use the wrong channel pointer when stopping and deallocating channels, leading to a leaked hardware state and redundant commands sent to the wrong channel.
- [High] The transfer ring memory (`gchan->ch_ring`) is permanently leaked if `gpi_ch_init()` fails during channel allocation in `gpi_alloc_chan_resources()`.
--

--- Patch [10]: [PATCH 10/26] dmaengine: qcom-gpi: Remove redundant dev_err()/dev_err_probe() ---
commit 2195d06ab56c88788ead88f1489a78636aee6d8b
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: qcom-gpi: Remove redundant dev_err()/dev_err_probe()

This patch removes driver-specific error logging when devm_request_irq()
fails. The core already logs detailed error messages automatically, making
these manual prints redundant.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..af824682e64c8 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in the error
cleanup path of gpi_ch_init() while reviewing this file:

drivers/dma/qcom/gpi.c:gpi_ch_init() {
    ...
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);
	}
	i = 2;
error_alloc_chan:
	for (i = i - 1; i >= 0; i--)
		gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
    ...
}

In both loops, the code calls gpi_send_cmd() and gpi_reset_chan() using the
fixed function argument gchan instead of the loop variable &gpii->gchan[i].

Does this leave the successfully initialized channels in an incorrect hardware
state and send duplicate reset and deallocate commands to the same channel?

[Severity: High]
This is also a pre-existing issue, but there appears to be a missing cleanup
step in the channel allocation path:

drivers/dma/qcom/gpi.c:gpi_alloc_chan_resources() {
    ...
	/* allocate memory for transfer ring */
	ret = gpi_alloc_ring(&gchan->ch_ring, CHAN_TRES,
			     sizeof(struct gpi_tre), gpii);
	if (ret)
		goto xfer_alloc_err;

	ret = gpi_ch_init(gchan);

	mutex_unlock(&gpii->ctrl_lock);

	return ret;
    ...
}

If gpi_ch_init() returns an error, the function returns directly without calling
gpi_free_ring() to free the newly allocated gchan->ch_ring.

Since the dmaengine core aborts the allocation on failure and will not invoke
device_free_chan_resources(), does this permanently leak the DMA coherent
memory allocated for the transfer ring?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709135846.97972-1-panchuang@vivo.com?part=10