[PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Marek Vasut]

In case of STM32MP15xC/F SoC, in case the RNG1 is enabled in DT, the RNG1
clock are managed by the driver. The RNG1 clock are toggled off on entry
to suspend and back on on resume. For reason thus far unknown (could this
be some chip issue?), when the system goes through repeated suspend/resume
cycles, the system eventually hangs after a few such cycles.

This can be reproduced with CONFIG_PM_DEBUG 'pm_test' this way:
"
echo core > /sys/power/pm_test
while true ; do
    echo mem > /sys/power/state
    sleep 2 ;
done
"
The system locks up after about a minute and if WDT is active, resets.

If the RNG1 clock are kept enabled across suspend/resume, either using
this change, or by keeping the clock enabled in RNG driver suspend/resume
callbacks, the system does not lock up.

NOTE: This patch is a workaround. It would be good to know why does this
      change make the hang go away, whether this is a chip issue or some
      other problem ?

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Gatien Chevallier <gatien.chevallier@foss.st.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Olivia Mackall <olivia@selenic.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Yang Yingliang <yangyingliang@huawei.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-clk@vger.kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
---
 drivers/char/hw_random/stm32-rng.c | 2 ++
 drivers/clk/stm32/clk-stm32mp1.c   | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
index 7d0de8ab5e7f5..ec0314f05ff3e 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -403,6 +403,7 @@ static int __maybe_unused stm32_rng_suspend(struct device *dev)
 
 	writel_relaxed(priv->pm_conf.cr, priv->base + RNG_CR);
 
+	// Keeping the clock enabled across suspend/resume helps too
 	clk_disable_unprepare(priv->clk);
 
 	return 0;
@@ -434,6 +435,7 @@ static int __maybe_unused stm32_rng_resume(struct device *dev)
 	int err;
 	u32 reg;
 
+	// Keeping the clock enabled across suspend/resume helps too
 	err = clk_prepare_enable(priv->clk);
 	if (err)
 		return err;
diff --git a/drivers/clk/stm32/clk-stm32mp1.c b/drivers/clk/stm32/clk-stm32mp1.c
index 7e2337297402a..1a6e853d935fa 100644
--- a/drivers/clk/stm32/clk-stm32mp1.c
+++ b/drivers/clk/stm32/clk-stm32mp1.c
@@ -2000,7 +2000,7 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	KCLK(SDMMC3_K, "sdmmc3_k", sdmmc3_src, 0, G_SDMMC3, M_SDMMC3),
 	KCLK(FMC_K, "fmc_k", fmc_src, 0, G_FMC, M_FMC),
 	KCLK(QSPI_K, "qspi_k", qspi_src, 0, G_QSPI, M_QSPI),
-	KCLK(RNG1_K, "rng1_k", rng_src, 0, G_RNG1, M_RNG1),
+	KCLK(RNG1_K, "rng1_k", rng_src, CLK_IS_CRITICAL, G_RNG1, M_RNG1),
 	KCLK(RNG2_K, "rng2_k", rng_src, 0, G_RNG2, M_RNG2),
 	KCLK(USBPHY_K, "usbphy_k", usbphy_src, 0, G_USBPHY, M_USBPHY),
 	KCLK(STGEN_K, "stgen_k", stgen_src, CLK_IS_CRITICAL, G_STGEN, M_STGEN),
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Gatien CHEVALLIER]

Hi Marek,

Strange indeed.
A potential reason that comes to my mind would be that something tries 
to get a random number after the driver suspended and fails to do so.
Else it might just be a bad clock balance.

Can you describe the software ecosystem that you're running please?
(SCMI/no SCMI)?

Do you have the 3 fixes of stm32_rng.c that you've sent recently in your
software when testing?

What if you add a trace in a random generation function in random.c?

After this, I'll try to reproduce the issue.

Thanks,
Gatien


