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

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@samsung.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

This patch is intended as fix for 3.18 rc[n] due to the regression for Exynos
SOCs.

I would also like to call for help in getting this thoroughly tested.

I have tested this on Arndale Dual, Exynos 5250. According the log attached in
the commit message as well.

I have tested this on UX500, which support for the generic PM domain is about
to be queued for 3.19. Since UX500 uses the AMBA bus/drivers, which uses
pm_runtime_set_active() instead of pm_runtime_get_sync() during ->probe(), I
could also verify that this behavior is maintained.

Finally I have also tested this patchset on UX500 and using the below patchset
to make sure the approach is suitable long term wise as well.
[PATCH v3 0/9] PM / Domains: Fix race conditions during boot
http://www.spinics.net/lists/arm-kernel/msg369409.html

---
 drivers/base/power/domain.c | 38 +++++++++++++++++++++++++++++++-------
 include/linux/pm_domain.h   |  2 +-
 2 files changed, 32 insertions(+), 8 deletions(-)

On 07/11/14 14:27, Ulf Hansson wrote:
[...]
> 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@samsung.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> This patch is intended as fix for 3.18 rc[n] due to the regression for Exynos
> SOCs.
> 
> I would also like to call for help in getting this thoroughly tested.
> 
> I have tested this on Arndale Dual, Exynos 5250. According the log attached in
> the commit message as well.

Thanks Ulf, I've tested this patch on top of v3.18-rc3 on Exynos4412 Trats2
board and it seems the unbalanced runtime_suspend() call issue is gone.

> I have tested this on UX500, which support for the generic PM domain is about
> to be queued for 3.19. Since UX500 uses the AMBA bus/drivers, which uses
> pm_runtime_set_active() instead of pm_runtime_get_sync() during ->probe(), I
> could also verify that this behavior is maintained.

I had some issues with such configuration, after modifying the drivers to
call pm_runtime_set_active() (instead of pm_runtime_get_sync()) in probe().
It looked like there is some access to the hardware with corresponding
power domain disabled. Couldn't debug it in reasonable time due to system
hanging, will try to get back to this on Wednesday.

> Finally I have also tested this patchset on UX500 and using the below patchset
> to make sure the approach is suitable long term wise as well.
> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
> http://www.spinics.net/lists/arm-kernel/msg369409.html

And with that additional patch series and the drivers modified everything
seemed to work too.

The patch looks good to me as a fix for v3.18, if others don't report
regressions feel free to add my:

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
(On Exynos4412 Trats2 board)
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Ulf Hansson <ulf.hansson@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@samsung.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Kevin Hilman <khilman@linaro.org>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Nov 07, 2014 at 11:47:53AM -0800, Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson@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@samsung.com>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Reviewed-by: Kevin Hilman <khilman@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 Torokhov <dmitry.torokhov@gmail.com> writes:

> On Fri, Nov 07, 2014 at 11:47:53AM -0800, Kevin Hilman wrote:
>> Ulf Hansson <ulf.hansson@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@samsung.com>
>> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> 
>> Reviewed-by: Kevin Hilman <khilman@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?

Yes, I agree, we have some things to sort out for v3.19, but as this one
is a regression fix, I think it needs to be in v3.18-rc.

Kevin

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Friday, November 07, 2014 02:27:27 PM Ulf Hansson wrote:
> 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@samsung.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> This patch is intended as fix for 3.18 rc[n] due to the regression for Exynos
> SOCs.
> 
> I would also like to call for help in getting this thoroughly tested.
> 
> I have tested this on Arndale Dual, Exynos 5250. According the log attached in
> the commit message as well.
> 
> I have tested this on UX500, which support for the generic PM domain is about
> to be queued for 3.19. Since UX500 uses the AMBA bus/drivers, which uses
> pm_runtime_set_active() instead of pm_runtime_get_sync() during ->probe(), I
> could also verify that this behavior is maintained.
> 
> Finally I have also tested this patchset on UX500 and using the below patchset
> to make sure the approach is suitable long term wise as well.
> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
> http://www.spinics.net/lists/arm-kernel/msg369409.html

I'm generally fine with the approach, but ->

