| Message ID | 20201007001934.18606-1-stanimir.varbanov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PM: runtime: Use pmruntime sync variant to put suppliers | expand |
On Wed, Oct 7, 2020 at 2:20 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Calling pm_runtime_put_sync over a device with suppliers with device > link flags PM_RUNTIME | RPM_ACTIVE it is observed that the supplier > is not put (turned off) at the end, but instead put asynchronously. Yes, that's by design. > In some case This could lead to issues for the callers which expects > that the pmruntime sync variants should also put the suppliers > synchronously. Why would anyone expect that to happen? > Also the opposite rpm_get_suppliers is already using pmruntime _sync > variant of the API. Yes, it does, because that is necessary. > Correct this by changing pmruntime_put to pmruntime_put_sync in > rpm_put_suppliers. It is not a correction, but a change in behavior without good enough rationale, as it stands. Thanks!
Hi Rafael, On 10/7/20 5:37 PM, Rafael J. Wysocki wrote: > On Wed, Oct 7, 2020 at 2:20 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Calling pm_runtime_put_sync over a device with suppliers with device >> link flags PM_RUNTIME | RPM_ACTIVE it is observed that the supplier >> is not put (turned off) at the end, but instead put asynchronously. > > Yes, that's by design. > >> In some case This could lead to issues for the callers which expects >> that the pmruntime sync variants should also put the suppliers >> synchronously. > > Why would anyone expect that to happen? It is logical to me that when I call pm_runtime_put_sync the device and its suppliers are put synchronously. If I want to put device and its suppliers asynchronously I'd use pm_runtime_put. Is that wrong assumption? > >> Also the opposite rpm_get_suppliers is already using pmruntime _sync >> variant of the API. > > Yes, it does, because that is necessary. > >> Correct this by changing pmruntime_put to pmruntime_put_sync in >> rpm_put_suppliers. > > It is not a correction, but a change in behavior without good enough > rationale, as it stands. In my driver case I want to deal with a recovery of a crash in remote processor (the remote processor is used to control and program hardware blocks and also to communicate with host processor through shared memory). To restart the remote processor I have to disable clocks and turn off few power domains (one of the power domains is made a supplier of my main device) in order to complete the cold-boot. The problem I'm facing with this design is that when I call runtime_put_sync (to disable device's clocks and turn off power domain) the clocks are disabled (part of pmruntime_suspend callback) but the pmdomain (the device supplier) is not turned synchronously. I workaround this by checking the supplier device via pm_runtime_active() until it becomes inactive and the continue with rest of the steps. From my point of view this check for supplier activity should be part of pmruntime API. > > Thanks! > -- regards, Stan
On Thu, Oct 8, 2020 at 3:08 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Rafael, > > On 10/7/20 5:37 PM, Rafael J. Wysocki wrote: > > On Wed, Oct 7, 2020 at 2:20 AM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Calling pm_runtime_put_sync over a device with suppliers with device > >> link flags PM_RUNTIME | RPM_ACTIVE it is observed that the supplier > >> is not put (turned off) at the end, but instead put asynchronously. > > > > Yes, that's by design. > > > >> In some case This could lead to issues for the callers which expects > >> that the pmruntime sync variants should also put the suppliers > >> synchronously. > > > > Why would anyone expect that to happen? > > It is logical to me that when I call pm_runtime_put_sync the device and > its suppliers are put synchronously. If I want to put device and its > suppliers asynchronously I'd use pm_runtime_put. Is that wrong assumption? The handling of suppliers is analogous to the handling of parents and the parents are suspended asynchronously when a child suspends. The difference between _put() and _put_sync() only applies to the device passed in as the argument. > >> Also the opposite rpm_get_suppliers is already using pmruntime _sync > >> variant of the API. > > > > Yes, it does, because that is necessary. > > > >> Correct this by changing pmruntime_put to pmruntime_put_sync in > >> rpm_put_suppliers. > > > > It is not a correction, but a change in behavior without good enough > > rationale, as it stands. > > In my driver case I want to deal with a recovery of a crash in remote > processor (the remote processor is used to control and program hardware > blocks and also to communicate with host processor through shared > memory). To restart the remote processor I have to disable clocks and > turn off few power domains (one of the power domains is made a supplier > of my main device) in order to complete the cold-boot. PM-runtime doesn't guarantee you the behavior that you'd like to see here. > The problem I'm facing with this design is that when I call > runtime_put_sync (to disable device's clocks and turn off power domain) > the clocks are disabled (part of pmruntime_suspend callback) but the > pmdomain (the device supplier) is not turned synchronously. I workaround > this by checking the supplier device via pm_runtime_active() until it > becomes inactive and the continue with rest of the steps. This is not a use case for PM-runtime at all. PM-runtime is all about going low-power opportunistically, whereas you want to enforce power down. > From my point of view this check for supplier activity should be part of > pmruntime API. But the API is not what you should be using for this purpose in the first place.
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 6f605f7820bb..8dab4fcab4e8 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -313,7 +313,7 @@ static void rpm_put_suppliers(struct device *dev) device_links_read_lock_held()) { while (refcount_dec_not_one(&link->rpm_active)) - pm_runtime_put(link->supplier); + pm_runtime_put_sync(link->supplier); } }
Calling pm_runtime_put_sync over a device with suppliers with device link flags PM_RUNTIME | RPM_ACTIVE it is observed that the supplier is not put (turned off) at the end, but instead put asynchronously. In some case This could lead to issues for the callers which expects that the pmruntime sync variants should also put the suppliers synchronously. Also the opposite rpm_get_suppliers is already using pmruntime _sync variant of the API. Correct this by changing pmruntime_put to pmruntime_put_sync in rpm_put_suppliers. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.1