Patchwork rtc: Fix unexpected poweron and unnecessary alarm disabling

login
register
mail settings
Submitter Jérémy Compostella
Date Dec. 28, 2011, 5:55 p.m.
Message ID <8762h08ttb.fsf@Apollo.jerryland.fr>
Download mbox | patch
Permalink /patch/133453/
State New
Headers show

Comments

Jérémy Compostella - Dec. 28, 2011, 5:55 p.m.
Hi,

On my laptop with 3.2.0-rc7 I have unexpected poweron after shutdown
300 seconds later. I found out that the "hwclock --systohw" plays with
UIE which lead to the rtc_disable_rtc() function call. This function
disables the alarm by setting the alarm 300 seconds later with the
disable flag which does not seems to be the best way since we could
use the alarm_irq_enable handler.

I found out that the real problem is not really related to a bad RTC
management but to a bad ACPI compliance. Actually, the platform should
not wake up when the PM1_ENABLE RTC_EN ACPI flag isn't set. But it does
on some machine like mine if there is a RTC CMOS alarm set in future even
with the AIE disabled.

Moreover, the rtc_timer_do_work calls the rtc_alarm_disable even if
there is no alarm to disable. Therefore, during the boot time, a
disabled alarm is set 300 seconds in the future which does not make
sense.

Please merge it or review it.

Best regards,

Jérémy
Jérémy Compostella - Dec. 31, 2011, 12:58 p.m.
Alessandro,

It's the first time I submit a kernel patch directly to the community. I
submitted
this patch this week and I have no answer so I was asking myself:
1. Do I miss something in the submit process ?
2. Is this patch is a mess ?
3. Maybe you are in vacation or too busy ?

This patch is really important to me and probably others who have the
same laptop or the same bug. So I will be very grateful if it this patch
was merged.

So let me know,

Thank you and Happy New Year 2012 !

Jérémy

2011/12/28 Jérémy Compostella <jeremy.compostella@gmail.com>

> Hi,
>
> On my laptop with 3.2.0-rc7 I have unexpected poweron after shutdown
> 300 seconds later. I found out that the "hwclock --systohw" plays with
> UIE which lead to the rtc_disable_rtc() function call. This function
> disables the alarm by setting the alarm 300 seconds later with the
> disable flag which does not seems to be the best way since we could
> use the alarm_irq_enable handler.
>
> I found out that the real problem is not really related to a bad RTC
> management but to a bad ACPI compliance. Actually, the platform should
> not wake up when the PM1_ENABLE RTC_EN ACPI flag isn't set. But it does
> on some machine like mine if there is a RTC CMOS alarm set in future even
> with the AIE disabled.
>
> Moreover, the rtc_timer_do_work calls the rtc_alarm_disable even if
> there is no alarm to disable. Therefore, during the boot time, a
> disabled alarm is set 300 seconds in the future which does not make
> sense.
>
> Please merge it or review it.
>
> Best regards,
>
> Jérémy
>
Linus Walleij - Jan. 4, 2012, 12:59 a.m.
2011/12/28 Jérémy Compostella <jeremy.compostella@gmail.com>:

> On my laptop with 3.2.0-rc7 I have unexpected poweron after shutdown
> 300 seconds later. I found out that the "hwclock --systohw" plays with
> UIE which lead to the rtc_disable_rtc() function call. This function
> disables the alarm by setting the alarm 300 seconds later with the
> disable flag which does not seems to be the best way since we could
> use the alarm_irq_enable handler.
>
> I found out that the real problem is not really related to a bad RTC
> management but to a bad ACPI compliance. Actually, the platform should
> not wake up when the PM1_ENABLE RTC_EN ACPI flag isn't set. But it does
> on some machine like mine if there is a RTC CMOS alarm set in future even
> with the AIE disabled.
>
> Moreover, the rtc_timer_do_work calls the rtc_alarm_disable even if
> there is no alarm to disable. Therefore, during the boot time, a
> disabled alarm is set 300 seconds in the future which does not make
> sense.

The patch looks very similar to a patch submitted by Rabin.
I would prefer if John could say something about the proper solution...

Yours,
Linus Walleij
John Stultz - Jan. 4, 2012, 1:08 a.m.
On Wed, 2012-01-04 at 01:59 +0100, Linus Walleij wrote:
> 2011/12/28 Jérémy Compostella <jeremy.compostella@gmail.com>:
> 
> > On my laptop with 3.2.0-rc7 I have unexpected poweron after shutdown
> > 300 seconds later. I found out that the "hwclock --systohw" plays with
> > UIE which lead to the rtc_disable_rtc() function call. This function
> > disables the alarm by setting the alarm 300 seconds later with the
> > disable flag which does not seems to be the best way since we could
> > use the alarm_irq_enable handler.
> >
> > I found out that the real problem is not really related to a bad RTC
> > management but to a bad ACPI compliance. Actually, the platform should
> > not wake up when the PM1_ENABLE RTC_EN ACPI flag isn't set. But it does
> > on some machine like mine if there is a RTC CMOS alarm set in future even
> > with the AIE disabled.
> >
> > Moreover, the rtc_timer_do_work calls the rtc_alarm_disable even if
> > there is no alarm to disable. Therefore, during the boot time, a
> > disabled alarm is set 300 seconds in the future which does not make
> > sense.
> 
> The patch looks very similar to a patch submitted by Rabin.
> I would prefer if John could say something about the proper solution...