> ---
>  drivers/base/power/domain.c | 38 +++++++++++++++++++++++++++++++-------
>  include/linux/pm_domain.h   |  2 +-
>  2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b520687..a9523a3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -361,9 +361,19 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
>  	struct device *dev = pdd->dev;
>  	int ret = 0;
>  
> -	if (gpd_data->need_restore)
> +	if (gpd_data->need_restore == 1)

-> I'd prefer checks for the sign rather than for a specific value, like

	if (gpd_data->need_restore > 0)

>  		return 0;
>  

and so on.

> +	/*
> +	 * If the value of the need_restore flag is still unknown at this point,
> +	 * we trust that pm_genpd_poweroff() has verified that the device is
> +	 * already runtime PM suspended.
> +	 */
> +	if (gpd_data->need_restore == -1) {
> +		gpd_data->need_restore = 1;
> +		return 0;
> +	}
> +
>  	mutex_unlock(&genpd->lock);
>  
>  	genpd_start_dev(genpd, dev);
> @@ -373,7 +383,7 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
>  	mutex_lock(&genpd->lock);
>  
>  	if (!ret)
> -		gpd_data->need_restore = true;
> +		gpd_data->need_restore = 1;
>  
>  	return ret;
>  }
> @@ -389,13 +399,17 @@ static void __pm_genpd_restore_device(struct pm_domain_data *pdd,
>  {
>  	struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
>  	struct device *dev = pdd->dev;
> -	bool need_restore = gpd_data->need_restore;
> +	int need_restore = gpd_data->need_restore;
>  
> -	gpd_data->need_restore = false;
> +	gpd_data->need_restore = 0;
>  	mutex_unlock(&genpd->lock);
>  
>  	genpd_start_dev(genpd, dev);
> -	if (need_restore)
> +	/*
> +	 * Make sure to also restore those devices which we until now, didn't
> +	 * know the state for.
> +	 */
> +	if (need_restore != 0)

and here you can do

	if (need_restore)

>  		genpd_restore_dev(genpd, dev);
>  
>  	mutex_lock(&genpd->lock);
> @@ -603,6 +617,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>  static int pm_genpd_runtime_suspend(struct device *dev)
>  {
>  	struct generic_pm_domain *genpd;
> +	struct generic_pm_domain_data *gpd_data;
>  	bool (*stop_ok)(struct device *__dev);
>  	int ret;
>  
> @@ -628,6 +643,15 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>  		return 0;
>  
>  	mutex_lock(&genpd->lock);
> +	/*
> +	 * If we have an unknown state of the need_restore flag, it means none
> +	 * of the runtime PM callbacks has been invoked yet. Let's update the
> +	 * flag to reflect that the current state is active.
> +	 */
> +	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> +	if (gpd_data->need_restore == -1)
> +		gpd_data->need_restore = 0;
> +
>  	genpd->in_progress++;
>  	pm_genpd_poweroff(genpd);
>  	genpd->in_progress--;
> @@ -1442,7 +1466,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>  	mutex_lock(&gpd_data->lock);
>  	gpd_data->base.dev = dev;
>  	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
> -	gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
> +	gpd_data->need_restore = -1;
>  	gpd_data->td.constraint_changed = true;
>  	gpd_data->td.effective_constraint_ns = -1;
>  	mutex_unlock(&gpd_data->lock);
> @@ -1546,7 +1570,7 @@ void pm_genpd_dev_need_restore(struct device *dev, bool val)
>  
>  	psd = dev_to_psd(dev);
>  	if (psd && psd->domain_data)
> -		to_gpd_data(psd->domain_data)->need_restore = val;
> +		to_gpd_data(psd->domain_data)->need_restore = val ? 1 : 0;
>  
>  	spin_unlock_irqrestore(&dev->power.lock, flags);
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b3ed776..2e0e06d 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -106,7 +106,7 @@ struct generic_pm_domain_data {
>  	struct notifier_block nb;
>  	struct mutex lock;
>  	unsigned int refcount;
> -	bool need_restore;
> +	int need_restore;
>  };
>  
>  #ifdef CONFIG_PM_GENERIC_DOMAINS
>

[...]

> 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?

That was the initial approach.

To my understanding, Rafael's primary reason for not accepting that
was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2

Now, even if we would reconsider doing as you propose, what would the
actual benefit be? Obviously, we would get less amount of code to
maintain, which is one reason, but are there more?

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

Isn't that exactly what I implemented in [1], what am I missing?

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

I haven't heard about this kind of issues, but I think your approach
seems reasonable.

Currently, I think we should focus on solving the problem in [1].
Those drivers/subsystems that use pm_runtime_set_active() during
->probe() are broken in this regards.

Now, if we move the attachment of devices to their PM domains from
buses into driver core, that could simplify [1]. Still, only if we
also decide to power on PM domains from driver core, else it wouldn't
matter much I think.


[1]
[PATCH v3 0/9] PM / Domains: Fix race conditions during boot
http://marc.info/?t=141320907000003&r=1&w=2

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Nov 10, 2014 at 04:18:50PM +0100, Ulf Hansson wrote:
> [...]
> 
> > 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?
> 
> That was the initial approach.
> 
> To my understanding, Rafael's primary reason for not accepting that
> was that it's not common, but it's platform-specific.
> http://marc.info/?l=linux-pm&m=140243462304669&w=2

I am not sure I agree with Rafael there:

 - when we are talking about the latest incarnation of power domains it
   is not really platform specific anymore (as it was when we were
   dealing with ACPI only case);

- I do not see why sirincling platform specific code around i2c, spi,
  etc bus code (which is not platform-specific) is OK, but a no-no for
  the driver ocre.

> 
> Now, even if we would reconsider doing as you propose, what would the
> actual benefit be? Obviously, we would get less amount of code to
> maintain, which is one reason, but are there more?

I think so - you would have complete picture of your power domain,
including data exposed in debugfs, etc.

> 
> >
> > 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.
> 
> Isn't that exactly what I implemented in [1], what am I missing?

Hm, OK. I guess the title of the patch series thrown me off because as
far as I am concerned it is not a race, we simply were not powering the
domain properly for probing.

Thanks.

On Mon, Nov 10, 2014 at 10:32:24AM -0800, Dmitry Torokhov wrote:

> - I do not see why sirincling platform specific code around i2c, spi,
>   etc bus code (which is not platform-specific) is OK, but a no-no for
>   the driver ocre.

sirincling?  In the case of I2C and SPI the buses don't define any sort
of power management or power domain so any management is going to be
device or system specific.

On November 10, 2014 11:39:31 AM PST, Mark Brown <broonie@kernel.org> wrote:
>On Mon, Nov 10, 2014 at 10:32:24AM -0800, Dmitry Torokhov wrote:
>
>> - I do not see why sirincling platform specific code around i2c, spi,
>>   etc bus code (which is not platform-specific) is OK, but a no-no
>for
>>   the driver ocre.
>
>sirincling?

Sorry, I was going for "sprinkling" but failed spectacularly.


Thanks.

[CC list trimmed]

On Monday, November 10, 2014 04:18:50 PM Ulf Hansson wrote:
> [...]
> 
> > 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?
> 
> That was the initial approach.
> 
> To my understanding, Rafael's primary reason for not accepting that
> was that it's not common, but it's platform-specific.
> http://marc.info/?l=linux-pm&m=140243462304669&w=2

For the record, I still believe this is platform-specific.

I also think that the knowledge about what power (or generally PM) domain
a device should belong to is not in a bus type or in the driver core.  That
knowledge is in the code that enumerates devices.

I wonder, then, if we could set the PM domain pointer at about the time
when we set the bus type pointer?  Things will be consistent all the way
through the entire device life cycle then.

> Now, even if we would reconsider doing as you propose, what would the
> actual benefit be? Obviously, we would get less amount of code to
> maintain, which is one reason, but are there more?

The same set of subsystem-level PM callbacks will be present for the device
throughout its life cycle.

> > 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.
> 
> Isn't that exactly what I implemented in [1], what am I missing?

Not really.  Dmitry is talking about a generic interface independent of
PM domains.

If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it
around really_probe() for all devices.  In theory.  But that's what we
started with when we were working on the runtime PM framework and we ended
up with what we have today.

Problem is, pm_power_up() would probably end up being pretty much the same as
pm_runtime_resume() without executing driver callbacks and similarly for
pm_power_down().  That's why I was thinking about running pm_runtime_resume()
at the time we know that driver callbacks are not present, just for the
purpose of powering up the device.  [That has a problem of working with
CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]

Now, Grygorii seems to be claiming that some drivers *require* their
.runtime_resume() callbacks to be executed during .probe() pretty much
before anything else which won't happen if pm_runtime_resume() is done
before really_probe().  I'm wondering, then, which drivers those are.

Essentially, the "power up" operation will depend on the PM domain and
bus type, so they'll need to provide callbacks that will most likely
duplicate runtime PM callbacks.  And those callbacks need to be available
when we do the "power up" thing.

How can we address that?

We seem to be dealing with a case in which PM is needed on some systems just to
make them work at the basic level and not for energy saving, which was what
runtime PM was designed for.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[...]

>>
>> To my understanding, Rafael's primary reason for not accepting that
>> was that it's not common, but it's platform-specific.
>> http://marc.info/?l=linux-pm&m=140243462304669&w=2
>
> For the record, I still believe this is platform-specific.
>
> I also think that the knowledge about what power (or generally PM) domain
> a device should belong to is not in a bus type or in the driver core.  That
> knowledge is in the code that enumerates devices.
>
> I wonder, then, if we could set the PM domain pointer at about the time
> when we set the bus type pointer?  Things will be consistent all the way
> through the entire device life cycle then.

Could you maybe give some examples of where such code should be
invoked from then?

I do see some difficulties in your suggestion, primarily since we
would need all PM domains to be initialized prior "device
enumeration". That in combination with doing PM domain initialization
from SOC specific code, could be a bit tricky to sort out.

>
>> Now, even if we would reconsider doing as you propose, what would the
>> actual benefit be? Obviously, we would get less amount of code to
>> maintain, which is one reason, but are there more?
>
> The same set of subsystem-level PM callbacks will be present for the device
> throughout its life cycle.

I also do like the consistency this would bring us.

>
>> > 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.
>>
>> Isn't that exactly what I implemented in [1], what am I missing?
>
> Not really.  Dmitry is talking about a generic interface independent of
> PM domains.
>
> If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it
> around really_probe() for all devices.  In theory.  But that's what we
> started with when we were working on the runtime PM framework and we ended
> up with what we have today.
>
> Problem is, pm_power_up() would probably end up being pretty much the same as
> pm_runtime_resume() without executing driver callbacks and similarly for
> pm_power_down().  That's why I was thinking about running pm_runtime_resume()
> at the time we know that driver callbacks are not present, just for the
> purpose of powering up the device.  [That has a problem of working with
> CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]

I take this as we are aligned on using the runtime PM API from the
driver core isn't a solution to the problem, right?

I think Alan in the other thread [1] also had some valuable points to
why we shouldn't use runtime PM from the driver core to power on PM
domains.

To that I would also like to add, that I really don't think the driver
core should enable runtime PM, that's up to each an every
subsystem/driver to decide when/if to do. Using pm_runtime_resume() or
any of the pm_runtime_get*() API from the driver core would have
required that as well.

>
> Now, Grygorii seems to be claiming that some drivers *require* their
> .runtime_resume() callbacks to be executed during .probe() pretty much
> before anything else which won't happen if pm_runtime_resume() is done
> before really_probe().  I'm wondering, then, which drivers those are.

I have looked around and there are certainly hole bunch of such drivers.

I guess the most important reason to why these drivers behave as
stated, is because that's been the common solution to make sure the PM
domain stays powered during probe. It's unfortunate that no one cared
about this until now.

Still, I am optimistic. :-) We should be able to cope with these
drivers as long as we don't use runtime PM to power on the PM domain
during probe.

>
> Essentially, the "power up" operation will depend on the PM domain and
> bus type, so they'll need to provide callbacks that will most likely
> duplicate runtime PM callbacks.  And those callbacks need to be available
> when we do the "power up" thing.

In principle what you suggest me to do is this, right?

1. Re-design how the device gets attached to its PM domain. That will
also change the ACPI PM domain, which I have limited knowledge about.
:-) In principle we want the PM domain pointer to be assigned to the
device at device enumeration. This is needed since you think we should
do the "power up" thing from the driver core.

2. Add "pm_power_up|down()" APIs with corresponding ->"power
up|down"() callbacks in the struct dev_pm_domain. Similar as done in
patch "[PATCH v3 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs"
[2]. Since the callbacks will be unique per PM domain those will be
assigned at PM domain initialization.

3. Invoke the new APIs, pm_power_up|down() around really_probe() in driver core.


So to summarize, without the requirement of doing "pm_power_up|down()"
from driver core, and instead leave that to be done by the buses, we
would be back to the solution I proposed in "[PATCH v3 0/9] PM /
Domains: Fix race conditions during boot" [3].

>
> How can we address that?
>
> We seem to be dealing with a case in which PM is needed on some systems just to
> make them work at the basic level and not for energy saving, which was what
> runtime PM was designed for.
>
> Rafael
>

Kind regards
Uffe

[1]
http://marc.info/?l=linux-pm&m=141511953314249&w=2
[2]
http://marc.info/?l=linux-pm&m=141320895322708&w=2
[3]
http://marc.info/?t=141320907000003&r=1&w=2
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 11/13/2014 04:52 AM, Rafael J. Wysocki wrote:
> [CC list trimmed]
> 
> On Monday, November 10, 2014 04:18:50 PM Ulf Hansson wrote:
>> [...]
>>
>>> 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?
>>
>> That was the initial approach.
>>
>> To my understanding, Rafael's primary reason for not accepting that
>> was that it's not common, but it's platform-specific.
>> http://marc.info/?l=linux-pm&m=140243462304669&w=2
> 
> For the record, I still believe this is platform-specific.
> 
> I also think that the knowledge about what power (or generally PM) domain
> a device should belong to is not in a bus type or in the driver core.  That
> knowledge is in the code that enumerates devices.
> 
> I wonder, then, if we could set the PM domain pointer at about the time
> when we set the bus type pointer?  Things will be consistent all the way
> through the entire device life cycle then.
> 
>> Now, even if we would reconsider doing as you propose, what would the
>> actual benefit be? Obviously, we would get less amount of code to
>> maintain, which is one reason, but are there more?
> 
> The same set of subsystem-level PM callbacks will be present for the device
> throughout its life cycle.
> 
>>> 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.
>>
>> Isn't that exactly what I implemented in [1], what am I missing?
> 
> Not really.  Dmitry is talking about a generic interface independent of
> PM domains.
> 
> If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it
> around really_probe() for all devices.  In theory.  But that's what we
> started with when we were working on the runtime PM framework and we ended
> up with what we have today.
> 
> Problem is, pm_power_up() would probably end up being pretty much the same as
> pm_runtime_resume() without executing driver callbacks and similarly for
> pm_power_down().  That's why I was thinking about running pm_runtime_resume()
> at the time we know that driver callbacks are not present, just for the
> purpose of powering up the device.  [That has a problem of working with
> CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]
> 
> Now, Grygorii seems to be claiming that some drivers *require* their
> .runtime_resume() callbacks to be executed during .probe() pretty much
> before anything else which won't happen if pm_runtime_resume() is done
> before really_probe().  I'm wondering, then, which drivers those are.

I've checked few folders and below few drivers i found which rely or may rely on their
.runtime_resume callback to be executed (It very hard to identify dependency for some of drivers): 
drivers/mfd/omap-usb-host.c
arizona-core.c

gpio/gpio-omap.c

video/fbdev/s3c-fb.c  sh_mobile_lcdcfb.c

omap2/dss/dss.c dsi.c dispc.c rfbi.c venc.c


> 
> Essentially, the "power up" operation will depend on the PM domain and
> bus type, so they'll need to provide callbacks that will most likely
> duplicate runtime PM callbacks.  And those callbacks need to be available
> when we do the "power up" thing.
> 
> How can we address that?
> 
> We seem to be dealing with a case in which PM is needed on some systems just to
> make them work at the basic level and not for energy saving, which was what
> runtime PM was designed for.
> 


Regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Nov 13, 2014 at 07:50:51PM +0200, Grygorii Strashko wrote:

> I've checked few folders and below few drivers i found which rely or may rely on their
> .runtime_resume callback to be executed (It very hard to identify dependency for some of drivers): 

> arizona-core.c

No, it doesn't - probe() leaves the device powered (you can see the
driver interacting with the device before it enables runtime PM).

On 11/13/2014 07:54 PM, Mark Brown wrote:
> On Thu, Nov 13, 2014 at 07:50:51PM +0200, Grygorii Strashko wrote:
> 
>> I've checked few folders and below few drivers i found which rely or may rely on their
>> .runtime_resume callback to be executed (It very hard to identify dependency for some of drivers):
> 
>> arizona-core.c
> 
> No, it doesn't - probe() leaves the device powered (you can see the
> driver interacting with the device before it enables runtime PM).
> 

What I've found there is:
arizona_dev_init()
 |- arizona_clk32k_enable()
    |-pm_runtime_get_sync() if arizona->pdata.clk32k_src == ARIZONA_32KZ_MCLK1

so it's added as "may rely"

regards,
-grygorii

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Nov 13, 2014 at 09:07:43PM +0200, Grygorii Strashko wrote:
> On 11/13/2014 07:54 PM, Mark Brown wrote:

> > No, it doesn't - probe() leaves the device powered (you can see the
> > driver interacting with the device before it enables runtime PM).

> What I've found there is:
> arizona_dev_init()
>  |- arizona_clk32k_enable()
>     |-pm_runtime_get_sync() if arizona->pdata.clk32k_src == ARIZONA_32KZ_MCLK1

> so it's added as "may rely"

