Regression in 5.16-rc1 with suspend to idle

Regression in 5.16-rc1 with suspend to idle

[Limonciello, Mario]

[Public]

Hi Rafael,

I've found recently that on kernel 5.17-rc4 some OEM AMD laptops are shutting down when entering suspend to idle.
I bisected this back to commit 8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-idle path")
which was introduced in 5.16-rc1.  As this code has changed since 5.16-rc1 (notably 23f62d7ab25), a simple revert
won't suffice.

# good: [8bb7eca972ad531c9b149c0a51ab43a417385813] Linux 5.15
git bisect good 8bb7eca972ad531c9b149c0a51ab43a417385813
# bad: [fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf] Linux 5.16-rc1
git bisect bad fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
# bad: [313b6ffc8e90173f1709b2f4bf9d30c4730a1dde] Merge tag 'linux-kselftest-kunit-5.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest
git bisect bad 313b6ffc8e90173f1709b2f4bf9d30c4730a1dde
# good: [84882cf72cd774cf16fd338bdbf00f69ac9f9194] Revert "net: avoid double accounting for pure zerocopy skbs"
git bisect good 84882cf72cd774cf16fd338bdbf00f69ac9f9194
# good: [79ef0c00142519bc34e1341447f3797436cc48bf] Merge tag 'trace-v5.16' of git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace
git bisect good 79ef0c00142519bc34e1341447f3797436cc48bf
# good: [0f3d2b680444d5697650b5529c9e749acbf7371f] drm/amdkfd: protect raven_device_info with KFD_SUPPORT_IOMMU_V2
git bisect good 0f3d2b680444d5697650b5529c9e749acbf7371f
# good: [a64a325bf6313aa5cde7ecd691927e92892d1b7f] Merge tag 'afs-next-20211102' of git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs
git bisect good a64a325bf6313aa5cde7ecd691927e92892d1b7f
# good: [d9bd054177fbd2c4762546aec40fc3071bfe4cc0] Merge tag 'amd-drm-next-5.16-2021-10-29' of https://gitlab.freedesktop.org/agd5f/linux into drm-next
git bisect good d9bd054177fbd2c4762546aec40fc3071bfe4cc0
# bad: [833db72142b93a89211c1e43ca0a1e2e16457756] Merge tag 'pm-5.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect bad 833db72142b93a89211c1e43ca0a1e2e16457756
# good: [33fb42636a938be01d951b4cee68127a59a1e7e4] Merge branch 'ucount-fixes-for-v5.16' of git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace
git bisect good 33fb42636a938be01d951b4cee68127a59a1e7e4
# good: [c0d6586afa3546a3d148cf4b9d9a407b4f79d0bb] Merge tag 'acpi-5.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect good c0d6586afa3546a3d148cf4b9d9a407b4f79d0bb
# bad: [b62b306469b36fae7030c0ad4ffa11de0c9b9957] Merge branch 'pm-sleep'
git bisect bad b62b306469b36fae7030c0ad4ffa11de0c9b9957
# good: [1fec16118ff9b822431d83a16430de60cf8e8769] Merge branch 'pm-pci'
git bisect good 1fec16118ff9b822431d83a16430de60cf8e8769
# good: [928265e3601cde78c7e0a3e518a93b27defed3b1] PM: sleep: Do not let "syscore" devices runtime-suspend during system transitions
git bisect good 928265e3601cde78c7e0a3e518a93b27defed3b1
# bad: [9f6abfcd67aae51374b4e8aa0b11f0ebd0d8562f] PM: suspend: Use valid_state() consistently
git bisect bad 9f6abfcd67aae51374b4e8aa0b11f0ebd0d8562f
# bad: [23f62d7ab25bd1a7dbbb89cfcd429df7735855af] PM: sleep: Pause cpuidle later and resume it earlier during system transitions
git bisect bad 23f62d7ab25bd1a7dbbb89cfcd429df7735855af
# bad: [8d89835b0467b7e618c1c93603c1aff85a0c3c66] PM: suspend: Do not pause cpuidle in the suspend-to-idle path
git bisect bad 8d89835b0467b7e618c1c93603c1aff85a0c3c66
# first bad commit: [8d89835b0467b7e618c1c93603c1aff85a0c3c66] PM: suspend: Do not pause cpuidle in the suspend-to-idle path

What would you suggest to be done in this case?  Revert both commits?  Or would you prefer to have a fixup on top
of that?

Thanks,

Re: Regression in 5.16-rc1 with suspend to idle

[Rafael J. Wysocki]

On Thu, Feb 17, 2022 at 7:15 AM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> Hi Rafael,
>
> I've found recently that on kernel 5.17-rc4 some OEM AMD laptops are shutting down when entering suspend to idle.

Interesting.  Can you identify the exact point when the shutdown occurs?

