From: khilman@baylibre•com (Kevin Hilman)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()
Date: Fri, 22 Apr 2016 13:27:07 -0700 [thread overview]
Message-ID: <7hwpnptmn8.fsf@baylibre.com> (raw)
In-Reply-To: <27575455.zVxi7rnRNK@avalon> (Laurent Pinchart's message of "Thu, 21 Apr 2016 23:57:32 +0300")
Laurent Pinchart <laurent.pinchart@ideasonboard•com> writes:
> On Thursday 21 Apr 2016 20:31:52 Laurent Pinchart wrote:
>> Hi Ulf,
>>
>> Thank you for the patch.
>>
>> On Thursday 21 Apr 2016 12:34:02 Ulf Hansson wrote:
>> > When the pm_runtime_force_suspend|resume() helpers were invented, we still
>> > had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>> >
>> > To make sure these helpers worked for all combinations and without
>> > introducing too much of complexity, the device was always resumed in
>> > pm_runtime_force_resume().
>> >
>> > More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>> > unset, we needed to resume the device as the subsystem/driver couldn't
>> > rely on using runtime PM to do it.
>> >
>> > As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>> > removed this combination, of using CONFIG_PM_SLEEP without the earlier
>> > CONFIG_PM_RUNTIME.
>> >
>> > For this reason we can now rely on the subsystem/driver to use runtime PM
>> > to resume the device, instead of forcing that to be done in all cases. In
>> > other words, let's defer this to a later point when it's actually needed.
>> >
>> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro•org>
>> > ---
>> >
>> > Note, this patch is based upon another not yet queued patch [1]. The
>> > reason
>> > is simply because that [1] is a more important patch as it fixes a
>> > problem.
>> > It was posted to linux-pm April 8th and I expect it (or a new revision of
>> > it) to be applied before $subject patch.
>> >
>> > [1]
>> > https://patchwork.kernel.org/patch/8782851
>> >
>> > ---
>> >
>> > drivers/base/power/runtime.c | 11 +++++++++++
>> > 1 file changed, 11 insertions(+)
>> >
>> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> > index b746904..a190ca0 100644
>> > --- a/drivers/base/power/runtime.c
>> > +++ b/drivers/base/power/runtime.c
>> > @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
>> >
>> > goto out;
>> >
>> > }
>> >
>> > + /*
>> > + * The PM core increases the runtime PM usage count in the system PM
>> > + * prepare phase. If the count is greather than 1 at this point,
> someone
>> > + * else has also increased it. In such case, let's make sure to runtime
>> > + * resume the device as that is likely what is expected. In other case
>> > + * we trust the subsystem/driver to runtime resume the device when it's
>> > + * actually needed.
>> > + */
>> > + if (atomic_read(&dev->power.usage_count) < 2)
>> > + goto out;
>> > +
>> >
>> > ret = pm_runtime_set_active(dev);
>> > if (ret)
>> >
>> > goto out;
>>
>> This works in the sense that it prevents devices from being PM resumed at
>> system resume time if not needed. However, devices that are part of a PM
>> domain and that were idle before system suspend are suspended twice (with
>> their .runtime_suspend() handler called twice), which is not good at all.
>>
>> The first suspend occurs at system suspend time, with
>> pm_runtime_force_suspend() rightfully suspending the device as the device is
>> active (due to being woken up by pm_genpd_prepare()). The second suspend
>> occurs at resume time due to device_complete() calling pm_runtime_put().
>>
>> I've tracked the issue to the fact that pm_genpd_complete() calls
>> pm_runtime_set_active() regardless of whether the device was PM resumed or
>> not. As pm_runtime_force_suspend() doesn't resume devices with this patch
>> applied, the pm_runtime_put() call from device_complete() will try to
>> runtime suspend the device a second time as the state is incorrectly set to
>> RPM_ACTIVE.
>>
>> With the current genpd implementation this patch isn't needed (and neither
>> is my patch), as genpd expects the device to be always active when the
>> system is resumed. However, when genpd isn't used,
>> pm_runtime_force_resume() needs to skip resuming devices that were
>> suspended before system suspend. This patch looks good to me to fix that
>> problem.
>>
>> Do we need to fix genpd first ?
>
> And for the record, while this patch would require fixing genpd first, "[PATCH
> v2] PM / Runtime: Only force-resume device if it has been force-suspended"
> doesn't (at least as far as I understand the problem).
Right, I'm thinking we should merge Laurent's patch first. It fixes a
current problem, and won't get in the way of doing the genpd
improvements progressively.
Kevin
next prev parent reply other threads:[~2016-04-22 20:27 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-21 10:34 [PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume() Ulf Hansson
2016-04-21 17:31 ` Laurent Pinchart
2016-04-21 20:57 ` Laurent Pinchart
2016-04-22 20:27 ` Kevin Hilman [this message]
2016-04-25 8:15 ` Ulf Hansson
2016-04-25 13:32 ` Ulf Hansson
2016-04-25 16:52 ` Laurent Pinchart
2016-04-27 14:23 ` Ulf Hansson
2016-05-12 19:01 ` Laurent Pinchart
2016-05-12 20:28 ` Rafael J. Wysocki
2016-05-13 13:59 ` Ulf Hansson
2016-04-21 19:23 ` Rafael J. Wysocki
2016-04-22 6:58 ` Ulf Hansson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7hwpnptmn8.fsf@baylibre.com \
--to=khilman@baylibre$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox