Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling

[Sudeep Holla]

On 13/01/16 21:58, Rafael J. Wysocki wrote:
> Hi,
>
> On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > Hi Rik,
> >
> > This change break idle on ARM64(may be on other ARM?) platform.
> > Sorry for reporting late, but missed to check cpuidle in -next.
>
> OK, so first of all, how exactly is idle broken on those systems?

Sorry for the hasty bug report.

> Do they crash or does something different happen?  If something
> different happens, then what's that?

No they just don't enter any idle states(as if CPUIdle was disabled)

[...]

> > diff --git i/drivers/cpuidle/governors/menu.c w/drivers/cpuidle/governors/menu.c
> > index 7b0971d97cc3..7c7f4059705a 100644
> > --- i/drivers/cpuidle/governors/menu.c
> > +++ w/drivers/cpuidle/governors/menu.c
> > @@ -330,7 +330,8 @@ static int menu_select(struct cpuidle_driver *drv,
> > struct cpuidle_device *dev)
> >           * We want to default to C1 (hlt), not to busy polling
> >           * unless the timer is happening really really soon.
> >           */
> > -       if (interactivity_req > 20 &&
> > +       if (((interactivity_req && interactivity_req > 20) ||
>
> Well, if interactivity_req > 20, then it also is different from 0, so
> the first check should not be necessary here.

True, I just wanted to avoid using interactivity_req when 0, it was just
a quick hack.

> > +               data->next_timer_us > 20) &&
>
> I guess that this simply restores the previous behavior on the
> platforms in question.

Yes.

> Now, the reason why it may matter is because
> CPUIDLE_DRIVER_STATE_START is 0 and so data->last_state_idx may end up
> as -1 on them.  So I think this piece of code only ever makes sense if
> CPUIDLE_DRIVER_STATE_START is 1.

That's correct.

--
Regards,
Sudeep