On 5/14/24 00:02, Marek Vasut wrote:
> In case of STM32MP15xC/F SoC, in case the RNG1 is enabled in DT, the RNG1
> clock are managed by the driver. The RNG1 clock are toggled off on entry
> to suspend and back on on resume. For reason thus far unknown (could this
> be some chip issue?), when the system goes through repeated suspend/resume
> cycles, the system eventually hangs after a few such cycles.
> 
> This can be reproduced with CONFIG_PM_DEBUG 'pm_test' this way:
> "
> echo core > /sys/power/pm_test
> while true ; do
>      echo mem > /sys/power/state
>      sleep 2 ;
> done
> "
> The system locks up after about a minute and if WDT is active, resets.
> 
> If the RNG1 clock are kept enabled across suspend/resume, either using
> this change, or by keeping the clock enabled in RNG driver suspend/resume
> callbacks, the system does not lock up.
> 
> NOTE: This patch is a workaround. It would be good to know why does this
>        change make the hang go away, whether this is a chip issue or some
>        other problem ?
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
> Cc: Gatien Chevallier <gatien.chevallier@foss.st.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Olivia Mackall <olivia@selenic.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> ---
>   drivers/char/hw_random/stm32-rng.c | 2 ++
>   drivers/clk/stm32/clk-stm32mp1.c   | 2 +-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
> index 7d0de8ab5e7f5..ec0314f05ff3e 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -403,6 +403,7 @@ static int __maybe_unused stm32_rng_suspend(struct device *dev)
>   
>   	writel_relaxed(priv->pm_conf.cr, priv->base + RNG_CR);
>   
> +	// Keeping the clock enabled across suspend/resume helps too
>   	clk_disable_unprepare(priv->clk);
>   
>   	return 0;
> @@ -434,6 +435,7 @@ static int __maybe_unused stm32_rng_resume(struct device *dev)
>   	int err;
>   	u32 reg;
>   
> +	// Keeping the clock enabled across suspend/resume helps too
>   	err = clk_prepare_enable(priv->clk);
>   	if (err)
>   		return err;
> diff --git a/drivers/clk/stm32/clk-stm32mp1.c b/drivers/clk/stm32/clk-stm32mp1.c
> index 7e2337297402a..1a6e853d935fa 100644
> --- a/drivers/clk/stm32/clk-stm32mp1.c
> +++ b/drivers/clk/stm32/clk-stm32mp1.c
> @@ -2000,7 +2000,7 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
>   	KCLK(SDMMC3_K, "sdmmc3_k", sdmmc3_src, 0, G_SDMMC3, M_SDMMC3),
>   	KCLK(FMC_K, "fmc_k", fmc_src, 0, G_FMC, M_FMC),
>   	KCLK(QSPI_K, "qspi_k", qspi_src, 0, G_QSPI, M_QSPI),
> -	KCLK(RNG1_K, "rng1_k", rng_src, 0, G_RNG1, M_RNG1),
> +	KCLK(RNG1_K, "rng1_k", rng_src, CLK_IS_CRITICAL, G_RNG1, M_RNG1),
>   	KCLK(RNG2_K, "rng2_k", rng_src, 0, G_RNG2, M_RNG2),
>   	KCLK(USBPHY_K, "usbphy_k", usbphy_src, 0, G_USBPHY, M_USBPHY),
>   	KCLK(STGEN_K, "stgen_k", stgen_src, CLK_IS_CRITICAL, G_STGEN, M_STGEN),

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Marek Vasut]

On 5/14/24 10:10 AM, Gatien CHEVALLIER wrote:
> Hi Marek,

Hi,

> Strange indeed.

Yes.

> A potential reason that comes to my mind would be that something tries 
> to get a random number after the driver suspended and fails to do so.

Possibly.

> Else it might just be a bad clock balance.

I don't think so, this would be reported by the kernel and it would show 
up in /sys/kernel/debug/clk/clk_summary as incrementing use count. It 
would also not happen in a non-deterministic manner like this happens 
here, the hang doesn't always happen after well defined suspend/resume 
cycle count.

> Can you describe the software ecosystem that you're running please?
> (SCMI/no SCMI)?

STM32MP157C DHCOM PDK2 with mainline U-Boot 2024.07-rc2 , no SCMI.

> Do you have the 3 fixes of stm32_rng.c that you've sent recently in your
> software when testing?

Yes, but this happens even without them.

> What if you add a trace in a random generation function in random.c?

Do you have a function name or line number for me ?

> After this, I'll try to reproduce the issue.

If you have a minute to test it on some ST MP15 board, that would be 
real nice. Thanks !

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Gatien CHEVALLIER]

Hi Marek,

