Re: [PATCH] PM / QoS: Fix default runtime_pm device resume latency

[Tero Kristo]

On 31/10/17 10:40, Rafael J. Wysocki wrote:
> On Tue, Oct 31, 2017 at 8:13 AM, Tero Kristo <t-kristo@xxxxxx> wrote:
> > On 31/10/17 01:27, Rafael J. Wysocki wrote:
> > > On Monday, October 30, 2017 11:19:08 AM CET Rafael J. Wysocki wrote:
> > > > On Mon, Oct 30, 2017 at 8:10 AM, Tero Kristo <t-kristo@xxxxxx> wrote:
> > > > > The recent change to the PM QoS framework to introduce a proper
> > > > > no constraint value overlooked to handle the devices which don't
> > > > > implement PM QoS OPS. Runtime PM is one of the more severely
> > > > > impacted subsystems, failing every attempt to runtime suspend
> > > > > a device. This leads into some nasty second level issues like
> > > > > probe failures and increased power consumption among other things.
> > > >
> > > >
> > > > Oh, that's bad.
> > > >
> > > > Sorry about breaking it and thanks for the fix!
> > > >
> > > > > Fix this by adding a proper return value for devices that don't
> > > > > implement PM QoS implicitly.
> > > > >
> > > > > Fixes: 0cc2b4e5a020 ("PM / QoS: Fix device resume latency PM QoS")
> > > > > Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
> > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > >
> > > >
> > > > Applied.
> > >
> > >
> > > And pushed to Linus.
> > >
> > > That said, probe shouldn't ever fail if PM QoS is set to the
> > > "never suspend" value.
> > >
> > > User space can set it that way, after all, so the drivers that fail to
> > > probe
> > > in that case aren't correct I'm afraid.
> >
> >
> > Ok interesting. The probe failure we had was a second order issue. A driver
> > (omap_nmailbox) was attempting to pm_runtime_get_sync() ...put_sync() during
> > probe, and checked the return value of pm_runtime_put_sync() which was
> > -EPERM and bailed out. Most of the time, drivers don't check the return
> > value of this and will just succeed. I did a grep on kernel and there are
> > few other drivers that check the return value also, didn't check if they do
> > this during probe though but it can potentially cause various issues
> > elsewhere also.
> >
> > So, you are saying we should not check the return value of
> > pm_runtime_put_x() ever, or should check if it is -EPERM and just pass in
> > that case?
>
> The latter.
>
> > Is there any point returning -EPERM from the runtime core at all
> > then? This should probably be filtered out within runtime core as a valid
> > situation and just return 0.
>
> Fair point.
>
> However, there are other situations in which pm_runtime_put_sync() can
> return an error code which needs to be checked, like -EBUSY or -EAGAIN
> returned if one of the reference counters is nonzero.
>
> In fact, the "no suspend" PM QoS constraint is somewhat similar to
> this situation, so what about changing the error code returned to
> -EAGAIN, for example?

Ok yea thats true. Its better to leave the decision to the drivers as 
the core most likely can't know what the driver will actually want to do 
with a prevented pm_runtime_put. That way the policy can be implemented 
on case-by-case basis.

Keeping the -EPERM in place is probably also just fine, if someone needs 
a detailed info of what prevented the pm_runtime_put from finishing 
properly, and if someone is using this value already.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki