[PATCH RESEND] alarmtimer: Use aie_timer from RTC device instead of own timer

[PATCH RESEND] alarmtimer: Use aie_timer from RTC device instead of own timer

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

It was reported that suspend-then-hibernate stopped working with modern
systemd versions on an AMD Cezanne system. The reason for this breakage
was because systemd switched to using alarmtimer instead of the wakealarm
sysfs file.

The wakealarm sysfs file programs the time to the `rtc->aie_timer` member
of the RTC, whereas the alarmtimer suspend routine programs it to it's
own device.

On AMD Cezanne systems rtc_read_alarm() is used to program a secondary
timer with the value of the timer. This behavior was introduced by
commit 59348401ebed9 ("platform/x86: amd-pmc: Add special handling
for timer based S0i3 wakeup").

As rtc_read_alarm() uses the `rtc->aie_timer` to report the cached
timer no alarm is provided as enabled.

To fix this issue, drop the use of a dedicated timer for the alarmtimer
and instead use `rtc->aie_timer` in the alarmtimer suspend/resume routines.

Link: https://github.com/systemd/systemd/commit/1bbbefe7a68059eb55d864c3e0e670d00269683a
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3591
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Originally sent as https://lore.kernel.org/all/20240910122258.129540-1-superm1@kernel.org/
No feedback for 3 weeks, travel, merge window etc so resending.
Rebased on 6.12-rc1.

 kernel/time/alarmtimer.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 8bf888641694..a774dc0a7253 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -59,7 +59,6 @@ static DEFINE_SPINLOCK(freezer_delta_lock);
 
 #ifdef CONFIG_RTC_CLASS
 /* rtc timer and device for setting alarm wakeups at suspend */
-static struct rtc_timer		rtctimer;
 static struct rtc_device	*rtcdev;
 static DEFINE_SPINLOCK(rtcdev_lock);
 
@@ -123,11 +122,6 @@ static int alarmtimer_rtc_add_device(struct device *dev)
 	return ret;
 }
 
-static inline void alarmtimer_rtc_timer_init(void)
-{
-	rtc_timer_init(&rtctimer, NULL, NULL);
-}
-
 static struct class_interface alarmtimer_rtc_interface = {
 	.add_dev = &alarmtimer_rtc_add_device,
 };
@@ -144,7 +138,6 @@ static void alarmtimer_rtc_interface_remove(void)
 #else
 static inline int alarmtimer_rtc_interface_setup(void) { return 0; }
 static inline void alarmtimer_rtc_interface_remove(void) { }
-static inline void alarmtimer_rtc_timer_init(void) { }
 #endif
 
 /**
@@ -287,7 +280,7 @@ static int alarmtimer_suspend(struct device *dev)
 	trace_alarmtimer_suspend(expires, type);
 
 	/* Setup an rtc timer to fire that far in the future */
-	rtc_timer_cancel(rtc, &rtctimer);
+	rtc_timer_cancel(rtc, &rtc->aie_timer);
 	rtc_read_time(rtc, &tm);
 	now = rtc_tm_to_ktime(tm);
 
@@ -304,7 +297,7 @@ static int alarmtimer_suspend(struct device *dev)
 	now = ktime_add(now, min);
 
 	/* Set alarm, if in the past reject suspend briefly to handle */
-	ret = rtc_timer_start(rtc, &rtctimer, now, 0);
+	ret = rtc_timer_start(rtc, &rtc->aie_timer, now, 0);
 	if (ret < 0)
 		pm_wakeup_event(dev, MSEC_PER_SEC);
 	return ret;
@@ -316,7 +309,7 @@ static int alarmtimer_resume(struct device *dev)
 
 	rtc = alarmtimer_get_rtcdev();
 	if (rtc)
-		rtc_timer_cancel(rtc, &rtctimer);
+		rtc_timer_cancel(rtc, &rtc->aie_timer);
 	return 0;
 }
 
@@ -939,8 +932,6 @@ static int __init alarmtimer_init(void)
 	int error;
 	int i;
 
-	alarmtimer_rtc_timer_init();
-
 	/* Initialize alarm bases */
 	alarm_bases[ALARM_REALTIME].base_clockid = CLOCK_REALTIME;
 	alarm_bases[ALARM_REALTIME].get_ktime = &ktime_get_real;
-- 
2.43.0

Re: [PATCH RESEND] alarmtimer: Use aie_timer from RTC device instead of own timer

[Thomas Gleixner]

On Mon, Sep 30 2024 at 13:29, Mario Limonciello wrote:
> It was reported that suspend-then-hibernate stopped working with modern
> systemd versions on an AMD Cezanne system. The reason for this breakage
> was because systemd switched to using alarmtimer instead of the wakealarm
> sysfs file.
>
> The wakealarm sysfs file programs the time to the `rtc->aie_timer` member
> of the RTC, whereas the alarmtimer suspend routine programs it to it's
> own device.
>
> On AMD Cezanne systems rtc_read_alarm() is used to program a secondary
> timer with the value of the timer. This behavior was introduced by
> commit 59348401ebed9 ("platform/x86: amd-pmc: Add special handling
> for timer based S0i3 wakeup").
>
> As rtc_read_alarm() uses the `rtc->aie_timer` to report the cached
> timer no alarm is provided as enabled.
>
> To fix this issue, drop the use of a dedicated timer for the alarmtimer
> and instead use `rtc->aie_timer` in the alarmtimer suspend/resume
> routines.

I'm not sure that this is correct. There is a reason why alarmtimer uses
a dedicated timer and this worked correctly so far.

I'd rather look at commit 59348401ebed9, which plays games with the RTC.

Thanks,

        tglx

Re: [PATCH RESEND] alarmtimer: Use aie_timer from RTC device instead of own timer

[Mario Limonciello]

On 10/1/2024 03:30, Thomas Gleixner wrote:
> On Mon, Sep 30 2024 at 13:29, Mario Limonciello wrote:
>> It was reported that suspend-then-hibernate stopped working with modern
>> systemd versions on an AMD Cezanne system. The reason for this breakage
>> was because systemd switched to using alarmtimer instead of the wakealarm
>> sysfs file.
>>
>> The wakealarm sysfs file programs the time to the `rtc->aie_timer` member
>> of the RTC, whereas the alarmtimer suspend routine programs it to it's
>> own device.
>>
>> On AMD Cezanne systems rtc_read_alarm() is used to program a secondary
>> timer with the value of the timer. This behavior was introduced by
>> commit 59348401ebed9 ("platform/x86: amd-pmc: Add special handling
>> for timer based S0i3 wakeup").
>>
>> As rtc_read_alarm() uses the `rtc->aie_timer` to report the cached
>> timer no alarm is provided as enabled.
>>
>> To fix this issue, drop the use of a dedicated timer for the alarmtimer
>> and instead use `rtc->aie_timer` in the alarmtimer suspend/resume
>> routines.
> 
> I'm not sure that this is correct. There is a reason why alarmtimer uses
> a dedicated timer 

Do you know what it is?  When I was looking at this problem I wasn't sure.

> and this worked correctly so far.
> 
> I'd rather look at commit 59348401ebed9, which plays games with the RTC.
> 
> Thanks,
> 
>          tglx

The workaround in commit 59348401ebed9 exists because of what appears to 
be a platform bug that is unique to Cezanne systems.  Newer stuff 
(Mendocino, Rembrandt, Phoenix, Strix etc) doesn't need or use it.

I'd love if we could tear it out; but I'm at a loss what else we can do 
from the Linux kernel side.

Re: [PATCH RESEND] alarmtimer: Use aie_timer from RTC device instead of own timer

[Thomas Gleixner]

On Tue, Oct 01 2024 at 08:41, Mario Limonciello wrote:
> On 10/1/2024 03:30, Thomas Gleixner wrote:
>> On Mon, Sep 30 2024 at 13:29, Mario Limonciello wrote:
>>> It was reported that suspend-then-hibernate stopped working with modern
>>> systemd versions on an AMD Cezanne system. The reason for this breakage
>>> was because systemd switched to using alarmtimer instead of the wakealarm
>>> sysfs file.
>>>
>>> The wakealarm sysfs file programs the time to the `rtc->aie_timer` member
>>> of the RTC, whereas the alarmtimer suspend routine programs it to it's
>>> own device.
>>>
>>> On AMD Cezanne systems rtc_read_alarm() is used to program a secondary
>>> timer with the value of the timer. This behavior was introduced by
>>> commit 59348401ebed9 ("platform/x86: amd-pmc: Add special handling
>>> for timer based S0i3 wakeup").
>>>
>>> As rtc_read_alarm() uses the `rtc->aie_timer` to report the cached
>>> timer no alarm is provided as enabled.
>>>
>>> To fix this issue, drop the use of a dedicated timer for the alarmtimer
>>> and instead use `rtc->aie_timer` in the alarmtimer suspend/resume
>>> routines.
>> 
>> I'm not sure that this is correct. There is a reason why alarmtimer uses
>> a dedicated timer 
>
> Do you know what it is?  When I was looking at this problem I wasn't sure.

Because you cannot just blindly overwrite the aie_timer. It might have
been set by something else. Both end up in the timerqueue and the first
expiring timer is armed first.

>> and this worked correctly so far.
>> 
>> I'd rather look at commit 59348401ebed9, which plays games with the RTC.
>
> The workaround in commit 59348401ebed9 exists because of what appears to 
> be a platform bug that is unique to Cezanne systems.  Newer stuff 
> (Mendocino, Rembrandt, Phoenix, Strix etc) doesn't need or use it.

The problem is that this hack looks at the aie_timer and not at the
first expiring timer in the timerqueue which arms the RTC. That's
obviously wrong. There is no interface to get that information, but
that's a solvable problem.

Thanks,

        tglx