On 5/14/24 16:37, Marek Vasut wrote:
> On 5/14/24 10:10 AM, Gatien CHEVALLIER wrote:
>> Hi Marek,
> 
> Hi,
> 
>> Strange indeed.
> 
> Yes.
> 
>> A potential reason that comes to my mind would be that something tries 
>> to get a random number after the driver suspended and fails to do so.
> 
> Possibly.
> 
>> Else it might just be a bad clock balance.
> 
> I don't think so, this would be reported by the kernel and it would show 
> up in /sys/kernel/debug/clk/clk_summary as incrementing use count. It 
> would also not happen in a non-deterministic manner like this happens 
> here, the hang doesn't always happen after well defined suspend/resume 
> cycle count.
> 
>> Can you describe the software ecosystem that you're running please?
>> (SCMI/no SCMI)?
> 
> STM32MP157C DHCOM PDK2 with mainline U-Boot 2024.07-rc2 , no SCMI.
> 
>> Do you have the 3 fixes of stm32_rng.c that you've sent recently in your
>> software when testing?
> 
> Yes, but this happens even without them.
> 
>> What if you add a trace in a random generation function in random.c?
> 
> Do you have a function name or line number for me ?

I put a trace in _get_random_bytes() in drivers/char/random.c. I'm not
100% sure but this should be the entry point when getting a random number.

> 
>> After this, I'll try to reproduce the issue.
> 
> If you have a minute to test it on some ST MP15 board, that would be 
> real nice. Thanks !

I tried to reproduce the issue you're facing on a STM32MP157C-DK2 no
SCMI on the 6.9-rc7 kernel tag. I uses OP-TEE and TF-A in the bootchain
but this should not have an impact here.

How did you manage to test using "echo core > /sys/power/pm_test"?
In kernel/power/suspend.c, enter_state(). If the pm_test_level is core,
then an error is fired with the following trace:
"Unsupported test mode for suspend to idle, please choose 
none/freezer/devices/platform."

I've tried using "echo devices > /sys/power/pm_test" so that I can at 
least test that the driver is put to sleep then wakes up. I do not
reproduce your issue.

[  169.026421] Filesystems sync: 0.013 seconds
[  169.031087] Freezing user space processes
[  169.036562] Freezing user space processes completed (elapsed 0.002 
seconds)
[  169.042238] OOM killer disabled.
[  169.045383] Freezing remaining freezable tasks
[  169.051408] Freezing remaining freezable tasks completed (elapsed 
0.001 seconds)
[  169.238226] dwc2 49000000.usb-otg: suspending usb gadget 
configfs-gadget.g1
[  169.270236] In stm32_rng_suspend
[  169.275501] PM: suspend debug: Waiting for 5 second(s).
[  174.283418] In stm32_rng_resume
[  174.284291] stm32-dwmac 5800a000.ethernet end0: configuring for 
phy/rgmii-id link mode
[  174.337714] dwmac4: Master AXI performs any burst length
[  174.341699] stm32-dwmac 5800a000.ethernet end0: No Safety Features 
support found
[  174.349138] stm32-dwmac 5800a000.ethernet end0: IEEE 1588-2008 
Advanced Timestamp supported
[  174.363442] dwc2 49000000.usb-otg: resuming usb gadget configfs-gadget.g1
[  174.667669] onboard-usb-hub 2-1: reset high-speed USB device number 2 
using ehci-platform
[  174.989075] OOM killer enabled.
[  174.990848] Restarting tasks ... done.
[  175.003976] random: crng reseeded on system resumption
[  175.009464] PM: suspend exit
[  175.011473] random: ASKING FOR 96 BYTES
[  175.011468] random: ASKING FOR 96 BYTES
[  175.015747] random: ASKING FOR 16 BYTES
[  175.044933] random: ASKING FOR 96 BYTES
[  175.059399] random: ASKING FOR 96 BYTES
[  175.070925] random: ASKING FOR 16 BYTES
[  175.079285] random: ASKING FOR 96 BYTES
[  175.082113] random: ASKING FOR 16 BYTES
[  175.096759] random: ASKING FOR 16 BYTES
[  175.098674] random: ASKING FOR 96 BYTES
[  175.295584] random: ASKING FOR 16 BYTES
[  175.302357] random: ASKING FOR 96 BYTES
[  175.311525] random: ASKING FOR 16 BYTES
[  175.312989] random: ASKING FOR 16 BYTES


Can you give it another shot with the trace so that we can ensure that
no random is asked after the driver is suspended in your case please?

Thanks,
Gatien




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Marek Vasut]