> I bisected this back to commit 8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-idle path")
> which was introduced in 5.16-rc1.  As this code has changed since 5.16-rc1 (notably 23f62d7ab25), a simple revert
> won't suffice.
>
> # good: [8bb7eca972ad531c9b149c0a51ab43a417385813] Linux 5.15
> git bisect good 8bb7eca972ad531c9b149c0a51ab43a417385813
> # bad: [fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf] Linux 5.16-rc1
> git bisect bad fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
> # bad: [313b6ffc8e90173f1709b2f4bf9d30c4730a1dde] Merge tag 'linux-kselftest-kunit-5.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest
> git bisect bad 313b6ffc8e90173f1709b2f4bf9d30c4730a1dde
> # good: [84882cf72cd774cf16fd338bdbf00f69ac9f9194] Revert "net: avoid double accounting for pure zerocopy skbs"
> git bisect good 84882cf72cd774cf16fd338bdbf00f69ac9f9194
> # good: [79ef0c00142519bc34e1341447f3797436cc48bf] Merge tag 'trace-v5.16' of git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace
> git bisect good 79ef0c00142519bc34e1341447f3797436cc48bf
> # good: [0f3d2b680444d5697650b5529c9e749acbf7371f] drm/amdkfd: protect raven_device_info with KFD_SUPPORT_IOMMU_V2
> git bisect good 0f3d2b680444d5697650b5529c9e749acbf7371f
> # good: [a64a325bf6313aa5cde7ecd691927e92892d1b7f] Merge tag 'afs-next-20211102' of git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs
> git bisect good a64a325bf6313aa5cde7ecd691927e92892d1b7f
> # good: [d9bd054177fbd2c4762546aec40fc3071bfe4cc0] Merge tag 'amd-drm-next-5.16-2021-10-29' of https://gitlab.freedesktop.org/agd5f/linux into drm-next
> git bisect good d9bd054177fbd2c4762546aec40fc3071bfe4cc0
> # bad: [833db72142b93a89211c1e43ca0a1e2e16457756] Merge tag 'pm-5.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> git bisect bad 833db72142b93a89211c1e43ca0a1e2e16457756
> # good: [33fb42636a938be01d951b4cee68127a59a1e7e4] Merge branch 'ucount-fixes-for-v5.16' of git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace
> git bisect good 33fb42636a938be01d951b4cee68127a59a1e7e4
> # good: [c0d6586afa3546a3d148cf4b9d9a407b4f79d0bb] Merge tag 'acpi-5.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> git bisect good c0d6586afa3546a3d148cf4b9d9a407b4f79d0bb
> # bad: [b62b306469b36fae7030c0ad4ffa11de0c9b9957] Merge branch 'pm-sleep'
> git bisect bad b62b306469b36fae7030c0ad4ffa11de0c9b9957
> # good: [1fec16118ff9b822431d83a16430de60cf8e8769] Merge branch 'pm-pci'
> git bisect good 1fec16118ff9b822431d83a16430de60cf8e8769
> # good: [928265e3601cde78c7e0a3e518a93b27defed3b1] PM: sleep: Do not let "syscore" devices runtime-suspend during system transitions
> git bisect good 928265e3601cde78c7e0a3e518a93b27defed3b1
> # bad: [9f6abfcd67aae51374b4e8aa0b11f0ebd0d8562f] PM: suspend: Use valid_state() consistently
> git bisect bad 9f6abfcd67aae51374b4e8aa0b11f0ebd0d8562f
> # bad: [23f62d7ab25bd1a7dbbb89cfcd429df7735855af] PM: sleep: Pause cpuidle later and resume it earlier during system transitions
> git bisect bad 23f62d7ab25bd1a7dbbb89cfcd429df7735855af
> # bad: [8d89835b0467b7e618c1c93603c1aff85a0c3c66] PM: suspend: Do not pause cpuidle in the suspend-to-idle path
> git bisect bad 8d89835b0467b7e618c1c93603c1aff85a0c3c66
> # first bad commit: [8d89835b0467b7e618c1c93603c1aff85a0c3c66] PM: suspend: Do not pause cpuidle in the suspend-to-idle path
>
> What would you suggest to be done in this case?  Revert both commits?  Or would you prefer to have a fixup on top
> of that?

I would prefer to fix the problem on top of the current 5.16-rc.

Thanks!

RE: Regression in 5.16-rc1 with suspend to idle

[Limonciello, Mario]

[Public]

> > I've found recently that on kernel 5.17-rc4 some OEM AMD laptops are
> shutting down when entering suspend to idle.
> 
> Interesting.  Can you identify the exact point when the shutdown occurs?

It looks like it's a platform firmware crash that causes the shutdown not a kernel
crash.

> > What would you suggest to be done in this case?  Revert both commits?  Or
> would you prefer to have a fixup on top
> > of that?
> 
> I would prefer to fix the problem on top of the current 5.16-rc.
> 

Here is the minimal patch I can come up with on top of 5.17-rc4 that fixes it:

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 04ea92cbd9cf..f5bf46782043 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -32,6 +32,7 @@
 #include <linux/suspend.h>
 #include <trace/events/power.h>
 #include <linux/cpufreq.h>
