Re: clockevents: fix resume logic

From: Andrew Morton
Date: Mon Sep 10 2007 - 17:56:19 EST

On Sun, 22 Jul 2007 01:59:11 GMT Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx> wrote:

> Gitweb: http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=18de5bc4c1f1f1fa5e14f354a7603bd6e9d4e3b6
> Commit: 18de5bc4c1f1f1fa5e14f354a7603bd6e9d4e3b6
> Parent: 93da56efcf8c6a111f0349f6b7651172d4745ca0
> Author: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> AuthorDate: Sat Jul 21 04:37:34 2007 -0700
> Committer: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> CommitDate: Sat Jul 21 17:49:15 2007 -0700
>
> clockevents: fix resume logic
>
> We need to make sure, that the clockevent devices are resumed, before
> the tick is resumed. The current resume logic does not guarantee this.
>
> Add CLOCK_EVT_MODE_RESUME and call the set mode functions of the clock
> event devices before resuming the tick / oneshot functionality.
>
> Fixup the existing users.
>
> Thanks to Nigel Cunningham for tracking down a long standing thinko,
> which affected the jinxed VAIO.
>

This patch broke the jinxed vaio.

Which is a bit odd, considering that I must have tested it at the time.
But I bisected it right down to this commit, and the below revert patch
fixed it up.

The symptoms are that with this patch applied, resume-from-RAM will get
stuck partway through doing its stuff. If I then repeatedly hit a key on
the keyboard, resume will struggle through and complete. The system time
is then a few seconds behind the time which `hwclock' says, so it looks
like we're also not restoring the time correctly.

Also, a `reboot -f' get stuck in the same way. No progress until I start
hitting a key.


With the below revert patch against current-Linus-mainline, resume-from-RAM
does work correctly. However suspend-to-disk is still busted, in the same
way: I need to repeatedly hit a key to make progress.



From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>

Revert:

commit 18de5bc4c1f1f1fa5e14f354a7603bd6e9d4e3b6
Author: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Date: Sat Jul 21 04:37:34 2007 -0700

clockevents: fix resume logic

We need to make sure, that the clockevent devices are resumed, before
the tick is resumed. The current resume logic does not guarantee this.

Add CLOCK_EVT_MODE_RESUME and call the set mode functions of the clock
event devices before resuming the tick / oneshot functionality.

Fixup the existing users.

Thanks to Nigel Cunningham for tracking down a long standing thinko,
which affected the jinxed VAIO.


It causes the following on the Vaio:

- resume-from-ram gets stuck and requires repeated hitting of a key for it
to make progress

