Re: [PATCH v2 1/2] spi: tegra210-quad: Allocate DMA memory for DMA engine

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unconditional DMA unmapping in tegra_qspi_dma_unmap_xfer() causes invalid unmaps for unpacked transfers.
- [High] The error path in tegra_qspi_dma_map_xfer() unmaps tx_dma without verifying if tx_buf was provided.
- [High] Aligning the DMA transfer length up to a multiple of 4 bytes can cause the DMA engine to write out of bounds.
- [High] DMA mappings are leaked when the transfer times out or when DMA setup fails.
--

commit 78250c2468331b7f824d5bbb2690d151a21edc13
Author: Aaron Kling <webgeek1234@gmail.com>

spi: tegra210-quad: Allocate DMA memory for DMA engine

When the SPI controllers are running in DMA mode, it is the DMA engine
that performs the memory accesses rather than the SPI controller. Pass
the DMA engine's struct device pointer to the DMA API to make sure the
correct DMA operations are used.

> diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
> index db28dd556484b..588a929a97850 100644
> --- a/drivers/spi/spi-tegra210-quad.c
> +++ b/drivers/spi/spi-tegra210-quad.c

[ ... ]

> @@ -574,15 +576,15 @@ static int tegra_qspi_dma_map_xfer(struct tegra_qspi *tqspi, struct spi_transfer
>  	len = DIV_ROUND_UP(tqspi->curr_dma_words * tqspi->bytes_per_word, 4) * 4;

[Severity: High]
This isn't a bug introduced by this patch, but could aligning this length up
to a multiple of 4 bytes cause the DMA engine to write out of bounds?

If the client requested a transfer length that is not a multiple of 4 bytes,
the DMA engine will write up to 3 bytes past the end of the client's buffer.

If the buffer lacks sufficient alignment padding, could this cause memory
corruption?

>  
>  	if (t->tx_buf) {
> -		t->tx_dma = dma_map_single(tqspi->dev, (void *)tx_buf, len, DMA_TO_DEVICE);
> -		if (dma_mapping_error(tqspi->dev, t->tx_dma))
> +		t->tx_dma = dma_map_single(tqspi->tx_dma_dev, (void *)tx_buf, len, DMA_TO_DEVICE);
> +		if (dma_mapping_error(tqspi->tx_dma_dev, t->tx_dma))
>  			return -ENOMEM;
>  	}
>  
>  	if (t->rx_buf) {
> -		t->rx_dma = dma_map_single(tqspi->dev, (void *)rx_buf, len, DMA_FROM_DEVICE);
> -		if (dma_mapping_error(tqspi->dev, t->rx_dma)) {
> -			dma_unmap_single(tqspi->dev, t->tx_dma, len, DMA_TO_DEVICE);
> +		t->rx_dma = dma_map_single(tqspi->rx_dma_dev, (void *)rx_buf, len, DMA_FROM_DEVICE);
> +		if (dma_mapping_error(tqspi->rx_dma_dev, t->rx_dma)) {
> +			dma_unmap_single(tqspi->tx_dma_dev, t->tx_dma, len, DMA_TO_DEVICE);

[Severity: High]
This isn't a bug introduced by this patch, but does this error path safely
unmap tx_dma?

If t->tx_buf is NULL during a half-duplex RX-only transfer, t->tx_dma is
never mapped.  If the rx_dma mapping then fails, we will attempt to unmap an
uninitialized DMA address here.

Could this corrupt the IOMMU state and trigger faults?

>  			return -ENOMEM;
>  		}
>  	}
> @@ -597,9 +599,9 @@ static void tegra_qspi_dma_unmap_xfer(struct tegra_qspi *tqspi, struct spi_trans
>  	len = DIV_ROUND_UP(tqspi->curr_dma_words * tqspi->bytes_per_word, 4) * 4;
>  
>  	if (t->tx_buf)
> -		dma_unmap_single(tqspi->dev, t->tx_dma, len, DMA_TO_DEVICE);
> +		dma_unmap_single(tqspi->tx_dma_dev, t->tx_dma, len, DMA_TO_DEVICE);
>  	if (t->rx_buf)
> -		dma_unmap_single(tqspi->dev, t->rx_dma, len, DMA_FROM_DEVICE);
> +		dma_unmap_single(tqspi->rx_dma_dev, t->rx_dma, len, DMA_FROM_DEVICE);

[Severity: High]
This isn't a bug introduced by this patch, but can this cause invalid DMA
unmaps for unpacked transfers?

For unpacked transfers, tegra_qspi_start_dma_based_transfer() skips mapping
client buffers and uses a pre-allocated bounce buffer instead.

Since this function is called unconditionally and only checks for the presence
of t->tx_buf and t->rx_buf, it will call dma_unmap_single() on the
uninitialized t->tx_dma and t->rx_dma fields when tqspi->is_packed is false.

>  }
>  
>  static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi *tqspi, struct spi_transfer *t)

[Severity: High]
This isn't a bug introduced by this patch, but are DMA mappings leaked when
the transfer times out or when DMA setup fails?

When tegra_qspi_dma_map_xfer() dynamically maps client buffers, if subsequent
steps like dmaengine_slave_config() fail in this function, the driver returns
immediately without unmapping the buffers.

Additionally, if the DMA transfer times out in
tegra_qspi_transfer_one_message(), it jumps to the exit label and returns
without calling tegra_qspi_dma_unmap_xfer().

Could this permanently leak mapped DMA addresses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-tegra194-qspi-iommu-v2-0-a11c53f804b2@gmail.com?part=1