Re: [PATCH 2/4] spi: sg2044-nor: Honor SPI clock limits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: sg2044-nor: AB-BA deadlock between spifmc->lock and CCF prepare_lock
--

commit fd13672e4c9fa0fb60925d0020a99edbeb412a79
Author: Han Gao <gaohan@iscas.ac.cn>

spi: sg2044-nor: Honor SPI clock limits

The fixed divider ignores device and operation clock limits. Calculate
the divider before each transfer so SCK does not exceed either limit.

> diff --git a/drivers/spi/spi-sg2044-nor.c b/drivers/spi/spi-sg2044-nor.c
> index ff026ca0f7663..f2a1074f68477 100644
> --- a/drivers/spi/spi-sg2044-nor.c
> +++ b/drivers/spi/spi-sg2044-nor.c

[ ... ]

> @@ -385,6 +385,29 @@ static ssize_t sg2044_spifmc_trans_reg(struct sg2044_spifmc *spifmc,
>  	return 0;
>  }
>  
> +static int sg2044_spifmc_set_speed(struct sg2044_spifmc *spifmc,
> +				   unsigned int speed_hz)
> +{
> +	unsigned long rate = clk_get_rate(spifmc->clk);

[Severity: High]
Does this sequence introduce an AB-BA deadlock between the local hardware 
lock and the global Common Clock Framework prepare_lock?

In sg2044_spifmc_exec_op(), the data-plane hot path for SPI NOR transfers 
acquires the local lock:

    mutex_lock(&spifmc->lock);

It then calls sg2044_spifmc_set_speed(), which unconditionally queries the 
clock framework here via clk_get_rate().

Under the Common Clock Framework, clk_get_rate() acquires the global 
prepare_lock mutex. This creates a lock hierarchy inversion. If a thread 
holding prepare_lock (e.g., during clk_prepare() which may require 
firmware or trigger a page fault) requires a read from the SPI NOR flash, 
it will block waiting for spifmc->lock. Concurrently, an SPI NOR operation 
holding spifmc->lock will block waiting for prepare_lock.

Additionally, repeatedly acquiring a global subsystem mutex in a hot data 
path can cause severe performance bottlenecks for clock operations 
system-wide.

Could the clock rate be cached during probe() instead of querying it per 
transfer?

> +	u64 div;
> +	u32 reg;
> +
> +	if (!rate || !speed_hz)
> +		return -EINVAL;

[ ... ]

> @@ -395,11 +418,18 @@ static int sg2044_spifmc_exec_op(struct spi_mem *mem,
>  
>  	mutex_lock(&spifmc->lock);
>  
> +	ret = sg2044_spifmc_set_speed(spifmc, op->max_freq ?
> +				    min(op->max_freq, mem->spi->max_speed_hz) :
> +				    mem->spi->max_speed_hz);
> +	if (ret)
> +		goto out_unlock;
> +
>  	if (op->addr.nbytes == 0)
>  		ret = sg2044_spifmc_trans_reg(spifmc, op);
>  	else
>  		ret = sg2044_spifmc_trans(spifmc, op);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-fix-sg204xspi-v1-0-5c95dac429a8@iscas.ac.cn?part=2