Patchwork [RFC] timely RTC wakeup events on the MMP2 RTC

login
register
mail settings
Submitter Paul Fox
Date Dec. 2, 2011, 7:50 p.m.
Message ID <14101.1322855457@foxharp.boston.ma.us>
Download mbox | patch
Permalink /patch/128948/
State New
Headers show

Comments

Paul Fox - Dec. 2, 2011, 7:50 p.m.
this message is an RFC, to see if i'm on the right track.  the patch
below will clearly be affected by the cleanup patch stream on this
driver that's currently in progress, so consider it a work-in-progress.
(also, i don't have a lot of experience with the RTC subsystem, so
bear with me.)

on the OLPC XO-1.75 laptop (MMP2-based), we're using the linux 3.0
<device>/power/wakeup_count facilities to help identify the primary
cause of a system wakeup, as well as the /sys/power/wakeup_count
facility to prevent wakeup/suspend races.  to that end, i've been
adding calls to pm_wake_event to our drivers of interest.  to be
clear, for the RTC driver, this looks like:

    [if i've already gone wrong at this point, the rest of this
    message may be moot.  i'm somewhat surprised that we're the first
    platform to need pm_wakeup_event() functionality in an RTC, but i
    haven't seen any other support for this in the rtc subsystem.]


    Author: Paul Fox <pgf@laptop.org>
    Date:   Thu Nov 10 17:27:10 2011 -0500

	generate a pm_wakeup when we get an RTC alarm interrupt
	
	this will cause them to be counted, and the count can then be
	used to determine wakeup source.

    diff --git a/drivers/rtc/rtc-mmp.c b/drivers/rtc/rtc-mmp.c
    index 65e67cb..2841bf5 100755
    --- a/drivers/rtc/rtc-mmp.c
    +++ b/drivers/rtc/rtc-mmp.c
    @@ -144,6 +144,8 @@ static irqreturn_t mmp_rtc_interrupt(int irq, void *dev_id)
     
	    spin_unlock(&mmp_rtc_lock);
     
    +       pm_wakeup_event(&pdev->dev, 1000);
    +
	    return IRQ_HANDLED;
     }


it turns out, though, that this doesn't always work.  because the
driver enables and disables interrupts in the open/close routines, no
interrupt can ever occur unless someone has the device open. 
furthermore, interrupts that would have occurred (after device close)
are delayed until someone next opens the device, usually long after
the actual alarm time.  wakeups, of course, do occur correctly,
because those aren't affected by whether the OS is actually going to
handle the interrupt.

the specific symptom scenario we see is this:  rtcwake is used to
schedule a wakeup sometime in the future.  before that wakeup has a
chance to happen, something else (e.g., a keystroke) wakes the system. 
rtcwake exits, leaving the alarm scheduled.  some time later that alarm
fires, and the irq is disabled, so the interrupt isn't handled.  time
passes, and rtcwake is again invoked, to put the system to sleep.  as
soon as the device is opened the RTC's pending interrupt is finally
handled.  the handler runs, and calls pm_wakeup_event() -- far too
late, of course.  rtcwake continues, sets a new alarm, and attempts to
put the system to sleep.  but since the /sys/power/wakeup_count
suspend protocol is in effect, the kernel aborts the suspend, since
there's already a pending wakeup event.

one solution would be to try to cancel the pending alarm when the
system wakes up, but that seems racy, and not necessarily what the
user wanted.  it seems far simpler just to let the alarm interrupt
happen.  the following patch does this.  the open and close routines
go away entirely, because there's no longer anything for them to do.

comments?

paul


(much of this patch -- all of the last two hunks -- is simply to let
pending alarms be cancelled during device probe.  that's really just a
safety/paranoia thing, and unrelated to my questions above.)


[PATCH] eliminate delayed RTC interrupts from the MMP2 RTC

don't enable/disable interrupts on every device open/close -- just
leave them enabled, and take interrupts as they occur.  otherwise
alarms which expire while the device is closed will cause the handler
to run at the time of the next open.  this in turn will cause a wakeup
event, which may confuse PM wakeup event processing.  the probe
routine is rearranged so hardware RTC interrupt source(s) can be
cleared before enabling interrupts.

