[PULL] clockevents: a couple of fixes for tip/timers/urgent

[PULL] clockevents: a couple of fixes for tip/timers/urgent

[Daniel Lezcano]

Hi Ingo,

this pull request contains the following changes:

  - Prevented to suspend or resume the sh_cmt clocksource when this one 
is not enabled (Geert Uytterhoeven)
  - Improved build coverage with COMPILE_TEST for the stm32 timer 
(Maxime Coquelin)

Thanks !

   -- Daniel

The following changes since commit 0f44705175347ec96935d60b765b5d14ecc763bb:

   tick: Move the export of tick_broadcast_oneshot_control to the proper 
place (2015-07-14 12:01:04 +0200)

are available in the git repository at:

   http://git.linaro.org/people/daniel.lezcano/linux.git 
clockevents/4.2-fixes

for you to fetch changes up to 1822a48be12f7ce2799d3423e49b967896794c4d:

   clockevents/drivers/sh_cmt: Only perform clocksource suspend/resume 
if enabled (2015-08-06 17:17:20 +0200)

----------------------------------------------------------------
Geert Uytterhoeven (1):
       clockevents/drivers/sh_cmt: Only perform clocksource 
suspend/resume if enabled

Maxime Coquelin (1):
       clockevents/drivers/timer-stm32: Improve dependencies of timer-stm32

  drivers/clocksource/Kconfig  | 4 ++--
  drivers/clocksource/sh_cmt.c | 6 ++++++
  2 files changed, 8 insertions(+), 2 deletions(-)


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 1/2] clockevents/drivers/timer-stm32: Improve dependencies of timer-stm32

[Daniel Lezcano]

From: Maxime Coquelin <mcoquelin.stm32@gmail.com>

This patch improves the build coverage with COMPILE_TEST so it is not only
limited to ARM builds.

The fix consists in making the STM32 timer depend on GENERIC_CLOCKEVENTS.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e57730..e110ec3 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -112,8 +112,8 @@ config CLKSRC_LPC32XX
 	select CLKSRC_OF
 
 config CLKSRC_STM32
-	bool "Clocksource for STM32 SoCs" if !ARCH_STM32
-	depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
+	bool "Clocksource for STM32 SoCs" if COMPILE_TEST
+	depends on OF && GENERIC_CLOCKEVENTS
 	select CLKSRC_MMIO
 
 config ARM_ARCH_TIMER
-- 
1.9.1

[PATCH 2/2] clockevents/drivers/sh_cmt: Only perform clocksource suspend/resume if enabled

[Daniel Lezcano]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Currently the sh_cmt clocksource timer is disabled or enabled
unconditionally on clocksource suspend resp. resume, even if a better
clocksource is present (e.g. arch_sys_counter) and the sh_cmt
clocksource is not enabled.

As sh_cmt is a syscore device when its timer is enabled, this may lead
to a genpd.prepared_count imbalance in the presence of PM Domains, which
may cause a lock-up during reboot after s2ram.

During suspend:
  - pm_genpd_prepare() is called for all non-syscore devices (incl.
    sh_cmt), increasing genpd.prepared_count for each device,
  - clocksource.suspend() is called for all clocksource devices,
  - sh_cmt_clocksource_suspend() calls sh_cmt_stop(), which is a no-op
    as the clocksource was not enabled.

During resume:
  - clocksource.resume() is called for all clocksource devices,
  - sh_cmt_clocksource_resume() calls sh_cmt_start(), which enables the
    clocksource timer, and turns sh_cmt into a syscore device,
  - pm_genpd_complete() is called for all non-syscore devices (excl.
    sh_cmt now!), decreasing genpd.prepared_count for each device but
    sh_cmt.

Now genpd.prepared_count of the PM Domain containing sh_cmt is still 1
instead of zero.  On subsequent suspend/resume cycles, sh_cmt is still a
syscore device, hence it's skipped for pm_genpd_{prepare,complete}(),
keeping the imbalance of genpd.prepared_count at 1.

During reboot:
  - platform_drv_shutdown() is called for any platform device that has
    a driver with a .shutdown() method (only rcar-dmac on R-Car Gen2),
  - platform_drv_shutdown() calls dev_pm_domain_detach(), which
    calls genpd_dev_pm_detach(),
  - genpd_dev_pm_detach() keeps calling pm_genpd_remove_device() until
    it doesn't return -EAGAIN[*],
  - If the device is part of the same PM Domain as sh_cmt,
    pm_genpd_remove_device() always fails with -EAGAIN due to
    genpd.prepared_count > 0.
  - Infinite loop in genpd_dev_pm_detach()[*].