No, that's adding a reference to stop it power down under runtime PM -
if runtime PM is inactive it will have no effect.

On 11/13/2014 06:40 PM, Ulf Hansson wrote:
> [...]
>
>>>
>>> To my understanding, Rafael's primary reason for not accepting that
>>> was that it's not common, but it's platform-specific.
>>> http://marc.info/?l=linux-pm&m=140243462304669&w=2
>>
>> For the record, I still believe this is platform-specific.
>>
>> I also think that the knowledge about what power (or generally PM) domain
>> a device should belong to is not in a bus type or in the driver core.  That
>> knowledge is in the code that enumerates devices.
>>
>> I wonder, then, if we could set the PM domain pointer at about the time
>> when we set the bus type pointer?  Things will be consistent all the way
>> through the entire device life cycle then.
>
> Could you maybe give some examples of where such code should be
> invoked from then?
>
> I do see some difficulties in your suggestion, primarily since we
> would need all PM domains to be initialized prior "device
> enumeration". That in combination with doing PM domain initialization
> from SOC specific code, could be a bit tricky to sort out.
>
>>
>>> Now, even if we would reconsider doing as you propose, what would the
>>> actual benefit be? Obviously, we would get less amount of code to
>>> maintain, which is one reason, but are there more?
>>
>> The same set of subsystem-level PM callbacks will be present for the device
>> throughout its life cycle.
>
> I also do like the consistency this would bring us.
>
>>
>>>> 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.
>>>
>>> Isn't that exactly what I implemented in [1], what am I missing?
>>
>> Not really.  Dmitry is talking about a generic interface independent of
>> PM domains.
>>
>> If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it
>> around really_probe() for all devices.  In theory.  But that's what we
>> started with when we were working on the runtime PM framework and we ended
>> up with what we have today.
>>
>> Problem is, pm_power_up() would probably end up being pretty much the same as
>> pm_runtime_resume() without executing driver callbacks and similarly for
>> pm_power_down().  That's why I was thinking about running pm_runtime_resume()
>> at the time we know that driver callbacks are not present, just for the
>> purpose of powering up the device.  [That has a problem of working with
>> CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]
>
> I take this as we are aligned on using the runtime PM API from the
> driver core isn't a solution to the problem, right?
>
> I think Alan in the other thread [1] also had some valuable points to
> why we shouldn't use runtime PM from the driver core to power on PM
> domains.
>
> To that I would also like to add, that I really don't think the driver
> core should enable runtime PM, that's up to each an every
> subsystem/driver to decide when/if to do. Using pm_runtime_resume() or
> any of the pm_runtime_get*() API from the driver core would have
> required that as well.
>
>>
>> Now, Grygorii seems to be claiming that some drivers *require* their
>> .runtime_resume() callbacks to be executed during .probe() pretty much
>> before anything else which won't happen if pm_runtime_resume() is done
>> before really_probe().  I'm wondering, then, which drivers those are.
>
> I have looked around and there are certainly hole bunch of such drivers.
>
> I guess the most important reason to why these drivers behave as
> stated, is because that's been the common solution to make sure the PM
> domain stays powered during probe. It's unfortunate that no one cared
> about this until now.

