From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Geert Uytterhoeven <geert+renesas@glider.be>, linux-pm@vger.kernel.org
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
Thomas Gleixner <tglx@linutronix.de>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Kevin Hilman <khilman@kernel.org>,
Ulf Hansson <ulf.hansson@linaro.org>,
Magnus Damm <damm@opensource.se>,
linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] clocksource: sh_cmt: Only perform clocksource suspend/resume if enabled
Date: Thu, 18 Jun 2015 17:14:29 +0300 [thread overview]
Message-ID: <1829815.c8lIHLtv9K@avalon> (raw)
In-Reply-To: <1434622954-26747-2-git-send-email-geert+renesas@glider.be>
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
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Geert Uytterhoeven <geert+renesas@glider.be>, linux-pm@vger.kernel.org
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
Thomas Gleixner <tglx@linutronix.de>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Kevin Hilman <khilman@kernel.org>,
Ulf Hansson <ulf.hansson@linaro.org>,
Magnus Damm <damm@opensource.se>,
linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] clocksource: sh_cmt: Only perform clocksource suspend/resume if enabled
Date: Thu, 18 Jun 2015 14:14:29 +0000 [thread overview]
Message-ID: <1829815.c8lIHLtv9K@avalon> (raw)
In-Reply-To: <1434622954-26747-2-git-send-email-geert+renesas@glider.be>
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
next prev parent reply other threads:[~2015-06-18 14:13 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-18 10:22 [PATCH 0/2] PM / Domains: Infinite loop during reboot Geert Uytterhoeven
2015-06-18 10:22 ` Geert Uytterhoeven
2015-06-18 10:22 ` [PATCH 1/2] clocksource: sh_cmt: Only perform clocksource suspend/resume if enabled Geert Uytterhoeven
2015-06-18 10:22 ` Geert Uytterhoeven
2015-06-18 14:14 ` Laurent Pinchart [this message]
2015-06-18 14:14 ` Laurent Pinchart
2015-06-18 14:19 ` Geert Uytterhoeven
2015-06-18 14:19 ` Geert Uytterhoeven
2015-06-18 10:22 ` [PATCH 2/2] PM / Domains: Avoid infinite loops in attach/detach code Geert Uytterhoeven
2015-06-18 10:22 ` Geert Uytterhoeven
2015-06-22 7:30 ` Geert Uytterhoeven
2015-06-22 7:30 ` Geert Uytterhoeven
2015-06-22 7:30 ` Geert Uytterhoeven
2015-06-22 7:31 ` Geert Uytterhoeven
2015-06-22 7:31 ` Geert Uytterhoeven
2015-06-22 7:31 ` Geert Uytterhoeven
2015-06-22 23:41 ` Rafael J. Wysocki
2015-06-22 23:41 ` Rafael J. Wysocki
2015-06-22 23:41 ` Rafael J. Wysocki
2015-06-23 7:16 ` Geert Uytterhoeven
2015-06-23 7:16 ` Geert Uytterhoeven
2015-06-23 7:16 ` Geert Uytterhoeven
2015-06-23 12:50 ` Ulf Hansson
2015-06-23 12:50 ` Ulf Hansson
2015-06-23 13:20 ` Geert Uytterhoeven
2015-06-23 13:20 ` Geert Uytterhoeven
2015-06-23 13:38 ` Rafael J. Wysocki
2015-06-23 13:38 ` Rafael J. Wysocki
2015-06-23 13:45 ` Geert Uytterhoeven
2015-06-23 13:45 ` Geert Uytterhoeven
2015-06-24 8:33 ` Ulf Hansson
2015-06-24 8:33 ` Ulf Hansson
2015-06-24 8:35 ` Geert Uytterhoeven
2015-06-24 8:35 ` Geert Uytterhoeven
2015-06-24 13:48 ` Rafael J. Wysocki
2015-06-24 13:48 ` Rafael J. Wysocki
2015-06-24 13:44 ` Rafael J. Wysocki
2015-06-24 14:10 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1829815.c8lIHLtv9K@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=damm@opensource.se \
--cc=daniel.lezcano@linaro.org \
--cc=geert+renesas@glider.be \
--cc=khilman@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=tglx@linutronix.de \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.