[PATCH v2 1/2] rtc-cmos: Ensure no expired alarm is left enabled after resume

[PATCH v2 1/2] rtc-cmos: Ensure no expired alarm is left enabled after resume

[Gabriele Mazzotta]

Some BIOSes, such as the one of the Dell XPS13 9333, wake the system
when an alarm goes off without informing the OS. If any of the
RTC_IRQMASK bits is set when this happens and hpet is enabled,
the alarm is not automatically cleared at resume. As consequence,
the user must manually clear the alarm writing 0 to wakealarm
before being able to set a new alarm. Ensure this never happens.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/rtc/rtc-cmos.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 1dec52f..62d5b33 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -900,10 +900,32 @@ static inline int cmos_poweroff(struct device *dev)
 
 #ifdef	CONFIG_PM_SLEEP
 
+static bool cmos_is_wkalrm_expired(struct device *dev)
+{
+	struct cmos_rtc *cmos = dev_get_drvdata(dev);
+	struct rtc_wkalrm alarm;
+	struct rtc_time now;
+	time64_t t_now;
+	time64_t t_expires;
+	int ret;
+
+	ret = rtc_read_alarm(cmos->rtc, &alarm);
+	if (ret) {
+		dev_err(dev, "Could not read alarm: err=%d\n", ret);
+		return false;
+	}
+	t_expires = rtc_tm_to_time64(&alarm.time);
+	cmos_read_time(dev, &now);
+	t_now = rtc_tm_to_time64(&now);
+
+	return t_expires <= t_now;
+}
+
 static int cmos_resume(struct device *dev)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
 	unsigned char tmp;
+	bool is_wkalrm_expired;
 
 	if (cmos->enabled_wake) {
 		if (cmos->wake_off)
@@ -913,6 +935,8 @@ static int cmos_resume(struct device *dev)
 		cmos->enabled_wake = 0;
 	}
 
+	is_wkalrm_expired = cmos_is_wkalrm_expired(dev);
+
 	spin_lock_irq(&rtc_lock);
 	tmp = cmos->suspend_ctrl;
 	cmos->suspend_ctrl = 0;
@@ -939,6 +963,17 @@ static int cmos_resume(struct device *dev)
 			tmp &= ~RTC_AIE;
 			hpet_mask_rtc_irq_bit(RTC_AIE);
 		} while (mask & RTC_AIE);
+
+		/*
+		 * If RTC_AIE is set and we have an alarm set to go off in the
+		 * past, then the BIOS woke the system when the alarm went off
+		 * and we now have to clear it.
+		 */
+		if ((tmp & RTC_AIE) && is_wkalrm_expired) {
+			rtc_update_irq(cmos->rtc, 1, mask);
+			tmp &= ~RTC_AIE;
+			CMOS_WRITE(tmp, RTC_CONTROL);
+		}
 	}
 	spin_unlock_irq(&rtc_lock);
 
-- 
2.9.3

[PATCH v2 2/2] rtc-cmos: Restore alarm after resume

[Gabriele Mazzotta]

Some platform firmware may interfere with the RTC alarm over suspend,
resulting in the kernel and hardware having different ideas about system
state but also potentially causing problems with firmware that assumes the
OS will clean this case up.  This patch restores the RTC alarm on resume
to ensure that kernel and hardware are in sync.

The case we've seen is Intel Rapid Start, which is a firmware-mediated
feature that automatically transitions systems from suspend-to-RAM to
suspend-to-disk without OS involvement.  It does this by setting the RTC
alarm and a flag that indicates that on wake it should perform the
transition rather than re-starting the OS.  However, if the OS has set a
wakeup alarm that would wake the machine earlier, it refuses to overwrite
it and allows the system to wake instead.

This fails in the following situation:

1) User configures Intel Rapid Start to transition after (say) 15
minutes
2) User suspends to RAM. Firmware sets the wakeup alarm for 15 minutes
in the future
3) User resumes after 5 minutes. Firmware does not reset the alarm, and
as such it is still set for 10 minutes in the future
4) User suspends after 5 minutes. Firmware notices that the alarm is set
for 5 minutes in the future, which is less than the 15 minute transition
threshold. It therefore assumes that the user wants the machine to wake
in 5 minutes
5) System resumes after 5 minutes

The worst case scenario here is that the user may have put the system in a
bag between (4) and (5), resulting in it running in a confined space and
potentially overheating.  This seems reasonably important.  The Rapid
Start support code got added in 3.11, but it can be configured in the
firmware regardless of kernel support.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/rtc/rtc-cmos.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 62d5b33..4cdb335 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -62,6 +62,8 @@ struct cmos_rtc {
 	u8			day_alrm;
 	u8			mon_alrm;
 	u8			century;
+
+	struct rtc_wkalrm	saved_wkalrm;
 };
 
 /* both platform and pnp busses use negative numbers for invalid irqs */