On 5/15/24 11:16 AM, Gatien CHEVALLIER wrote:

Hi,

>>> What if you add a trace in a random generation function in random.c?
>>
>> Do you have a function name or line number for me ?
> 
> I put a trace in _get_random_bytes() in drivers/char/random.c. I'm not
> 100% sure but this should be the entry point when getting a random number.

You're right, there is a read attempt right before the hang, and 
__clk_is_enabled() returns 0 in stm32_read_rng() . In fact, it is the 
pm_runtime_get_sync() which is returning -EACCES instead of zero, and 
this is currently not checked so the failure is not detected before 
register access takes place, to register file with clock disabled, which 
triggers a hard hang.

I'll be sending a patch shortly, thanks for this hint !

>>> After this, I'll try to reproduce the issue.
>>
>> If you have a minute to test it on some ST MP15 board, that would be 
>> real nice. Thanks !
> 
> I tried to reproduce the issue you're facing on a STM32MP157C-DK2 no
> SCMI on the 6.9-rc7 kernel tag. I uses OP-TEE and TF-A in the bootchain
> but this should not have an impact here.
> 
> How did you manage to test using "echo core > /sys/power/pm_test"?
> In kernel/power/suspend.c, enter_state(). If the pm_test_level is core,
> then an error is fired with the following trace:
> "Unsupported test mode for suspend to idle, please choose 
> none/freezer/devices/platform."

Could this be firmware related ?

> I've tried using "echo devices > /sys/power/pm_test" so that I can at 
> least test that the driver is put to sleep then wakes up. I do not
> reproduce your issue.

Can you try 'processors' ?

I did also notice it sometimes takes much longer than a minute to hang, 
but eventually it does hang. Maybe let it cycle for an hour or a few ?

[...]

-- 
Best regards,
Marek Vasut

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Gatien CHEVALLIER]

Hi Marek

On 5/16/24 03:06, Marek Vasut wrote:
> On 5/15/24 11:16 AM, Gatien CHEVALLIER wrote:
> 
> Hi,
> 
>>>> What if you add a trace in a random generation function in random.c?
>>>
>>> Do you have a function name or line number for me ?
>>
>> I put a trace in _get_random_bytes() in drivers/char/random.c. I'm not
>> 100% sure but this should be the entry point when getting a random 
>> number.
> 
> You're right, there is a read attempt right before the hang, and 
> __clk_is_enabled() returns 0 in stm32_read_rng() . In fact, it is the 
> pm_runtime_get_sync() which is returning -EACCES instead of zero, and 
> this is currently not checked so the failure is not detected before 
> register access takes place, to register file with clock disabled, which 
> triggers a hard hang.
> 
> I'll be sending a patch shortly, thanks for this hint !
> 

Great news, indeed the return code isn't checked. Let's use
pm_runtime_resume_and_get().

>>>> After this, I'll try to reproduce the issue.
>>>
>>> If you have a minute to test it on some ST MP15 board, that would be 
>>> real nice. Thanks !
>>
>> I tried to reproduce the issue you're facing on a STM32MP157C-DK2 no
>> SCMI on the 6.9-rc7 kernel tag. I uses OP-TEE and TF-A in the bootchain
>> but this should not have an impact here.
>>
>> How did you manage to test using "echo core > /sys/power/pm_test"?
>> In kernel/power/suspend.c, enter_state(). If the pm_test_level is core,
>> then an error is fired with the following trace:
>> "Unsupported test mode for suspend to idle, please choose 
>> none/freezer/devices/platform."
> 
> Could this be firmware related ?
> 
>> I've tried using "echo devices > /sys/power/pm_test" so that I can at 
>> least test that the driver is put to sleep then wakes up. I do not
>> reproduce your issue.
> 
> Can you try 'processors' ?
> 

Given this:
#ifdef CONFIG_PM_DEBUG
		if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
			pr_warn("Unsupported test mode for suspend to idle, please choose 
none/freezer/devices/platform.\n");
			return -EAGAIN;
		}
#endif

and this

static const char * const pm_tests[__TEST_AFTER_LAST] = {
	[TEST_NONE] = "none",
	[TEST_CORE] = "core",
	[TEST_CPUS] = "processors",
	[TEST_PLATFORM] = "platform",
	[TEST_DEVICES] = "devices",
	[TEST_FREEZER] = "freezer",
};