Signed-off-by: Paul Fox <pgf@laptop.org>
---
 drivers/rtc/rtc-mmp.c |   66 +++++++++++++++++-------------------------------
 1 files changed, 24 insertions(+), 42 deletions(-)
Jean-Christophe PLAGNIOL-VILLARD - Dec. 3, 2011, 12:58 p.m.
On 14:50 Fri 02 Dec     , Paul Fox wrote:
> this message is an RFC, to see if i'm on the right track.  the patch
> below will clearly be affected by the cleanup patch stream on this
> driver that's currently in progress, so consider it a work-in-progress.
> (also, i don't have a lot of experience with the RTC subsystem, so
> bear with me.)
> 
> on the OLPC XO-1.75 laptop (MMP2-based), we're using the linux 3.0
> <device>/power/wakeup_count facilities to help identify the primary
> cause of a system wakeup, as well as the /sys/power/wakeup_count
> facility to prevent wakeup/suspend races.  to that end, i've been
> adding calls to pm_wake_event to our drivers of interest.  to be
> clear, for the RTC driver, this looks like:
> 
>     [if i've already gone wrong at this point, the rest of this
>     message may be moot.  i'm somewhat surprised that we're the first
>     platform to need pm_wakeup_event() functionality in an RTC, but i
>     haven't seen any other support for this in the rtc subsystem.]
> 
> 
>     Author: Paul Fox <pgf@laptop.org>
>     Date:   Thu Nov 10 17:27:10 2011 -0500
> 
> 	generate a pm_wakeup when we get an RTC alarm interrupt
> 	
> 	this will cause them to be counted, and the count can then be
> 	used to determine wakeup source.
> 
>     diff --git a/drivers/rtc/rtc-mmp.c b/drivers/rtc/rtc-mmp.c
>     index 65e67cb..2841bf5 100755
>     --- a/drivers/rtc/rtc-mmp.c
>     +++ b/drivers/rtc/rtc-mmp.c
>     @@ -144,6 +144,8 @@ static irqreturn_t mmp_rtc_interrupt(int irq, void *dev_id)
>      
> 	    spin_unlock(&mmp_rtc_lock);
>      
>     +       pm_wakeup_event(&pdev->dev, 1000);
>     +
> 	    return IRQ_HANDLED;
>      }
> 
> 
> it turns out, though, that this doesn't always work.  because the
> driver enables and disables interrupts in the open/close routines, no
> interrupt can ever occur unless someone has the device open. 
> furthermore, interrupts that would have occurred (after device close)
> are delayed until someone next opens the device, usually long after
> the actual alarm time.  wakeups, of course, do occur correctly,
> because those aren't affected by whether the OS is actually going to
> handle the interrupt.
> 
> the specific symptom scenario we see is this:  rtcwake is used to
> schedule a wakeup sometime in the future.  before that wakeup has a
> chance to happen, something else (e.g., a keystroke) wakes the system. 
> rtcwake exits, leaving the alarm scheduled.  some time later that alarm
> fires, and the irq is disabled, so the interrupt isn't handled.  time
> passes, and rtcwake is again invoked, to put the system to sleep.  as
> soon as the device is opened the RTC's pending interrupt is finally
> handled.  the handler runs, and calls pm_wakeup_event() -- far too
> late, of course.  rtcwake continues, sets a new alarm, and attempts to
> put the system to sleep.  but since the /sys/power/wakeup_count
> suspend protocol is in effect, the kernel aborts the suspend, since
> there's already a pending wakeup event.
> 
> one solution would be to try to cancel the pending alarm when the
> system wakes up, but that seems racy, and not necessarily what the
> user wanted.  it seems far simpler just to let the alarm interrupt
> happen.  the following patch does this.  the open and close routines
> go away entirely, because there's no longer anything for them to do.
> 
> comments?
you change too much the behavior of the driver.

I've the same kind of issue before and think this will not be driver specific
but rtc specific and should be handle at rtc bus level

ditto for the pm wakeup event

and enable via kconfig, ioctl or kernel param same same as the read of the
current time by the kernel boot time