For 3.2 I've requested the problematic patch be reverted. We can try
again for a proper fix for 3.3.

thanks
-john
Jérémy Compostella - Jan. 4, 2012, 9:45 a.m.
2012/1/4 John Stultz <john.stultz@linaro.org>

> On Wed, 2012-01-04 at 01:59 +0100, Linus Walleij wrote:
> > 2011/12/28 Jérémy Compostella <jeremy.compostella@gmail.com>:
> >
> > > On my laptop with 3.2.0-rc7 I have unexpected poweron after shutdown
> > > 300 seconds later. I found out that the "hwclock --systohw" plays with
> > > UIE which lead to the rtc_disable_rtc() function call. This function
> > > disables the alarm by setting the alarm 300 seconds later with the
> > > disable flag which does not seems to be the best way since we could
> > > use the alarm_irq_enable handler.
> > >
> > > I found out that the real problem is not really related to a bad RTC
> > > management but to a bad ACPI compliance. Actually, the platform should
> > > not wake up when the PM1_ENABLE RTC_EN ACPI flag isn't set. But it does
> > > on some machine like mine if there is a RTC CMOS alarm set in future
> even
> > > with the AIE disabled.
> > >
> > > Moreover, the rtc_timer_do_work calls the rtc_alarm_disable even if
> > > there is no alarm to disable. Therefore, during the boot time, a
> > > disabled alarm is set 300 seconds in the future which does not make
> > > sense.
> >
> > The patch looks very similar to a patch submitted by Rabin.
>
I took a look at the Rabin patch. I do agree it's very similar except that
my
patch avoid useless alarm disabling too.

FYI, I successfully passed the rtc_test program with this patch applied.

> I would prefer if John could say something about the proper solution...
>
> For 3.2 I've requested the problematic patch be reverted. We can try
> again for a proper fix for 3.3.
>
> thanks
> -john
>
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.
>

Patch

From 9f73443f1774f9858975453764fe2ea8589414b4 Mon Sep 17 00:00:00 2001
From: Jeremy Compostella <jeremy.compostella@gmail.com>
Date: Wed, 28 Dec 2011 18:49:12 +0100
Subject: [PATCH] rtc: Fix unexpected poweron and unnecessary alarm disabling

On my laptop with 3.2.0-rc7 I have unexpected poweron after shutdown
300 seconds later. I found out that the "hwclock --systohw" plays with
UIE which lead to the rtc_disable_rtc() function call. This function
disables the alarm by setting the alarm 300 seconds later with the
disable flag which does not seems to be the best way since we could
use the alarm_irq_enable handler.

Moreover, the rtc_timer_do_work calls the rtc_alarm_disable event if
there is no alarm to disable. Therefore, during the boot time, a
disabled alarm is set 300 seconds in the future which does not make
sense.

Signed-off-by: Jeremy Compostella <jeremy.compostella@gmail.com>
---
 drivers/rtc/interface.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 3bcc7cf..9299415 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -778,16 +778,8 @@  static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
 
 static void rtc_alarm_disable(struct rtc_device *rtc)
 {
-	struct rtc_wkalrm alarm;
-	struct rtc_time tm;
-
-	__rtc_read_time(rtc, &tm);
-
-	alarm.time = rtc_ktime_to_tm(ktime_add(rtc_tm_to_ktime(tm),
-				     ktime_set(300, 0)));
-	alarm.enabled = 0;
-
-	___rtc_set_alarm(rtc, &alarm);
+	if (rtc->ops && rtc->ops->alarm_irq_enable)
+		rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
 }
 
 /**
@@ -835,7 +827,7 @@  static void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer)
  */
 void rtc_timer_do_work(struct work_struct *work)
 {
-	struct rtc_timer *timer;
+	struct rtc_timer *timer = NULL;
 	struct timerqueue_node *next;
 	ktime_t now;
 	struct rtc_time tm;
@@ -876,7 +868,7 @@  again:
 		err = __rtc_set_alarm(rtc, &alarm);
 		if (err == -ETIME)
 			goto again;
-	} else
+	} else if (timer && timer->enabled == 0)
 		rtc_alarm_disable(rtc);
 
 	mutex_unlock(&rtc->ops_lock);
-- 
1.7.2.5