REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

[Andreas Friedrich]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Hello,

I have found a RTC problem on my Toshiba Tecra notebook:

After shuting down my system (init 0), it powers on automatically
after 300 seconds!

I've detected this problem the first time with the stable kernel 3.1.5.
Bisecting leads me to the following patch:

"rtc: Disable the alarm in the hardware"
Stable 3.1.5:     0cbc008c56f7b4a11ba6fe80e196d7ab322baabf
Mainline 3.2-rcX: c0afabd3d553c521e003779c127143ffde55a16f

This change makes the newer kernels unusable for me. Maybe the Toshiba
BIOS does not work adequate, but my BIOS is already the newest version
and sorry, I can't fix the BIOS.

I've added some system info:
   dmesg.txt
   3.2-rcX.config
   lshw.txt
   lspci-vvv.txt
   proc_driver_rtc.txt
   proc_version.txt

With best regards,
Andreas Friedrich

[-- Attachment #2: auto_poweron.tar.gz --]
[-- Type: application/x-compressed-tar, Size: 40166 bytes --]

Re: REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

[Andreas Friedrich]

On Mo, Dez 26, 2011 at 03:58:41 +0100, Andreas Friedrich wrote:

Hello Rabin,
...
> After shuting down my system (init 0), it powers on automatically
> after 300 seconds!
> 
> I've detected this problem the first time with the stable kernel 3.1.5.
> Bisecting leads me to the following patch:
> 
> "rtc: Disable the alarm in the hardware"
> Stable 3.1.5:     0cbc008c56f7b4a11ba6fe80e196d7ab322baabf
> Mainline 3.2-rcX: c0afabd3d553c521e003779c127143ffde55a16f
...

the 5 minutes auto-poweron problem was caused by the new function
drivers/rtc/interface.c -> rtc_alarm_disable():
   ...
   alarm.time = rtc_ktime_to_tm(ktime_add(rtc_tm_to_ktime(tm),
				        ktime_set(300, 0)));
   alarm.enabled = 0;
   ...

In this function the RTC alarm shall be disabled. Why do you set up a
5 minutes interval just before disabling the alarm?

I have changed
   ktime_set(300, 0)));
to
   ktime_set(0, 0)));
and all worked fine with my notebook.

Please check if this could be a common solution of the problem.

With best regards,
Andreas Friedrich

Re: REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

[Rabin Vincent]

On Tue, Dec 27, 2011 at 16:27, Andreas Friedrich <afrie@gmx.net> wrote:
> On Mo, Dez 26, 2011 at 03:58:41 +0100, Andreas Friedrich wrote:
> the 5 minutes auto-poweron problem was caused by the new function
> drivers/rtc/interface.c -> rtc_alarm_disable():
>   ...
>   alarm.time = rtc_ktime_to_tm(ktime_add(rtc_tm_to_ktime(tm),
>                                        ktime_set(300, 0)));
>   alarm.enabled = 0;
>   ...
>
> In this function the RTC alarm shall be disabled. Why do you set up a
> 5 minutes interval just before disabling the alarm?

The 5 minutes is taken from rtc-sysfs.c ("Provide a valid future alarm
time" etc.).  Although perhaps the "future" part is just to get past the
check in __rtc_set_alarm(), which we anyway don't use in
rtc_alarm_disable().

>
> I have changed
>   ktime_set(300, 0)));
> to
>   ktime_set(0, 0)));
> and all worked fine with my notebook.
>
> Please check if this could be a common solution of the problem.

Probably not, because if your hardware really doesn't disable the alarm
then you could get the same problem if someone say changes the RTC time
to a few minutes earlier after a call to rtc_alarm_disable().

Perhaps we can avoid your five-minute problem by just attempting
to disable the irq without setting a new alarm time (not yet tested):

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 3bcc7cf..54a3b5e 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -778,16 +778,10 @@ 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;
+	if (!rtc->ops || !rtc->ops->alarm_irq_enable)
+		return;

-	___rtc_set_alarm(rtc, &alarm);
+	rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
 }

