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
:
> 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(_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

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
:
> 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(_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

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(_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

[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(_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]

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 
> ---
>  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, );
> + if (ret) {
> + dev_err(dev, "Could not read alarm: err=%d\n", ret);
> + return false;
> + }
> + t_expires = rtc_tm_to_time64();
> + cmos_read_time(dev, );
> + t_now = rtc_tm_to_time64();
> +
> + 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(_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(_lock);
>  
> 

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 
> ---
>  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, );
> + if (ret) {
> + dev_err(dev, "Could not read alarm: err=%d\n", ret);
> + return false;
> + }
> + t_expires = rtc_tm_to_time64();
> + cmos_read_time(dev, );
> + t_now = rtc_tm_to_time64();
> +
> + 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(_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(_lock);
>  
> 

[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 
---
 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, );
+   if (ret) {
+   dev_err(dev, "Could not read alarm: err=%d\n", ret);
+   return false;
+   }
+   t_expires = rtc_tm_to_time64();
+   cmos_read_time(dev, );
+   t_now = rtc_tm_to_time64();
+
+   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(_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(_lock);
 
-- 
2.9.3

[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 
---
 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, );
+   if (ret) {
+   dev_err(dev, "Could not read alarm: err=%d\n", ret);
+   return false;
+   }
+   t_expires = rtc_tm_to_time64();
+   cmos_read_time(dev, );
+   t_now = rtc_tm_to_time64();
+
+   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(_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(_lock);
 
-- 
2.9.3