Best Regards,
J.
Paul Fox - Dec. 3, 2011, 4:55 p.m.
jean-christophe wrote:
 > On 14:50 Fri 02 Dec     , Paul Fox wrote:
 > > this message is an RFC, to see if i'm on the right track.  the patch
 > > below will clearly be affected by the cleanup patch stream on this
 > > driver that's currently in progress, so consider it a work-in-progress.
 > > (also, i don't have a lot of experience with the RTC subsystem, so
 > > bear with me.)
 > > 
 > > on the OLPC XO-1.75 laptop (MMP2-based), we're using the linux 3.0
 > > <device>/power/wakeup_count facilities to help identify the primary
 > > cause of a system wakeup, as well as the /sys/power/wakeup_count
 > > facility to prevent wakeup/suspend races.  to that end, i've been
 > > adding calls to pm_wake_event to our drivers of interest.  to be
 > > clear, for the RTC driver, this looks like:
 > > 
 > >     [if i've already gone wrong at this point, the rest of this
 > >     message may be moot.  i'm somewhat surprised that we're the first
 > >     platform to need pm_wakeup_event() functionality in an RTC, but i
 > >     haven't seen any other support for this in the rtc subsystem.]
 > > 
 > > 
 > >     Author: Paul Fox <pgf@laptop.org>
 > >     Date:   Thu Nov 10 17:27:10 2011 -0500
 > > 
 > > 	generate a pm_wakeup when we get an RTC alarm interrupt
 > > 	
 > > 	this will cause them to be counted, and the count can then be
 > > 	used to determine wakeup source.
 > > 
 > >     diff --git a/drivers/rtc/rtc-mmp.c b/drivers/rtc/rtc-mmp.c
 > >     index 65e67cb..2841bf5 100755
 > >     --- a/drivers/rtc/rtc-mmp.c
 > >     +++ b/drivers/rtc/rtc-mmp.c
 > >     @@ -144,6 +144,8 @@ static irqreturn_t mmp_rtc_interrupt(int irq, void 
 > *dev_id)
 > >      
 > > 	    spin_unlock(&mmp_rtc_lock);
 > >      
 > >     +       pm_wakeup_event(&pdev->dev, 1000);
 > >     +
 > > 	    return IRQ_HANDLED;
 > >      }
 > > 
 > > 
 > > it turns out, though, that this doesn't always work.  because the
 > > driver enables and disables interrupts in the open/close routines, no
 > > interrupt can ever occur unless someone has the device open. 
 > > furthermore, interrupts that would have occurred (after device close)
 > > are delayed until someone next opens the device, usually long after
 > > the actual alarm time.  wakeups, of course, do occur correctly,
 > > because those aren't affected by whether the OS is actually going to
 > > handle the interrupt.
 > > 
 > > the specific symptom scenario we see is this:  rtcwake is used to
 > > schedule a wakeup sometime in the future.  before that wakeup has a
 > > chance to happen, something else (e.g., a keystroke) wakes the system. 
 > > rtcwake exits, leaving the alarm scheduled.  some time later that alarm
 > > fires, and the irq is disabled, so the interrupt isn't handled.  time
 > > passes, and rtcwake is again invoked, to put the system to sleep.  as
 > > soon as the device is opened the RTC's pending interrupt is finally
 > > handled.  the handler runs, and calls pm_wakeup_event() -- far too
 > > late, of course.  rtcwake continues, sets a new alarm, and attempts to
 > > put the system to sleep.  but since the /sys/power/wakeup_count
 > > suspend protocol is in effect, the kernel aborts the suspend, since
 > > there's already a pending wakeup event.
 > > 
 > > one solution would be to try to cancel the pending alarm when the
 > > system wakes up, but that seems racy, and not necessarily what the
 > > user wanted.  it seems far simpler just to let the alarm interrupt
 > > happen.  the following patch does this.  the open and close routines
 > > go away entirely, because there's no longer anything for them to do.
 > > 
 > > comments?
 >
 > you change too much the behavior of the driver.

thanks for taking a look.  yes, that's one of the aspects that
worries me.  can you be more specific about what behaviour has
changed that's undesirable?

 > 
 > I've the same kind of issue before and think this will not be
 > driver specific but rtc specific and should be handle at rtc bus
 > level
 > 
 > ditto for the pm wakeup event