@@ -880,6 +882,8 @@ static int cmos_suspend(struct device *dev)
 			enable_irq_wake(cmos->irq);
 	}
 
+	cmos_read_alarm(dev, &cmos->saved_wkalrm);
+
 	dev_dbg(dev, "suspend%s, ctrl %02x\n",
 			(tmp & RTC_AIE) ? ", alarm may wake" : "",
 			tmp);
@@ -900,6 +904,22 @@ static inline int cmos_poweroff(struct device *dev)
 
 #ifdef	CONFIG_PM_SLEEP
 
+static void cmos_check_wkalrm(struct device *dev)
+{
+	struct cmos_rtc *cmos = dev_get_drvdata(dev);
+	struct rtc_wkalrm current_alarm;
+	time64_t t_current_expires;
+	time64_t t_saved_expires;
+
+	cmos_read_alarm(dev, &current_alarm);
+	t_current_expires = rtc_tm_to_time64(&current_alarm.time);
+	t_saved_expires = rtc_tm_to_time64(&cmos->saved_wkalrm.time);
+	if (t_current_expires != t_saved_expires ||
+	    cmos->saved_wkalrm.enabled != current_alarm.enabled) {
+		cmos_set_alarm(dev, &cmos->saved_wkalrm);
+	}
+}
+
 static bool cmos_is_wkalrm_expired(struct device *dev)
 {
 	struct cmos_rtc *cmos = dev_get_drvdata(dev);
@@ -935,6 +955,9 @@ static int cmos_resume(struct device *dev)
 		cmos->enabled_wake = 0;
 	}
 
+	/* The BIOS might have changed the alarm, restore it */
+	cmos_check_wkalrm(dev);
+
 	is_wkalrm_expired = cmos_is_wkalrm_expired(dev);
 
 	spin_lock_irq(&rtc_lock);
-- 
2.9.3

Re: [PATCH v2 1/2] rtc-cmos: Ensure no expired alarm is left enabled after resume

[Gabriele Mazzotta]

On 01/09/2016 00:58, Gabriele Mazzotta wrote:
> Some BIOSes, such as the one of the Dell XPS13 9333, wake the system
> when an alarm goes off without informing the OS. If any of the

A clarification on this first sentence. I was looking at the ACPI
specification [1] and it seems that there are two optional bits
that can be used to determine if the RTC alarm was the reason of
the resume. However, the value reported can be inaccurate when
resuming from S4 (there's an additional flag to know if the value
is accurate or not), so a fallback mechanism such as the one of
this patch is probably still needed.

I will have a better look at this.

Regards,
Gabriele

[1] http://www.acpi.info/DOWNLOADS/ACPI_5_Errata%20A.pdf

> RTC_IRQMASK bits is set when this happens and hpet is enabled,
> the alarm is not automatically cleared at resume. As consequence,
> the user must manually clear the alarm writing 0 to wakealarm
> before being able to set a new alarm. Ensure this never happens.
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---
>  drivers/rtc/rtc-cmos.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 1dec52f..62d5b33 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -900,10 +900,32 @@ static inline int cmos_poweroff(struct device *dev)
>  
>  #ifdef	CONFIG_PM_SLEEP
>  
> +static bool cmos_is_wkalrm_expired(struct device *dev)
> +{
> +	struct cmos_rtc *cmos = dev_get_drvdata(dev);
> +	struct rtc_wkalrm alarm;
> +	struct rtc_time now;
> +	time64_t t_now;
> +	time64_t t_expires;
> +	int ret;
> +
> +	ret = rtc_read_alarm(cmos->rtc, &alarm);
> +	if (ret) {
> +		dev_err(dev, "Could not read alarm: err=%d\n", ret);
> +		return false;
> +	}
> +	t_expires = rtc_tm_to_time64(&alarm.time);
> +	cmos_read_time(dev, &now);
> +	t_now = rtc_tm_to_time64(&now);
> +
> +	return t_expires <= t_now;
> +}
> +
>  static int cmos_resume(struct device *dev)
>  {
>  	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
>  	unsigned char tmp;
> +	bool is_wkalrm_expired;
>  
>  	if (cmos->enabled_wake) {
>  		if (cmos->wake_off)
> @@ -913,6 +935,8 @@ static int cmos_resume(struct device *dev)
>  		cmos->enabled_wake = 0;
>  	}
>  
> +	is_wkalrm_expired = cmos_is_wkalrm_expired(dev);
> +
>  	spin_lock_irq(&rtc_lock);
>  	tmp = cmos->suspend_ctrl;
>  	cmos->suspend_ctrl = 0;
> @@ -939,6 +963,17 @@ static int cmos_resume(struct device *dev)
>  			tmp &= ~RTC_AIE;
>  			hpet_mask_rtc_irq_bit(RTC_AIE);
>  		} while (mask & RTC_AIE);
> +
> +		/*
> +		 * If RTC_AIE is set and we have an alarm set to go off in the
> +		 * past, then the BIOS woke the system when the alarm went off
> +		 * and we now have to clear it.
> +		 */
> +		if ((tmp & RTC_AIE) && is_wkalrm_expired) {
> +			rtc_update_irq(cmos->rtc, 1, mask);
> +			tmp &= ~RTC_AIE;
> +			CMOS_WRITE(tmp, RTC_CONTROL);
> +		}
>  	}
>  	spin_unlock_irq(&rtc_lock);
>  
> 