PM domain is smth. relatively new :) Initial intention is to
execute Bus's PM callbacks and wake up parent devices.

regards,
-grygorii


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mark,

On 11/13/2014 09:11 PM, Mark Brown wrote:
> On Thu, Nov 13, 2014 at 09:07:43PM +0200, Grygorii Strashko wrote:
>> On 11/13/2014 07:54 PM, Mark Brown wrote:
> 
>>> No, it doesn't - probe() leaves the device powered (you can see the
>>> driver interacting with the device before it enables runtime PM).
> 
>> What I've found there is:
>> arizona_dev_init()
>>   |- arizona_clk32k_enable()
>>      |-pm_runtime_get_sync() if arizona->pdata.clk32k_src == ARIZONA_32KZ_MCLK1
            |-arizona_runtime_resume() if dev->driver.pm == arizona_pm_ops 

> 
>> so it's added as "may rely"
> 
> No, that's adding a reference to stop it power down under runtime PM -
> if runtime PM is inactive it will have no effect.
> 
NP. You know better - it's not about something wrong. It's just small
list of drivers where .runtime_resume() is called or may be called from
.probe() (just as an example) :)

regards,
-grygorii



--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[...]

>> Essentially, the "power up" operation will depend on the PM domain and
>> bus type, so they'll need to provide callbacks that will most likely
>> duplicate runtime PM callbacks.  And those callbacks need to be available
>> when we do the "power up" thing.
>
> In principle what you suggest me to do is this, right?
>
> 1. Re-design how the device gets attached to its PM domain. That will
> also change the ACPI PM domain, which I have limited knowledge about.
> :-) In principle we want the PM domain pointer to be assigned to the
> device at device enumeration. This is needed since you think we should
> do the "power up" thing from the driver core.
>
> 2. Add "pm_power_up|down()" APIs with corresponding ->"power
> up|down"() callbacks in the struct dev_pm_domain. Similar as done in
> patch "[PATCH v3 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs"
> [2]. Since the callbacks will be unique per PM domain those will be
> assigned at PM domain initialization.
>
> 3. Invoke the new APIs, pm_power_up|down() around really_probe() in driver core.
>