- `reboot -f' fails in the same way, with the same fix

- the system time after resume is wrong: it is behind the hwclock time by a
period which appears to be equal to the time spent in suspend


Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: john stultz <johnstul@xxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxx>
Cc: "Rafael J. Wysocki" <rjw@xxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---

arch/arm/mach-davinci/time.c | 2
arch/arm/mach-imx/time.c | 1
arch/arm/mach-ixp4xx/common.c | 2
arch/arm/mach-omap1/time.c | 1
arch/arm/plat-omap/timer32k.c | 2
arch/i386/kernel/apic.c | 3 -
arch/i386/kernel/hpet.c | 71 ++++++++++++++++++++++++++--
arch/i386/kernel/i8253.c | 29 +++++------
arch/i386/kernel/vmiclock.c | 1
arch/i386/xen/time.c | 3 -
arch/sh/kernel/timers/timer-tmu.c | 1
arch/sparc64/kernel/time.c | 1
drivers/lguest/lguest.c | 2
include/linux/clockchips.h | 1
kernel/time/tick-broadcast.c | 6 --
kernel/time/tick-common.c | 16 ++----
16 files changed, 88 insertions(+), 54 deletions(-)

diff -puN arch/arm/mach-davinci/time.c~a arch/arm/mach-davinci/time.c
--- a/arch/arm/mach-davinci/time.c~a
+++ a/arch/arm/mach-davinci/time.c
@@ -285,8 +285,6 @@ static void davinci_set_mode(enum clock_
case CLOCK_EVT_MODE_SHUTDOWN:
t->opts = TIMER_OPTS_DISABLED;
break;
- case CLOCK_EVT_MODE_RESUME:
- break;
}
}

diff -puN arch/arm/mach-imx/time.c~a arch/arm/mach-imx/time.c
--- a/arch/arm/mach-imx/time.c~a
+++ a/arch/arm/mach-imx/time.c
@@ -159,7 +159,6 @@ static void imx_set_mode(enum clock_even
break;
case CLOCK_EVT_MODE_SHUTDOWN:
case CLOCK_EVT_MODE_UNUSED:
- case CLOCK_EVT_MODE_RESUME:
/* Left event sources disabled, no more interrupts appears */
break;
}
diff -puN arch/arm/mach-ixp4xx/common.c~a arch/arm/mach-ixp4xx/common.c
--- a/arch/arm/mach-ixp4xx/common.c~a
+++ a/arch/arm/mach-ixp4xx/common.c
@@ -459,8 +459,6 @@ static void ixp4xx_set_mode(enum clock_e
default:
osrt = opts = 0;
break;
- case CLOCK_EVT_MODE_RESUME:
- break;
}

*IXP4XX_OSRT1 = osrt | opts;
diff -puN arch/arm/mach-omap1/time.c~a arch/arm/mach-omap1/time.c
--- a/arch/arm/mach-omap1/time.c~a
+++ a/arch/arm/mach-omap1/time.c
@@ -156,7 +156,6 @@ static void omap_mpu_set_mode(enum clock
break;
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
- case CLOCK_EVT_MODE_RESUME:
break;
}
}
diff -puN arch/arm/plat-omap/timer32k.c~a arch/arm/plat-omap/timer32k.c
--- a/arch/arm/plat-omap/timer32k.c~a
+++ a/arch/arm/plat-omap/timer32k.c
@@ -157,8 +157,6 @@ static void omap_32k_timer_set_mode(enum
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
break;
- case CLOCK_EVT_MODE_RESUME:
- break;
}
}

diff -puN arch/i386/kernel/apic.c~a arch/i386/kernel/apic.c
--- a/arch/i386/kernel/apic.c~a
+++ a/arch/i386/kernel/apic.c
@@ -264,9 +264,6 @@ static void lapic_timer_setup(enum clock
v |= (APIC_LVT_MASKED | LOCAL_TIMER_VECTOR);
apic_write_around(APIC_LVTT, v);
break;
- case CLOCK_EVT_MODE_RESUME:
- /* Nothing to do here */
- break;
}

local_irq_restore(flags);
diff -puN arch/i386/kernel/hpet.c~a arch/i386/kernel/hpet.c
--- a/arch/i386/kernel/hpet.c~a
+++ a/arch/i386/kernel/hpet.c
@@ -188,10 +188,6 @@ static void hpet_set_mode(enum clock_eve
cfg &= ~HPET_TN_ENABLE;
hpet_writel(cfg, HPET_T0_CFG);
break;
-
- case CLOCK_EVT_MODE_RESUME:
- hpet_enable_int();
- break;
}
}

@@ -222,7 +218,6 @@ static struct clocksource clocksource_hp
.mask = HPET_MASK,
.shift = HPET_SHIFT,
.flags = CLOCK_SOURCE_IS_CONTINUOUS,
- .resume = hpet_start_counter,
};

/*
@@ -319,6 +314,7 @@ int __init hpet_enable(void)

clocksource_register(&clocksource_hpet);

+
if (id & HPET_ID_LEGSUP) {
hpet_enable_int();
hpet_reserve_platform_timers(id);
@@ -551,3 +547,68 @@ irqreturn_t hpet_rtc_interrupt(int irq,
return IRQ_HANDLED;
}
#endif
+
+
+/*
+ * Suspend/resume part
+ */
+
+#ifdef CONFIG_PM
+
+static int hpet_suspend(struct sys_device *sys_device, pm_message_t state)
+{
+ unsigned long cfg = hpet_readl(HPET_CFG);
+
+ cfg &= ~(HPET_CFG_ENABLE|HPET_CFG_LEGACY);
+ hpet_writel(cfg, HPET_CFG);
+
+ return 0;
+}
+
+static int hpet_resume(struct sys_device *sys_device)
+{
+ unsigned int id;
+
+ hpet_start_counter();
+
+ id = hpet_readl(HPET_ID);
+
+ if (id & HPET_ID_LEGSUP)
+ hpet_enable_int();
+
+ return 0;
+}
+
+static struct sysdev_class hpet_class = {
+ set_kset_name("hpet"),
+ .suspend = hpet_suspend,
+ .resume = hpet_resume,
+};
+
+static struct sys_device hpet_device = {
+ .id = 0,
+ .cls = &hpet_class,
+};
+
+
+static __init int hpet_register_sysfs(void)
+{
+ int err;
+
+ if (!is_hpet_capable())
+ return 0;
+
+ err = sysdev_class_register(&hpet_class);
+
+ if (!err) {
+ err = sysdev_register(&hpet_device);
+ if (err)
+ sysdev_class_unregister(&hpet_class);
+ }
+
+ return err;
+}
+
+device_initcall(hpet_register_sysfs);
+
+#endif
diff -puN arch/i386/kernel/i8253.c~a arch/i386/kernel/i8253.c
--- a/arch/i386/kernel/i8253.c~a
+++ a/arch/i386/kernel/i8253.c
@@ -3,11 +3,11 @@
*
*/
#include <linux/clockchips.h>
-#include <linux/init.h>
-#include <linux/interrupt.h>
+#include <linux/spinlock.h>
#include <linux/jiffies.h>
+#include <linux/sysdev.h>
#include <linux/module.h>
-#include <linux/spinlock.h>
+#include <linux/init.h>

#include <asm/smp.h>
#include <asm/delay.h>
@@ -40,27 +40,26 @@ static void init_pit_timer(enum clock_ev
case CLOCK_EVT_MODE_PERIODIC:
/* binary, mode 2, LSB/MSB, ch 0 */
outb_p(0x34, PIT_MODE);
+ udelay(10);
outb_p(LATCH & 0xff , PIT_CH0); /* LSB */
+ udelay(10);
outb(LATCH >> 8 , PIT_CH0); /* MSB */
break;

+ /*
+ * Avoid unnecessary state transitions, as it confuses
+ * Geode / Cyrix based boxen.
+ */
case CLOCK_EVT_MODE_SHUTDOWN:
+ if (evt->mode == CLOCK_EVT_MODE_UNUSED)
+ break;
case CLOCK_EVT_MODE_UNUSED:
- if (evt->mode == CLOCK_EVT_MODE_PERIODIC ||
- evt->mode == CLOCK_EVT_MODE_ONESHOT) {
- outb_p(0x30, PIT_MODE);
- outb_p(0, PIT_CH0);
- outb_p(0, PIT_CH0);
- }
- break;
-
+ if (evt->mode == CLOCK_EVT_MODE_SHUTDOWN)
+ break;
case CLOCK_EVT_MODE_ONESHOT:
/* One shot setup */
outb_p(0x38, PIT_MODE);
- break;
-
- case CLOCK_EVT_MODE_RESUME:
- /* Nothing to do here */
+ udelay(10);
break;
}
spin_unlock_irqrestore(&i8253_lock, flags);
diff -puN arch/i386/kernel/vmiclock.c~a arch/i386/kernel/vmiclock.c
--- a/arch/i386/kernel/vmiclock.c~a
+++ a/arch/i386/kernel/vmiclock.c
@@ -143,7 +143,6 @@ static void vmi_timer_set_mode(enum cloc

switch (mode) {
case CLOCK_EVT_MODE_ONESHOT:
- case CLOCK_EVT_MODE_RESUME:
break;
case CLOCK_EVT_MODE_PERIODIC:
cycles_per_hz = vmi_timer_ops.get_cycle_frequency();
diff -puN arch/i386/xen/time.c~a arch/i386/xen/time.c
--- a/arch/i386/xen/time.c~a
+++ a/arch/i386/xen/time.c
@@ -412,7 +412,6 @@ static void xen_timerop_set_mode(enum cl
break;

case CLOCK_EVT_MODE_ONESHOT:
- case CLOCK_EVT_MODE_RESUME:
break;

case CLOCK_EVT_MODE_UNUSED:
@@ -475,8 +474,6 @@ static void xen_vcpuop_set_mode(enum clo
HYPERVISOR_vcpu_op(VCPUOP_stop_periodic_timer, cpu, NULL))
BUG();
break;
- case CLOCK_EVT_MODE_RESUME:
- break;
}
}

diff -puN arch/sh/kernel/timers/timer-tmu.c~a arch/sh/kernel/timers/timer-tmu.c
--- a/arch/sh/kernel/timers/timer-tmu.c~a
+++ a/arch/sh/kernel/timers/timer-tmu.c
@@ -80,7 +80,6 @@ static void tmu_set_mode(enum clock_even
break;
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
- case CLOCK_EVT_MODE_RESUME:
break;
}
}
diff -puN arch/sparc64/kernel/time.c~a arch/sparc64/kernel/time.c
--- a/arch/sparc64/kernel/time.c~a
+++ a/arch/sparc64/kernel/time.c
@@ -882,7 +882,6 @@ static void sparc64_timer_setup(enum clo
{
switch (mode) {
case CLOCK_EVT_MODE_ONESHOT:
- case CLOCK_EVT_MODE_RESUME:
break;

case CLOCK_EVT_MODE_SHUTDOWN:
diff -puN drivers/lguest/lguest.c~a drivers/lguest/lguest.c
--- a/drivers/lguest/lguest.c~a
+++ a/drivers/lguest/lguest.c
@@ -730,8 +730,6 @@ static void lguest_clockevent_set_mode(e
break;
case CLOCK_EVT_MODE_PERIODIC:
BUG();
- case CLOCK_EVT_MODE_RESUME:
- break;
}
}

diff -puN include/linux/clockchips.h~a include/linux/clockchips.h
--- a/include/linux/clockchips.h~a
+++ a/include/linux/clockchips.h
@@ -23,7 +23,6 @@ enum clock_event_mode {
CLOCK_EVT_MODE_SHUTDOWN,
CLOCK_EVT_MODE_PERIODIC,
CLOCK_EVT_MODE_ONESHOT,
- CLOCK_EVT_MODE_RESUME,
};

/* Clock event notification values */
diff -puN kernel/time/tick-broadcast.c~a kernel/time/tick-broadcast.c
--- a/kernel/time/tick-broadcast.c~a
+++ a/kernel/time/tick-broadcast.c
@@ -55,7 +55,7 @@ cpumask_t *tick_get_broadcast_mask(void)
*/
static void tick_broadcast_start_periodic(struct clock_event_device *bc)
{
- if (bc)
+ if (bc && bc->mode == CLOCK_EVT_MODE_SHUTDOWN)
tick_setup_periodic(bc, 1);
}

@@ -316,7 +316,7 @@ void tick_suspend_broadcast(void)
spin_lock_irqsave(&tick_broadcast_lock, flags);

bc = tick_broadcast_device.evtdev;
- if (bc)
+ if (bc && tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
clockevents_set_mode(bc, CLOCK_EVT_MODE_SHUTDOWN);

spin_unlock_irqrestore(&tick_broadcast_lock, flags);
@@ -333,8 +333,6 @@ int tick_resume_broadcast(void)
bc = tick_broadcast_device.evtdev;

if (bc) {
- clockevents_set_mode(bc, CLOCK_EVT_MODE_RESUME);
-
switch (tick_broadcast_device.mode) {
case TICKDEV_MODE_PERIODIC:
if(!cpus_empty(tick_broadcast_mask))
diff -puN kernel/time/tick-common.c~a kernel/time/tick-common.c
--- a/kernel/time/tick-common.c~a
+++ a/kernel/time/tick-common.c
@@ -318,17 +318,12 @@ static void tick_resume(void)
{
struct tick_device *td = &__get_cpu_var(tick_cpu_device);
unsigned long flags;
- int broadcast = tick_resume_broadcast();

spin_lock_irqsave(&tick_device_lock, flags);
- clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME);
-
- if (!broadcast) {
- if (td->mode == TICKDEV_MODE_PERIODIC)
- tick_setup_periodic(td->evtdev, 0);
- else
- tick_resume_oneshot();
- }
+ if (td->mode == TICKDEV_MODE_PERIODIC)
+ tick_setup_periodic(td->evtdev, 0);
+ else
+ tick_resume_oneshot();
spin_unlock_irqrestore(&tick_device_lock, flags);
}

@@ -365,7 +360,8 @@ static int tick_notify(struct notifier_b
break;

case CLOCK_EVT_NOTIFY_RESUME:
- tick_resume();
+ if (!tick_resume_broadcast())
+ tick_resume();
break;

default:
_

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/