just so i understand your objection:  to handle the pm wakeup event
correctly means that interrupts must remain enabled.  it can't work
any other way.  so i think you're just saying that this is a change
of architecture that should be made to all the drivers, so that
the wakeup event will work for any of them.

 > 
 > and enable via kconfig, ioctl or kernel param same same as the read of the
 > current time by the kernel boot time

again, just to be sure i understand:  you're saying there should be
additional ways of enabling/disabling the pm wake event, rather than
just the <device>/power/wakeup sysfs node.

we won't have time to make any of these broader changes now, since
we're in the middle of a product development cycle.  for now i mainly
want to be sure i haven't done something foolhardy, and that the
essential approach is sane.

paul

 > 
 > Best Regards,
 > J.
 > 
 > -- 
 > 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.

=---------------------
 paul fox, pgf@laptop.org
Jean-Christophe PLAGNIOL-VILLARD - Dec. 4, 2011, 8 a.m.
On 11:55 Sat 03 Dec     , Paul Fox wrote:
> jean-christophe wrote:
>  > On 14:50 Fri 02 Dec     , Paul Fox wrote:
>  > > this message is an RFC, to see if i'm on the right track.  the patch
>  > > below will clearly be affected by the cleanup patch stream on this
>  > > driver that's currently in progress, so consider it a work-in-progress.
>  > > (also, i don't have a lot of experience with the RTC subsystem, so
>  > > bear with me.)
>  > > 
>  > > on the OLPC XO-1.75 laptop (MMP2-based), we're using the linux 3.0
>  > > <device>/power/wakeup_count facilities to help identify the primary
>  > > cause of a system wakeup, as well as the /sys/power/wakeup_count
>  > > facility to prevent wakeup/suspend races.  to that end, i've been
>  > > adding calls to pm_wake_event to our drivers of interest.  to be
>  > > clear, for the RTC driver, this looks like:
>  > > 
>  > >     [if i've already gone wrong at this point, the rest of this
>  > >     message may be moot.  i'm somewhat surprised that we're the first
>  > >     platform to need pm_wakeup_event() functionality in an RTC, but i
>  > >     haven't seen any other support for this in the rtc subsystem.]
>  > > 
>  > > 
>  > >     Author: Paul Fox <pgf@laptop.org>
>  > >     Date:   Thu Nov 10 17:27:10 2011 -0500
>  > > 
>  > > 	generate a pm_wakeup when we get an RTC alarm interrupt
>  > > 	
>  > > 	this will cause them to be counted, and the count can then be
>  > > 	used to determine wakeup source.
>  > > 
>  > >     diff --git a/drivers/rtc/rtc-mmp.c b/drivers/rtc/rtc-mmp.c
>  > >     index 65e67cb..2841bf5 100755
>  > >     --- a/drivers/rtc/rtc-mmp.c
>  > >     +++ b/drivers/rtc/rtc-mmp.c
>  > >     @@ -144,6 +144,8 @@ static irqreturn_t mmp_rtc_interrupt(int irq, void 
>  > *dev_id)
>  > >      
>  > > 	    spin_unlock(&mmp_rtc_lock);
>  > >      
>  > >     +       pm_wakeup_event(&pdev->dev, 1000);
>  > >     +
>  > > 	    return IRQ_HANDLED;
>  > >      }
>  > > 
>  > > 
>  > > it turns out, though, that this doesn't always work.  because the
>  > > driver enables and disables interrupts in the open/close routines, no
>  > > interrupt can ever occur unless someone has the device open. 
>  > > furthermore, interrupts that would have occurred (after device close)
>  > > are delayed until someone next opens the device, usually long after
>  > > the actual alarm time.  wakeups, of course, do occur correctly,
>  > > because those aren't affected by whether the OS is actually going to
>  > > handle the interrupt.
>  > > 
>  > > the specific symptom scenario we see is this:  rtcwake is used to
>  > > schedule a wakeup sometime in the future.  before that wakeup has a
>  > > chance to happen, something else (e.g., a keystroke) wakes the system. 
>  > > rtcwake exits, leaving the alarm scheduled.  some time later that alarm
>  > > fires, and the irq is disabled, so the interrupt isn't handled.  time
>  > > passes, and rtcwake is again invoked, to put the system to sleep.  as
>  > > soon as the device is opened the RTC's pending interrupt is finally
>  > > handled.  the handler runs, and calls pm_wakeup_event() -- far too
>  > > late, of course.  rtcwake continues, sets a new alarm, and attempts to
>  > > put the system to sleep.  but since the /sys/power/wakeup_count
>  > > suspend protocol is in effect, the kernel aborts the suspend, since
>  > > there's already a pending wakeup event.
>  > > 
>  > > one solution would be to try to cancel the pending alarm when the
>  > > system wakes up, but that seems racy, and not necessarily what the
>  > > user wanted.  it seems far simpler just to let the alarm interrupt
>  > > happen.  the following patch does this.  the open and close routines
>  > > go away entirely, because there's no longer anything for them to do.
>  > > 
>  > > comments?
>  >
>  > you change too much the behavior of the driver.
> 
> thanks for taking a look.  yes, that's one of the aspects that
> worries me.  can you be more specific about what behaviour has
> changed that's undesirable?
> 
>  > 
>  > I've the same kind of issue before and think this will not be
>  > driver specific but rtc specific and should be handle at rtc bus
>  > level
>  > 
>  > ditto for the pm wakeup event
> 
> just so i understand your objection:  to handle the pm wakeup event
> correctly means that interrupts must remain enabled.  it can't work
> any other way.  so i think you're just saying that this is a change
> of architecture that should be made to all the drivers, so that
> the wakeup event will work for any of them.
> 
>  > 
>  > and enable via kconfig, ioctl or kernel param same same as the read of the
>  > current time by the kernel boot time
> 
> again, just to be sure i understand:  you're saying there should be
> additional ways of enabling/disabling the pm wake event, rather than
> just the <device>/power/wakeup sysfs node.
yeap
> 
> we won't have time to make any of these broader changes now, since
> we're in the middle of a product development cycle.  for now i mainly
> want to be sure i haven't done something foolhardy, and that the
> essential approach is sane.
I understand your time concern but you can do this for your production.
For the mainline we need to do it the right way.

