>From : Rafael J. Wysocki <[email protected]>
Update rtc-cmos shutdown handling to leave RTC alarms active,
resolving http://bugzilla.kernel.org/show_bug.cgi?id=11411 on
several boards. There are still some systems where the ACPI event
handling doesn't cooperate. (Possibly related to bugid 11312,
reporting the spontaneous disabling of RTC events.)
Bug 11411 reported that changes to work around some ACPI event
issues broke wake-from-S5 handling, as used for DVR applications.
(They like to power off, then wake later to record programs.)
[ [email protected]: add shutdown for PNP devices ]
[ [email protected]: update comments ]
Signed-off-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Zhao Yakui <[email protected]>
Signed-off-by: Zhang Rui <[email protected]>
Signed-off-by: David Brownell <[email protected]>
---
drivers/rtc/rtc-cmos.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
--- g26.orig/drivers/rtc/rtc-cmos.c 2008-08-23 14:27:19.000000000 -0700
+++ g26/drivers/rtc/rtc-cmos.c 2008-08-26 15:24:19.000000000 -0700
@@ -894,7 +894,6 @@ static void __exit cmos_do_remove(struct
static int cmos_suspend(struct device *dev, pm_message_t mesg)
{
struct cmos_rtc *cmos = dev_get_drvdata(dev);
- int do_wake = device_may_wakeup(dev);
unsigned char tmp;
/* only the alarm might be a wakeup event source */
@@ -903,7 +902,7 @@ static int cmos_suspend(struct device *d
if (tmp & (RTC_PIE|RTC_AIE|RTC_UIE)) {
unsigned char mask;
- if (do_wake)
+ if (device_may_wakeup(dev))
mask = RTC_IRQMASK & ~RTC_AIE;
else
mask = RTC_IRQMASK;
@@ -933,6 +932,17 @@ static int cmos_suspend(struct device *d
return 0;
}
+/* We want RTC alarms to wake us from e.g. ACPI G2/S5 "soft off", even
+ * after a detour through G3 "mechanical off", although the ACPI spec
+ * says wakeup should only work from G1/S4 "hibernate". To most users,
+ * distinctions between S4 and S5 are pointless. So when the hardware
+ * allows, don't draw that distinction.
+ */
+static inline int cmos_poweroff(struct device *dev)
+{
+ return cmos_suspend(dev, PMSG_HIBERNATE);
+}
+
static int cmos_resume(struct device *dev)
{
struct cmos_rtc *cmos = dev_get_drvdata(dev);
@@ -983,6 +993,12 @@ static int cmos_resume(struct device *de
#else
#define cmos_suspend NULL
#define cmos_resume NULL
+
+static inline int cmos_poweroff(struct device *dev)
+{
+ return -ENOSYS;
+}
+
#endif
/*----------------------------------------------------------------*/
@@ -1002,10 +1018,6 @@ static int cmos_resume(struct device *de
static int __devinit
cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
{
- /* REVISIT paranoia argues for a shutdown notifier, since PNP
- * drivers can't provide shutdown() methods to disable IRQs.
- * Or better yet, fix PNP to allow those methods...
- */
if (pnp_port_start(pnp,0) == 0x70 && !pnp_irq_valid(pnp,0))
/* Some machines contain a PNP entry for the RTC, but
* don't define the IRQ. It should always be safe to
@@ -1041,6 +1053,13 @@ static int cmos_pnp_resume(struct pnp_de
#define cmos_pnp_resume NULL
#endif
+static void cmos_pnp_shutdown(struct device *pdev)
+{
+ if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(pdev))
+ return;
+
+ cmos_do_shutdown();
+}
static const struct pnp_device_id rtc_ids[] = {
{ .id = "PNP0b00", },
@@ -1060,6 +1079,10 @@ static struct pnp_driver cmos_pnp_driver
.flags = PNP_DRIVER_RES_DO_NOT_CHANGE,
.suspend = cmos_pnp_suspend,
.resume = cmos_pnp_resume,
+ .driver = {
+ .name = (char *)driver_name,
+ .shutdown = cmos_pnp_shutdown,
+ }
};
#endif /* CONFIG_PNP */
@@ -1085,6 +1108,9 @@ static int __exit cmos_platform_remove(s
static void cmos_platform_shutdown(struct platform_device *pdev)
{
+ if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(&pdev->dev))
+ return;
+
cmos_do_shutdown();
}
On Thu, 28 Aug 2008 11:29:35 -0700
David Brownell <[email protected]> wrote:
> + if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(pdev))
erp, system_state is a pretty horrid thing. It's a global with
relatively poorly defined transition conditions which have actually
changed over time.
It was not my greatest ever idea. It was simple and expedient at the
time and expanded use of it was "discouraged" (rofl).
Is there no alternative?
On Thursday 28 August 2008, Andrew Morton wrote:
> On Thu, 28 Aug 2008 11:29:35 -0700
> David Brownell <[email protected]> wrote:
>
> > + if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(pdev))
>
> erp, system_state is a pretty horrid thing. It's a global with
> relatively poorly defined transition conditions which have actually
> changed over time.
True, but it's the best we've got for this kind of thing.
Globals ... yeech.
> It was not my greatest ever idea. It was simple and expedient at the
> time and expanded use of it was "discouraged" (rofl).
>
> Is there no alternative?
My general belief is that there should be a set of predicates
that drivers use to test whether or not the target system state
satisfies various prerequisites. Like whether a clock or power
domain must be disabled, and so on.
In this specific case, a system_is_powering_down() predicate is
the logical application of that policy to this problem.
- Dave
On Friday, 29 of August 2008, David Brownell wrote:
> On Thursday 28 August 2008, Andrew Morton wrote:
> > On Thu, 28 Aug 2008 11:29:35 -0700
> > David Brownell <[email protected]> wrote:
> >
> > > + if (system_state == SYSTEM_POWER_OFF && !cmos_poweroff(pdev))
> >
> > erp, system_state is a pretty horrid thing. It's a global with
> > relatively poorly defined transition conditions which have actually
> > changed over time.
>
> True, but it's the best we've got for this kind of thing.
Exactly.
> Globals ... yeech.
>
>
> > It was not my greatest ever idea. It was simple and expedient at the
> > time and expanded use of it was "discouraged" (rofl).
> >
> > Is there no alternative?
>
> My general belief is that there should be a set of predicates
> that drivers use to test whether or not the target system state
> satisfies various prerequisites. Like whether a clock or power
> domain must be disabled, and so on.
>
> In this specific case, a system_is_powering_down() predicate is
> the logical application of that policy to this problem.
Well, we still need to store that information somewhere and make it globally
available, this way or another.
The problem is actually deeper, because there are quirks that have to be
called in specific situations from within relatively low-level functions
(e.g. http://bugzilla.kernel.org/show_bug.cgi?id=8855#c36), so if there's
a better approach than using system_state for that, I'd like to use it. :-)
Thanks,
Rafael