Re: [PATCH v2 2/4] spi: amlogic: spisg: Fix transfer size limit and timeout handling

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

Pre-existing issues:
- [High] Missing max_transfer_size callback leads to silent truncation of large transfers.
- [High] Incomplete timeout handling leaves interrupts unmasked, allowing a delayed interrupt to prematurely complete a subsequent transfer.
- [High] Immediate DMA buffer unmapping on transfer timeout causes memory corruption if the hardware DMA engine is still active.
--

--- Patch [2]: [PATCH v2 2/4] spi: amlogic: spisg: Fix transfer size limit and timeout handling ---
commit d149d3bcf81987c3bc95c5cc0053e984ee79e15e
Author: Xianwei Zhao <[email protected]>
Subject: spi: amlogic: spisg: Fix transfer size limit and timeout handling

This commit reduces SPISG_BLOCK_MAX to 0xfffff to avoid programming zero-length
transfers because the CFG_BLOCK_NUM field can only encode 20 bits. It also
fixes a delay calculation overflow and attempts to stop the controller on
transfer timeout by clearing the descriptor list register.

Fixes: cef9991e04ae ("spi: Add Amlogic SPISG driver")

> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
> @@ -95,7 +95,7 @@
>  
>  #define SPISG_MAX_REG			0x40
>  
> -#define SPISG_BLOCK_MAX			0x100000
> +#define SPISG_BLOCK_MAX			0xFFFFF

[Severity: High]
This isn't a bug introduced by this patch, but modifying SPISG_BLOCK_MAX
brings up a question about large transfers.

Are large I/O requests being silently truncated?

In aml_spisg_setup_transfer(), the hardware block count is dynamically clamped:

    blocks = min_t(int, blocks, SPISG_BLOCK_MAX);

However, the driver does not provide a ctlr->max_transfer_size callback.
The SPI core assumes SIZE_MAX is supported and won't split large transfers.
The hardware will stop after transferring SPISG_BLOCK_MAX blocks.

Later in aml_spisg_transfer_one_message(), the driver unconditionally reports
success for the full length:

    msg->actual_length = msg->frame_length;

Could this cause silent data corruption for transfers larger than
SPISG_BLOCK_MAX * (bits_per_word / 8) bytes, since the caller is unaware the
transfer was truncated?

[ ... ]

> @@ -565,10 +565,13 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  	aml_spisg_pending(spisg, descs_paddr, false, true);
>  	if (wait_for_completion_timeout(&spisg->completion,
>  					spi_controller_is_target(spisg->controller) ?
> -					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms)))
> +					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms))) {
>  		ret = spisg->status ? -EIO : 0;
> -	else
> +	} else {
> +		/* stop transfer */
> +		regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
>  		ret = -ETIMEDOUT;
> +	}

[Severity: High]
This is a pre-existing issue, but does the timeout path safely halt the DMA
engine before unmapping memory?

After writing 0 to SPISG_REG_DESC_LIST_H to abort the transfer, the driver
immediately calls dma_unmap_single() and then aml_spisg_cleanup_transfer()
to unmap the data buffers.

If the hardware DMA engine requires time to flush outstanding AXI transactions
or if the register write is delayed, could the hardware access unmapped pages
or memory reallocated to other components, resulting in memory corruption or
IOMMU faults? Is it necessary to poll the hardware to confirm it has halted
before proceeding?

[Severity: High]
This is also a pre-existing issue, but is there a race condition with
delayed interrupts on a timeout?

If a transfer times out, the driver issues the stop command but does not
mask the controller's interrupt (SPISG_REG_IRQ_ENABLE) or use synchronize_irq()
to ensure the handler is not running.

The interrupt handler aml_spisg_irq() unconditionally calls:

    complete(&spisg->completion);

If a delayed interrupt fires concurrently with a subsequent transfer that has
just called reinit_completion(&spisg->completion), might the new transfer's
wait_for_completion_timeout() return immediately as a false success? This could
cause the driver to unmap DMA buffers while the hardware is still actively
processing the new transfer.

>  
>  	dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
>  end:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic