Hi,
I'd like to ask those of you who own a system that supports Intel Rapid
Start Technology (IRST) to perform a simple test to determine whether my
laptop (XPS13 9333) is the only system affected by a bug related to IRST
that I recently found or not.
In the past months I noticed random transitions from S3 to S0 with no
apparent reason.
After some recent tests I found a way to trigger those automatic state
transitions. It seems that the timer in charge of the resume of the
system for the transition to S4 is not re-initialized on resume/suspend.
Exactly "wakeup_time" minutes after the transition from S0 to S3, the
system will automatically transition from S3 to S0 instead of S4 if
within that interval it transitioned to S0 at least once.
Everything works as expected if the system is never resumed or if the
system is suspended after "wakeup_time" minutes have passed since the
first suspension to ram.
In addition to that, changing the values of wakeup_time and
wakeup_events seems to have no effect until the timer has expired.
According to the Implementation Guide [1] there are no special
requirements other than the standard S3 ACPI support. Still, IRST works
as intended only on Windows. This makes me wonder whether the problem is
due to some vendor specific change or not.
To quickly replicate the problem, follow the following steps: set the
timer to 1 minute and suspend the system. Immediately after that, resume
and suspend the system again. If it automatically wakes up after 1
minute has passed since the first suspension, then the system is
affected by the bug. If this is the case, I'd suggest to default
wakeup_events to 0 or 2 to prevent issues and add some sort of warning
somewhere to let users know about this problem.
Regards,
Gabriele
[1] http://downloadmirror.intel.com/22647/eng/Intel%20Rapid%20Start%20Technology%20Deployment%20Guide%20v1.0.pdf
Huh. Yeah, I actually see the same behaviour on my Thinkpad. Let me play
with this a little.
--
Matthew Garrett | [email protected]
Ok, I suspect I know what's going on here - the firmware is probably
not cancelling the wakeup timer on resume, and on the second suspend
makes the assumption that the OS has set the wakeup timer and so decides
that the user wants the machine to wake up. We could add workaround code
to the irst driver to handle that case, but that would involve it being
rather more friendly with the system clock than we want. I think the
right fix is probably to have the rtc driver reset the alarm on resume.
--
Matthew Garrett | [email protected]
Can you try this diff?
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 5b2e761..637f980 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -48,6 +48,7 @@ struct cmos_rtc {
struct device *dev;
int irq;
struct resource *iomem;
+ struct rtc_wkalrm alm;
void (*wake_on)(struct device *);
void (*wake_off)(struct device *);
@@ -59,6 +60,8 @@ struct cmos_rtc {
u8 day_alrm;
u8 mon_alrm;
u8 century;
+
+ bool valid_alarm;
};
/* both platform and pnp busses use negative numbers for invalid irqs */
@@ -879,6 +882,7 @@ static int cmos_suspend(struct device *dev)
cmos_checkintr(cmos, tmp);
}
+ cmos->valid_alarm = !!cmos_read_alarm(dev, &cmos->alm);
spin_unlock_irq(&rtc_lock);
if (tmp & RTC_AIE) {
@@ -949,6 +953,10 @@ static int cmos_resume(struct device *dev)
hpet_mask_rtc_irq_bit(RTC_AIE);
} while (mask & RTC_AIE);
}
+
+ if (cmos->valid_alarm)
+ cmos_set_alarm(dev, &cmos->alm);
+
spin_unlock_irq(&rtc_lock);
dev_dbg(dev, "resume, ctrl %02x\n", tmp);
--
Matthew Garrett | [email protected]
On Monday 22 December 2014 14:59:49 Matthew Garrett wrote:
> Can you try this diff?
Unfortunately it doesn't work (I made a change, see here below).
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 5b2e761..637f980 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -48,6 +48,7 @@ struct cmos_rtc {
> struct device *dev;
> int irq;
> struct resource *iomem;
> + struct rtc_wkalrm alm;
>
> void (*wake_on)(struct device *);
> void (*wake_off)(struct device *);
> @@ -59,6 +60,8 @@ struct cmos_rtc {
> u8 day_alrm;
> u8 mon_alrm;
> u8 century;
> +
> + bool valid_alarm;
> };
>
> /* both platform and pnp busses use negative numbers for invalid irqs */
> @@ -879,6 +882,7 @@ static int cmos_suspend(struct device *dev)
>
> cmos_checkintr(cmos, tmp);
> }
> + cmos->valid_alarm = !!cmos_read_alarm(dev, &cmos->alm);
I moved this after spin_unlock_irq(), it causes a deadlock otherwise.
> spin_unlock_irq(&rtc_lock);
>
> if (tmp & RTC_AIE) {
> @@ -949,6 +953,10 @@ static int cmos_resume(struct device *dev)
> hpet_mask_rtc_irq_bit(RTC_AIE);
> } while (mask & RTC_AIE);
> }
> +
> + if (cmos->valid_alarm)
> + cmos_set_alarm(dev, &cmos->alm);
> +
> spin_unlock_irq(&rtc_lock);
>
> dev_dbg(dev, "resume, ctrl %02x\n", tmp);
>
>
>
On Monday 22 December 2014 16:50:37 Gabriele Mazzotta wrote:
> On Monday 22 December 2014 14:59:49 Matthew Garrett wrote:
> > Can you try this diff?
>
> Unfortunately it doesn't work (I made a change, see here below).
I take a better look at the code and noticed a couple of mistakes in
your, but you nailed it.
Here below the working patch. There was an extra ! and another deadlock.
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 5b2e761..4035927 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -48,6 +48,7 @@ struct cmos_rtc {
struct device *dev;
int irq;
struct resource *iomem;
+ struct rtc_wkalrm alm;
void (*wake_on)(struct device *);
void (*wake_off)(struct device *);
@@ -59,6 +60,8 @@ struct cmos_rtc {
u8 day_alrm;
u8 mon_alrm;
u8 century;
+
+ bool valid_alarm;
};
/* both platform and pnp busses use negative numbers for invalid irqs */
@@ -881,6 +884,8 @@ static int cmos_suspend(struct device *dev)
}
spin_unlock_irq(&rtc_lock);
+ cmos->valid_alarm = !cmos_read_alarm(dev, &cmos->alm);
+
if (tmp & RTC_AIE) {
cmos->enabled_wake = 1;
if (cmos->wake_on)
@@ -951,6 +956,9 @@ static int cmos_resume(struct device *dev)
}
spin_unlock_irq(&rtc_lock);
+ if (cmos->valid_alarm)
+ cmos_set_alarm(dev, &cmos->alm);
+
dev_dbg(dev, "resume, ctrl %02x\n", tmp);
return 0;
On Mon, Dec 22, 2014 at 04:50:37PM +0100, Gabriele Mazzotta wrote:
> On Monday 22 December 2014 14:59:49 Matthew Garrett wrote:
> > Can you try this diff?
>
> Unfortunately it doesn't work (I made a change, see here below).
Ok. Can you hack the resume path to dump the RTC registers on resume? I
wonder if there's actually a separate set of EFI clock registers. That
would be irritating.
--
Matthew Garrett | [email protected]
Oh, ha, wonderful. I'll nail something together and send it upstream.
The RTC maintainers may have opinions on how to do this cleanly, so
we'll see how that goes.
--
Matthew Garrett | [email protected]
Getting this right is a little bit annoying - if the timer would have
expired during the time we were suspended, we don't want to re-enable
it. I *think* this covers all cases, and does it in generic code rather
than special-casing it. Any chance you can give it a test?
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 472a5ad..4e3a2a1 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -91,6 +91,8 @@ static int rtc_suspend(struct device *dev)
old_system = timespec64_sub(old_system, delta_delta);
}
+ rtc->valid_alarm = !rtc_read_alarm(rtc, &rtc->alarm);
+
return 0;
}
@@ -145,6 +147,29 @@ static int rtc_resume(struct device *dev)
if (sleep_time.tv_sec >= 0)
timekeeping_inject_sleeptime64(&sleep_time);
rtc_hctosys_ret = 0;
+
+ /*
+ * If there was an alarm set before suspend, make sure that the
+ * platform hasn't overwritten it
+ */
+ if (rtc->valid_alarm) {
+ struct rtc_time tm;
+ long now, scheduled;
+
+ rtc_read_time(rtc, &tm);
+ rtc_tm_to_time(&rtc->alarm.time, &scheduled);
+ rtc_tm_to_time(&tm, &now);
+
+ /* Clear the alarm if it went off during suspend */
+ if (scheduled <= now) {
+ rtc_time_to_tm(0, &rtc->alarm.time);
+ rtc->alarm.enabled = 0;
+ }
+
+ if (rtc->ops && rtc->ops->set_alarm)
+ rtc->ops->set_alarm(rtc->dev.parent, &rtc->alarm);
+ }
+
return 0;
}
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 6d6be09..bc805ff 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -133,6 +133,10 @@ struct rtc_device
/* Some hardware can't support UIE mode */
int uie_unsupported;
+#ifdef CONFIG_PM_SLEEP
+ struct rtc_wkalrm alarm;
+ bool valid_alarm;
+#endif
#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
struct work_struct uie_task;
struct timer_list uie_timer;
--
Matthew Garrett | [email protected]
On Monday 22 December 2014 18:02:54 Matthew Garrett wrote:
> Getting this right is a little bit annoying - if the timer would have
> expired during the time we were suspended, we don't want to re-enable
> it. I *think* this covers all cases, and does it in generic code rather
> than special-casing it. Any chance you can give it a test?
I've done a quick test. has_persistent_clock() is false on my system,
so this patch has no effect as it is.
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 472a5ad..4e3a2a1 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -91,6 +91,8 @@ static int rtc_suspend(struct device *dev)
> old_system = timespec64_sub(old_system, delta_delta);
> }
>
> + rtc->valid_alarm = !rtc_read_alarm(rtc, &rtc->alarm);
> +
> return 0;
> }
>
> @@ -145,6 +147,29 @@ static int rtc_resume(struct device *dev)
> if (sleep_time.tv_sec >= 0)
> timekeeping_inject_sleeptime64(&sleep_time);
> rtc_hctosys_ret = 0;
> +
> + /*
> + * If there was an alarm set before suspend, make sure that the
> + * platform hasn't overwritten it
> + */
> + if (rtc->valid_alarm) {
> + struct rtc_time tm;
> + long now, scheduled;
> +
> + rtc_read_time(rtc, &tm);
> + rtc_tm_to_time(&rtc->alarm.time, &scheduled);
> + rtc_tm_to_time(&tm, &now);
> +
> + /* Clear the alarm if it went off during suspend */
> + if (scheduled <= now) {
> + rtc_time_to_tm(0, &rtc->alarm.time);
> + rtc->alarm.enabled = 0;
> + }
> +
> + if (rtc->ops && rtc->ops->set_alarm)
> + rtc->ops->set_alarm(rtc->dev.parent, &rtc->alarm);
> + }
> +
> return 0;
> }
>
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 6d6be09..bc805ff 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -133,6 +133,10 @@ struct rtc_device
> /* Some hardware can't support UIE mode */
> int uie_unsupported;
>
> +#ifdef CONFIG_PM_SLEEP
> + struct rtc_wkalrm alarm;
> + bool valid_alarm;
> +#endif
> #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
> struct work_struct uie_task;
> struct timer_list uie_timer;
>
>
>
On Monday 22 December 2014 19:41:52 Gabriele Mazzotta wrote:
> On Monday 22 December 2014 18:02:54 Matthew Garrett wrote:
> > Getting this right is a little bit annoying - if the timer would have
> > expired during the time we were suspended, we don't want to re-enable
> > it. I *think* this covers all cases, and does it in generic code rather
> > than special-casing it. Any chance you can give it a test?
>
> I've done a quick test. has_persistent_clock() is false on my system,
> so this patch has no effect as it is.
I did another quick test and moved the code so that it is executed
before has_persistent_clock() is checked. Everything seems to be
working as intended. I tried to suspend and resume multiple times
within one minute and exactly one minute after the last suspension
the system transitioned from S3 to S4.
I haven't checked properly whether it is OK or not to simply move
the code or not, but I wanted to let you know that it works.
> > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> > index 472a5ad..4e3a2a1 100644
> > --- a/drivers/rtc/class.c
> > +++ b/drivers/rtc/class.c
> > @@ -91,6 +91,8 @@ static int rtc_suspend(struct device *dev)
> > old_system = timespec64_sub(old_system, delta_delta);
> > }
> >
> > + rtc->valid_alarm = !rtc_read_alarm(rtc, &rtc->alarm);
> > +
> > return 0;
> > }
> >
> > @@ -145,6 +147,29 @@ static int rtc_resume(struct device *dev)
> > if (sleep_time.tv_sec >= 0)
> > timekeeping_inject_sleeptime64(&sleep_time);
> > rtc_hctosys_ret = 0;
> > +
> > + /*
> > + * If there was an alarm set before suspend, make sure that the
> > + * platform hasn't overwritten it
> > + */
> > + if (rtc->valid_alarm) {
> > + struct rtc_time tm;
> > + long now, scheduled;
> > +
> > + rtc_read_time(rtc, &tm);
> > + rtc_tm_to_time(&rtc->alarm.time, &scheduled);
> > + rtc_tm_to_time(&tm, &now);
> > +
> > + /* Clear the alarm if it went off during suspend */
> > + if (scheduled <= now) {
> > + rtc_time_to_tm(0, &rtc->alarm.time);
> > + rtc->alarm.enabled = 0;
> > + }
> > +
> > + if (rtc->ops && rtc->ops->set_alarm)
> > + rtc->ops->set_alarm(rtc->dev.parent, &rtc->alarm);
> > + }
> > +
> > return 0;
> > }
> >
> > diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> > index 6d6be09..bc805ff 100644
> > --- a/include/linux/rtc.h
> > +++ b/include/linux/rtc.h
> > @@ -133,6 +133,10 @@ struct rtc_device
> > /* Some hardware can't support UIE mode */
> > int uie_unsupported;
> >
> > +#ifdef CONFIG_PM_SLEEP
> > + struct rtc_wkalrm alarm;
> > + bool valid_alarm;
> > +#endif
> > #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
> > struct work_struct uie_task;
> > struct timer_list uie_timer;
> >
> >
> >