[PATCH V3] PM / Domains: Remove intermediate states from the power off sequence

[Kevin Hilman]

Lina Iyer <lina.iyer at linaro.org> writes:

> On Mon, Jun 15 2015 at 17:49 -0600, Kevin Hilman wrote:
>>Ulf Hansson <ulf.hansson at linaro.org> writes:
>>
>>> Genpd's ->runtime_suspend() (assigned to pm_genpd_runtime_suspend())
>>> doesn't immediately walk the hierarchy of ->runtime_suspend() callbacks.
>>> Instead, pm_genpd_runtime_suspend() calls pm_genpd_poweroff() which
>>> postpones that until *all* the devices in the genpd are runtime suspended.
>>>
>>> When pm_genpd_poweroff() discovers that the last device in the genpd is
>>> about to be runtime suspended, it calls __pm_genpd_save_device() for *all*
>>> the devices in the genpd sequentially. Furthermore,
>>> __pm_genpd_save_device() invokes the ->start() callback, walks the
>>> hierarchy of the ->runtime_suspend() callbacks and invokes the ->stop()
>>> callback. This causes a "thundering herd" problem.
>>>
>>> Let's address this issue by having pm_genpd_runtime_suspend() immediately
>>> walk the hierarchy of the ->runtime_suspend() callbacks, instead of
>>> postponing that to the power off sequence via pm_genpd_poweroff(). If the
>>> selected ->runtime_suspend() callback doesn't return an error code, call
>>> pm_genpd_poweroff() to see if it's feasible to also power off the PM
>>> domain.
>>>
>>> Adopting this change enables us to simplify parts of the code in genpd,
>>> for example the locking mechanism. Additionally, it gives some positive
>>> side effects, as described below.
>>>
>>> i)
>>> One device's ->runtime_resume() latency is no longer affected by other
>>> devices' latencies in a genpd.
>>>
>>> The complexity genpd has to support the option to abort the power off
>>> sequence suffers from latency issues. More precisely, a device that is
>>> requested to be runtime resumed, may end up waiting for
>>> __pm_genpd_save_device() to complete its operations for *another* device.
>>> That's because pm_genpd_poweroff() can't confirm an abort request while it
>>> waits for __pm_genpd_save_device() to return.
>>>
>>> As this patch removes the intermediate states in pm_genpd_poweroff() while
>>> powering off the PM domain, we no longer need the ability to abort that
>>> sequence.
>>>
>>> ii)
>>> Make pm_runtime[_status]_suspended() reliable when used with genpd.
>>>
>>> Until the last device in a genpd becomes idle, pm_genpd_runtime_suspend()
>>> will return 0 without actually walking the hierarchy of the
>>> ->runtime_suspend() callbacks. However, by returning 0 the runtime PM core
>>> considers the device as runtime_suspended, so
>>> pm_runtime[_status]_suspended() will return true, even though the device
>>> isn't (yet) runtime suspended.
>>>
>>> After this patch, since pm_genpd_runtime_suspend() immediately walks the
>>> hierarchy of the ->runtime_suspend() callbacks,
>>> pm_runtime[_status]_suspended() will accurately reflect the status of the
>>> device.
>>>
>>> iii)
>>> Enable fine-grained PM through runtime PM callbacks in drivers/subsystems.
>>>
>>> There are currently cases were drivers/subsystems implements runtime PM
>>> callbacks to deploy fine-grained PM (e.g. gate clocks, move pinctrl to
>>> power-save state, etc.). While using the genpd, pm_genpd_runtime_suspend()
>>> postpones invoking these callbacks until *all* the devices in the genpd
>>> are runtime suspended. In essence, one runtime resumed device prevents
>>> fine-grained PM for other devices within the same genpd.
>>>
>>> After this patch, since pm_genpd_runtime_suspend() immediately walks the
>>> hierarchy of the ->runtime_suspend() callbacks, fine-grained PM is enabled
>>> throughout all the levels of runtime PM callbacks.
>>>
>>> Unfortunately this patch also comes with a drawback, as described in the
>>> summary below.
>>>
>>> Driver's/subsystem's runtime PM callbacks may be invoked even when the
>>> genpd hasn't actually powered off the PM domain, potentially introducing
>>> unnecessary latency.
>>>
>>> However, in most cases, saving/restoring register contexts for devices are
>>> typically fast operations or can be optimized in device specific ways
>>> (e.g. shadow copies of register contents in memory, device-specific checks
>>> to see if context has been lost before restoring context, etc.).
>>>
>>> Still, in some cases the driver/subsystem may suffer from latency if
>>> runtime PM is used in a very fine-grained manner (e.g. for each IO request
>>> or xfer). To prevent that extra overhead, the driver/subsystem may deploy
>>> the runtime PM autosuspend feature.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
>>
>>Really like the updated changelog.  Very well described, and IMO you
>>make a very strong case for this change, which I completely agree is the
>>right way to go forward.  IMO, it greatly simplifies understanding this
>>code as well.
>>
>>I have some minor nits below, but overall really like this approach and
>>would like to see this hit -next early in the v4.3 cycle so it gets good
>>test exposure.
>>
>>I know Lina has been testing this (or an earlier version) on qcom
>>hardware so would be nice to get a tested-by from her, and would also be
>>nice to see if Geert has a chance to take this for a spin on his Renesas
>>hardware and anyone can take this for a spin on Exynos.
>>
> Will do.
>
>>[...]
>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 2327613..2e03f68 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -133,41 +133,6 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>>>  	smp_mb__after_atomic();
>>>  }
>>>
>>> -static void genpd_acquire_lock(struct generic_pm_domain *genpd)
>>> -{
>>> -	DEFINE_WAIT(wait);
>>> -
>>> -	mutex_lock(&genpd->lock);
>>> -	/*
>>> -	 * Wait for the domain to transition into either the active,
>>> -	 * or the power off state.
>>> -	 */
>>> -	for (;;) {
>>> -		prepare_to_wait(&genpd->status_wait_queue, &wait,
>>> -				TASK_UNINTERRUPTIBLE);
>>> -		if (genpd->status == GPD_STATE_ACTIVE
>>> -		    || genpd->status == GPD_STATE_POWER_OFF)
>>> -			break;
>>> -		mutex_unlock(&genpd->lock);
>>> -
>>> -		schedule();
>>> -
>>> -		mutex_lock(&genpd->lock);
>>> -	}
>>> -	finish_wait(&genpd->status_wait_queue, &wait);
>>> -}
>>> -
>>> -static void genpd_release_lock(struct generic_pm_domain *genpd)
>>> -{
>>> -	mutex_unlock(&genpd->lock);
>>> -}
>>> -
>>> -static void genpd_set_active(struct generic_pm_domain *genpd)
>>> -{
>>> -	if (genpd->resume_count == 0)
>>> -		genpd->status = GPD_STATE_ACTIVE;
>>> -}
>>
>>Minor nit: I think you should leave these 3 helpers and just simplify
>>them.  It will make the changes below easier to read as well.
>>
>>Also, for the locking, Lina will be adding these locking functions back
>>anyways, so let's just avoid the extra churn.
>>
>
> Actually, these locks are no longer useful after this patch and can
> be removed as part of this series. Will make a clean cut to overlay my
> patch.

Today: genpd_acquire_lock()

Ulf: mutex_lock()

Yours: genpd_lock_domain()

What I'm suggesting is we just leave the genpd_acquire_lock() function
*name* alone, and just change it's implementation over the series.
IMO, that makes the locking changes much easier to review because it's
then clear that the users of the lock are not change, just the
implemention of the lock is changing.

Kevin