Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Raul Rangel]

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>

[PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Mario Limonciello]

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

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Mario Limonciello]

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!

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Alexandre Belloni]

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!

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Pavel Machek]

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

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
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Mario Limonciello]

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/

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Alexandre Belloni]

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,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Alexandre Belloni]

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

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Mario Limonciello]

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.

Re: [PATCH] rtc: cmos: Use ACPI alarm for non-Intel x86 systems too

[Raul Rangel]

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