+#include <linux/cpuidle.h>
 #include <linux/devfreq.h>
 #include <linux/timer.h>

@@ -1350,6 +1351,8 @@ int dpm_suspend_noirq(pm_message_t state)
 {
        int ret;

+       cpuidle_pause();
+
        device_wakeup_arm_wake_irqs();
        suspend_device_irqs();

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 6fcdee7e87a5..1708a643ba5d 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -97,6 +97,7 @@ static void s2idle_enter(void)
        raw_spin_unlock_irq(&s2idle_lock);

        cpus_read_lock();
+       cpuidle_resume();

        /* Push all the CPUs into the idle loop. */
        wake_up_all_idle_cpus();
@@ -104,6 +105,7 @@ static void s2idle_enter(void)
        swait_event_exclusive(s2idle_wait_head,
                    s2idle_state == S2IDLE_STATE_WAKE);

+       cpuidle_pause();
        cpus_read_unlock();

        raw_spin_lock_irq(&s2idle_lock);


* Removing the cpuidle_pause call from s2idle_enter will fix the crash,
  but the system doesn't enter the deepest sleep state.

* removing the cpuidle_pause call from dpm_suspend_noirq the firmware continues to
   crash.

I also confirmed that reverting both of these commits together on top of 5.17-rc4 fixes it:

8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-idle path")
23f62d7ab25b ("PM: sleep: Pause cpuidle later and resume it earlier during system transitions")

The commit messages at least make it sound like it was just a rework for unification of the codepaths,
not supposed to be for anything to be actually fixed, so I would think it should be safe to revert.

So please advise which way you want to go and I'll send up a patch (or if you want to write one
I'm happy to take it and test it since I can readily reproduce it).

Re: Regression in 5.16-rc1 with suspend to idle

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 3933 bytes --]

On Thu, Feb 17, 2022 at 9:16 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> > > I've found recently that on kernel 5.17-rc4 some OEM AMD laptops are
> > shutting down when entering suspend to idle.
> >
> > Interesting.  Can you identify the exact point when the shutdown occurs?
>
> It looks like it's a platform firmware crash that causes the shutdown not a kernel
> crash.

It looks like we'll need to quirk the system in question then.

> > > What would you suggest to be done in this case?  Revert both commits?  Or
> > would you prefer to have a fixup on top
> > > of that?
> >
> > I would prefer to fix the problem on top of the current 5.16-rc.
> >
>
> Here is the minimal patch I can come up with on top of 5.17-rc4 that fixes it:
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 04ea92cbd9cf..f5bf46782043 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -32,6 +32,7 @@
>  #include <linux/suspend.h>
>  #include <trace/events/power.h>
>  #include <linux/cpufreq.h>
> +#include <linux/cpuidle.h>
>  #include <linux/devfreq.h>
>  #include <linux/timer.h>
>
> @@ -1350,6 +1351,8 @@ int dpm_suspend_noirq(pm_message_t state)
>  {
>         int ret;
>
> +       cpuidle_pause();

Can you replace this with wake_up_all_idle_cpus() and remove the
cpuidle_resume()/cpuidle_pause() calls from s2idle_enter() and see
what happens?

> +
>         device_wakeup_arm_wake_irqs();
>         suspend_device_irqs();
>
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 6fcdee7e87a5..1708a643ba5d 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -97,6 +97,7 @@ static void s2idle_enter(void)
>         raw_spin_unlock_irq(&s2idle_lock);
>
>         cpus_read_lock();
> +       cpuidle_resume();
>
>         /* Push all the CPUs into the idle loop. */
>         wake_up_all_idle_cpus();
> @@ -104,6 +105,7 @@ static void s2idle_enter(void)
>         swait_event_exclusive(s2idle_wait_head,
>                     s2idle_state == S2IDLE_STATE_WAKE);
>
> +       cpuidle_pause();
>         cpus_read_unlock();
>
>         raw_spin_lock_irq(&s2idle_lock);
>
>
> * Removing the cpuidle_pause call from s2idle_enter will fix the crash,
>   but the system doesn't enter the deepest sleep state.

I think you mean that removing it doesn't make a difference except
that it prevents the deepest state from being entered.

Pausing cpuidle here is kind of redundant, because it just flips the
"initialized" flag that's going to be flipped again by
cpuidle_resume() in the next iteration.  [BTW, your patch is missing
one an additional cpuidle_resume() in the resume path to match this
cpuidle_pause().]  Also calling  wake_up_all_idle_cpus() is not likely
to make a difference, so I'm guessing that synchronize_rcu() does the
trick.

> * removing the cpuidle_pause call from dpm_suspend_noirq the firmware continues to
>    crash.

So the crash occurs while running dpm_suspend_noirq().

> I also confirmed that reverting both of these commits together on top of 5.17-rc4 fixes it:
>
> 8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-idle path")
> 23f62d7ab25b ("PM: sleep: Pause cpuidle later and resume it earlier during system transitions")
>
> The commit messages at least make it sound like it was just a rework for unification of the codepaths,
> not supposed to be for anything to be actually fixed, so I would think it should be safe to revert.

There is a deeper issue related to these commits and I'm not inclined
to go back to the old code if there is only one system affected by
this and the problem is related to the platform firmware on that
system.

> So please advise which way you want to go and I'll send up a patch (or if you want to write one
> I'm happy to take it and test it since I can readily reproduce it).

To start with, please test the attached debug patch on top of -rc4.

[-- Attachment #2: pm-suspend-cpuidle-debug.patch --]
[-- Type: text/x-patch, Size: 957 bytes --]

---
 drivers/base/power/main.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -32,6 +32,7 @@
 #include <linux/suspend.h>
 #include <trace/events/power.h>
 #include <linux/cpufreq.h>
+#include <linux/cpuidle.h>
 #include <linux/devfreq.h>
 #include <linux/timer.h>
 
@@ -1294,6 +1295,8 @@ static int dpm_noirq_suspend_devices(pm_
 	ktime_t starttime = ktime_get();
 	int error = 0;
 
+	cpuidle_pause();
+
 	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
 	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
@@ -1336,6 +1339,9 @@ static int dpm_noirq_suspend_devices(pm_
 	}
 	dpm_show_time(starttime, state, error, "noirq");
 	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, false);
+
+	cpuidle_resume();
+
 	return error;
 }
 

Re: Regression in 5.16-rc1 with suspend to idle

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 4409 bytes --]

