On Mon, 2012-11-12 at 11:32 -0500, Alan Stern wrote:

On Mon, 12 Nov 2012, Huang Ying wrote:

quoted

quoted

quoted

Is it absolute necessary to call pm_runtime_set_suspended?  If the
device is disabled, the transition to SUSPENDED state will not be
triggered even if the device is ACTIVE.

It's not absolutely necessary to do this, but we ought to because it 
will allow the device's parent to be suspended.  If we leave the device 
in the ACTIVE state then the parent can't be suspended, even when the 
device is disabled.

I think this is the hard part of the issue.  Now "disabled" and
SUSPENDED state is managed by hand.  IMHO, if we changed
pm_runtime_allow() as you said, we need to change the rule too.  Maybe
something as follow:

- remove pm_runtime_set_suspended/pm_runtime_set_active

We can't remove them entirely.  They are needed for situations where 
the device's physical state is different from what the PM core thinks; 
they tell the PM core what the actual state is.

quoted

- in pm_runtime_disable/pm_runtime_allow, put device into SUSPENDED
state if runtime PM is not forbidden.

That doesn't make sense.  Runtime PM is never forbidden after 
pm_runtime_allow is called; that's its purpose.

Sorry, my original idea is:

	pm_runtime_disable will put device into SUSPENDED state if
	dev->power.runtime_auto is clear.  pm_runtime_allow will put
	device into SUSPENDED state if dev->power.disable_depth > 0.

So in general, my original idea is to manage device runtime power state
automatically instead of manually, especially when device is in disabled
state.

	disabled + forbidden	-> ACTIVE
	disabled + !forbidden	-> SUSPENDED
	enabled + forbidden	-> ACTIVE
	enabled + !forbidden	-> auto

Why we can not do that?

quoted

- in pm_runtime_forbid/pm_runtime_enable, put device into ACTIVE state.

Why should pm_runtime_enable put the device into the ACTIVE state?

No, I think a better approach is simply to say that the device will
never be allowed to be in the SUSPENDED state if runtime PM is
forbidden.  We already enforce this when the device is enabled for 
runtime PM, so we would have to start enforcing it when the device is 
disabled.

This means:

	pm_runtime_set_suspended should fail if dev->power.runtime_auto
	is clear.

I think we can WARN_ON() here.  Because the caller should responsible
for state consistence if they decide to manage runtime power state
manually.

	pm_runtime_forbid should call pm_runtime_set_active if
	dev->power.disable_depth > 0.  (This would run into a problem
	if the parent is suspended and disabled.  Maybe 
	pm_runtime_forbid should fail when this happens.)

pm_runtime_forbid() may be called via echo "on" > .../power/control.  I
think it is hard to refuse the request from user space to forbid runtime
PM.  Device can always work with full power.

Finally, we probably should make a third change even though it isn't
strictly necessary:

	pm_runtime_allow should call pm_runtime_set_suspended if
	dev->power.disable_depth > 0.

I think this is something similar to manage device power state
automatically if disabled.

Best Regards,
Huang Ying

Rafael, what do you think?

Alan Stern