My idea is to manage this via a kernelapi automaticly done at startup as done
for the time read at boot time
so this could open the device at boot time and when an interrupt is fired
the generic code will update the wakeup event .

This way we do noit need to change the behavior of the driver

Best Regards,
J.
Paul Fox - Dec. 4, 2011, 6:43 p.m.
jean-christophe wrote:
 > On 11:55 Sat 03 Dec     , Paul Fox wrote:
 > > jean-christophe wrote:
 > >  > On 14:50 Fri 02 Dec     , Paul Fox wrote:
 ....
 > > just so i understand your objection:  to handle the pm wakeup event
 > > correctly means that interrupts must remain enabled.  it can't work
 > > any other way.  so i think you're just saying that this is a change
 > > of architecture that should be made to all the drivers, so that
 > > the wakeup event will work for any of them.
 > > 
 > >  > 
 > >  > and enable via kconfig, ioctl or kernel param same same as the read of the
 > >  > current time by the kernel boot time
 > > 
 > > again, just to be sure i understand:  you're saying there should be
 > > additional ways of enabling/disabling the pm wake event, rather than
 > > just the <device>/power/wakeup sysfs node.
 >
 > yeap
 >
 > > 
 > > we won't have time to make any of these broader changes now, since
 > > we're in the middle of a product development cycle.  for now i mainly
 > > want to be sure i haven't done something foolhardy, and that the
 > > essential approach is sane.
 >
 > I understand your time concern but you can do this for your production.
 > For the mainline we need to do it the right way.

of course!  :-)

 > 
 > My idea is to manage this via a kernelapi automaticly done at
 > startup as done for the time read at boot time so this could open
 > the device at boot time and when an interrupt is fired the generic
 > code will update the wakeup event .

i see.  yes, that might work.

thanks again for your comments.

paul

 > 
 > This way we do noit need to change the behavior of the driver
 > 
 > Best Regards,
 > J.
 > 
 > -- 
 > 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.

