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

Rafael J. Wysocki
2010-10-27T21:44:21Z

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