Re: [BUGFIX] PM: Fix active child counting when disabled and forbidden

[Huang Ying <ying.huang@intel.com>]

On Wed, 2012-11-14 at 10:52 +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 14, 2012 09:08:28 AM Huang Ying wrote:
> > On Tue, 2012-11-13 at 11:10 -0500, Alan Stern wrote:
> > > On Tue, 13 Nov 2012, Huang Ying wrote:
> > > 
> > > > > This is not quite right.  Consider a device that is in runtime suspend 
> > > > > when a system sleep starts.  When the system sleep ends, the device 
> > > > > will be resumed but the PM core will still think its state is 
> > > > > SUSPENDED.  The subsystem has to tell the PM core that the device is 
> > > > > now ACTIVE.  Currently, subsystems do this by calling 
> > > > > pm_runtime_disable, pm_runtime_set_active, pm_runtime_enable.  Under 
> > > > > your scheme this wouldn't work; the pm_runtime_set_active call would 
> > > > > fail because the device was !forbidden.
> > > > 
> > > > Thanks for your information.  For this specific situation, is it
> > > > possible to call pm_runtime_resume() or pm_request_resume() for the
> > > > device?
> > > 
> > > No, because the device already is at full power.  The subsystem just
> > > needs to tell the PM core that it is.
> > > 
> > > > > > PM.  Device can always work with full power.
> > > > > 
> > > > > It can't if the parent is in SUSPEND.  If necessary, the user can write 
> > > > > "on" to the parent's power/control attribute first.
> > > > 
> > > > Is it possible to call pm_runtime_set_active() for the parent if the
> > > > parent is disabled and SUSPENDED.
> > > 
> > > Doing that is possible, but it might not work.  The parent might
> > > actually be at low power; calling pm_runtime_set_active wouldn't change
> > > the physical power level.  Basically, it's not safe to assume anything
> > > about devices that are disabled for runtime PM.
> > > 
> > > > It appears that there is race condition between this and the
> > > > pm_runtime_disable, pm_runtime_set_active, pm_runtime_enable sequence
> > > > you mentioned ealier.
> > > > 
> > > > thread 1			thread 2
> > > > pm_runtime_disable
> > > > pm_runtime_set_active
> > > > 				pm_runtime_allow
> > > > 				  pm_runtime_set_suspended
> > > > pm_runtime_enable
> > > 
> > > This can't happen in the situation I described earlier because during
> > > system sleep transitions, no other user threads are allowed to run.  
> > > All of them except the one actually carrying out the transition are
> > > frozen.
> > 
> > Thanks for your kind explanation.
> > 
> > After talking with you, my feeling is that the disabled state is obscure
> > and error-prone.  So I suggest not to use it if possible.  Maybe we can
> > 
> >  - make changes suggested by Alan to make disabled state better.
> 
> What changes specifically do you mean to be precise?

I mean the following changes from Alan's email.

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

        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.)

For the second one, is it possible that the device is really in low
power state when pm_runtime_forbid is called?  That situation is hard to
deal with too.

> >  - use Rafael's solution to solve this specific issue, and avoid the
> > usage of disabled state here.
> 
> Well, I think that the PCI subsystem should just enable runtime PM for
> all devices upfront and keep it enabled going forward.
> 
> My patch is incomplete, however, because it doesn't deal with probe/remove
> correctly at this point (which Alan pointed out earlier in the thread).

Yes.

Best Regards,
Huang Ying