On Wed, Aug 21, 2024 at 1:15 PM Dhruva Gole <d-g...@ti.com> wrote:
>
> Hi,
>
> On May 10, 2015 at 01:19:52 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> >
> > If tick_broadcast_enter() fails in cpuidle_enter_state(),
> > try to find another idle state to enter instead of invoking
> > default_idle_call() immediately and returning -EBUSY which
> > should increase the chances of saving some energy in those
> > cases.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> > ---
>
> Found this during code review, hence dug up this old thread again,
>
> > drivers/cpuidle/cpuidle.c | 20 +++++++++++++++-----
> > 1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/cpuidle/cpuidle.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> > +++ linux-pm/drivers/cpuidle/cpuidle.c
> > @@ -73,7 +73,10 @@ int cpuidle_play_dead(void)
> > }
> >
> > static int find_deepest_state(struct cpuidle_driver *drv,
> > - struct cpuidle_device *dev, bool freeze)
> > + struct cpuidle_device *dev,
> > + unsigned int max_latency,
> > + unsigned int forbidden_flags,
> > + bool freeze)
> > {
> > unsigned int latency_req = 0;
> > int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
> > @@ -83,6 +86,8 @@ static int find_deepest_state(struct cpu
> > struct cpuidle_state_usage *su = &dev->states_usage[i];
> >
> > if (s->disabled || su->disable || s->exit_latency <=
> > latency_req
> > + || s->exit_latency > max_latency
> > + || (s->flags & forbidden_flags)
> > || (freeze && !s->enter_freeze))
> > continue;
> >
> > @@ -100,7 +105,7 @@ static int find_deepest_state(struct cpu
> > int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
> > struct cpuidle_device *dev)
> > {
> > - return find_deepest_state(drv, dev, false);
> > + return find_deepest_state(drv, dev, UINT_MAX, 0, false);
> > }
> >
> > static void enter_freeze_proper(struct cpuidle_driver *drv,
> > @@ -139,7 +144,7 @@ int cpuidle_enter_freeze(struct cpuidle_
> > * that interrupts won't be enabled when it exits and allows the tick
> > to
> > * be frozen safely.
> > */
> > - index = find_deepest_state(drv, dev, true);
> > + index = find_deepest_state(drv, dev, UINT_MAX, 0, true);
> > if (index >= 0)
> > enter_freeze_proper(drv, dev, index);
> >
> > @@ -168,8 +173,13 @@ int cpuidle_enter_state(struct cpuidle_d
> > * CPU as a broadcast timer, this call may fail if it is not
> > available.
> > */
> > if (broadcast && tick_broadcast_enter()) {
> > - default_idle_call();
> > - return -EBUSY;
> > + index = find_deepest_state(drv, dev,
> > target_state->exit_latency,
> > + CPUIDLE_FLAG_TIMER_STOP, false);
> > + if (index < 0) {
>
> Would this condition ever meet?
> If you see, the ret inside find_deepest_state is always starting with a 0 and
> then nobody is ever really making it negative again. So the func either
> returns a 0 or some positive value right?
>
> Since nobody has probably raised an issue about this in 9 years, is this
> basically dead code inside the if?
Yes, it is dead code now.
> Let me know what needs to be done here, I'd be happy to patch this up.
Please feel free to send a patch removing the redundant check.
> > + default_idle_call();
> > + return -EBUSY;
> > + }
> > + target_state = &drv->states[index];
> > }
> >
> > /* Take note of the planned idle state. */
> >
>
> --
