On 8/20/24 11:15, Thomas Richard wrote: > On 8/13/24 19:18, Kevin Hilman wrote: >> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes: >> >>> On Fri, Aug 09, 2024 at 12:04:23PM -0700, Kevin Hilman wrote: >>>> Thomas Richard <thomas.richard@xxxxxxxxxxx> writes: >>>> >>>>> If the console suspend is disabled, the genpd of the console shall not >>>>> be powered-off during suspend. >>>>> Set the flag GENPD_FLAG_ALWAYS_ON to the corresponding genpd during >>>>> suspend, and restore the original value during the resume. >>>>> >>>>> Signed-off-by: Thomas Richard <thomas.richard@xxxxxxxxxxx> >>>> >>>> Hmm, this patch got merged upstream (commit 68e6939ea9ec) even after >>>> disagreements about the approach. >>>> >>>> Even worse, it actually causes a crash during suspend on platforms that >>>> don't use PM domains (like AM335x Beaglebone Black.) >>>> >>>> Details on why this crashes below. >>>> >>>> Thomas, could you please submit a revert for this (with a Fixes: tag) >>>> and then follow up with the approach as discussed later in this thread? >>> >>> Did this revert happen yet? >> >> No. >> >> Could you revert it since it's caused regressions? I will follow up >> with Thomas on the right fix for the original issue (as discussed later >> in this thread.) > > Hello Kevin, > > The series, which implements Théo's proposal, was ready to be sent when > I tried to go back to suspend after a resume. This causes a kernel panic > during the suspend. > > There is the issue in both cases (console suspend and no console suspend). > > The issue comes from the other uarts (not the one used by the console). > If I disable all other uarts, there is no panic. > > It seems the uarts are not resumed correctly. > > More investigation is needed. Hello Kevin, I finally found the culprit. Just adding the GENPD_FLAG_ACTIVE_WAKEUP flag by default caused the panic, even without the call of device_set_wakeup_path(). With some debug, I found that the wakeup_path of all my uarts (even other uarts not used by the console) was set. So the corresponding power domains were not powered off [1]. Consequently they are not powered on during the resume [2]. But on my platform (J7200), the SoC is fully off during suspend-to-ram. Even if a power domain is not powered off by Linux, at the end all the SoC is off. And if Linux doesn't power off a power domain, it doesn't power on it. The wakeup_path of all my uarts is set here [3] because devices are wakeup capable [4]. It was added by commit 8512220c5782d [5]. If I remove the wakeup_path modification (diff at the end of the email), Théo's proposal works well. But it's probably too rough, and I have no idea about the impact on other platforms. If you have an idea to fix correctly this wakeup_path issue, please let me know :) [1] https://elixir.bootlin.com/linux/v6.11/source/drivers/pmdomain/core.c#L1372 [2] https://elixir.bootlin.com/linux/v6.11/source/drivers/pmdomain/core.c#L1427 [3] https://elixir.bootlin.com/linux/v6.10.10/source/drivers/base/power/main.c#L1687 [4] https://elixir.bootlin.com/linux/v6.11/source/drivers/tty/serial/8250/8250_omap.c#L1526 [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8512220c5782d Best Regards, Thomas 8< ------------------------------- diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 4a67e83300e1..e3d9153a9a81 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1683,9 +1683,6 @@ static int device_suspend(struct device *dev, pm_message_t state, bool async) End: if (!error) { dev->power.is_suspended = true; - if (device_may_wakeup(dev)) - dev->power.wakeup_path = true; - dpm_propagate_wakeup_to_parent(dev); dpm_clear_superiors_direct_complete(dev); } @@ -1795,8 +1792,6 @@ static int device_prepare(struct device *dev, pm_message_t state) device_lock(dev); - dev->power.wakeup_path = false; - if (dev->power.no_pm_callbacks) goto unlock;
