[PATCH 0/2] PM / Domains: Infinite loop during reboot

[PATCH 0/2] PM / Domains: Infinite loop during reboot

[Geert Uytterhoeven]

	Hi all,

This patch series fixes an infinite loop during reboot after s2ram, as
seen on r8a7791/koelsch with the CPG Clock Domain:
  - Patch 1 fixes the cause in the sh_cmt driver,
  - Patch 2 hardens the PM Domain core code against infinite loops.

Both patches individually fix the symptom (lock up during reboot), but
only the first one fixes the root cause.
Patches are against next-20150618.

The discussion thread where the problem was originally reported is "PM /
Domains: Infinite loop during reboot"
(http://permalink.gmane.org/gmane.linux.ports.sh.devel/45249).

Thanks!

Geert Uytterhoeven (2):
  clocksource: sh_cmt: Only perform clocksource suspend/resume if
    enabled
  PM / Domains: Avoid infinite loops in attach/detach code

 drivers/base/power/domain.c  | 15 +++++++++++++--
 drivers/clocksource/sh_cmt.c |  6 ++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

-- 
1.9.1

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

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

[Geert Uytterhoeven]

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().

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>
---
 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 b8ff3c64cc452a16..c96de14036a0adeb 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: [PATCH 1/2] clocksource: sh_cmt: Only perform clocksource suspend/resume if enabled

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 18 June 2015 12:22:33 Geert Uytterhoeven wrote:
> 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().
> 
> 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>

Good catch. The patch looks good to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I've quickly checked the MTU2 and TMU timer drivers and they should be immune 
to the issue, but I'd appreciate if you could confirm that.

> ---
>  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 b8ff3c64cc452a16..c96de14036a0adeb 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);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] clocksource: sh_cmt: Only perform clocksource suspend/resume if enabled

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Jun 18, 2015 at 4:14 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 18 June 2015 12:22:33 Geert Uytterhoeven wrote:
>> 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.

>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Good catch. The patch looks good to me.

While the solution was simple, it was hard to catch (engineers and
lightbulbs... euh timers ;-)

> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

> I've quickly checked the MTU2 and TMU timer drivers and they should be immune
> to the issue, but I'd appreciate if you could confirm that.

I had concluded the same, thanks for verifying!

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