Re: [RFC][PATCH] PM: Introduce core framework for run-time PM of I/O devices (rev. 8)

[Magnus Damm <magnus.damm@gmail.com>]

Hi Rafael,

On Mon, Jul 6, 2009 at 9:52 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> Hi,
>
> There's a rev. 8 of the run-time PM framework patch.
>
> Highlights:
> * I did my best to follow the design we've recently discussed.
> * pm_runtime_[get|put]() and the sync versions call
>  pm_[request|runtime]_[resume|idle](), because I don't see much point
>  manipulating the usage counter alone.
> * pm_runtime_disable() carries out a (synchronous) wake-up if there's a
>  resume request pending.
>
> Comments welcome.

I've now jumped from v5 to v8 and I feel that the code is getting
cleaner and cleaner. Very nice.

My intention was to post a SuperH prototype last week, but I got side
tracked with other stuff. And today I ran into some problems related
to probe() that I'd like to ask about right away. At this point I've
got a few device drivers converted and some simple bus
runtime_suspend()/runtime_resume() code that stop and start clocks.

Issue 1:
------------
Device drivers which do not perform any hardware access in probe()
work fine. During software setup in probe() the runtime pm code is
initialized with the following:

+	pm_suspend_ignore_children(&dev->dev, true);
+	pm_runtime_set_suspended(&dev->dev);
+	pm_runtime_enable(&dev->dev);

Before accessing hardware I perform:
+	pm_runtime_resume(pd->dev);

When done with the hardware I do:
+	pm_runtime_suspend(pd->dev);

Not so complicated. Am I supposed to initialize something else as well?

All good with the code above, but there seem to be some issue with how
usage_count is counted up and down and when runtime_disabled is set:

1. pm_runtime_init(): usage_count = 1, runtime_disabled = true
2. driver_probe_device(): pm_runtime_get_sync()
3. pm_runtime_get_sync(): usage_count = 2
4. device driver probe(): pm_runtime_enable()
5. pm_runtime_enable(): usage_count = 1
6. driver_probe_device(): pm_runtime_put()
7. pm_runtime_put(): usage_count = 0

I expect runtime_disabled = false in 7. Modifying the get/put calls to
do enable/disable may work around the issue, but that's probably not
what you guys want.

Issue 2:
------------
I cannot get any bus ->runtime_resume() callbacks from probe(). This
also seems related to usage_count and pm_runtime_get_sync() in
driver_probe_device(). Basically, from probe(), calling
pm_runtime_resume() after pm_runtime_set_suspended() results in error
and not in a ->runtime_resume() callback. Some device drives access
hardware in probe(), so the ->runtime_resume() callback is needed at
that point to turn on clocks before the hardware can be accessed.

Random thought:
-------------------------
The runtime_pm_get() and runtime_pm_put() look very nice. I assume
that inteface is supposed to be used by bus code. I wonder if it would
be cleaner to use a similar counter based interface from the driver
instead of the pm_runtime_idle()/suspend()/resume()...

Let me know what you think!

Cheers,

/ magnus