Answering my own email, I guess am starting to be paranoid. :-)

I assume that the above solution is close to what you had in mind. If
not, please just ignore this response.

As I stated earlier, the 1) could be a bit tricky to sort out.
Especially since there are SOC specific patchsets that's been queued
for 3.19 already which adds genpd support. I am not saying that we
can't do it, but I am bit concerned that we wont solve the _real_
problem before the issues really starts to show up for several SOCs.

Therefore, I wonder if you could consider the following somewhat
intermediate step, instead of going directly towards 1):

- Move the calls to dev_pm_domain_attach/detach() from the various
buses ->probe(), into the driver core (really_probe()). In that way,
we will be able to power up the PM domain from the driver core, since
the device's PM domain pointer would be assigned.

I do realize that you rejected this approach earlier, but I guess we
have some new "data" to consider!? :-)

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> [CC list trimmed]
>
> On Monday, November 10, 2014 04:18:50 PM Ulf Hansson wrote:
>> [...]
>> 
>> > 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?
>> 
>> That was the initial approach.
>> 
>> To my understanding, Rafael's primary reason for not accepting that
>> was that it's not common, but it's platform-specific.
>> http://marc.info/?l=linux-pm&m=140243462304669&w=2
>
> For the record, I still believe this is platform-specific.
>
> I also think that the knowledge about what power (or generally PM) domain
> a device should belong to is not in a bus type or in the driver core.  That
> knowledge is in the code that enumerates devices.
>
> I wonder, then, if we could set the PM domain pointer at about the time
> when we set the bus type pointer?  Things will be consistent all the way
> through the entire device life cycle then.
>
>> Now, even if we would reconsider doing as you propose, what would the
>> actual benefit be? Obviously, we would get less amount of code to
>> maintain, which is one reason, but are there more?
>
> The same set of subsystem-level PM callbacks will be present for the device
> throughout its life cycle.
>
>> > 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.
>> 
>> Isn't that exactly what I implemented in [1], what am I missing?
>
> Not really.  Dmitry is talking about a generic interface independent of
> PM domains.
>
> If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it
> around really_probe() for all devices.  In theory.  But that's what we
> started with when we were working on the runtime PM framework and we ended
> up with what we have today.