=---------------------
 paul fox, pgf@laptop.org

Patch

diff --git a/drivers/rtc/rtc-mmp.c b/drivers/rtc/rtc-mmp.c
index 2841bf5..8e20aad 100755
--- a/drivers/rtc/rtc-mmp.c
+++ b/drivers/rtc/rtc-mmp.c
@@ -1,5 +1,5 @@ 
 /*
- * Real Time Clock interface for StrongARM SA1x00 and XScale PXA2xx
+ * Real Time Clock interface for Marvell MMP2
  *
  * Copyright (c) 2000 Nils Faerber
  *
@@ -16,7 +16,8 @@ 
  *   by Richard Purdie <rpurdie@rpsys.net>
  *
  * Support RTC on Marvell MMP
- *   by Bin Yang <bin.yang@marvell.com>
+ *   Bin Yang <bin.yang@marvell.com>
+ *   Paul Fox <pgf@laptop.org>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -149,24 +150,6 @@  static irqreturn_t mmp_rtc_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int mmp_rtc_open(struct device *dev)
-{
-	spin_lock_irq(&mmp_rtc_lock);
-	enable_irq(irq_1hz);
-	enable_irq(irq_alrm);
-	spin_unlock_irq(&mmp_rtc_lock);
-	return 0;
-}
-
-static void mmp_rtc_release(struct device *dev)
-{
-	spin_lock_irq(&mmp_rtc_lock);
-	disable_irq(irq_1hz);
-	disable_irq(irq_alrm);
-	spin_unlock_irq(&mmp_rtc_lock);
-}
-
-
 static int mmp_rtc_ioctl(struct device *dev, unsigned int cmd,
 		unsigned long arg)
 {
@@ -276,8 +259,6 @@  static int mmp_rtc_proc(struct device *dev, struct seq_file *seq)
 }
 
 static const struct rtc_class_ops mmp_rtc_ops = {
-	.open = mmp_rtc_open,
-	.release = mmp_rtc_release,
 	.ioctl = mmp_rtc_ioctl,
 	.read_time = mmp_rtc_read_time,
 	.set_time = mmp_rtc_set_time,
@@ -312,26 +293,6 @@  static int mmp_rtc_probe(struct platform_device *pdev)
 	}
 	clk_enable(clk);
 
-	ret = request_irq(irq_1hz, mmp_rtc_interrupt, IRQF_DISABLED,
-				"rtc 1Hz", dev);
-	if (ret) {
-		dev_err(dev, "IRQ %d already in use.\n", irq_1hz);
-		irq_1hz = irq_alrm = -1;
-		ret = -ENXIO;
-		goto err;
-	}
-	disable_irq(irq_1hz);
-
-	ret = request_irq(irq_alrm, mmp_rtc_interrupt, IRQF_DISABLED,
-				"rtc Alrm", dev);
-	if (ret) {
-		dev_err(dev, "IRQ %d already in use.\n", irq_alrm);
-		irq_alrm = -1;
-		ret = -ENXIO;
-		goto err;
-	}
-	disable_irq(irq_alrm);
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (res == NULL) {
 		dev_err(&pdev->dev, "failed to get I/O memory\n");
@@ -353,6 +314,27 @@  static int mmp_rtc_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	/* Clear any pending interrupts */
+	RTSR = RTSR_AL | RTSR_HZ;
+
+	ret = request_irq(irq_1hz, mmp_rtc_interrupt, IRQF_DISABLED,
+				"rtc 1Hz", dev);
+	if (ret) {
+		dev_err(dev, "IRQ %d already in use.\n", irq_1hz);
+		irq_1hz = irq_alrm = -1;
+		ret = -ENXIO;
+		goto err;
+	}
+
+	ret = request_irq(irq_alrm, mmp_rtc_interrupt, IRQF_DISABLED,
+				"rtc Alrm", dev);
+	if (ret) {
+		dev_err(dev, "IRQ %d already in use.\n", irq_alrm);
+		irq_alrm = -1;
+		ret = -ENXIO;
+		goto err;
+	}
+
 	/*
 	 * According to the manual we should be able to let RTTR be zero
 	 * and then a default diviser for a 32.768KHz clock is used.