All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kevin Hilman <khilman@ti.com>
To: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: linux-pm <linux-pm@lists.linux-foundation.org>
Subject: Re: use of pm_runtime_disable() from driver probe?
Date: Tue, 10 Jul 2012 11:11:17 -0700	[thread overview]
Message-ID: <87r4sjo5a2.fsf@ti.com> (raw)
In-Reply-To: <201207072125.26189.rjw@sisk.pl> (Rafael J. Wysocki's message of "Sat, 7 Jul 2012 21:25:26 +0200")

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> Hi,
>
> On Saturday, July 07, 2012, Kevin Hilman wrote:
>> Hello,
>> 
>> I've got a question around the proper use of pm_runtime_disable() in a
>> driver's ->probe method.  Basically, using pm_runtime_disable() in
>> ->probe() (e.g because of a error/failure) can result in the device
>> remaning runtime enabled forever.
>
> Er, no.

Right, poor choice of words.  What I meant was "...can result in the
device remaining powered on forever."  

>> This is because the driver core wraps
>> the probe in _get_noresume()/_put_sync() so any usage of _put_sync() in
>> ->probe itself does not trigger an actual device runtime suspend.  If
>> the driver also calls pm_runtime_disable() in probe, that results in the
>> device remaining runtime enabled.
>
> I'm not quite sure what way.  pm_runtime_disable() increments
> dev->power.disable_count and if that is not decremented later, the
> device remains runtime disabled.
>
> You probably wanted to say that the device will end up in the full-power
> state in that case, which isn't the same and is intentional.

Correct.  I'm glad you understood what I meant instead of what I said.
I wish my kids could do that.  ;)

[...]

>> So, the question is: is it right to use pm_runtime_disable() this way?
>
> If your intention is to put the device into a low-power state in case of
> a failing .probe(), this isn't the way to do it.  pm_runtime_disable()
> means "don't do any runtime power management on this device any more"
> regardless of the current state of the device (it may resume the device
> in some situations, but it won't suspend it, although it may wait for
> a suspend in progress to complete).

That's correct, but I also think that is what driver writers probably
believe they are doing.  IOW, after a probe failure, the driver writer
does indeed mean "don't do runtime PM on this device any more."  But
they really mean "don't do runtime PM on this device *after* it's been
powered down."

However, because of the get/put in the driver core, it's not entirely
obvious to driver writers that calling pm_runtime_disable() actually
keeps the device powered up, even though they just called _put_sync().

Another way of stating the problem is that the _put_sync() in the driver
core's driver_probe_device() assumes that runtime PM is enabled for that
device.

> That said, I'm not aware of any other generally reliable way to do that
> either.  

That's what I was afraid of. :(

> The problem is, if .probe() is failing we don't have callbacks
> allowing us to power manage the device (there may be bus type callbacks
> knowing how to do that, but not for the platform bus type).

There are callbacks for the PM domains, but they are not called either
if the driver calls pm_runtime_disable().

>> Removing it means the device is left in runtime suspended state even in
>> the error case.  However, without the pm_runtime_disable(), if this
>> driver was a module, repeated attempts to load would result in
>> unbalalced calls to pm_runtime_enable.
>> 
>> What is the right way to handle the runtime PM enable/disable in a
>> failed probe attempt?
>
> I would recommend using pm_runtime_disable() and then either
> pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the
> real power state of the device is at this point.

That doesn't help because it only changes the runtime PM core's notion
of the power state, not the physical power state because it doesn't call
the subsystem callbacks.

Or, are you suggesting to set the state to suspended, and then use a
late_initcall to compare the runtime status with the physical device
status and power down devices accordingly?

> Anyway, you can't force the device into a low-power state using
> runtime PM after a failing probe, at least in general.

Well, using PM domains, that's exactly what can happen if the driver
doesn't call pm_runtime_disable() because the _put_sync() in the driver
core will trigger the PM domain callbacks.

Kevin

  parent reply	other threads:[~2012-07-10 18:11 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-07-06 22:29 use of pm_runtime_disable() from driver probe? Kevin Hilman
2012-07-07 19:25 ` Rafael J. Wysocki
2012-07-08  2:01   ` Alan Stern
2012-07-08 14:59     ` Rafael J. Wysocki
2012-07-10 18:11   ` Kevin Hilman [this message]
2012-07-10 18:31     ` Rafael J. Wysocki
2012-07-10 18:47       ` Alan Stern
2012-07-10 19:14         ` Rafael J. Wysocki
2012-07-10 19:41           ` Rafael J. Wysocki
2012-07-10 20:17             ` Alan Stern
2012-07-10 21:04               ` Kevin Hilman
2012-07-10 22:31               ` Rafael J. Wysocki
2012-07-11 14:20                 ` Alan Stern
2012-07-11 17:36                   ` Rafael J. Wysocki
2012-07-11 23:07                     ` Kevin Hilman
2012-07-11 23:16                       ` Rafael J. Wysocki

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=87r4sjo5a2.fsf@ti.com \
    --to=khilman@ti.com \
    --cc=linux-pm@lists.linux-foundation.org \
    --cc=rjw@sisk.pl \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.