Re: [PATCH v2 1/2] rtc-cmos: Ensure no expired alarm is left enabled after resume

[Alexandre Belloni]

Hi

On 01/09/2016 at 00:58:59 +0200, Gabriele Mazzotta wrote :
>  static int cmos_resume(struct device *dev)
>  {
>  	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
>  	unsigned char tmp;
> +	bool is_wkalrm_expired;
>  
>  	if (cmos->enabled_wake) {
>  		if (cmos->wake_off)
> @@ -913,6 +935,8 @@ static int cmos_resume(struct device *dev)
>  		cmos->enabled_wake = 0;
>  	}
>  
> +	is_wkalrm_expired = cmos_is_wkalrm_expired(dev);
> +
>  	spin_lock_irq(&rtc_lock);
>  	tmp = cmos->suspend_ctrl;
>  	cmos->suspend_ctrl = 0;
> @@ -939,6 +963,17 @@ static int cmos_resume(struct device *dev)
>  			tmp &= ~RTC_AIE;
>  			hpet_mask_rtc_irq_bit(RTC_AIE);
>  		} while (mask & RTC_AIE);
> +
> +		/*
> +		 * If RTC_AIE is set and we have an alarm set to go off in the
> +		 * past, then the BIOS woke the system when the alarm went off
> +		 * and we now have to clear it.
> +		 */
> +		if ((tmp & RTC_AIE) && is_wkalrm_expired) {

Is there any issue dropping is_wkalrm_expired and calling
cmos_is_wkalrm_expired() here? That would avoid calling
cmos_is_wkalrm_expired on each wakeup.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/2] rtc-cmos: Ensure no expired alarm is left enabled after resume

[Gabriele Mazzotta]

2016-09-06 0:26 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> Hi
>
> On 01/09/2016 at 00:58:59 +0200, Gabriele Mazzotta wrote :
>>  static int cmos_resume(struct device *dev)
>>  {
>>       struct cmos_rtc *cmos = dev_get_drvdata(dev);
>>       unsigned char tmp;
>> +     bool is_wkalrm_expired;
>>
>>       if (cmos->enabled_wake) {
>>               if (cmos->wake_off)
>> @@ -913,6 +935,8 @@ static int cmos_resume(struct device *dev)
>>               cmos->enabled_wake = 0;
>>       }
>>
>> +     is_wkalrm_expired = cmos_is_wkalrm_expired(dev);
>> +
>>       spin_lock_irq(&rtc_lock);
>>       tmp = cmos->suspend_ctrl;
>>       cmos->suspend_ctrl = 0;
>> @@ -939,6 +963,17 @@ static int cmos_resume(struct device *dev)
>>                       tmp &= ~RTC_AIE;
>>                       hpet_mask_rtc_irq_bit(RTC_AIE);
>>               } while (mask & RTC_AIE);
>> +
>> +             /*
>> +              * If RTC_AIE is set and we have an alarm set to go off in the
>> +              * past, then the BIOS woke the system when the alarm went off
>> +              * and we now have to clear it.
>> +              */
>> +             if ((tmp & RTC_AIE) && is_wkalrm_expired) {
>
> Is there any issue dropping is_wkalrm_expired and calling
> cmos_is_wkalrm_expired() here? That would avoid calling
> cmos_is_wkalrm_expired on each wakeup.

Yes, get_rtc_time() (cmos_read_time()) tries to aquire rtc_lock.

> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com