Re: [PATCH 2/2] PM: sleep: Fix runtime PM based cpuidle support

[Ulf Hansson]

[...]

> > > > > >
> > > > > > > Additionally, since find_deepest_state() is being called for
> > > > > > > cpuidle_enter_s2idle() too, we would need to treat the new
> > > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right?
> > > > > >
> > > > > > No, it already checks "disabled".
> > > > >
> > > > > Yes, but that would be wrong.
> > > >
> > > > Hmmm.
> > > >
> > > > > The use case I want to support, for cpuidle-psci, is to allow all idle
> > > > > states in suspend-to-idle,
> > > >
> > > > So does PM-runtime work in suspend-to-idle?  How?
> > > >
> > > > > but prevent those that rely on runtime PM
> > > > > (after it has been disabled) for the regular idle path.
> > > >
> > > > Do you have a special suspend-to-idle handling of those states that
> > > > doesn't require PM-runtime?
> > >
> > > Regardless, pausing cpuidle in the suspend-to-idle path simply doesn't
> > > make sense at all, so this needs to be taken care of in the first
> > > place.
> >
> > Right, I do agree, don't get me wrong. But, do we really want to treat
> > s2-to-idle differently, compared to s2-to-ram in regards to this?
> >
> > Wouldn't it be a lot easier to let cpuidle drivers to opt-out for
> > cpuidle_pause|resume(), no matter whether it's for s2-to-idle or
> > s2-to-ram?
>
> I don't think so.
>
> Suspend-to-idle resume cpuidle after pausing it which is just plain
> confusing and waste of energy and the fact that the system-wide
> suspend flow interferes with using PM-runtime for implementing cpuidle
> callbacks at the low level really is an orthogonal problem.

It's certainly an orthogonal problem, I agree. However, trying to
solve it in two different ways, may not really be worth the effort, in
my opinion.

As I kind of pointed out in the earlier reply, I am not sure there are
any other relatively easy solutions available, to fix the problem for
runtime PM based cpuidle drivers. We probably need to call
cpuidle_pause() (or similar) in some way.

>
> > >
> > > The problem with PM-runtime being unavailable after dpm_suspend()
> > > needs to be addressed in a different way IMO, because it only affects
> > > one specific use case.
> >
> > It's one specific case so far, but we have the riscv driver on its
> > way, which would suffer from the same problem.
>
> So perhaps they should be advised about this issue.

Yes, I will let them know - and hopefully I will soon also be able to
provide them with a fix. :-)

>
> > Anyway, an option is to figure out what platforms and cpuidle drivers,
> > that really needs cpuidle_pause|resume() at this point and make an
> > opt-in solution instead.
>
> None of them need to pause cpuidle for suspend-to-idle AFAICS.

I assume so too, otherwise things would have been broken when
cpuidle_resume() is called in s2idle_enter(). But, it's still a bit
unclear.

>
> Some may want it in the non-s2idle suspend path, but I'm not sure
> about the exact point where cpuidle needs to be paused in this case.
> Possibly before offlining the nonboot CPUs.

Okay.

Note that, I assume it would be okay to also pause cpuidle a bit
earlier in these cases, like in dpm_suspend() for example. The point
is, it's really a limited short period of time for when cpuidle would
be paused, so I doubt it would have any impact on the consumed energy.
Right?

>
> > This could then be used by runtime PM based
> > cpuidle drivers as well. Would that be a way forward?
>
> The PM-runtime case should be addressed directly IMO, we only need to
> figure out how to do that.

If you have any other suggestions, I am listening. :-)

>
> I'm wondering how you are dealing with the case when user space
> prevents pd_dev from suspending via sysfs, for that matter.

That should work fine during runtime - because runtime PM is enabled
for the device.

Kind regards
Uffe