| Message ID | 1415366847-4807-1-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-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 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.
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) return 0; + /* + * 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) 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
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(-)