I'm getting the error as well.

> I did also notice it sometimes takes much longer than a minute to hang, 
> but eventually it does hang. Maybe let it cycle for an hour or a few ?
> 

I'll let it loop for some time then for device pm state.

> [...]
> 

Thanks for investigating this.

Cheers,
Gatien

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Marek Vasut]

On 5/16/24 9:42 AM, Gatien CHEVALLIER wrote:

Hi,

>>>>> What if you add a trace in a random generation function in random.c?
>>>>
>>>> Do you have a function name or line number for me ?
>>>
>>> I put a trace in _get_random_bytes() in drivers/char/random.c. I'm not
>>> 100% sure but this should be the entry point when getting a random 
>>> number.
>>
>> You're right, there is a read attempt right before the hang, and 
>> __clk_is_enabled() returns 0 in stm32_read_rng() . In fact, it is the 
>> pm_runtime_get_sync() which is returning -EACCES instead of zero, and 
>> this is currently not checked so the failure is not detected before 
>> register access takes place, to register file with clock disabled, 
>> which triggers a hard hang.
>>
>> I'll be sending a patch shortly, thanks for this hint !
>>
> 
> Great news, indeed the return code isn't checked. Let's use
> pm_runtime_resume_and_get().

Yes please.

I will wonder why we get EACCES though, that basically means we are 
suspending already. Is it safe to return -errno from rng read function 
in that case ?

