Hi Ulf,
On Thursday 21 Apr 2016 15:52:06 Ulf Hansson wrote:
> On 21 April 2016 at 14:41, Laurent Pinchart wrote:
> > On Thursday 21 Apr 2016 11:10:19 Ulf Hansson wrote:
> >> On 21 April 2016 at 01:30, Laurent Pinchart wrote:
> >>> On Monday 07 Mar 2016 11:10:08 Ulf Hansson wrote:
> >>>> [...]
> >>>>
> >>>>>> I agree, that's a better idea. Drivers shouldn't call
> >>>>>> pm_runtime_force_resume() if they haven't called
> >>>>>> pm_runtime_force_suspend(), so checking the PM use count should be
> >>>>>> fine. I'll modify the patch, test it and resubmit.
> >>>>>
> >>>>> I gave it an unfortunately unsuccessful try. The problem I ran into
> >>>>> is that device_prepare() calls pm_runtime_get_noresume() calls
> >>>>> pm_runtime_get_noresume(), with the corresponding pm_runtime_put()
> >>>>> call being performed in device_complete(). The device power
> >>>>> usage_count is thus always non-zero in the system resume handler, so
> >>>>> I can't base the decision on that.
> >>>>
> >>>> As Alan said, let's just check against 1 instead.
> >>>
> >>> I gave this a try, and unfortunately it won't work.
> >>>
> >>> pm_genpd_prepare() resumes devices without increasing the usage count,
> >>> which
> >>
> >> It doesn't resume them, it only increases the usage count.
> >
> > Maybe there's something I don't get, but pm_genpd_prepare() ends with
>
> I realize that I did read *pm_genpd_prepare()* as *device_prepare()*,
> which represents the behaviour of the PM core during the prepare phase
> (drivers/base/power/main.c). In such cases my earlier reply would make
> more sense, I believe.
>
> Apologize for giving you the wrong input by not reading your response
> in more detail!
No worries, thanks for taking the time to reply now.
> > /*
> > * The PM domain must be in the GPD_STATE_ACTIVE state at this point,
> > * so genpd_poweron() will return immediately, but if the device
> > * is suspended (e.g. it's been stopped by genpd_stop_dev()), we need
> > * to make it operational.
> > */
> > pm_runtime_resume(dev);
> > __pm_runtime_disable(dev, false);
> >
> > ret = pm_generic_prepare(dev);
> > if (ret) {
> > ... /* irrelevant error handling */
> > }
> >
> > pm_runtime_put(dev);
> > return ret;
> >
> > The device is thus resumed. Note that the pm_runtime_put() call will
> > decrease the usage count that was increased at the beginning of the
> > function (and thus makes pm_genpd_prepare() neutral from a usage count
> > point of view) but won't resume the device as the disable depth is
> > increased by __pm_runtime_disable().
> >
> > As far as I understand, the device is thus effectively active when
> > entering the driver's system suspend handler, with a usage count equal to
> > 1 or more. pm_runtime_force_suspend(), which decides whether to suspend
> > the device based on the device pm state and not the usage count, will thus
> > always pm suspend the device.
> >
> > Is the above analysis correct ?
>
> Yes.
>
> Although, genpd is not behaving as it should here. It must *not*
> resume all devices, just because the domain is powered.
We agree on that.
> > Now I'm a bit lost as to what happens (and what should happen) at resume
> > time. Does genpd and the PM core try to suspend the device after calling
> > the driver's resume handler (pm_runtime_force_resume() in this case)
> > under some conditions, or does the resume handler have the last word in
> > deciding the PM runtime status of the device after system resume ?
>
> The resume callback of genpd decides what will happen, as its a PM
> domain and it sits on top of the hierarchy of a devices PM callbacks.
>
> As you have realized, there are issues in genpd regarding the system
> PM code, there have even been reports about it.
>
> For example, genpd prevents subsystem/drivers from manage their
> devices during system PM - when it start the system PM phase with the
> domain in powered off state.
>
> That's not an okay behaviour, as it can't know whether a device needs
> to be used to serve a request after that point, causing the device to
> be runtime resumed. As genpd prevents it to be suspended via system
> PM, it will stay resumed during the system PM suspend phase, at least
> that is my understanding and it seems like bad idea.
>
> In general, the system PM code in genpd needs to be modernized. It
> somewhat duplicating some code from the PM core and I think the code
> can be greatly simplified.
>
> I have started to work on this quite recently, once the first steps
> are done I will consider optimizations, such as not runtime resuming
> devices unnecessarily.
>
> >>> leads to the device always being active in pm_runtime_force_suspend().
> >>> The usage count will be 1 if the device was suspended prior to entering
> >>> system suspend (due to the pm_runtime_get_noresume() call in
> >>> device_prepare()) or higher than 1 if the device was active.
> >>
> >> It's only greater than 1 - if someone else than the PM core has
> >> increased the usage count.
> >
> > That I agree with.
>
> Okay, great! :-)
>
> [...]
>
> Again, sorry for the giving the wrong input earlier!
>
> Future wise, I will keep you on cc on any related genpd patches.
Yeah \o/ more patches in my inbox, thank you so much ;-)
--
Regards,
Laurent Pinchart
