Re: [PATCH v2 3/4] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

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

Pre-existing issues:
- [High] Missing pm_runtime_disable() and pm_runtime_put() in probe error path and remove().
- [High] Double-disable of clocks managed by devm_clk_get_enabled().
- [High] Race condition causing spurious -EBUSY transfer failures.
- [High] Target abort incorrectly signals successful transfer completion.
--

--- Patch [3]: [PATCH v2 3/4] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor ---
commit c9f43474b4fa19eea599df7045d868e70af19bd8
Author: Sunny Luo <[email protected]>
spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

This commit fixes the incorrect handling of the keep_ss state for the last
descriptor in the Amlogic SPISG driver. It ensures the CFG_KEEP_SS bit is
properly set based on the final transfer's cs_change value, ensuring the
chip select behaves correctly even when a cs-hold delay is present.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index a6123975a37cf..2d66f00dccd45 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c

[ ... ]

> @@ -539,13 +542,17 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  				       xfer->effective_speed_hz);
>  	}
>  
> -	if (cs_hold_in_sclk)
> +	if (cs_hold_in_sclk) {
>  		/* additional null-descriptor to achieve the cs-hold delay */
>  		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
> -	else
>  		desc--;
> +		desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 1);
> +		desc++;
> +	} else {
> +		desc--;
> +	}
>  
> -	desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 0);
> +	FIELD_MODIFY(CFG_KEEP_SS, &desc->cfg_bus, last_xfer_keep_ss);
>  	desc->cfg_start |= FIELD_PREP(CFG_EOC, 1);
>  
>  	/* some tolerances */

[Severity: High]
This is a pre-existing issue, but does calling spi_finalize_current_message()
before releasing the hardware lock open a race window for the next transfer?

In aml_spisg_transfer_one_message(), we have:

	msg->status = ret;
	spi_finalize_current_message(ctlr);
	aml_spisg_sem_up_write(spisg);

Since spi_finalize_current_message() signals the SPI core that the controller
is ready, could the SPI core immediately dispatch the next transfer on another
CPU, which then fails aml_spisg_sem_down_read() and aborts with -EBUSY?

[Severity: High]
This isn't a bug introduced by this patch, but does the target abort handler
incorrectly signal a successful transfer?

In aml_spisg_target_abort():

	spisg->status = 0;
	regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
	complete(&spisg->completion);

And in aml_spisg_transfer_one_message():

	if (wait_for_completion_timeout(&spisg->completion,
					spi_controller_is_target(spisg->controller) ?
					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms))) {
		ret = spisg->status ? -EIO : 0;

If a transfer is aborted, status is set to 0 and the wait completes. Does
this cause the SPI client to incorrectly interpret an aborted transfer as
fully successful?

[Severity: High]
This is a pre-existing issue, but do the probe error path and remove
functions leak PM runtime references?

In aml_spisg_probe(), the out_clk error path exits without disabling
PM runtime:

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

	return ret;

Additionally, aml_spisg_remove() does not call pm_runtime_disable():

static void aml_spisg_remove(struct platform_device *pdev)
{
...
	if (!pm_runtime_suspended(&pdev->dev)) {

Does this prevent the device's power domain from suspending and cause
a kernel warning when rebinding?

[Severity: High]
This isn't a bug introduced by this patch, but are the devm-managed clocks
being double-disabled?

In aml_spisg_clk_init(), pclk is explicitly disabled:

	clk_set_min_rate(spisg->pclk, SPISG_PCLK_RATE_MIN);

	clk_disable_unprepare(spisg->pclk);

In the aml_spisg_probe() error path, both clocks are manually disabled:

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

And in aml_spisg_remove(), they are disabled again:

		clk_disable_unprepare(spisg->core);
		clk_disable_unprepare(spisg->pclk);

Since these clocks are acquired using devm_clk_get_enabled(), which
automatically registers a cleanup action, will manually disabling them
underflow the clock enable count and trigger warnings from the clock
framework?

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

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