>>>>> After this, I'll try to reproduce the issue.
>>>>
>>>> If you have a minute to test it on some ST MP15 board, that would be 
>>>> real nice. Thanks !
>>>
>>> I tried to reproduce the issue you're facing on a STM32MP157C-DK2 no
>>> SCMI on the 6.9-rc7 kernel tag. I uses OP-TEE and TF-A in the bootchain
>>> but this should not have an impact here.
>>>
>>> How did you manage to test using "echo core > /sys/power/pm_test"?
>>> In kernel/power/suspend.c, enter_state(). If the pm_test_level is core,
>>> then an error is fired with the following trace:
>>> "Unsupported test mode for suspend to idle, please choose 
>>> none/freezer/devices/platform."
>>
>> Could this be firmware related ?
>>
>>> I've tried using "echo devices > /sys/power/pm_test" so that I can at 
>>> least test that the driver is put to sleep then wakes up. I do not
>>> reproduce your issue.
>>
>> Can you try 'processors' ?
>>
> 
> Given this:
> #ifdef CONFIG_PM_DEBUG
>          if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
>              pr_warn("Unsupported test mode for suspend to idle

You're supposed to be suspending to 'mem' , not 'idle' . Could that be it ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Gatien CHEVALLIER]

On 5/16/24 12:43, Marek Vasut wrote:
> On 5/16/24 9:42 AM, Gatien CHEVALLIER wrote:
> 
> Hi,
> 
>>>>>> What if you add a trace in a random generation function in random.c?
>>>>>
>>>>> Do you have a function name or line number for me ?
>>>>
>>>> I put a trace in _get_random_bytes() in drivers/char/random.c. I'm not
>>>> 100% sure but this should be the entry point when getting a random 
>>>> number.
>>>
>>> You're right, there is a read attempt right before the hang, and 
>>> __clk_is_enabled() returns 0 in stm32_read_rng() . In fact, it is the 
>>> pm_runtime_get_sync() which is returning -EACCES instead of zero, and 
>>> this is currently not checked so the failure is not detected before 
>>> register access takes place, to register file with clock disabled, 
>>> which triggers a hard hang.
>>>
>>> I'll be sending a patch shortly, thanks for this hint !
>>>
>>
>> Great news, indeed the return code isn't checked. Let's use
>> pm_runtime_resume_and_get().
> 
> Yes please.
> 
> I will wonder why we get EACCES though, that basically means we are 
> suspending already. Is it safe to return -errno from rng read function 
> in that case ?

The framework expects a function that can return an error code so I
don't see why not. Else the framework would have an issue.

I still haven't figured out what is happening.

Could it be that the kernel is getting entropy with hwrng_fillfn()
like it does periodically to feed the entropy pool and it happens at the
same time as your pm test sequence?

FYI, I have been running your script with (echo devices > 
/sys/power/pm_test) for 5 hours now and haven't been able to reproduce 
the issue.

> 
>>>>>> After this, I'll try to reproduce the issue.
>>>>>
>>>>> If you have a minute to test it on some ST MP15 board, that would 
>>>>> be real nice. Thanks !
>>>>
>>>> I tried to reproduce the issue you're facing on a STM32MP157C-DK2 no
>>>> SCMI on the 6.9-rc7 kernel tag. I uses OP-TEE and TF-A in the bootchain
>>>> but this should not have an impact here.
>>>>
>>>> How did you manage to test using "echo core > /sys/power/pm_test"?
>>>> In kernel/power/suspend.c, enter_state(). If the pm_test_level is core,
>>>> then an error is fired with the following trace:
>>>> "Unsupported test mode for suspend to idle, please choose 
>>>> none/freezer/devices/platform."
>>>
>>> Could this be firmware related ?
>>>
>>>> I've tried using "echo devices > /sys/power/pm_test" so that I can 
>>>> at least test that the driver is put to sleep then wakes up. I do not
>>>> reproduce your issue.
>>>
>>> Can you try 'processors' ?
>>>
>>
>> Given this:
>> #ifdef CONFIG_PM_DEBUG
>>          if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
>>              pr_warn("Unsupported test mode for suspend to idle
> 
> You're supposed to be suspending to 'mem' , not 'idle' . Could that be it ?

Yes you're right, I've been missing that. I do not have "deep" available
in /sys/power/mem_sleep... not upstreamed yet maybe... Have you coded a
PSCI service for this in U-Boot?

I'm either missing something or I can't reproduce your setup.

Thanks,
Gatien


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Marek Vasut]

On 5/16/24 4:35 PM, Gatien CHEVALLIER wrote:

Hi,

>>>>>>> What if you add a trace in a random generation function in random.c?
>>>>>>
>>>>>> Do you have a function name or line number for me ?
>>>>>
>>>>> I put a trace in _get_random_bytes() in drivers/char/random.c. I'm not
>>>>> 100% sure but this should be the entry point when getting a random 
>>>>> number.
>>>>
>>>> You're right, there is a read attempt right before the hang, and 
>>>> __clk_is_enabled() returns 0 in stm32_read_rng() . In fact, it is 
>>>> the pm_runtime_get_sync() which is returning -EACCES instead of 
>>>> zero, and this is currently not checked so the failure is not 
>>>> detected before register access takes place, to register file with 
>>>> clock disabled, which triggers a hard hang.
>>>>
>>>> I'll be sending a patch shortly, thanks for this hint !
>>>>
>>>
>>> Great news, indeed the return code isn't checked. Let's use
>>> pm_runtime_resume_and_get().
>>
>> Yes please.
>>
>> I will wonder why we get EACCES though, that basically means we are 
>> suspending already. Is it safe to return -errno from rng read function 
>> in that case ?
> 
> The framework expects a function that can return an error code so I
> don't see why not. Else the framework would have an issue.
> 
> I still haven't figured out what is happening.
> 
> Could it be that the kernel is getting entropy with hwrng_fillfn()
> like it does periodically to feed the entropy pool and it happens at the
> same time as your pm test sequence?

Possibly. I use script as init which contains basically #!/bin/sh , 
mount of a few filesystems like dev, proc, sys, and then the pm_test 
sequence to avoid wasting time booting full userspace.

> FYI, I have been running your script with (echo devices > 
> /sys/power/pm_test) for 5 hours now and haven't been able to reproduce 
> the issue.

Maybe the 'devices' test is not enough and the deeper pm_test states 
have some sort of impact ?

>>>>>>> After this, I'll try to reproduce the issue.
>>>>>>
>>>>>> If you have a minute to test it on some ST MP15 board, that would 
>>>>>> be real nice. Thanks !
>>>>>
>>>>> I tried to reproduce the issue you're facing on a STM32MP157C-DK2 no
>>>>> SCMI on the 6.9-rc7 kernel tag. I uses OP-TEE and TF-A in the 
>>>>> bootchain
>>>>> but this should not have an impact here.
>>>>>
>>>>> How did you manage to test using "echo core > /sys/power/pm_test"?
>>>>> In kernel/power/suspend.c, enter_state(). If the pm_test_level is 
>>>>> core,
>>>>> then an error is fired with the following trace:
>>>>> "Unsupported test mode for suspend to idle, please choose 
>>>>> none/freezer/devices/platform."
>>>>
>>>> Could this be firmware related ?
>>>>
>>>>> I've tried using "echo devices > /sys/power/pm_test" so that I can 
>>>>> at least test that the driver is put to sleep then wakes up. I do not
>>>>> reproduce your issue.
>>>>
>>>> Can you try 'processors' ?
>>>>
>>>
>>> Given this:
>>> #ifdef CONFIG_PM_DEBUG
>>>          if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
>>>              pr_warn("Unsupported test mode for suspend to idle
>>
>> You're supposed to be suspending to 'mem' , not 'idle' . Could that be 
>> it ?
> 
> Yes you're right, I've been missing that. I do not have "deep" available
> in /sys/power/mem_sleep... not upstreamed yet maybe... Have you coded a
> PSCI service for this in U-Boot?
> 
> I'm either missing something or I can't reproduce your setup.

The PSCI provider in U-Boot has been in place for years, there's no need 
to code anything, just compile it and that's all:

$ make stm32mp15_basic_defconfig && make -j`nproc`

This gets you u-boot-spl.stm32 and u-boot.itb as FSBL/SSBL .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Gatien CHEVALLIER]

On 5/16/24 22:01, Marek Vasut wrote:
> On 5/16/24 4:35 PM, Gatien CHEVALLIER wrote:
> 
> Hi,
> 
>>>>>>>> What if you add a trace in a random generation function in 
>>>>>>>> random.c?
>>>>>>>
>>>>>>> Do you have a function name or line number for me ?
>>>>>>
>>>>>> I put a trace in _get_random_bytes() in drivers/char/random.c. I'm 
>>>>>> not
>>>>>> 100% sure but this should be the entry point when getting a random 
>>>>>> number.
>>>>>
>>>>> You're right, there is a read attempt right before the hang, and 
>>>>> __clk_is_enabled() returns 0 in stm32_read_rng() . In fact, it is 
>>>>> the pm_runtime_get_sync() which is returning -EACCES instead of 
>>>>> zero, and this is currently not checked so the failure is not 
>>>>> detected before register access takes place, to register file with 
>>>>> clock disabled, which triggers a hard hang.
>>>>>
>>>>> I'll be sending a patch shortly, thanks for this hint !
>>>>>
>>>>
>>>> Great news, indeed the return code isn't checked. Let's use
>>>> pm_runtime_resume_and_get().
>>>
>>> Yes please.
>>>
>>> I will wonder why we get EACCES though, that basically means we are 
>>> suspending already. Is it safe to return -errno from rng read 
>>> function in that case ?
>>
>> The framework expects a function that can return an error code so I
>> don't see why not. Else the framework would have an issue.
>>
>> I still haven't figured out what is happening.
>>
>> Could it be that the kernel is getting entropy with hwrng_fillfn()
>> like it does periodically to feed the entropy pool and it happens at the
>> same time as your pm test sequence?
> 
> Possibly. I use script as init which contains basically #!/bin/sh , 
> mount of a few filesystems like dev, proc, sys, and then the pm_test 
> sequence to avoid wasting time booting full userspace.
> 
Ok,

The strangest thing is not being to enable the clock, maybe there's
something on the clock driver side. Tracking clock enable/disable
may lead to something.

>> FYI, I have been running your script with (echo devices > 
>> /sys/power/pm_test) for 5 hours now and haven't been able to reproduce 
>> the issue.
> 
> Maybe the 'devices' test is not enough and the deeper pm_test states 
> have some sort of impact ?
>

Maybe, I don't have the knowledge to confirm or invalidate this.
Tasks should be frozen before drivers are put to sleep so my instinct
would say no but you can't take it for granted :)

>>>>>>>> After this, I'll try to reproduce the issue.
>>>>>>>
>>>>>>> If you have a minute to test it on some ST MP15 board, that would 
>>>>>>> be real nice. Thanks !
>>>>>>
>>>>>> I tried to reproduce the issue you're facing on a STM32MP157C-DK2 no
>>>>>> SCMI on the 6.9-rc7 kernel tag. I uses OP-TEE and TF-A in the 
>>>>>> bootchain
>>>>>> but this should not have an impact here.
>>>>>>
>>>>>> How did you manage to test using "echo core > /sys/power/pm_test"?
>>>>>> In kernel/power/suspend.c, enter_state(). If the pm_test_level is 
>>>>>> core,
>>>>>> then an error is fired with the following trace:
>>>>>> "Unsupported test mode for suspend to idle, please choose 
>>>>>> none/freezer/devices/platform."
>>>>>
>>>>> Could this be firmware related ?
>>>>>
>>>>>> I've tried using "echo devices > /sys/power/pm_test" so that I can 
>>>>>> at least test that the driver is put to sleep then wakes up. I do not
>>>>>> reproduce your issue.
>>>>>
>>>>> Can you try 'processors' ?
>>>>>
>>>>
>>>> Given this:
>>>> #ifdef CONFIG_PM_DEBUG
>>>>          if (pm_test_level != TEST_NONE && pm_test_level <= 
>>>> TEST_CPUS) {
>>>>              pr_warn("Unsupported test mode for suspend to idle
>>>
>>> You're supposed to be suspending to 'mem' , not 'idle' . Could that 
>>> be it ?
>>
>> Yes you're right, I've been missing that. I do not have "deep" available
>> in /sys/power/mem_sleep... not upstreamed yet maybe... Have you coded a
>> PSCI service for this in U-Boot?
>>
>> I'm either missing something or I can't reproduce your setup.
> 
> The PSCI provider in U-Boot has been in place for years, there's no need 
> to code anything, just compile it and that's all:
> 
> $ make stm32mp15_basic_defconfig && make -j`nproc`
> 
> This gets you u-boot-spl.stm32 and u-boot.itb as FSBL/SSBL .

Ok thanks.

Best regards,
Gatien

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Marek Vasut]

On 5/17/24 5:39 PM, Gatien CHEVALLIER wrote:

Hi,

>> Possibly. I use script as init which contains basically #!/bin/sh , 
>> mount of a few filesystems like dev, proc, sys, and then the pm_test 
>> sequence to avoid wasting time booting full userspace.
>>
> Ok,
> 
> The strangest thing is not being to enable the clock, maybe there's
> something on the clock driver side. Tracking clock enable/disable
> may lead to something.

I suspect the problem is that rng_read and runtime suspend/resume can 
run in parallel, that's why this problem occurs.

>>> FYI, I have been running your script with (echo devices > 
>>> /sys/power/pm_test) for 5 hours now and haven't been able to 
>>> reproduce the issue.
>>
>> Maybe the 'devices' test is not enough and the deeper pm_test states 
>> have some sort of impact ?
>>
> 
> Maybe, I don't have the knowledge to confirm or invalidate this.
> Tasks should be frozen before drivers are put to sleep so my instinct
> would say no but you can't take it for granted :)

Could it be the kernel that requires randomness ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] clk: stm32mp1: Keep RNG1 clock always running

[Gatien CHEVALLIER]

On 5/21/24 12:27, Marek Vasut wrote:
> On 5/17/24 5:39 PM, Gatien CHEVALLIER wrote:
> 
> Hi,
> 
>>> Possibly. I use script as init which contains basically #!/bin/sh , 
>>> mount of a few filesystems like dev, proc, sys, and then the pm_test 
>>> sequence to avoid wasting time booting full userspace.
>>>
>> Ok,
>>
>> The strangest thing is not being to enable the clock, maybe there's
>> something on the clock driver side. Tracking clock enable/disable
>> may lead to something.
> 
> I suspect the problem is that rng_read and runtime suspend/resume can 
> run in parallel, that's why this problem occurs.
> 

Hum, this looks strange... This would need to be confirmed in your
use case. That would mean that flags aren't synced at the entry of these
functions?

>>>> FYI, I have been running your script with (echo devices > 
>>>> /sys/power/pm_test) for 5 hours now and haven't been able to 
>>>> reproduce the issue.
>>>
>>> Maybe the 'devices' test is not enough and the deeper pm_test states 
>>> have some sort of impact ?
>>>
>>
>> Maybe, I don't have the knowledge to confirm or invalidate this.
>> Tasks should be frozen before drivers are put to sleep so my instinct
>> would say no but you can't take it for granted :)
> 
> Could it be the kernel that requires randomness ?

That can be confirmed by adding traces to the entry point in random.c.
Maybe activating CONFIG_WARN_ALL_UNSEEDED_RANDOM will help investigate
this. It will add verbosity if crng isn't ready.

Or maybe try calling directely rng_is_initialized() to see if the crng
is ready when your issue occurs.

Best regards,
Gatien

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel