Re: [linux-pm] [PATCH] PERF(kernel): Cleanup power events V2

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

On Wednesday, October 27, 2010, Mathieu Desnoyers wrote:
> * Rafael J. Wysocki (rjw@sisk.pl) wrote:
...
> 
> Hrm, then why export pm_runtime_get_noresume() at all ?

Basically, the PM core needs it for some obscure stuff.  Beyond that people
really should use it with care (preferably avoid using it at all).

> I don't feel comfortable with some of the pm_runtime_get_noresume() callers.
> 
> > 
> > So if you don't want to resume the device immediately after increasing its
> > usage_count (in which case it's better to use pm_runtime_get_sync()), you
> > should do something like this:
> > 
> > 1) pm_runtime_get_noresume(dev);
> > 1a) pm_runtime_barrier(dev);  // That takes care of all pending requests etc.
> > 
> > 2) ...
> >  
> > 3) pm_runtime_put_noidle(dev);
> > 
> > [The meaning of pm_runtime_barrier() is that all of the runtime PM activity
> > started before the barrier has been completed when it returns.]
> > 
> > There's one place in the PM core where that really is necessary, but I wouldn't
> > recommend anyone doing anything like it in a driver.
> 
> grep -r pm_runtime_get_noresume drivers/    hands out very interesting info.
> 
> e.g.:
> 
> drivers/usb/core/drivers.c: usb_autopm_get_interface_async()
> 
>         pm_runtime_get_noresume(&intf->dev);
>         s = ACCESS_ONCE(intf->dev.power.runtime_status);
>         if (s == RPM_SUSPENDING || s == RPM_SUSPENDED)
>                 status = pm_request_resume(&intf->dev);
> 
> How is this supposed to work ?
> 
> If the ACCESS_ONCE can be reordered before the atomic_inc(), then I fear the
> device can be suspended even after the check.
> 
> My point is that a get/put semantic should imply memory barriers, especially if
> these are exported APIs.

Well, IMO adding a memory barrier to pm_runtime_get_noresume() wouldn't really
change a lot (it still would be racy with respect to some other runtime PM helper
funtions).  That said I guess we should put a "handle with care" sticker on it.

Thanks,
Rafael