The scheduler imposes a requirement to sched_clock()
which is to stop the clock during suspend, if we don't
do that IRQ threads will be rescheduled in the future
which might cause transfers to timeout depending on
how driver is written.
This became an issue on OMAP when we converted omap-i2c.c
to use threaded IRQs, it turned out that depending on how
much time we spent on suspend, the I2C IRQ thread would
end up being rescheduled so far in the future that I2C
transfers would timeout and, because omap_hsmmc depends
on an I2C-connected device to detect if an MMC card is
inserted in the slot, our rootfs would just vanish.
arch/arm/kernel/sched_clock.c already had an optional
implementation (sched_clock_needs_suspend()) which would
handle scheduler's requirement properly, what this patch
does is simply to make that implementation non-optional.
This has been tested with beagleboard XM (OMAP3630) and
pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
after this patch.
Signed-off-by: Felipe Balbi <[email protected]>
---
arch/arm/include/asm/sched_clock.h | 2 --
arch/arm/kernel/sched_clock.c | 18 ++++--------------
2 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/arch/arm/include/asm/sched_clock.h b/arch/arm/include/asm/sched_clock.h
index 05b8e82..e3f7572 100644
--- a/arch/arm/include/asm/sched_clock.h
+++ b/arch/arm/include/asm/sched_clock.h
@@ -10,7 +10,5 @@
extern void sched_clock_postinit(void);
extern void setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate);
-extern void setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
- unsigned long rate);
#endif
diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index e21bac2..fc6692e 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -107,13 +107,6 @@ static void sched_clock_poll(unsigned long wrap_ticks)
update_sched_clock();
}
-void __init setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
- unsigned long rate)
-{
- setup_sched_clock(read, bits, rate);
- cd.needs_suspend = true;
-}
-
void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
{
unsigned long r, w;
@@ -189,18 +182,15 @@ void __init sched_clock_postinit(void)
static int sched_clock_suspend(void)
{
sched_clock_poll(sched_clock_timer.data);
- if (cd.needs_suspend)
- cd.suspended = true;
+ cd.suspended = true;
return 0;
}
static void sched_clock_resume(void)
{
- if (cd.needs_suspend) {
- cd.epoch_cyc = read_sched_clock();
- cd.epoch_cyc_copy = cd.epoch_cyc;
- cd.suspended = false;
- }
+ cd.epoch_cyc = read_sched_clock();
+ cd.epoch_cyc_copy = cd.epoch_cyc;
+ cd.suspended = false;
}
static struct syscore_ops sched_clock_ops = {
--
1.8.0.rc0
Hi,
On Mon, Oct 22, 2012 at 02:54:37PM +0300, Felipe Balbi wrote:
> The scheduler imposes a requirement to sched_clock()
> which is to stop the clock during suspend, if we don't
> do that IRQ threads will be rescheduled in the future
> which might cause transfers to timeout depending on
> how driver is written.
>
> This became an issue on OMAP when we converted omap-i2c.c
> to use threaded IRQs, it turned out that depending on how
> much time we spent on suspend, the I2C IRQ thread would
> end up being rescheduled so far in the future that I2C
> transfers would timeout and, because omap_hsmmc depends
> on an I2C-connected device to detect if an MMC card is
> inserted in the slot, our rootfs would just vanish.
>
> arch/arm/kernel/sched_clock.c already had an optional
> implementation (sched_clock_needs_suspend()) which would
> handle scheduler's requirement properly, what this patch
> does is simply to make that implementation non-optional.
>
> This has been tested with beagleboard XM (OMAP3630) and
> pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
> after this patch.
>
> Signed-off-by: Felipe Balbi <[email protected]>
just adding more guys to Cc. Please add more if I have missed someone.
> ---
> arch/arm/include/asm/sched_clock.h | 2 --
> arch/arm/kernel/sched_clock.c | 18 ++++--------------
> 2 files changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/include/asm/sched_clock.h b/arch/arm/include/asm/sched_clock.h
> index 05b8e82..e3f7572 100644
> --- a/arch/arm/include/asm/sched_clock.h
> +++ b/arch/arm/include/asm/sched_clock.h
> @@ -10,7 +10,5 @@
>
> extern void sched_clock_postinit(void);
> extern void setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate);
> -extern void setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
> - unsigned long rate);
>
> #endif
> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
> index e21bac2..fc6692e 100644
> --- a/arch/arm/kernel/sched_clock.c
> +++ b/arch/arm/kernel/sched_clock.c
> @@ -107,13 +107,6 @@ static void sched_clock_poll(unsigned long wrap_ticks)
> update_sched_clock();
> }
>
> -void __init setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
> - unsigned long rate)
> -{
> - setup_sched_clock(read, bits, rate);
> - cd.needs_suspend = true;
> -}
> -
> void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
> {
> unsigned long r, w;
> @@ -189,18 +182,15 @@ void __init sched_clock_postinit(void)
> static int sched_clock_suspend(void)
> {
> sched_clock_poll(sched_clock_timer.data);
> - if (cd.needs_suspend)
> - cd.suspended = true;
> + cd.suspended = true;
> return 0;
> }
>
> static void sched_clock_resume(void)
> {
> - if (cd.needs_suspend) {
> - cd.epoch_cyc = read_sched_clock();
> - cd.epoch_cyc_copy = cd.epoch_cyc;
> - cd.suspended = false;
> - }
> + cd.epoch_cyc = read_sched_clock();
> + cd.epoch_cyc_copy = cd.epoch_cyc;
> + cd.suspended = false;
> }
>
> static struct syscore_ops sched_clock_ops = {
> --
> 1.8.0.rc0
>
--
balbi
+Colin Cross, Barry Song also
Felipe Balbi <[email protected]> writes:
> The scheduler imposes a requirement to sched_clock()
> which is to stop the clock during suspend, if we don't
> do that IRQ threads will be rescheduled in the future
> which might cause transfers to timeout depending on
> how driver is written.
It's not just about IRQ threads, it's about RT throttling. IOW, not
just IRQ threads will be postponed, but all RT tasks will be throttled
temporarily as well.
The changelog should also mention that this has an inconvenient side
effect of stopping the printk times during suspend. Based on the
original thread where this feature was discussed and introduced, some
platforms wanted to opt out of this behavior[1], so the optional API was
added.
However, in light of RT throttling, this a correctness issue for process
accounting, so I agree that this should be done for all platforms
instead of providing an optional 'needs suspend' version of the API,
even though it means printk times no longer reflect time spent
suspended.
After a discussion with peterz on this topic, it seems that x86
already ensures that sched_clock stops during suspend for similar
reasons[2].
The question then is whether this is a fix that belongs in v3.7.
Technically, it is not a regression, so I think this should probably be
v3.8 material. If that's the decision, then the threaded IRQ support
for the OMAP I2C driver needs to be reverted for v3.7 until this fix is merged.
Kevin
[1] http://marc.info/?l=linux-arm-kernel&m=134307004508708&w=2
[2] http://marc.info/?l=linux-arm-kernel&m=135065529907297&w=2
On Mon, Oct 22, 2012 at 7:05 PM, Kevin Hilman
<[email protected]> wrote:
> However, in light of RT throttling, this a correctness issue for process
> accounting, so I agree that this should be done for all platforms
> instead of providing an optional 'needs suspend' version of the API,
> even though it means printk times no longer reflect time spent
> suspended.
Maybe we should get printk() to use the best clocksource
instead.
The reason AFAICT that printk() is using sched_clock() is that
it's supposed to be fast. But now it seems that it's not going
to return what printk() needs anymore.
(Dragging Stultz & Gleixner into this...)
Yours,
Linus Walleij
On Mon, Oct 22, 2012 at 1:54 PM, Felipe Balbi <[email protected]> wrote:
> The scheduler imposes a requirement to sched_clock()
> which is to stop the clock during suspend, if we don't
> do that IRQ threads will be rescheduled in the future
> which might cause transfers to timeout depending on
> how driver is written.
>
> This became an issue on OMAP when we converted omap-i2c.c
> to use threaded IRQs, it turned out that depending on how
> much time we spent on suspend, the I2C IRQ thread would
> end up being rescheduled so far in the future that I2C
> transfers would timeout and, because omap_hsmmc depends
> on an I2C-connected device to detect if an MMC card is
> inserted in the slot, our rootfs would just vanish.
>
> arch/arm/kernel/sched_clock.c already had an optional
> implementation (sched_clock_needs_suspend()) which would
> handle scheduler's requirement properly, what this patch
> does is simply to make that implementation non-optional.
>
> This has been tested with beagleboard XM (OMAP3630) and
> pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
> after this patch.
>
> Signed-off-by: Felipe Balbi <[email protected]>
After Russell explains so I get it:
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij