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

[Sudeep Holla <sudeep.holla@arm.com>]

On 13/01/16 21:58, Rafael J. Wysocki wrote:
> Hi,
>
> On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@arm.com> 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