From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH] PM / Domains: Fix initial default state of the need_restore flag Date: Fri, 07 Nov 2014 11:47:53 -0800 Message-ID: <7hr3xerdw6.fsf@deeprootsystems.com> References: <1415366847-4807-1-git-send-email-ulf.hansson@linaro.org> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: <1415366847-4807-1-git-send-email-ulf.hansson@linaro.org> (Ulf Hansson's message of "Fri, 7 Nov 2014 14:27:27 +0100") Sender: linux-samsung-soc-owner@vger.kernel.org To: Ulf Hansson Cc: "Rafael J. Wysocki" , Len Brown , Pavel Machek , linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, Geert Uytterhoeven , Alan Stern , Greg Kroah-Hartman , Tomasz Figa , Simon Horman , Magnus Damm , Ben Dooks , Kukjin Kim , Philipp Zabel , Mark Brown , Wolfram Sang , Russell King , Dmitry Torokhov , Jack Dai , Jinkun Hong , Aaron Lu , Sylwester Nawrocki List-Id: linux-pm@vger.kernel.org Ulf Hansson writes: > The initial state of the device's need_restore flag should'nt depend on > the current state of the PM domain. For example it should be perfectly > valid to attach an inactive device to a powered PM domain. > > The pm_genpd_dev_need_restore() API allow us to update the need_restore > flag to somewhat cope with such scenarios. Typically that should have > been done from drivers/buses ->probe() since it's those that put the > requirements on the value of the need_restore flag. > > Until recently, the Exynos SOCs were the only user of the > pm_genpd_dev_need_restore() API, though invoking it from a centralized > location while adding devices to their PM domains. > > Due to that Exynos now have swithed to the generic OF-based PM domain > look-up, it's no longer possible to invoke the API from a centralized > location. The reason is because devices are now added to their PM > domains during the probe sequence. > > Commit "ARM: exynos: Move to generic PM domain DT bindings" > did the switch for Exynos to the generic OF-based PM domain look-up, > but it also removed the call to pm_genpd_dev_need_restore(). This > caused a regression for some of the Exynos drivers. > > To handle things more properly in the generic PM domain, let's change > the default initial value of the need_restore flag to reflect that the > state is unknown. As soon as some of the runtime PM callbacks gets > invoked, update the initial value accordingly. > > Moreover, since the generic PM domain is verifying that all device's > are both runtime PM enabled and suspended, using pm_runtime_suspended() > while pm_genpd_poweroff() is invoked from the scheduled work, we can be > sure of that the PM domain won't be powering off while having active > devices. > > Do note that, the generic PM domain can still only know about active > devices which has been activated through invoking its runtime PM resume > callback. In other words, buses/drivers using pm_runtime_set_active() > during ->probe() will still suffer from a race condition, potentially > probing a device without having its PM domain being powered. That issue > will have to be solved using a different approach. > > This a log from the boot regression for Exynos5, which is being fixed in > this patch. > > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30() > Modules linked in: > CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10 > Workqueue: pm pm_runtime_work > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x70/0xbc) > [] (dump_stack) from [] (warn_slowpath_common+0x64/0x88) > [] (warn_slowpath_common) from [] (warn_slowpath_null+0x1c/0x24) > [] (warn_slowpath_null) from [] (clk_disable+0x24/0x30) > [] (clk_disable) from [] (gsc_runtime_suspend+0x128/0x160) > [] (gsc_runtime_suspend) from [] (pm_generic_runtime_suspend+0x2c/0x38) > [] (pm_generic_runtime_suspend) from [] (pm_genpd_default_save_state+0x2c/0x8c) > [] (pm_genpd_default_save_state) from [] (pm_genpd_poweroff+0x224/0x3ec) > [] (pm_genpd_poweroff) from [] (pm_genpd_runtime_suspend+0x9c/0xcc) > [] (pm_genpd_runtime_suspend) from [] (__rpm_callback+0x2c/0x60) > [] (__rpm_callback) from [] (rpm_callback+0x20/0x74) > [] (rpm_callback) from [] (rpm_suspend+0xd4/0x43c) > [] (rpm_suspend) from [] (pm_runtime_work+0x80/0x90) > [] (pm_runtime_work) from [] (process_one_work+0x12c/0x314) > [] (process_one_work) from [] (worker_thread+0x3c/0x4b0) > [] (worker_thread) from [] (kthread+0xcc/0xe8) > [] (kthread) from [] (ret_from_fork+0x14/0x3c) > ---[ end trace 40cd58bcd6988f12 ]--- > > Fixes: a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings) > Reported-by: Sylwester Nawrocki > Signed-off-by: Ulf Hansson Reviewed-by: Kevin Hilman And looks like we need this for v3.18-rc since it does fix the regression. A minor nit: you add a few new multi-line comment blocks which should have a new empty line before them, but currently they're right up against the previous code. Please add a blank line for separation and to be consisitent with the rest of the file. Thanks, Kevin