| Message ID | 14101.1322855457@foxharp.boston.ma.us |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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
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.
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
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.
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(-)