linux-kernel - Re: [PATCH v3 1/1] spi: spi-cadence-quadspi: Fix pm runtime unbalance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ab51dfce-a7d1-4eb3-b469-af35529dfbbb@sabinyo.mountain>
Date: Thu, 26 Jun 2025 23:37:53 -0500
From: Dan Carpenter <dan.carpenter@...aro.org>
To: khairul.anuar.romli@...era.com
Cc: Mark Brown <broonie@...nel.org>,
	"open list:SPI SUBSYSTEM" <linux-spi@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	Matthew Gerlach <matthew.gerlach@...era.com>,
	Khairul Anuar Romli <khairulanuar.romli@...era.com>
Subject: Re: [PATCH v3 1/1] spi: spi-cadence-quadspi: Fix pm runtime unbalance

On Mon, Jun 16, 2025 at 09:13:53AM +0800, khairul.anuar.romli@...era.com wrote:
> From: Khairul Anuar Romli <khairul.anuar.romli@...era.com>
> 
> Having PM put sync in remove function is causing PM underflow during
> remove operation. This is caused by the function, runtime_pm_get_sync,
> not being called anywhere during the op. Ensure that calls to
> pm_runtime_enable()/pm_runtime_disable() and
> pm_runtime_get_sync()/pm_runtime_put_sync() match.
> 
> echo 108d2000.spi > /sys/bus/platform/drivers/cadence-qspi/unbind
> [   49.644256] Deleting MTD partitions on "108d2000.spi.0":
> [   49.649575] Deleting u-boot MTD partition
> [   49.684087] Deleting root MTD partition
> [   49.724188] cadence-qspi 108d2000.spi: Runtime PM usage count underflow!
> 
> Continuous bind/unbind will result in an "Unbalanced pm_runtime_enable" error.
> Subsequent unbind attempts will return a "No such device" error, while bind
> attempts will return a "Resource temporarily unavailable" error.
> 
> [   47.592434] cadence-qspi 108d2000.spi: Runtime PM usage count underflow!
> [   49.592233] cadence-qspi 108d2000.spi: detected FIFO depth (1024) different from config (128)
> [   53.232309] cadence-qspi 108d2000.spi: Runtime PM usage count underflow!
> [   55.828550] cadence-qspi 108d2000.spi: detected FIFO depth (1024) different from config (128)
> [   57.940627] cadence-qspi 108d2000.spi: Runtime PM usage count underflow!
> [   59.912490] cadence-qspi 108d2000.spi: detected FIFO depth (1024) different from config (128)
> [   61.876243] cadence-qspi 108d2000.spi: Runtime PM usage count underflow!
> [   61.883000] platform 108d2000.spi: Unbalanced pm_runtime_enable!
> [  532.012270] cadence-qspi 108d2000.spi: probe with driver cadence-qspi failed1
> 
> Also, change clk_disable_unprepare() to clk_disable() since continuous
> bind and unbind operations will trigger a warning indicating that the clock is
> already unprepared.
> 
> Fixes: 4892b374c9b7 ("mtd: spi-nor: cadence-quadspi: Add runtime PM support")
> cc: stable@...r.kernel.org # 6.6+
> Signed-off-by: Khairul Anuar Romli <khairul.anuar.romli@...era.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@...era.com>
> ---
> Changes in v3:
>     - v2 was sent without a "Changes in v2" section.
> 
> Changes in v2:
>     - Remove the runtime_pm variable from the struct, as it’s not needed for the fix.
> ---
>  drivers/spi/spi-cadence-quadspi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index c90462783b3f..506a139fbd2c 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1958,10 +1958,10 @@ static int cqspi_probe(struct platform_device *pdev)
>  			goto probe_setup_failed;
>  	}
>  
> -	ret = devm_pm_runtime_enable(dev);
> -	if (ret) {
> -		if (cqspi->rx_chan)
> -			dma_release_channel(cqspi->rx_chan);
> +	pm_runtime_enable(dev);
> +
> +	if (cqspi->rx_chan) {
> +		dma_release_channel(cqspi->rx_chan);
>  		goto probe_setup_failed;
>  	}

This will totally break the driver.  It was supposed to be

	if (ret) {
		if (cqspi->rx_chan)
			dma_release_channel(cqspi->rx_chan);
		goto probe_setup_failed;
  	}

In other words, if we failed there was some slightly complicated
cleanup to do.  But now it will do the cleanup and free things on the
success path if we're in cqspi->use_direct_mode.

regards,
dan carpenter


>  
> @@ -1981,6 +1981,7 @@ static int cqspi_probe(struct platform_device *pdev)
>  	return 0;
>  probe_setup_failed:
>  	cqspi_controller_enable(cqspi, 0);
> +	pm_runtime_disable(dev);
>  probe_reset_failed:
>  	if (cqspi->is_jh7110)
>  		cqspi_jh7110_disable_clk(pdev, cqspi);
> @@ -1999,7 +2000,8 @@ static void cqspi_remove(struct platform_device *pdev)
>  	if (cqspi->rx_chan)
>  		dma_release_channel(cqspi->rx_chan);
>  
> -	clk_disable_unprepare(cqspi->clk);
> +	if (pm_runtime_get_sync(&pdev->dev) >= 0)
> +		clk_disable(cqspi->clk);
>  
>  	if (cqspi->is_jh7110)
>  		cqspi_jh7110_disable_clk(pdev, cqspi);
> -- 
> 2.35.3
>