[*] Commit 93af5e9354432828 ("PM / Domains: Avoid infinite loops in
    attach/detach code") already limited the number of loop iterations,
    avoiding the lock-up.

To fix this, only disable or enable the clocksource timer on clocksource
suspend resp. resume if the clocksource was enabled.

This was tested on r8a7791/koelsch with the CPG Clock Domain:
  - using arch_sys_counter as the clocksource, which is the default, and
    which showed the problem,
  - using sh_cmt as a clocksource ("echo ffca0000.timer > \
    /sys/devices/system/clocksource/clocksource0/current_clocksource"),
    which behaves the same as before.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/sh_cmt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index b8ff3c6..c96de14 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -661,6 +661,9 @@ static void sh_cmt_clocksource_suspend(struct clocksource *cs)
 {
 	struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
 
+	if (!ch->cs_enabled)
+		return;
+
 	sh_cmt_stop(ch, FLAG_CLOCKSOURCE);
 	pm_genpd_syscore_poweroff(&ch->cmt->pdev->dev);
 }
@@ -669,6 +672,9 @@ static void sh_cmt_clocksource_resume(struct clocksource *cs)
 {
 	struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
 
+	if (!ch->cs_enabled)
+		return;
+
 	pm_genpd_syscore_poweron(&ch->cmt->pdev->dev);
 	sh_cmt_start(ch, FLAG_CLOCKSOURCE);
 }
-- 
1.9.1

Re: [PULL] clockevents: a couple of fixes for tip/timers/urgent

[Ingo Molnar]

* Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> 
> Hi Ingo,
> 
> this pull request contains the following changes:
> 
>  - Prevented to suspend or resume the sh_cmt clocksource when this one is
> not enabled (Geert Uytterhoeven)
>  - Improved build coverage with COMPILE_TEST for the stm32 timer (Maxime
> Coquelin)
> 
> Thanks !
> 
>   -- Daniel
> 
> The following changes since commit 0f44705175347ec96935d60b765b5d14ecc763bb:
> 
>   tick: Move the export of tick_broadcast_oneshot_control to the proper
> place (2015-07-14 12:01:04 +0200)
> 
> are available in the git repository at:
> 
>   http://git.linaro.org/people/daniel.lezcano/linux.git
> clockevents/4.2-fixes
> 
> for you to fetch changes up to 1822a48be12f7ce2799d3423e49b967896794c4d:
> 
>   clockevents/drivers/sh_cmt: Only perform clocksource suspend/resume if
> enabled (2015-08-06 17:17:20 +0200)
> 
> ----------------------------------------------------------------
> Geert Uytterhoeven (1):
>       clockevents/drivers/sh_cmt: Only perform clocksource suspend/resume if
> enabled
> 
> Maxime Coquelin (1):
>       clockevents/drivers/timer-stm32: Improve dependencies of timer-stm32
> 
>  drivers/clocksource/Kconfig  | 4 ++--

Hm, so why is this an urgent fix, i.e. a regression fix? It appears to widen build 
coverage:

 config CLKSRC_STM32
-       bool "Clocksource for STM32 SoCs" if !ARCH_STM32
-       depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
+       bool "Clocksource for STM32 SoCs" if COMPILE_TEST
+       depends on OF && GENERIC_CLOCKEVENTS

that can at most introduce build regressions - not fix anything.

If there's any other purpose of this change it's not mentioned in the changelog.

Also, I think the change is broken to begin with:

+       bool "Clocksource for STM32 SoCs" if COMPILE_TEST

this will turn off the driver even in its target platform!

The right way to increase testing this way is via:

	depends on ... || COMPILE_TEST

Thanks,

	Ingo

Re: [PULL] clockevents: a couple of fixes for tip/timers/urgent

[Geert Uytterhoeven]

On Fri, Aug 7, 2015 at 9:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> Maxime Coquelin (1):
>>       clockevents/drivers/timer-stm32: Improve dependencies of timer-stm32
>>
>>  drivers/clocksource/Kconfig  | 4 ++--
>
> Hm, so why is this an urgent fix, i.e. a regression fix? It appears to widen build
> coverage:
>
>  config CLKSRC_STM32
> -       bool "Clocksource for STM32 SoCs" if !ARCH_STM32
> -       depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
> +       bool "Clocksource for STM32 SoCs" if COMPILE_TEST
> +       depends on OF && GENERIC_CLOCKEVENTS
>
> that can at most introduce build regressions - not fix anything.
>
> If there's any other purpose of this change it's not mentioned in the changelog.
>
> Also, I think the change is broken to begin with:
>
> +       bool "Clocksource for STM32 SoCs" if COMPILE_TEST
>
> this will turn off the driver even in its target platform!

FWIW, ARCH_STM32 in arch/arm/Kconfig selects CLKSRC_STM32

> The right way to increase testing this way is via:
>
>         depends on ... || COMPILE_TEST

Indeed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[tip:timers/urgent] clockevents/drivers/sh_cmt: Only perform clocksource suspend/resume if enabled

[tip-bot for Geert Uytterhoeven]

Commit-ID:  54d46b7fbcbd00fe4b20a27208e5909facc714e3
Gitweb:     http://git.kernel.org/tip/54d46b7fbcbd00fe4b20a27208e5909facc714e3
Author:     Geert Uytterhoeven <geert+renesas@glider.be>
AuthorDate: Thu, 6 Aug 2015 17:32:06 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 8 Aug 2015 10:50:08 +0200

clockevents/drivers/sh_cmt: Only perform clocksource suspend/resume if enabled

Currently the sh_cmt clocksource timer is disabled or enabled
unconditionally on clocksource suspend resp. resume, even if a
better clocksource is present (e.g. arch_sys_counter) and the
sh_cmt clocksource is not enabled.

As sh_cmt is a syscore device when its timer is enabled, this
may lead to a genpd.prepared_count imbalance in the presence of
PM Domains, which may cause a lock-up during reboot after s2ram.

During suspend:
  - pm_genpd_prepare() is called for all non-syscore devices (incl.
    sh_cmt), increasing genpd.prepared_count for each device,
  - clocksource.suspend() is called for all clocksource devices,
  - sh_cmt_clocksource_suspend() calls sh_cmt_stop(), which is a no-op
    as the clocksource was not enabled.

During resume:
  - clocksource.resume() is called for all clocksource devices,
  - sh_cmt_clocksource_resume() calls sh_cmt_start(), which enables the
    clocksource timer, and turns sh_cmt into a syscore device,
  - pm_genpd_complete() is called for all non-syscore devices (excl.
    sh_cmt now!), decreasing genpd.prepared_count for each device but
    sh_cmt.

Now genpd.prepared_count of the PM Domain containing sh_cmt is
still 1 instead of zero.  On subsequent suspend/resume cycles,
sh_cmt is still a syscore device, hence it's skipped for
pm_genpd_{prepare,complete}(), keeping the imbalance of
genpd.prepared_count at 1.

During reboot:

  - platform_drv_shutdown() is called for any platform device that has
    a driver with a .shutdown() method (only rcar-dmac on R-Car Gen2),

  - platform_drv_shutdown() calls dev_pm_domain_detach(), which
    calls genpd_dev_pm_detach(),

  - genpd_dev_pm_detach() keeps calling pm_genpd_remove_device() until
    it doesn't return -EAGAIN[*],

  - If the device is part of the same PM Domain as sh_cmt,
    pm_genpd_remove_device() always fails with -EAGAIN due to
    genpd.prepared_count > 0.

  - Infinite loop in genpd_dev_pm_detach()[*].

[*] Commit 93af5e9354432828 ("PM / Domains: Avoid infinite loops in
    attach/detach code") already limited the number of loop iterations,
    avoiding the lock-up.

To fix this, only disable or enable the clocksource timer on
clocksource suspend resp. resume if the clocksource was enabled.

This was tested on r8a7791/koelsch with the CPG Clock Domain:

  - using arch_sys_counter as the clocksource, which is the default, and
    which showed the problem,

  - using sh_cmt as a clocksource ("echo ffca0000.timer > \
    /sys/devices/system/clocksource/clocksource0/current_clocksource"),
    which behaves the same as before.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1438875126-12596-2-git-send-email-daniel.lezcano@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/clocksource/sh_cmt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index b8ff3c6..c96de14 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -661,6 +661,9 @@ static void sh_cmt_clocksource_suspend(struct clocksource *cs)
 {
 	struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
 
+	if (!ch->cs_enabled)
+		return;
+
 	sh_cmt_stop(ch, FLAG_CLOCKSOURCE);
 	pm_genpd_syscore_poweroff(&ch->cmt->pdev->dev);
 }
@@ -669,6 +672,9 @@ static void sh_cmt_clocksource_resume(struct clocksource *cs)
 {
 	struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
 
+	if (!ch->cs_enabled)
+		return;
+
 	pm_genpd_syscore_poweron(&ch->cmt->pdev->dev);
 	sh_cmt_start(ch, FLAG_CLOCKSOURCE);
 }