Message-ID: <74a63457-52fd-2f16-1b76-d3b3497b5a5d@redhat.com>
Date:   Mon, 23 Jan 2023 14:55:36 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH 1/2] platform/x86: amd: pmc: Disable IRQ1 wakeup for
 RN/CZN
Content-Language: en-US
To:     Mario Limonciello <mario.limonciello@amd.com>,
        Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Cc:     kai.heng.feng@canonical.com, xaver.hugl@gmail.com,
        Mark Gross <markgross@kernel.org>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230120191519.15926-1-mario.limonciello@amd.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230120191519.15926-1-mario.limonciello@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 <kai.heng.feng@canonical.com>
> Reported-by: Xaver Hugl <xaver.hugl@gmail.com>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2115
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1951
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

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,