Re: [PATCH v2] spi: imx: reconfigure for PIO when DMA cannot be started

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing SPI_TRANS_FAIL_NO_START flag when TX DMA preparation or submission fails, preventing PIO fallback.
- [High] Mid-transfer DMA failure incorrectly triggers a full PIO fallback, causing duplicate data transmission and corrupting the SPI stream.
- [High] Failure to clear `SPI_TRANS_FAIL_NO_START` after an internal PIO fallback leaks stale error state, causing subsequent transfers to silently duplicate data.
--

commit 5c8ab49e90372d3c0d5a4aca24ae8e97fcecafbc
Author: Javier Fernandez Pastrana <javier.pastrana@linutronix.de>

spi: imx: reconfigure for PIO when DMA cannot be started

When ECSPI DMA descriptor preparation fails, the driver falls back to PIO
internally without un-doing the DMA-mode controller configuration, leading
to corrupted data. This commit forces the controller to reconfigure for
PIO by setting the fallback flag and re-running the setup transfer
routine.

> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index ae9912905c67a..79a6c1a60b0a0 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -2152,7 +2152,8 @@ static int spi_imx_transfer_one(struct spi_controller *controller,
>  	if (spi_imx->usedma) {
>  		ret = spi_imx_dma_transfer(spi_imx, transfer);
>  		if (transfer->error & SPI_TRANS_FAIL_NO_START) {

[Severity: High]
This isn't a bug introduced by this patch, but if a transfer succeeds
internally via the PIO fallback here, doesn't transfer->error still retain
the SPI_TRANS_FAIL_NO_START flag?

If an SPI client reuses the spi_transfer object, and the next transfer
finishes successfully via DMA, this condition checks only the error flag
without verifying if the DMA transfer actually failed (ret < 0).

Would this cause the successful DMA transfer to incorrectly evaluate this
condition as true and redundantly re-transmit the data via PIO?

> -			spi_imx->usedma = false;
> +			controller->fallback = true;
> +			spi_imx_setupxfer(spi, transfer);
>  			if (spi_imx->target_mode)
>  				return spi_imx_pio_transfer_target(spi, transfer);
>  			else
>  				return spi_imx_pio_transfer(spi, transfer);
>  		}

[Severity: High]
This is also a pre-existing issue, but does this fallback logic account for
partially completed DMA transmissions?

If a large transfer is split into multiple DMA packages in
spi_imx_dma_transfer() and a subsequent package fails,
spi_imx_dma_package_transfer() unconditionally sets SPI_TRANS_FAIL_NO_START:

spi_imx_dma_package_transfer() {
    ...
    dma_failure_no_start:
        transfer->error |= SPI_TRANS_FAIL_NO_START;
        return ret;
}

Since earlier packages were already physically transmitted over the SPI bus,
would triggering a full PIO fallback here restart the transfer from the
beginning and corrupt the SPI stream by transmitting duplicate data?

[Severity: High]
Another pre-existing issue related to the DMA fallback: what happens if TX
DMA descriptor preparation or submission fails in spi_imx_dma_submit()?

drivers/spi/spi-imx.c:spi_imx_dma_submit() {
    ...
    desc_tx = dmaengine_prep_slave_single(controller->dma_tx, ...);
    if (!desc_tx)
        goto dmaengine_terminate_rx;
    ...
    cookie = dmaengine_submit(desc_tx);
    if (dma_submit_error(cookie)) {
        ...
        goto dmaengine_terminate_tx;
    }
    ...
}

It appears these error paths return without setting the
SPI_TRANS_FAIL_NO_START flag. Does this prevent the PIO fallback from
triggering for TX DMA failures, causing the transfer to drop instead?

>  		return ret;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624151958.18626-1-javier.pastrana@linutronix.de?part=1