[RFC] [PATCH 2/3] Call RTC wake_on hook function when alarm is enabled

[RFC] [PATCH 2/3] Call RTC wake_on hook function when alarm is enabled

[Zhao Yakui]

Subject: ACPI: Call RTC wake_on hook function when alarm is enabled
>From : Zhao Yakui <yakui.zhao@intel.com>

According to ACPI spec the FIXED_RTC bit of acpi fadt feature flag
indicates whether RTC wake status is supported in fixed register space.
Zero indicates the RTC wake status is supported in fixed register space.
In such case the rtc_info should be initialized:
        rtc_info.wake_on = rtc_wake_on
        rtc_info.wake_off = rtc_wake_off;


In my test if RTC_wake_on hook function isn't called, the RTC alarm can 
be fired only once.( The Fixed_RTC event is enabled in the rtc_wake_on
hook function for rtc-cmos driver) This is caused by the following :
      The Fixed_RTC event is enabled when the Fixed_RTC event handler is 
      installed. When the RTC alarm is set, it can be fired for the first time.
      But after RTC alarm is fired, the Fixed_RTC event is disabled in the rtc
      event handler. When the RTC alarm is set again, the Fixed_RTC event is 
      still disabled and no RTC alarm can be fired again.

So it is necessary to call the rtc_wake_on hook function when RTC alarm is
enabled.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
Signed-off-by: Zhang Rui  <rui.zhang@intel.com>

---
 drivers/acpi/glue.c    |   14 ++++++++++++--
 drivers/rtc/rtc-cmos.c |    8 ++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/glue.c
===================================================================
--- linux-2.6.orig/drivers/acpi/glue.c
+++ linux-2.6/drivers/acpi/glue.c
@@ -309,8 +309,18 @@ static int __init acpi_rtc_init(void)
 
 	if (dev) {
 		rtc_wake_setup();
-		rtc_info.wake_on = rtc_wake_on;
-		rtc_info.wake_off = rtc_wake_off;
+		/*
+		 * According to the ACPI spec the FIXED_RTC bit indicates
+		 * whether RTC wake status is supported in Fixed Register
+		 * space. 0  supported; 1 not supported.
+		 * If it is zero, it means that RTC wake status is supported
+		 * in fixed register space and rtc_info.wake_on/off should be
+		 * set. Otherwise they should be cleared.
+		 */
+		if (!(acpi_gbl_FADT.flags & ACPI_FADT_FIXED_RTC)) {
+			rtc_info.wake_on = rtc_wake_on;
+			rtc_info.wake_off = rtc_wake_off;
+		}
 
 		/* workaround bug in some ACPI tables */
 		if (acpi_gbl_FADT.month_alarm && !acpi_gbl_FADT.day_alarm) {
Index: linux-2.6/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-2.6.orig/drivers/rtc/rtc-cmos.c
+++ linux-2.6/drivers/rtc/rtc-cmos.c
@@ -248,6 +248,14 @@ static int cmos_set_alarm(struct device 
 
 	spin_unlock_irq(&rtc_lock);
 
+	/*
+	 * When the wake_on hook function exists, it is necessary to
+	 * call the wake_on hook function in order to enable RTC alarm.
+	 */
+	if (t->enabled)
+		if (cmos->wake_on)
+			cmos->wake_on(dev);
+
 	return 0;
 }
 

Re: [RFC] [PATCH 2/3] Call RTC wake_on hook function when alarm is enabled

[David Brownell]

On Tuesday 08 April 2008, Zhao Yakui wrote:
> +       if (t->enabled)
> +               if (cmos->wake_on)
> +                       cmos->wake_on(dev);

That looks ugly (bracketing) ... and suspicious in
terms of conformance to the RTC interface.

Shouldn't it be:

	if (t->enabled) {
		if (cmos->wake_on)
			cmos->wake_on(dev);
	} else {
		if (cmos->wake_off)
			cmos->wake_off(dev);
	}

instead?  Else I don't see what would disable the
alarm when it's supposed to be disabled.  Plus, if
that's done here, I suspect it also needs to be
done in rtc_ioctl() for RTC_AIE_{ON,OFF} and *NOT*
done in cmos_{suspend,resume)() ... very odd for a
wakeup hook, since none of those presume the system
is entering a state from which it could wake!

But in general this raises the question of how
to properly hook up to ACPI.  If ACPI's hook is for
normal alarm behavior (supplanting the RTC IRQ),
that's odd ... and it shouldn't fit into a hook
designed to ensure only that wake-from-sleep works.

At any rate, this patch clearly can't merge as-is.

- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 2/3] Call RTC wake_on hook function when alarm is enabled

[David Brownell]

On Wednesday 09 April 2008, Zhao Yakui wrote:
> 
> > instead?  Else I don't see what would disable the
> > alarm when it's supposed to be disabled.  Plus, if
> > that's done here, I suspect it also needs to be
> > done in rtc_ioctl() for RTC_AIE_{ON,OFF} and *NOT*
> > done in cmos_{suspend,resume)() ... very odd for a
> > wakeup hook, since none of those presume the system
> > is entering a state from which it could wake!
> 
> If alarm is only used as the wake event source from sleeping state, it
> is enough to call hook function in suspend/resume.

From what other state could it "wake" though ???
It doesn't "wake" except from sleeping states...


> But if the system is booted with acpi enabled , I suspect whether RTC
> alarm can trigger RTC IRQ.

That is, you just "suspect" it won't work?  That seems
like a weak reason to change things.  And if RTC IRQs
don't actually work at runtime, that'd seem wierd.


> If the RTC alarm is dedicated as the wake 
> event source, it will be also OK to call the hook function when the
> alarm is enabled.

Well, *today* the only client of that hook is ACPI.  But
if there's no actual need to call those wake hooks for
non-wake code paths, I'd sure rather avoid doing that...

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 2/3] Call RTC wake_on hook function when alarm is enabled

