Re: [PATCH][Alternative][RFC] PM / Runtime: Introduce driver runtime PM work routine

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, August 13, 2012, Alan Stern wrote:
> On Mon, 13 Aug 2012, Rafael J. Wysocki wrote:
> 
> > > __pm_runtime_barrier() has never made very strong guarantees about 
> > > runtime_resume callbacks.  But the kerneldoc does claim that any 
> > > pending callbacks will have been completed, and that claim evidently is 
> > > violated in the rpm_resume(parent,0) case.
> > 
> > "Flush all pending requests for the device from pm_wq and wait for all
> > runtime PM operations involving the device in progress to complete."
> > 
> > It doesn't mention the parent.
> 
> You're missing the point.  Suppose you do an async resume and while the
> workqueue routine is executing pm_resume(parent,0), another thread
> calls pm_runtime_barrier() for the same device.  The barrier will
> return more or less immediately, even though there is a runtime PM
> operation involving the device (that is, the async resume) still in
> progress.  The rpm_resume() routine was running before
> pm_runtime_barrier() was called and will still be running afterward.

I see what you mean now.

> > But I agree, it's not very clear.
> > 
> > > Maybe the kerneldoc needs to be changed, or maybe we need to fix the code.
> > 
> > If anything, I'd change the kerneldoc.  The code pretty much has to be
> > what it is in this respect.
> 
> I'm not sure what guarantees pm_runtime_barrier() _can_ make about
> runtime_resume callbacks.  If you call that routine while the device is
> suspended then a runtime_resume callback could occur at any moment,
> because userspace can write "on" to the power/control attribute
> whenever it wants to.
> 
> I guess the best we can say is that if you call pm_runtime_barrier()  
> after updating the dev_pm_ops method pointers then after the barrier
> returns, the old method pointers will not be invoked and the old method
> routines will not be running.  So we need an equivalent guarantee with
> regard to the pm_runtime_work pointer.  (Yes, we could use a better 
> name for that pointer.)
> 
> Which means the code in the patch isn't quite right, because it saves 
> the pm_runtime_work pointer before calling rpm_resume().  Maybe we 
> should avoid looking at the pointer until rpm_resume() returns.

Yes, we can do that.

Alternatively, we can set power.work_in_progress before calling
rpm_resume(dev, 0) (i.e. regard the resume as a part of the work) to make
the barrier wait for all of it to complete.

> > I think that it's better to reorder the checks so that the final ordering is:
> > 
> > * check power.no_callbacks and parent status
> > * check RPM_RUN_WORK
> > * check RPM_RESUMING || RPM_SUSPENDING
> > * check RPM_ASYNC
> > 
> > so that we don't schedule the execution of pm_runtime_work() if
> > power.no_callbacks is set and the parent is active and we still do the
> > power.deferred_resume optimization if RPM_RUN_WORK is unset.
> 
> That seems reasonable.

OK

Thanks,
Rafael