By default when the system is configured for low power idle in the FADT
the keyboard is set up as a wake source. This matches the behavior that
Windows uses for Modern Standby as well.
It has been reported that a variety of AMD based designs there are
spurious wakeups are happening where two IRQ sources are active.
For example:
```
PM: Triggering wakeup from IRQ 9
PM: Triggering wakeup from IRQ 1
```
In these designs IRQ 9 is the ACPI SCI and IRQ 1 is the keyboard.
One way to trigger this problem is to suspend the laptop and then unplug
the AC adapter. The SOC will be in a hardware sleep state and plugging
in the AC adapter returns control to the kernel's s2idle loop.
Normally if just IRQ 9 was active the s2idle loop would advance any EC
transactions and no other IRQ being active would cause the s2idle loop
to put the SOC back into hardware sleep state.
When this bug occurred IRQ 1 is also active even if no keyboard activity
occurred. This causes the s2idle loop to break and the system to wake.
This is a platform firmware bug triggering IRQ1 without keyboard activity.
This occurs in Windows as well, but Windows will enter "SW DRIPS" and
then with no activity enters back into "HW DRIPS" (hardware sleep state).
This issue affects Renoir, Lucienne, Cezanne, and Barcelo platforms. It
does not happen on newer systems such as Mendocino or Rembrandt.
It's been fixed in newer platform firmware. To avoid triggering the bug
on older systems check the SMU F/W version and adjust the policy at suspend
time for s2idle wakeup from keyboard on these systems. A lot of thought
and experimentation has been given around the timing of disabling IRQ1,
and to make it work the "suspend" PM callback is restored.
Reported-by: Kai-Heng Feng <[email protected]>
Reported-by: Xaver Hugl <[email protected]>
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2115
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1951
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/platform/x86/amd/pmc.c | 50 ++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 8d924986381be..be1b49824edbd 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -22,6 +22,7 @@
#include <linux/pci.h>
#include <linux/platform_device.h>
#include <linux/rtc.h>
+#include <linux/serio.h>
#include <linux/suspend.h>
#include <linux/seq_file.h>
#include <linux/uaccess.h>
@@ -653,6 +654,33 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
return -EINVAL;
}
+static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
+{
+ struct device *d;
+ int rc;
+
+ if (!pdev->major) {
+ rc = amd_pmc_get_smu_version(pdev);
+ if (rc)
+ return rc;
+ }
+
+ if (pdev->major > 64 || (pdev->major == 64 && pdev->minor > 65))
+ return 0;
+
+ d = bus_find_device_by_name(&serio_bus, NULL, "serio0");
+ if (!d)
+ return 0;
+ if (device_may_wakeup(d)) {
+ dev_info_once(d, "Disabling IRQ1 wakeup source to avoid platform firmware bug\n");
+ disable_irq_wake(1);
+ device_set_wakeup_enable(d, false);
+ }
+ put_device(d);
+
+ return 0;
+}
+
static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
{
struct rtc_device *rtc_device;
@@ -782,6 +810,25 @@ static struct acpi_s2idle_dev_ops amd_pmc_s2idle_dev_ops = {
.check = amd_pmc_s2idle_check,
.restore = amd_pmc_s2idle_restore,
};
+
+static int __maybe_unused amd_pmc_suspend_handler(struct device *dev)
+{
+ struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
+
+ if (pdev->cpu_id == AMD_CPU_ID_CZN) {
+ int rc = amd_pmc_czn_wa_irq1(pdev);
+
+ if (rc) {
+ dev_err(pdev->dev, "failed to adjust keyboard wakeup: %d\n", rc);
+ return rc;
+ }
+ }
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler, NULL);
+
#endif
static const struct pci_device_id pmc_pci_ids[] = {
@@ -980,6 +1027,9 @@ static struct platform_driver amd_pmc_driver = {
.name = "amd_pmc",
.acpi_match_table = amd_pmc_acpi_ids,
.dev_groups = pmc_groups,
+#ifdef CONFIG_SUSPEND
+ .pm = &amd_pmc_pm,
+#endif
},
.probe = amd_pmc_probe,
.remove = amd_pmc_remove,
--
2.34.1
Hi,
On 1/20/23 20:15, Mario Limonciello wrote:
> By default when the system is configured for low power idle in the FADT
> the keyboard is set up as a wake source. This matches the behavior that
> Windows uses for Modern Standby as well.
>
> It has been reported that a variety of AMD based designs there are
> spurious wakeups are happening where two IRQ sources are active.
>
> For example:
> ```
> PM: Triggering wakeup from IRQ 9
> PM: Triggering wakeup from IRQ 1
> ```
>
> In these designs IRQ 9 is the ACPI SCI and IRQ 1 is the keyboard.
> One way to trigger this problem is to suspend the laptop and then unplug
> the AC adapter. The SOC will be in a hardware sleep state and plugging
> in the AC adapter returns control to the kernel's s2idle loop.
>
> Normally if just IRQ 9 was active the s2idle loop would advance any EC
> transactions and no other IRQ being active would cause the s2idle loop
> to put the SOC back into hardware sleep state.
>
> When this bug occurred IRQ 1 is also active even if no keyboard activity
> occurred. This causes the s2idle loop to break and the system to wake.
>
> This is a platform firmware bug triggering IRQ1 without keyboard activity.
> This occurs in Windows as well, but Windows will enter "SW DRIPS" and
> then with no activity enters back into "HW DRIPS" (hardware sleep state).
>
> This issue affects Renoir, Lucienne, Cezanne, and Barcelo platforms. It
> does not happen on newer systems such as Mendocino or Rembrandt.
>
> It's been fixed in newer platform firmware. To avoid triggering the bug
> on older systems check the SMU F/W version and adjust the policy at suspend
> time for s2idle wakeup from keyboard on these systems. A lot of thought
> and experimentation has been given around the timing of disabling IRQ1,
> and to make it work the "suspend" PM callback is restored.
>
> Reported-by: Kai-Heng Feng <[email protected]>
> Reported-by: Xaver Hugl <[email protected]>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2115
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1951
> Signed-off-by: Mario Limonciello <[email protected]>
Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> ---
> drivers/platform/x86/amd/pmc.c | 50 ++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index 8d924986381be..be1b49824edbd 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -22,6 +22,7 @@
> #include <linux/pci.h>
> #include <linux/platform_device.h>
> #include <linux/rtc.h>
> +#include <linux/serio.h>
> #include <linux/suspend.h>
> #include <linux/seq_file.h>
> #include <linux/uaccess.h>
> @@ -653,6 +654,33 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
> return -EINVAL;
> }
>
> +static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
> +{
> + struct device *d;
> + int rc;
> +
> + if (!pdev->major) {
> + rc = amd_pmc_get_smu_version(pdev);
> + if (rc)
> + return rc;
> + }
> +
> + if (pdev->major > 64 || (pdev->major == 64 && pdev->minor > 65))
> + return 0;
> +
> + d = bus_find_device_by_name(&serio_bus, NULL, "serio0");
> + if (!d)
> + return 0;
> + if (device_may_wakeup(d)) {
> + dev_info_once(d, "Disabling IRQ1 wakeup source to avoid platform firmware bug\n");
> + disable_irq_wake(1);
> + device_set_wakeup_enable(d, false);
> + }
> + put_device(d);
> +
> + return 0;
> +}
> +
> static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
> {
> struct rtc_device *rtc_device;
> @@ -782,6 +810,25 @@ static struct acpi_s2idle_dev_ops amd_pmc_s2idle_dev_ops = {
> .check = amd_pmc_s2idle_check,
> .restore = amd_pmc_s2idle_restore,
> };
> +
> +static int __maybe_unused amd_pmc_suspend_handler(struct device *dev)
> +{
> + struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
> +
> + if (pdev->cpu_id == AMD_CPU_ID_CZN) {
> + int rc = amd_pmc_czn_wa_irq1(pdev);
> +
> + if (rc) {
> + dev_err(pdev->dev, "failed to adjust keyboard wakeup: %d\n", rc);
> + return rc;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler, NULL);
> +
> #endif
>
> static const struct pci_device_id pmc_pci_ids[] = {
> @@ -980,6 +1027,9 @@ static struct platform_driver amd_pmc_driver = {
> .name = "amd_pmc",
> .acpi_match_table = amd_pmc_acpi_ids,
> .dev_groups = pmc_groups,
> +#ifdef CONFIG_SUSPEND
> + .pm = &amd_pmc_pm,
> +#endif
> },
> .probe = amd_pmc_probe,
> .remove = amd_pmc_remove,