linux-kernel - Re: [PATCH] spi: cadence-quadspi: Fix clock enable underflows due to runtime PM

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: <cf8e7003-ebca-4817-8350-f29332d75fab@ti.com>
Date: Tue, 9 Dec 2025 15:22:43 +0530
From: "Dutta, Anurag" <a-dutta@...com>
To: Mark Brown <broonie@...nel.org>
CC: Nishanth Menon <nm@...com>, Francesco Dolcini <francesco@...cini.it>,
	Siddharth Vadapalli <s-vadapalli@...com>, <linux-spi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>, "Gujulan Elango,
 Hari Prasath" <gehariprasath@...com>, "Kumar, Udit" <u-kumar1@...com>
Subject: Re: [PATCH] spi: cadence-quadspi: Fix clock enable underflows due to
 runtime PM


On 09-12-2025 11:13, Dutta, Anurag wrote:
>
> On 05-12-2025 18:55, Mark Brown wrote:
>> On Fri, Dec 05, 2025 at 06:28:06PM +0530, Dutta, Anurag wrote:
>>
>>> Hi Mark and Nishanth The below seems to work for me on j721e. Let me 
>>> know
>>> your thoughts.
>>> Also, the error actually comes from :
>>> if (cqspi->use_direct_mode) {
>>>      ret = cqspi_request_mmap_dma(cqspi);
>>>      if (ret == -EPROBE_DEFER)
>>>      goto probe_setup_failed;
>>> }
>>> And not from flash_setup().
>> Great, thanks for confirming.  We should probably ensure that has some
>> logging...
>>
>>> @@ -2024,7 +2024,7 @@ static int cqspi_probe(struct platform_device 
>>> *pdev)
>>>   probe_reset_failed:
>>>          if (cqspi->is_jh7110)
>>>                  cqspi_jh7110_disable_clk(pdev, cqspi);
>>> -       clk_disable_unprepare(cqspi->clk);
>>> +       pm_runtime_force_suspend(dev);
>>>   probe_clk_failed:
>> The trouble with that is that in the !CONFIG_PM case
>> pm_runtime_force_suspend() is defined as:
>>
>>    static inline int pm_runtime_force_suspend(struct device *dev) { 
>> return 0; }
>>
>> so we'll leak the clock enable.  If we could just require runtime PM
>> this would be an awful lot easier to deal with.
> So, can we maintain an internal state of the clock(enabled/disabled) 
> in the  struct cqspi_st ?
> Before every, clk_disable_unprepare()/clk_prepare_enable(), we check 
> if the clock is actually
> enabled/disabled by checking the state of "atomic_t clock_enabled" 
> within struct cqspi_st.
> And, when we do clk_disable_unprepare()/clk_prepare_enable(), we set 
> the value of clock_enabled
> accordingly.
>
> Is this a good approach, given we take care of race conditions as well ?
>
>
Another solution :

diff --git a/drivers/spi/spi-cadence-quadspi.c 
b/drivers/spi/spi-cadence-quadspi.c
index af6d050da1c8..4d298b945f09 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -2024,7 +2024,11 @@ static int cqspi_probe(struct platform_device *pdev)
  probe_reset_failed:
         if (cqspi->is_jh7110)
                 cqspi_jh7110_disable_clk(pdev, cqspi);
-       clk_disable_unprepare(cqspi->clk);
+
+       if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
+               if (pm_runtime_get_sync(&pdev->dev) >= 0)
+                       clk_disable_unprepare(cqspi->clk);
+       }
  probe_clk_failed:
         return ret;
  }

pm_runtime_get_sync() will increment the usage count thereby preventing 
from runtime_suspend()
getting invoked, thereby preventing double clock_disable()

This will work for !CONFIG_PM as well because pm_runtime_get_sync() will 
return 1 then.
and the runtime_suspend() is never going to be invoked.

logs :
with CONFIG_PM : 
https://gist.github.com/anuragdutta731/c0080a7592de992e71103e9947639b2a
without CONFIG_PM : 
https://gist.github.com/anuragdutta731/f9a79302b65088d53db2c970b6b8eff0

It work for both cases on j721e. Please share your thoughts.
Regards
Anurag