[PATCH] PM: runtime: Use pmruntime sync variant to put suppliers

[PATCH] PM: runtime: Use pmruntime sync variant to put suppliers

[Stanimir Varbanov]

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(-)

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);
 	}
 }
 
-- 
2.17.1

Re: [PATCH] PM: runtime: Use pmruntime sync variant to put suppliers

[Rafael J. Wysocki]

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!

Re: [PATCH] PM: runtime: Use pmruntime sync variant to put suppliers

[Stanimir Varbanov]

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

Re: [PATCH] PM: runtime: Use pmruntime sync variant to put suppliers

[Rafael J. Wysocki]

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.