Re: [PATCH 1/4] dmaengine: qcom: gpi: Add UART transfer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: qcom: gpi: incorrect nr_tre calculation for UART transfers
--

commit 8dcb83edc68c0730c98ffc9a0682fbaf3ad23874
Author: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>

dmaengine: qcom: gpi: Add UART transfer support

The GPI DMA driver currently supports TRE construction for SPI and I2C
clients, but not for UART clients. This adds the UART CONFIG0 and GO TRE
definitions and exposes a UART peripheral configuration structure.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
[ ... ]
> @@ -1841,11 +1930,17 @@ gpi_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  		return NULL;
>  
>  	/* create TREs for xfer */
> -	if (gchan->protocol == QCOM_GPI_SPI) {
> +	switch (gchan->protocol) {
> +	case QCOM_GPI_SPI:
>  		i = gpi_create_spi_tre(gchan, gpi_desc, sgl, direction);
> -	} else if (gchan->protocol == QCOM_GPI_I2C) {
> +		break;
> +	case QCOM_GPI_I2C:
>  		i = gpi_create_i2c_tre(gchan, gpi_desc, sgl, direction, flags);
> -	} else {
> +		break;
> +	case QCOM_GPI_UART:
> +		i = gpi_create_serial_tre(gchan, gpi_desc, sgl, direction);

[Severity: High]
Does this new uart creation path safely interact with the existing nr_tre
calculation earlier in gpi_prep_slave_sg()?

Earlier in gpi_prep_slave_sg(), nr_tre is calculated like this:

	nr_tre = 3;
	set_config = *(u32 *)gchan->config;
	if (!set_config)
		nr_tre = 2;
	if (direction == DMA_DEV_TO_MEM) /* rx */
		nr_tre = 1;

For uart, gchan->config points to a struct gpi_serial_config, where the first
field is rx_stale_timeout. Reading *(u32 *)gchan->config will use
rx_stale_timeout instead of a boolean configuration flag, which might cause
nr_tre to be miscalculated.

Additionally, the earlier code forces nr_tre = 1 for DMA_DEV_TO_MEM,
but gpi_create_serial_tre() appears to generate up to 3 transfer elements.

Could this mismatch cause the ring space safeguard (if (nr < nr_tre)) to pass
incorrectly, leading to dropped elements or a hardware ring overflow?

Note: I see this was successfully resolved in a follow-up commit upstream
"dmaengine: qcom: gpi: Calculate UART TRE count per direction", but I am
highlighting it here as part of the review of this specific patch.

> +		break;
> +	default:
>  		dev_err(dev, "invalid peripheral: %d\n", gchan->protocol);
>  		kfree(gpi_desc);
>  		return NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-add-gpi-dma-support-for-uart-v1-0-c69e728df686@oss.qualcomm.com?part=1