[Zhao Yakui]

On Wed, 2008-04-09 at 00:48 -0700, David Brownell wrote:
> On Tuesday 08 April 2008, Zhao Yakui wrote:
> > +       if (t->enabled)
> > +               if (cmos->wake_on)
> > +                       cmos->wake_on(dev);
> 
> That looks ugly (bracketing) ... and suspicious in
> terms of conformance to the RTC interface.
> 
> Shouldn't it be:
> 
> 	if (t->enabled) {
> 		if (cmos->wake_on)
> 			cmos->wake_on(dev);
> 	} else {
> 		if (cmos->wake_off)
> 			cmos->wake_off(dev);
> 	}
> 
> instead?  Else I don't see what would disable the
> alarm when it's supposed to be disabled.  Plus, if
> that's done here, I suspect it also needs to be
> done in rtc_ioctl() for RTC_AIE_{ON,OFF} and *NOT*
> done in cmos_{suspend,resume)() ... very odd for a
> wakeup hook, since none of those presume the system
> is entering a state from which it could wake!
If alarm is only used as the wake event source from sleeping state, it
is enough to call hook function in suspend/resume.
But if the system is booted with acpi enabled , I suspect whether RTC
alarm can trigger RTC IRQ. If the RTC alarm is dedicated as the wake
event source, it will be also OK to call the hook function when the
alarm is enabled. 
> But in general this raises the question of how
> to properly hook up to ACPI.  If ACPI's hook is for
> normal alarm behavior (supplanting the RTC IRQ),
> that's odd ... and it shouldn't fit into a hook
> designed to ensure only that wake-from-sleep works.
> 
> At any rate, this patch clearly can't merge as-is.
> 
> - Dave

Re: [RFC] [PATCH 2/3] Call RTC wake_on hook function when alarm is enabled

[Zhang, Rui]

On Wed, 2008-04-09 at 18:08 +0800, David Brownell wrote:
> On Wednesday 09 April 2008, Zhao Yakui wrote:
> >
> > > instead?  Else I don't see what would disable the
> > > alarm when it's supposed to be disabled.  Plus, if
> > > that's done here, I suspect it also needs to be
> > > done in rtc_ioctl() for RTC_AIE_{ON,OFF} and *NOT*
> > > done in cmos_{suspend,resume)() ... very odd for a
> > > wakeup hook, since none of those presume the system
> > > is entering a state from which it could wake!
> >
> > If alarm is only used as the wake event source from sleeping state,
> it
> > is enough to call hook function in suspend/resume.
> 
> From what other state could it "wake" though ???
> It doesn't "wake" except from sleeping states...
> 
> 
> > But if the system is booted with acpi enabled , I suspect whether
> RTC
> > alarm can trigger RTC IRQ.
> 
> That is, you just "suspect" it won't work?  That seems
> like a weak reason to change things.  And if RTC IRQs
> don't actually work at runtime, that'd seem wierd.
> 
> 
> > If the RTC alarm is dedicated as the wake
> > event source, it will be also OK to call the hook function when the
> > alarm is enabled.
> 
> Well, *today* the only client of that hook is ACPI.  But
> if there's no actual need to call those wake hooks for
> non-wake code paths, I'd sure rather avoid doing that...
Reasonable.
As the ACPI wakup event of RTC is useless at runtime, it's okay with me
to drop this patch.

thanks,
rui