btw, if you haven't done so yet, could you please confirm that it's not
something in your userspace which is _asking_ for the alarm to be
enabled?  You could add a printk() of the 'enabled' argument into
cmos_alarm_irq_enable() to do this (and one in cmos_set_alarm()
wouldn't hurt too).

Re: REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

[Andreas Friedrich]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Di, Dez 27, 2011 at 08:07:05 +0530, Rabin Vincent wrote:

Hello Rabin,
...
> Perhaps we can avoid your five-minute problem by just attempting
> to disable the irq without setting a new alarm time (not yet tested):
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
...
I have tested your patch and for me it works!

> btw, if you haven't done so yet, could you please confirm that it's not
> something in your userspace which is _asking_ for the alarm to be
> enabled?  You could add a printk() of the 'enabled' argument into
> cmos_alarm_irq_enable() to do this (and one in cmos_set_alarm()
> wouldn't hurt too).

No, I haven't done this test, because my environment didn't change
when testing the two different kernel versions.

Nevertheless I have added the printk() calls starting with the string
'afrie:' in cmos_alarm_irq_enable() and cmos_set_alarm(). The console
output was captured by a second PC via serial line:

When booting there are no extra log messages (see boot.log).

Wenn calling 'init 0' there are 3 extra messages. I have tested it
with and without your patch (refer to init_0_with_patch.log and
init_0_without_patch.log). The last one is always a 'disable' message
but I found the following difference: In the 'patched' case the
disable message comes from cmos_alarm_irq_enable(). In the other case
the disable message comes from cmos_set_alarm(). May be you can
explain what's going on?

With best regards,
Andreas

[-- Attachment #2: logs.tar.gz --]
[-- Type: application/x-compressed-tar, Size: 13336 bytes --]

Re: REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

[John Stultz]

On Tue, 2011-12-27 at 20:07 +0530, Rabin Vincent wrote:
> On Tue, Dec 27, 2011 at 16:27, Andreas Friedrich <afrie@gmx.net> wrote:
> > On Mo, Dez 26, 2011 at 03:58:41 +0100, Andreas Friedrich wrote:
> > the 5 minutes auto-poweron problem was caused by the new function
> > drivers/rtc/interface.c -> rtc_alarm_disable():
> >   ...
> >   alarm.time = rtc_ktime_to_tm(ktime_add(rtc_tm_to_ktime(tm),
> >                                        ktime_set(300, 0)));
> >   alarm.enabled = 0;
> >   ...
> >
> > In this function the RTC alarm shall be disabled. Why do you set up a
> > 5 minutes interval just before disabling the alarm?
> 
> The 5 minutes is taken from rtc-sysfs.c ("Provide a valid future alarm
> time" etc.).  Although perhaps the "future" part is just to get past the
> check in __rtc_set_alarm(), which we anyway don't use in
> rtc_alarm_disable().
> 
> >
> > I have changed
> >   ktime_set(300, 0)));
> > to
> >   ktime_set(0, 0)));
> > and all worked fine with my notebook.
> >
> > Please check if this could be a common solution of the problem.
> 
> Probably not, because if your hardware really doesn't disable the alarm
> then you could get the same problem if someone say changes the RTC time
> to a few minutes earlier after a call to rtc_alarm_disable().
> 
> Perhaps we can avoid your five-minute problem by just attempting
> to disable the irq without setting a new alarm time (not yet tested):
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 3bcc7cf..54a3b5e 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -778,16 +778,10 @@ 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;
> +	if (!rtc->ops || !rtc->ops->alarm_irq_enable)
> +		return;
> 
> -	___rtc_set_alarm(rtc, &alarm);
> +	rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
>  }
> 
> btw, if you haven't done so yet, could you please confirm that it's not
> something in your userspace which is _asking_ for the alarm to be
> enabled?  You could add a printk() of the 'enabled' argument into
> cmos_alarm_irq_enable() to do this (and one in cmos_set_alarm()
> wouldn't hurt too).


Hey Rabin,
	Sorry to chime in late, just getting back from the holidays. Since it
sounds like the above patch would need more testing and we're way too
late in the -rc cycle, I'm suggesting we revert your original patch
c0afabd3d553c521e003779c127143ffde55a16f and we can try to get this
solved and tested properly for 3.3.

Linus: Could you revert c0afabd3d553c521e003779c127143ffde55a16f ? 

thanks
-john

Re: REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

[John Stultz]

On Tue, 2011-12-27 at 20:07 +0530, Rabin Vincent wrote:
> Perhaps we can avoid your five-minute problem by just attempting
> to disable the irq without setting a new alarm time (not yet tested):
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 3bcc7cf..54a3b5e 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -778,16 +778,10 @@ 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;
> +	if (!rtc->ops || !rtc->ops->alarm_irq_enable)
> +		return;
> 
> -	___rtc_set_alarm(rtc, &alarm);
> +	rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
>  }
> 

Hey Rabin,
	Just wanted to close out on this. I know this change worked for
Andreas, but did it also resolve the original issue for you? I'm trying
to get this ready to be queued.

thanks
-john

Re: REGRESSION 3.2-rcX: RTC auto poweron after 5 minutes

[Rabin Vincent]

On Mon, Jan 23, 2012 at 04:31:13PM -0800, John Stultz wrote:
> On Tue, 2011-12-27 at 20:07 +0530, Rabin Vincent wrote:
> > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> > index 3bcc7cf..54a3b5e 100644
> > --- a/drivers/rtc/interface.c
> > +++ b/drivers/rtc/interface.c
> > @@ -778,16 +778,10 @@ 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;
> > +	if (!rtc->ops || !rtc->ops->alarm_irq_enable)
> > +		return;
> > 
> > -	___rtc_set_alarm(rtc, &alarm);
> > +	rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
> >  }
> > 
> 
> 	Just wanted to close out on this. I know this change worked for
> Andreas, but did it also resolve the original issue for you?

Yes, the original issue remains fixed after this change.

Thanks.