| Message ID | 20231106162310.85711-1-mario.limonciello@amd.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: cmos: Use ACPI alarm for non-Intel x86 systems too | expand |
On Mon, Nov 6, 2023 at 9:14 AM Mario Limonciello <mario.limonciello@amd.com> wrote: > > Intel systems > 2015 have been configured to use ACPI alarm instead > of HPET to avoid s2idle issues. > > Having HPET programmed for wakeup causes problems on AMD systems with > s2idle as well. > > One particular case is that the systemd "SuspendThenHibernate" feature > doesn't work properly on the Framework 13" AMD model. Switching to > using ACPI alarm fixes the issue. > > Adjust the quirk to apply to AMD/Hygon systems from 2021 onwards. > This matches what has been tested and is specifically to avoid potential > risk to older systems. > > Cc: stable@vger.kernel.org # 6.1+ > Reported-by: alvin.zhuge@gmail.com > Reported-by: renzhamin@gmail.com > Closes: https://github.com/systemd/systemd/issues/24279 > Reported-by: Kelvie Wong <kelvie@kelvie.ca> > Closes: https://community.frame.work/t/systemd-suspend-then-hibernate-wakes-up-after-5-minutes/39392 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/rtc/rtc-cmos.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c > index 228fb2d11c70..696cfa7025de 100644 > --- a/drivers/rtc/rtc-cmos.c > +++ b/drivers/rtc/rtc-cmos.c > @@ -818,18 +818,24 @@ static void rtc_wake_off(struct device *dev) > } > > #ifdef CONFIG_X86 > -/* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */ > static void use_acpi_alarm_quirks(void) > { > - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) > + switch (boot_cpu_data.x86_vendor) { > + case X86_VENDOR_INTEL: > + if (dmi_get_bios_year() < 2015) > + return; > + break; > + case X86_VENDOR_AMD: > + case X86_VENDOR_HYGON: > + if (dmi_get_bios_year() < 2021) > + return; > + break; > + default: > return; > - > + } > if (!is_hpet_enabled()) > return; > > - if (dmi_get_bios_year() < 2015) > - return; > - > use_acpi_alarm = true; > } > #else > -- > 2.34.1 > Acked-by: Raul E Rangel <rrangel@chromium.org>
On 11/6/2023 10:17, Raul Rangel wrote: > On Mon, Nov 6, 2023 at 9:14 AM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> Intel systems > 2015 have been configured to use ACPI alarm instead >> of HPET to avoid s2idle issues. >> >> Having HPET programmed for wakeup causes problems on AMD systems with >> s2idle as well. >> >> One particular case is that the systemd "SuspendThenHibernate" feature >> doesn't work properly on the Framework 13" AMD model. Switching to >> using ACPI alarm fixes the issue. >> >> Adjust the quirk to apply to AMD/Hygon systems from 2021 onwards. >> This matches what has been tested and is specifically to avoid potential >> risk to older systems. >> >> Cc: stable@vger.kernel.org # 6.1+ >> Reported-by: alvin.zhuge@gmail.com >> Reported-by: renzhamin@gmail.com >> Closes: https://github.com/systemd/systemd/issues/24279 >> Reported-by: Kelvie Wong <kelvie@kelvie.ca> >> Closes: https://community.frame.work/t/systemd-suspend-then-hibernate-wakes-up-after-5-minutes/39392 >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/rtc/rtc-cmos.c | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c >> index 228fb2d11c70..696cfa7025de 100644 >> --- a/drivers/rtc/rtc-cmos.c >> +++ b/drivers/rtc/rtc-cmos.c >> @@ -818,18 +818,24 @@ static void rtc_wake_off(struct device *dev) >> } >> >> #ifdef CONFIG_X86 >> -/* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */ >> static void use_acpi_alarm_quirks(void) >> { >> - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) >> + switch (boot_cpu_data.x86_vendor) { >> + case X86_VENDOR_INTEL: >> + if (dmi_get_bios_year() < 2015) >> + return; >> + break; >> + case X86_VENDOR_AMD: >> + case X86_VENDOR_HYGON: >> + if (dmi_get_bios_year() < 2021) >> + return; >> + break; >> + default: >> return; >> - >> + } >> if (!is_hpet_enabled()) >> return; >> >> - if (dmi_get_bios_year() < 2015) >> - return; >> - >> use_acpi_alarm = true; >> } >> #else >> -- >> 2.34.1 >> > Acked-by: Raul E Rangel <rrangel@chromium.org> Now that the merge window is over, can this be picked up? All of those people who reported it have also reported on the matching trackers that it helped their issue. Thanks!
On 13/11/2023 15:36:28-0600, Mario Limonciello wrote: > Now that the merge window is over, can this be picked up? > I'd be happy to invoice AMD so they get a quick response time. > All of those people who reported it have also reported on the matching > trackers that it helped their issue. > > Thanks!
On Mon 2023-11-13 23:38:19, Alexandre Belloni wrote: > On 13/11/2023 15:36:28-0600, Mario Limonciello wrote: > > Now that the merge window is over, can this be picked up? > > > > I'd be happy to invoice AMD so they get a quick response time. That is a really bad joke. Pavel
On 14/11/2023 10:06:36+0100, Pavel Machek wrote: > On Mon 2023-11-13 23:38:19, Alexandre Belloni wrote: > > On 13/11/2023 15:36:28-0600, Mario Limonciello wrote: > > > Now that the merge window is over, can this be picked up? > > > > > > > I'd be happy to invoice AMD so they get a quick response time. > > That is a really bad joke. Why would it be a joke? From what I get this is an issue since 2021, I don't get how this is so urgent that I get a ping less than 24h after the end of the merge window. This touches a driver that handle a gazillion of broken x86 hardware that I don't know anything about and as soon as I apply this patch, this becomes my problem so I need to be careful there. On the other hand, I need to pay my bills so actually, yesterday I was actually looking at the patch but got interrupted by a project that ironically involved the radeon driver not working properly in 6.5. So no, I don't actually expect AMD to have to pay to get me to review and apply patches but at the same time, they can't expect me to prioritize their patches over projects that pay the bills.
On 11/14/2023 16:28, Alexandre Belloni wrote: > On 14/11/2023 10:06:36+0100, Pavel Machek wrote: >> On Mon 2023-11-13 23:38:19, Alexandre Belloni wrote: >>> On 13/11/2023 15:36:28-0600, Mario Limonciello wrote: >>>> Now that the merge window is over, can this be picked up? >>>> >>> >>> I'd be happy to invoice AMD so they get a quick response time. >> >> That is a really bad joke. > > Why would it be a joke? > > From what I get this is an issue since 2021, I don't get how this is so > urgent that I get a ping less than 24h after the end of the merge > window. It's possibly longer; but I don't have a large enough sample to say that it's safe that far back. > > This touches a driver that handle a gazillion of broken x86 hardware > that I don't know anything about and as soon as I apply this patch, this > becomes my problem so I need to be careful there. > Don't feel the risk needs to sit on you as subsystem maintainer. If something does come up caused by this I'm happy to help come up with a solution. And if there was a regression caused by this having rtc_cmos.use_acpi_alarm=0 to restore the old behavior would help a reporter's system. > On the other hand, I need to pay my bills so actually, yesterday I was > actually looking at the patch but got interrupted by a project that > ironically involved the radeon driver not working properly in 6.5. > > So no, I don't actually expect AMD to have to pay to get me to review > and apply patches but at the same time, they can't expect me to > prioritize their patches over projects that pay the bills. > My apologies on the preempted ping. Of course you have your own priorities. I mostly wanted to make sure it didn't get lost during the merge window. I do see that you have a patchwork [1] that you actively use so I'll refrain from pings in the future on any submissions. [1] https://patchwork-proxy.ozlabs.org/project/rtc-linux/list/
On Mon, 06 Nov 2023 10:23:10 -0600, Mario Limonciello wrote: > Intel systems > 2015 have been configured to use ACPI alarm instead > of HPET to avoid s2idle issues. > > Having HPET programmed for wakeup causes problems on AMD systems with > s2idle as well. > > One particular case is that the systemd "SuspendThenHibernate" feature > doesn't work properly on the Framework 13" AMD model. Switching to > using ACPI alarm fixes the issue. > > [...] Applied, thanks! [1/1] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too commit: 3d762e21d56370a43478b55e604b4a83dd85aafc Best regards,
On 14/11/2023 18:15:02-0600, Mario Limonciello wrote: > On 11/14/2023 16:28, Alexandre Belloni wrote: > > On 14/11/2023 10:06:36+0100, Pavel Machek wrote: > > > On Mon 2023-11-13 23:38:19, Alexandre Belloni wrote: > > > > On 13/11/2023 15:36:28-0600, Mario Limonciello wrote: > > > > > Now that the merge window is over, can this be picked up? > > > > > > > > > > > > > I'd be happy to invoice AMD so they get a quick response time. > > > > > > That is a really bad joke. > > > > Why would it be a joke? > > > > From what I get this is an issue since 2021, I don't get how this is so > > urgent that I get a ping less than 24h after the end of the merge > > window. > > It's possibly longer; but I don't have a large enough sample to say that > it's safe that far back. Would this help this one: https://bugzilla.kernel.org/show_bug.cgi?id=68331
On 11/17/2023 16:57, Alexandre Belloni wrote: > On 14/11/2023 18:15:02-0600, Mario Limonciello wrote: >> On 11/14/2023 16:28, Alexandre Belloni wrote: >>> On 14/11/2023 10:06:36+0100, Pavel Machek wrote: >>>> On Mon 2023-11-13 23:38:19, Alexandre Belloni wrote: >>>>> On 13/11/2023 15:36:28-0600, Mario Limonciello wrote: >>>>>> Now that the merge window is over, can this be picked up? >>>>>> >>>>> >>>>> I'd be happy to invoice AMD so they get a quick response time. >>>> >>>> That is a really bad joke. >>> >>> Why would it be a joke? >>> >>> From what I get this is an issue since 2021, I don't get how this is so >>> urgent that I get a ping less than 24h after the end of the merge >>> window. >> >> It's possibly longer; but I don't have a large enough sample to say that >> it's safe that far back. > > Would this help this one: > https://bugzilla.kernel.org/show_bug.cgi?id=68331 > The BIOS cutoff is set to 2021 in my patch, so unless they have an updated BIOS for that system I think it's unlikely to do anything. They can certainly try with the kernel command line though to see if it could help. If it does we could add another case for it or adjust boundaries.
On Fri, Nov 17, 2023 at 3:57 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 14/11/2023 18:15:02-0600, Mario Limonciello wrote: > > On 11/14/2023 16:28, Alexandre Belloni wrote: > > > On 14/11/2023 10:06:36+0100, Pavel Machek wrote: > > > > On Mon 2023-11-13 23:38:19, Alexandre Belloni wrote: > > > > > On 13/11/2023 15:36:28-0600, Mario Limonciello wrote: > > > > > > Now that the merge window is over, can this be picked up? > > > > > > > > > > > > > > > > I'd be happy to invoice AMD so they get a quick response time. > > > > > > > > That is a really bad joke. > > > > > > Why would it be a joke? > > > > > > From what I get this is an issue since 2021, I don't get how this is so > > > urgent that I get a ping less than 24h after the end of the merge > > > window. > > > > It's possibly longer; but I don't have a large enough sample to say that > > it's safe that far back. > > Would this help this one: > https://bugzilla.kernel.org/show_bug.cgi?id=68331 Honestly, the HPET_EMULATE_RTC code is what drove me to switch the AMD chromebooks over to using the ACPI timer: https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2355073. Ideally if the HPET was configured with proper IRQs by the firmware, the kernel wouldn't enable the HPET legacy emulation bit. The HPET spec defines that when the legacy replacement option bit is set, the HPET takes control of the RTC (#8) and timer (#2) interrupts. Why it takes over the RTC interrupt I've never understood. The HPET is not an RTC, so it results in software having to emulate it which results in extra complexity. If the kernel didn't set the HPET legacy emulation bit when the HPET declared proper IRQs, then the RTC wake interrupt would keep working as expected. I guess there's not much benefit to fixing this anymore, now that this ACPI timer patch has landed. If a platform were to release new firmware to define proper HPET IRQs, the date would bump, and it would fall into the ACPI path. > > > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c index 228fb2d11c70..696cfa7025de 100644 --- a/drivers/rtc/rtc-cmos.c +++ b/drivers/rtc/rtc-cmos.c @@ -818,18 +818,24 @@ static void rtc_wake_off(struct device *dev) } #ifdef CONFIG_X86 -/* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */ static void use_acpi_alarm_quirks(void) { - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) + switch (boot_cpu_data.x86_vendor) { + case X86_VENDOR_INTEL: + if (dmi_get_bios_year() < 2015) + return; + break; + case X86_VENDOR_AMD: + case X86_VENDOR_HYGON: + if (dmi_get_bios_year() < 2021) + return; + break; + default: return; - + } if (!is_hpet_enabled()) return; - if (dmi_get_bios_year() < 2015) - return; - use_acpi_alarm = true; } #else
Intel systems > 2015 have been configured to use ACPI alarm instead of HPET to avoid s2idle issues. Having HPET programmed for wakeup causes problems on AMD systems with s2idle as well. One particular case is that the systemd "SuspendThenHibernate" feature doesn't work properly on the Framework 13" AMD model. Switching to using ACPI alarm fixes the issue. Adjust the quirk to apply to AMD/Hygon systems from 2021 onwards. This matches what has been tested and is specifically to avoid potential risk to older systems. Cc: stable@vger.kernel.org # 6.1+ Reported-by: alvin.zhuge@gmail.com Reported-by: renzhamin@gmail.com Closes: https://github.com/systemd/systemd/issues/24279 Reported-by: Kelvie Wong <kelvie@kelvie.ca> Closes: https://community.frame.work/t/systemd-suspend-then-hibernate-wakes-up-after-5-minutes/39392 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/rtc/rtc-cmos.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)