[PATCH] PM / Domains: Fix initial default state of the need_restore flag

[Dmitry Torokhov]

On Fri, Nov 07, 2014 at 11:47:53AM -0800, Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson at linaro.org> 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
> > [<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
> > [<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
> > [<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
> > [<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
> > [<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
> > [<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
> > [<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
> > [<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
> > [<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
> > [<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
> > [<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
> > [<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
> > [<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
> > [<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
> > [<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
> > [<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
> > [<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
> > [<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
> > ---[ end trace 40cd58bcd6988f12 ]---
> >
> > Fixes: a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)
> > Reported-by: Sylwester Nawrocki <s.nawrocki at samsung.com>
> > Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
> 
> Reviewed-by: Kevin Hilman <khilman at linaro.org>
> 
> And looks like we need this for v3.18-rc since it does fix the
> regression.

I guess we do need it for 3.18, but when we are talking about 3.19,
before we make any more changes can we outline how power domains are
supposed to work?

1. How do we attach a device to power domain? Right now it seems that
individual buses are responsible for attaching their devices to power
domains. Should we move it into driver core (device_pm_add?) so that
device starts its life in its proper power domain?

2. When do we power up the devices (and the domains)? Right now devices
in ACPI power domain are powered when they are attached to the power
domain (which coincides with probing), but generic power domains do not
do that. Can we add a separate API to explicitly power up the device (and
its domain if it is powered down) and do it again, either in device core
or in individual buses. This way, regardless of runtime PM or not, we
will have devices powered on when driver tries to bind to them. If
binding fails we can power down the device.

I've heard there is a concern about power domain bouncing up and down
during probing. Is it really a concern? Would not we run into the same
issues past booting up with aggressive PM policy? Anyway, we could
probably work around this by refusing to actually power down the domains
until we are done booting, similarly to who genpd_poweroff_unused works.

Thanks.

-- 
Dmitry