OK, I'll admit it. I'm getting confused. 

In the context of PM domains, what does it mean for a device to be
"powered"?  Isn't it the domain that is powered up or down?  Devices
within the domain can be runtime resumed or suspended but the domain is
what is powered.

That being the case, I'm not seing the point of a per-device
power_up/power_down API.  As you said, it seems to basically duplicate
the runtime PM API.

> Problem is, pm_power_up() would probably end up being pretty much the same as
> pm_runtime_resume() without executing driver callbacks and similarly for
> pm_power_down().  That's why I was thinking about running pm_runtime_resume()
> at the time we know that driver callbacks are not present, just for the
> purpose of powering up the device.  [That has a problem of working with
> CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]

I think I'm tending o agree with Alan[1], that this is really best
handled by the sybsystem or bus-type, not the core.

> Now, Grygorii seems to be claiming that some drivers *require* their
> .runtime_resume() callbacks to be executed during .probe() pretty much
> before anything else which won't happen if pm_runtime_resume() is done
> before really_probe().  I'm wondering, then, which drivers those are.

Well, it's not technically a requirement, but an assumption that when a
_get_sync() is done in a driver's ->probe, it's ->runtime_resume() will
be called.

> Essentially, the "power up" operation will depend on the PM domain and
> bus type, so they'll need to provide callbacks that will most likely
> duplicate runtime PM callbacks.  And those callbacks need to be available
> when we do the "power up" thing.

