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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added timeout abort path stops the controller but fails to synchronize or disable pending interrupts, causing a race condition where a late interrupt can falsely complete a subsequent transfer.

Pre-existing issues:
- [High] Transfers exceeding SPISG_BLOCK_MAX blocks are silently truncated by the driver because it caps the block count but fails to implement the max_transfer_size controller callback.
- [High] The driver incorrectly mixes `devm_clk_get_enabled()` with manual `clk_disable_unprepare()` calls, leading to unbalanced clock reference counting.
--

--- Patch [2]: [PATCH v3 2/4] spi: amlogic: spisg: Fix transfer size limit and timeout handling ---
commit f3597777f9b3ab326fe08698fcd9389c455e53d4
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

spi: amlogic: spisg: Fix transfer size limit and timeout handling

The commit reduces SPISG_BLOCK_MAX to avoid zero-length transfers and fixes
a 64-bit delay calculation. It also clears the descriptor list register on a
transfer timeout to stop the controller.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> --- 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 does the driver silently
truncate large transfers?

In aml_spisg_probe(), the driver sets ctlr->max_dma_len = SPISG_BLOCK_MAX,
but it does not implement the max_transfer_size controller callback.

When a transfer is larger than SPISG_BLOCK_MAX, aml_spisg_setup_transfer()
silently caps it:

	blocks = min_t(int, blocks, SPISG_BLOCK_MAX);

Since the SPI core isn't aware of the maximum transfer size limit, it could
submit oversized transfers that get silently truncated. This could leave DMA
buffers partially uninitialized on RX. Should we implement the
ctlr->max_transfer_size callback to prevent this?

[ ... ]

> @@ -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]
Could this timeout path race with a late interrupt?

By stopping the controller without synchronizing or masking the IRQ, a
delayed interrupt might still arrive. If it does, aml_spisg_irq()
unconditionally completes it:

	complete(&spisg->completion);

If the SPI core immediately enqueues the next message, could this late
interrupt prematurely complete the new transfer and prematurely unmap its
active DMA buffers?

[Severity: High]
This is a pre-existing issue, but does the driver have an unbalanced clock
reference count issue?

The driver acquires clocks using devm_clk_get_enabled(), but manually
disables them during error handling and removal. For example, in the error
path of aml_spisg_probe():

out_clk:
	if (spisg->core)
		clk_disable_unprepare(spisg->core);
	clk_disable_unprepare(spisg->pclk);

Since these are devm-managed clocks, the cleanup framework will also
disable them, which results in a double-disable. Should these manual
clk_disable_unprepare() calls be removed, or should the driver use the
non-devm clock getters?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-a9-spisg-v3-0-a15da3f70029@amlogic.com?part=2