On Fri, Feb 18, 2022 at 5:15 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 17, 2022 at 9:16 PM Limonciello, Mario
> <Mario.Limonciello@amd.com> wrote:
> >
> > [Public]
> >
> > > > I've found recently that on kernel 5.17-rc4 some OEM AMD laptops are
> > > shutting down when entering suspend to idle.
> > >
> > > Interesting.  Can you identify the exact point when the shutdown occurs?
> >
> > It looks like it's a platform firmware crash that causes the shutdown not a kernel
> > crash.
>
> It looks like we'll need to quirk the system in question then.
>
> > > > What would you suggest to be done in this case?  Revert both commits?  Or
> > > would you prefer to have a fixup on top
> > > > of that?
> > >
> > > I would prefer to fix the problem on top of the current 5.16-rc.
> > >
> >
> > Here is the minimal patch I can come up with on top of 5.17-rc4 that fixes it:
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 04ea92cbd9cf..f5bf46782043 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/suspend.h>
> >  #include <trace/events/power.h>
> >  #include <linux/cpufreq.h>
> > +#include <linux/cpuidle.h>
> >  #include <linux/devfreq.h>
> >  #include <linux/timer.h>
> >
> > @@ -1350,6 +1351,8 @@ int dpm_suspend_noirq(pm_message_t state)
> >  {
> >         int ret;
> >
> > +       cpuidle_pause();
>
> Can you replace this with wake_up_all_idle_cpus() and remove the
> cpuidle_resume()/cpuidle_pause() calls from s2idle_enter() and see
> what happens?
>
> > +
> >         device_wakeup_arm_wake_irqs();
> >         suspend_device_irqs();
> >
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 6fcdee7e87a5..1708a643ba5d 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -97,6 +97,7 @@ static void s2idle_enter(void)
> >         raw_spin_unlock_irq(&s2idle_lock);
> >
> >         cpus_read_lock();
> > +       cpuidle_resume();
> >
> >         /* Push all the CPUs into the idle loop. */
> >         wake_up_all_idle_cpus();
> > @@ -104,6 +105,7 @@ static void s2idle_enter(void)
> >         swait_event_exclusive(s2idle_wait_head,
> >                     s2idle_state == S2IDLE_STATE_WAKE);
> >
> > +       cpuidle_pause();
> >         cpus_read_unlock();
> >
> >         raw_spin_lock_irq(&s2idle_lock);
> >
> >
> > * Removing the cpuidle_pause call from s2idle_enter will fix the crash,
> >   but the system doesn't enter the deepest sleep state.
>
> I think you mean that removing it doesn't make a difference except
> that it prevents the deepest state from being entered.
>
> Pausing cpuidle here is kind of redundant, because it just flips the
> "initialized" flag that's going to be flipped again by
> cpuidle_resume() in the next iteration.  [BTW, your patch is missing
> one an additional cpuidle_resume() in the resume path to match this
> cpuidle_pause().]  Also calling  wake_up_all_idle_cpus() is not likely
> to make a difference, so I'm guessing that synchronize_rcu() does the
> trick.
>
> > * removing the cpuidle_pause call from dpm_suspend_noirq the firmware continues to
> >    crash.
>
> So the crash occurs while running dpm_suspend_noirq().
>
> > I also confirmed that reverting both of these commits together on top of 5.17-rc4 fixes it:
> >
> > 8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-idle path")
> > 23f62d7ab25b ("PM: sleep: Pause cpuidle later and resume it earlier during system transitions")
> >
> > The commit messages at least make it sound like it was just a rework for unification of the codepaths,
> > not supposed to be for anything to be actually fixed, so I would think it should be safe to revert.
>
> There is a deeper issue related to these commits and I'm not inclined
> to go back to the old code if there is only one system affected by
> this and the problem is related to the platform firmware on that
> system.
>
> > So please advise which way you want to go and I'll send up a patch (or if you want to write one
> > I'm happy to take it and test it since I can readily reproduce it).
>
> To start with, please test the attached debug patch on top of -rc4.

Attached is another patch to try, testing the hypothesis that the
observed crash is related to CPUs being in idle state that are too
deep for some reason during late suspend and early resume.

[-- Attachment #2: pm-suspend-cpu-latency-qos.patch --]
[-- Type: text/x-patch, Size: 1477 bytes --]

---
 kernel/power/suspend.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -23,6 +23,7 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/pm_qos.h>
 #include <linux/suspend.h>
 #include <linux/syscore_ops.h>
 #include <linux/swait.h>
@@ -367,6 +368,20 @@ static int suspend_prepare(suspend_state
 	return error;
 }
 
+static struct pm_qos_request pm_suspend_cpu_latency_qos_req;
+
+static void pm_suspend_cpu_latency_qos_set(void)
+{
+	cpu_latency_qos_add_request(&pm_suspend_cpu_latency_qos_req, 3);
+	wake_up_all_idle_cpus();
+}
+
+static void pm_suspend_cpu_latency_qos_clear(void)
+{
+	cpu_latency_qos_remove_request(&pm_suspend_cpu_latency_qos_req);
+	wake_up_all_idle_cpus();
+}
+
 /* default implementation */
 void __weak arch_suspend_disable_irqs(void)
 {
@@ -403,6 +418,8 @@ static int suspend_enter(suspend_state_t
 	if (error)
 		goto Devices_early_resume;
 
+	pm_suspend_cpu_latency_qos_set();
+
 	error = dpm_suspend_noirq(PMSG_SUSPEND);
 	if (error) {
 		pr_err("noirq suspend of devices failed\n");
@@ -457,6 +474,7 @@ static int suspend_enter(suspend_state_t
 	dpm_resume_noirq(PMSG_RESUME);
 
  Platform_early_resume:
+	pm_suspend_cpu_latency_qos_clear();
 	platform_resume_early(state);
 
  Devices_early_resume:

RE: Regression in 5.16-rc1 with suspend to idle

[Limonciello, Mario]

[Public]

> it:
> > >
> > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > > index 04ea92cbd9cf..f5bf46782043 100644
> > > --- a/drivers/base/power/main.c
> > > +++ b/drivers/base/power/main.c
> > > @@ -32,6 +32,7 @@
> > >  #include <linux/suspend.h>
> > >  #include <trace/events/power.h>
> > >  #include <linux/cpufreq.h>
> > > +#include <linux/cpuidle.h>
> > >  #include <linux/devfreq.h>
> > >  #include <linux/timer.h>
> > >
> > > @@ -1350,6 +1351,8 @@ int dpm_suspend_noirq(pm_message_t state)
> > >  {
> > >         int ret;
> > >
> > > +       cpuidle_pause();
> >
> > Can you replace this with wake_up_all_idle_cpus() and remove the
> > cpuidle_resume()/cpuidle_pause() calls from s2idle_enter() and see
> > what happens?
> >
> > > +
> > >         device_wakeup_arm_wake_irqs();
> > >         suspend_device_irqs();
> > >
> > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > > index 6fcdee7e87a5..1708a643ba5d 100644
> > > --- a/kernel/power/suspend.c
> > > +++ b/kernel/power/suspend.c
> > > @@ -97,6 +97,7 @@ static void s2idle_enter(void)
> > >         raw_spin_unlock_irq(&s2idle_lock);
> > >
> > >         cpus_read_lock();
> > > +       cpuidle_resume();
> > >
> > >         /* Push all the CPUs into the idle loop. */
> > >         wake_up_all_idle_cpus();
> > > @@ -104,6 +105,7 @@ static void s2idle_enter(void)
> > >         swait_event_exclusive(s2idle_wait_head,
> > >                     s2idle_state == S2IDLE_STATE_WAKE);
> > >
> > > +       cpuidle_pause();
> > >         cpus_read_unlock();
> > >
> > >         raw_spin_lock_irq(&s2idle_lock);
> > >
> > >
> > > * Removing the cpuidle_pause call from s2idle_enter will fix the crash,
> > >   but the system doesn't enter the deepest sleep state.
> >
> > I think you mean that removing it doesn't make a difference except
> > that it prevents the deepest state from being entered.
> >
> > Pausing cpuidle here is kind of redundant, because it just flips the
> > "initialized" flag that's going to be flipped again by
> > cpuidle_resume() in the next iteration.  [BTW, your patch is missing
> > one an additional cpuidle_resume() in the resume path to match this
> > cpuidle_pause().]  Also calling  wake_up_all_idle_cpus() is not likely
> > to make a difference, so I'm guessing that synchronize_rcu() does the
> > trick.
> >
> > > * removing the cpuidle_pause call from dpm_suspend_noirq the
> firmware continues to
> > >    crash.
> >
> > So the crash occurs while running dpm_suspend_noirq().
> >
> > > I also confirmed that reverting both of these commits together on top of
> 5.17-rc4 fixes it:
> > >
> > > 8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-
> idle path")
> > > 23f62d7ab25b ("PM: sleep: Pause cpuidle later and resume it earlier
> during system transitions")
> > >
> > > The commit messages at least make it sound like it was just a rework for
> unification of the codepaths,
> > > not supposed to be for anything to be actually fixed, so I would think it
> should be safe to revert.
> >
> > There is a deeper issue related to these commits and I'm not inclined
> > to go back to the old code if there is only one system affected by
> > this and the problem is related to the platform firmware on that
> > system.
> >
> > > So please advise which way you want to go and I'll send up a patch (or if
> you want to write one
> > > I'm happy to take it and test it since I can readily reproduce it).
> >
> > To start with, please test the attached debug patch on top of -rc4.
> 
> Attached is another patch to try, testing the hypothesis that the
> observed crash is related to CPUs being in idle state that are too
> deep for some reason during late suspend and early resume.

I tried 3 test kernels:
* 5.17-rc4 + Your second debugging patch
* 5.17-rc4+ Your first debugging patch
* 5.17-rc4 + A hack I wrote that pushed amd-pmc into "later" in the suspend
using a global symbol called after LPS0 instead of letting it run in noirq stage
 
It works properly on all of those, tried about 5x time in each.

Then I confirmed I could still crash it on 5.17-rc4 with my control kernel.

Re: Regression in 5.16-rc1 with suspend to idle

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 4512 bytes --]

On Sat, Feb 19, 2022 at 4:01 AM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> > it:
> > > >
> > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > > > index 04ea92cbd9cf..f5bf46782043 100644
> > > > --- a/drivers/base/power/main.c
> > > > +++ b/drivers/base/power/main.c
> > > > @@ -32,6 +32,7 @@
> > > >  #include <linux/suspend.h>
> > > >  #include <trace/events/power.h>
> > > >  #include <linux/cpufreq.h>
> > > > +#include <linux/cpuidle.h>
> > > >  #include <linux/devfreq.h>
> > > >  #include <linux/timer.h>
> > > >
> > > > @@ -1350,6 +1351,8 @@ int dpm_suspend_noirq(pm_message_t state)
> > > >  {
> > > >         int ret;
> > > >
> > > > +       cpuidle_pause();
> > >
> > > Can you replace this with wake_up_all_idle_cpus() and remove the
> > > cpuidle_resume()/cpuidle_pause() calls from s2idle_enter() and see
> > > what happens?
> > >
> > > > +
> > > >         device_wakeup_arm_wake_irqs();
> > > >         suspend_device_irqs();
> > > >
> > > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > > > index 6fcdee7e87a5..1708a643ba5d 100644
> > > > --- a/kernel/power/suspend.c
> > > > +++ b/kernel/power/suspend.c
> > > > @@ -97,6 +97,7 @@ static void s2idle_enter(void)
> > > >         raw_spin_unlock_irq(&s2idle_lock);
> > > >
> > > >         cpus_read_lock();
> > > > +       cpuidle_resume();
> > > >
> > > >         /* Push all the CPUs into the idle loop. */
> > > >         wake_up_all_idle_cpus();
> > > > @@ -104,6 +105,7 @@ static void s2idle_enter(void)
> > > >         swait_event_exclusive(s2idle_wait_head,
> > > >                     s2idle_state == S2IDLE_STATE_WAKE);
> > > >
> > > > +       cpuidle_pause();
> > > >         cpus_read_unlock();
> > > >
> > > >         raw_spin_lock_irq(&s2idle_lock);
> > > >
> > > >
> > > > * Removing the cpuidle_pause call from s2idle_enter will fix the crash,
> > > >   but the system doesn't enter the deepest sleep state.
> > >
> > > I think you mean that removing it doesn't make a difference except
> > > that it prevents the deepest state from being entered.
> > >
> > > Pausing cpuidle here is kind of redundant, because it just flips the
> > > "initialized" flag that's going to be flipped again by
> > > cpuidle_resume() in the next iteration.  [BTW, your patch is missing
> > > one an additional cpuidle_resume() in the resume path to match this
> > > cpuidle_pause().]  Also calling  wake_up_all_idle_cpus() is not likely
> > > to make a difference, so I'm guessing that synchronize_rcu() does the
> > > trick.
> > >
> > > > * removing the cpuidle_pause call from dpm_suspend_noirq the
> > firmware continues to
> > > >    crash.
> > >
> > > So the crash occurs while running dpm_suspend_noirq().
> > >
> > > > I also confirmed that reverting both of these commits together on top of
> > 5.17-rc4 fixes it:
> > > >
> > > > 8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-
> > idle path")
> > > > 23f62d7ab25b ("PM: sleep: Pause cpuidle later and resume it earlier
> > during system transitions")
> > > >
> > > > The commit messages at least make it sound like it was just a rework for
> > unification of the codepaths,
> > > > not supposed to be for anything to be actually fixed, so I would think it
> > should be safe to revert.
> > >
> > > There is a deeper issue related to these commits and I'm not inclined
> > > to go back to the old code if there is only one system affected by
> > > this and the problem is related to the platform firmware on that
> > > system.
> > >
> > > > So please advise which way you want to go and I'll send up a patch (or if
> > you want to write one
> > > > I'm happy to take it and test it since I can readily reproduce it).
> > >
> > > To start with, please test the attached debug patch on top of -rc4.
> >
> > Attached is another patch to try, testing the hypothesis that the
> > observed crash is related to CPUs being in idle state that are too
> > deep for some reason during late suspend and early resume.
>
> I tried 3 test kernels:
> * 5.17-rc4 + Your second debugging patch
> * 5.17-rc4+ Your first debugging patch
> * 5.17-rc4 + A hack I wrote that pushed amd-pmc into "later" in the suspend
> using a global symbol called after LPS0 instead of letting it run in noirq stage
>
> It works properly on all of those, tried about 5x time in each.
>
> Then I confirmed I could still crash it on 5.17-rc4 with my control kernel.

I would do something like the attached patch, then (provided that it works).

[-- Attachment #2: amd-pmc-suspend.patch --]
[-- Type: text/x-patch, Size: 3212 bytes --]

---
 drivers/platform/x86/amd-pmc.c |   36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/platform/x86/amd-pmc.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/amd-pmc.c
+++ linux-pm/drivers/platform/x86/amd-pmc.c
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm_qos.h>
 #include <linux/rtc.h>
 #include <linux/suspend.h>
 #include <linux/seq_file.h>
@@ -144,6 +145,7 @@ static struct amd_pmc_dev pmc;
 static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
 static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
 static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
+static struct pm_qos_request amd_pmc_pm_qos_req;
 
 static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
 {
@@ -531,6 +533,14 @@ static int __maybe_unused amd_pmc_suspen
 	u8 msg;
 	u32 arg = 1;
 
+	/*
+	 * Prevent CPUs from getting into idle states while running the code
+	 * below which is generally safe to run when none of the CPUs are in
+	 * idle states.
+	 */
+	cpu_latency_qos_update_request(&amd_pmc_pm_qos_req, 0);
+	wake_up_all_idle_cpus();
+
 	/* Reset and Start SMU logging - to monitor the s0i3 stats */
 	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
 	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
@@ -539,7 +549,7 @@ static int __maybe_unused amd_pmc_suspen
 	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
 		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
 		if (rc < 0)
-			return rc;
+			goto out;
 	}
 
 	/* Dump the IdleMask before we send hint to SMU */
@@ -551,10 +561,11 @@ static int __maybe_unused amd_pmc_suspen
 
 	if (enable_stb)
 		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
-	if (rc)	{
+	if (rc)
 		dev_err(pdev->dev, "error writing to STB\n");
-		return rc;
-	}
+
+out:
+	cpu_latency_qos_update_request(&amd_pmc_pm_qos_req, PM_QOS_DEFAULT_VALUE);
 
 	return rc;
 }
@@ -565,6 +576,14 @@ static int __maybe_unused amd_pmc_resume
 	int rc;
 	u8 msg;
 
+	/*
+	 * Prevent CPUs from getting into idle states while running the code
+	 * below which is generally safe to run when none of the CPUs are in
+	 * idle states.
+	 */
+	cpu_latency_qos_update_request(&amd_pmc_pm_qos_req, 0);
+	wake_up_all_idle_cpus();
+
 	msg = amd_pmc_get_os_hint(pdev);
 	rc = amd_pmc_send_cmd(pdev, 0, NULL, msg, 0);
 	if (rc)
@@ -579,12 +598,12 @@ static int __maybe_unused amd_pmc_resume
 	/* Write data incremented by 1 to distinguish in stb_read */
 	if (enable_stb)
 		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF + 1);
-	if (rc)	{
+	if (rc)
 		dev_err(pdev->dev, "error writing to STB\n");
-		return rc;
-	}
 
-	return 0;
+	cpu_latency_qos_update_request(&amd_pmc_pm_qos_req, PM_QOS_DEFAULT_VALUE);
+
+	return rc;
 }
 
 static const struct dev_pm_ops amd_pmc_pm_ops = {
@@ -722,6 +741,7 @@ static int amd_pmc_probe(struct platform
 	amd_pmc_get_smu_version(dev);
 	platform_set_drvdata(pdev, dev);
 	amd_pmc_dbgfs_register(dev);
+	cpu_latency_qos_add_request(&amd_pmc_pm_qos_req, PM_QOS_DEFAULT_VALUE);
 	return 0;
 
 err_pci_dev_put:

RE: Regression in 5.16-rc1 with suspend to idle

[Limonciello, Mario]

[AMD Official Use Only]

> > > Attached is another patch to try, testing the hypothesis that the
> > > observed crash is related to CPUs being in idle state that are too
> > > deep for some reason during late suspend and early resume.
> >
> > I tried 3 test kernels:
> > * 5.17-rc4 + Your second debugging patch
> > * 5.17-rc4+ Your first debugging patch
> > * 5.17-rc4 + A hack I wrote that pushed amd-pmc into "later" in the
> suspend
> > using a global symbol called after LPS0 instead of letting it run in noirq stage
> >
> > It works properly on all of those, tried about 5x time in each.
> >
> > Then I confirmed I could still crash it on 5.17-rc4 with my control kernel.
> 
> I would do something like the attached patch, then (provided that it works).

I got a variation of this to work.  Let me clean it up some, do some more testing and I'll send
it out to review.

Long term - are you opposed to drivers/acpi/x86/s2idle.c moving to drivers/platform/x86/?
I'd really like the stuff amd-pmc does to be a callback after lps0 (which is closer to how it works
on Windows - it's the very last thing).

I feel like keeping the stuff it does as noirq is generally fragile, and I want to avoid this kind
of breakage.

Re: Regression in 5.16-rc1 with suspend to idle

[Rafael J. Wysocki]

On Mon, Feb 21, 2022 at 7:42 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [AMD Official Use Only]
>
> > > > Attached is another patch to try, testing the hypothesis that the
> > > > observed crash is related to CPUs being in idle state that are too
> > > > deep for some reason during late suspend and early resume.
> > >
> > > I tried 3 test kernels:
> > > * 5.17-rc4 + Your second debugging patch
> > > * 5.17-rc4+ Your first debugging patch
> > > * 5.17-rc4 + A hack I wrote that pushed amd-pmc into "later" in the
> > suspend
> > > using a global symbol called after LPS0 instead of letting it run in noirq stage
> > >
> > > It works properly on all of those, tried about 5x time in each.
> > >
> > > Then I confirmed I could still crash it on 5.17-rc4 with my control kernel.
> >
> > I would do something like the attached patch, then (provided that it works).
>
> I got a variation of this to work.  Let me clean it up some, do some more testing and I'll send
> it out to review.

OK

> Long term - are you opposed to drivers/acpi/x86/s2idle.c moving to drivers/platform/x86/?

It is tied to the code in sleep.c, so I'd rather not move it.

> I'd really like the stuff amd-pmc does to be a callback after lps0 (which is closer to how it works
> on Windows - it's the very last thing).

I see.

A notifier-based driver interface to be invoked from s2idle.c should
work for that.

> I feel like keeping the stuff it does as noirq is generally fragile, and I want to avoid this kind
> of breakage.

Sure.

RE: Regression in 5.16-rc1 with suspend to idle

[Limonciello, Mario]

[AMD Official Use Only]

> 
> OK

https://lore.kernel.org/platform-driver-x86/20220221200728.2323469-1-mario.limonciello@amd.com/T/#u

> 
> > Long term - are you opposed to drivers/acpi/x86/s2idle.c moving to
> drivers/platform/x86/?
> 
> It is tied to the code in sleep.c, so I'd rather not move it.

OK if we're keeping it there, would you be open to a patch exporting a symbol to
let other drivers register another callback after LPS0?  It's somewhat similar to what my
test hack did and might be a better long term solution if so.

> 
> > I'd really like the stuff amd-pmc does to be a callback after lps0 (which is closer
> to how it works
> > on Windows - it's the very last thing).
> 
> I see.
> 
> A notifier-based driver interface to be invoked from s2idle.c should
> work for that.
> 
> > I feel like keeping the stuff it does as noirq is generally fragile, and I want to
> avoid this kind
> > of breakage.
> 
> Sure.

Re: Regression in 5.16-rc1 with suspend to idle

[Rafael J. Wysocki]

On Mon, Feb 21, 2022 at 9:14 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [AMD Official Use Only]
>
> >
> > OK
>
> https://lore.kernel.org/platform-driver-x86/20220221200728.2323469-1-mario.limonciello@amd.com/T/#u

The comment in amd_pmc_resume() doesn't match the code, because the
QoS request is updated to "don't care" in there.  Also, I'd move that
update to the end of the function in case what happens in
amd_pmc_resume() interferes with idle states.

Moreover, since the "no idle states" period covers the entire suspend
time now AFAICS, I would use a small int (eg. 3) instead of 0 in the
cpu_latency_qos_update_request() call in amd_pmc_verify_czn_rtc() to
allow C1 to be entered via cpuidle during that period (which shouldn't
really hurt).  And the comment would need to be updated accordingly.

> >
> > > Long term - are you opposed to drivers/acpi/x86/s2idle.c moving to
> > drivers/platform/x86/?
> >
> > It is tied to the code in sleep.c, so I'd rather not move it.
>
> OK if we're keeping it there, would you be open to a patch exporting a symbol to
> let other drivers register another callback after LPS0?  It's somewhat similar to what my
> test hack did and might be a better long term solution if so.

Well, please send the patch and I will tell you if it looks good to me.