Yuck.  More callbacks.  Just what we need. ;)

I think I'm still not quite getting the fundamental problem being
addressed.  Why isn't a pm_runtime_get_sync() by the first device in a
PM domain sufficient for powering on the domain?  

Yes, I understand there are some issues around *keeping* the domain
powered, but I think that's a separate problem.

> How can we address that?
>
> We seem to be dealing with a case in which PM is needed on some systems just to
> make them work at the basic level and not for energy saving, which was what
> runtime PM was designed for.

I think they're the same thing.  Power is needed for devices to work at
a basic level, and cutting power is needed for energy savings.  Runtime
PM is designed for that.  :)

Kevin

[1] http://marc.info/?l=linux-pm&m=141511953314249&w=2
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Friday, November 14, 2014 11:16:49 AM Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

[cut]

> > Essentially, the "power up" operation will depend on the PM domain and
> > bus type, so they'll need to provide callbacks that will most likely
> > duplicate runtime PM callbacks.  And those callbacks need to be available
> > when we do the "power up" thing.
> 
> Yuck.  More callbacks.  Just what we need. ;)

Precisely.

> I think I'm still not quite getting the fundamental problem being
> addressed.  Why isn't a pm_runtime_get_sync() by the first device in a
> PM domain sufficient for powering on the domain?  
> 
> Yes, I understand there are some issues around *keeping* the domain
> powered, but I think that's a separate problem.
> 
> > How can we address that?
> >
> > We seem to be dealing with a case in which PM is needed on some systems just to
> > make them work at the basic level and not for energy saving, which was what
> > runtime PM was designed for.
> 
> I think they're the same thing.  Power is needed for devices to work at
> a basic level, and cutting power is needed for energy savings.  Runtime
> PM is designed for that.  :)

Yes, that pretty much is my conclusion.

Consequently, the best way to go in my view is to require PM_RUNTIME to be
set when we need to do runtime PM.  Which is when we need to dynamically
power up and power down things as needed, basically.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html