| Message ID | 20240513220349.183568-1-marex@denx.de (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] clk: stm32mp1: Keep RNG1 clock always running | expand |
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),
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 !
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
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 ? [...]
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
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 ?
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
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 .
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
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 ?
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
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),
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(-)