Re: [RFC PATCH] cpuidle: mvebu: indicate failure to enter deeper sleep states

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 10/03/2015 10:24 AM, Russell King wrote:
> The cpuidle ->enter method expects the return value to be the sleep
> state we entered.  Returning negative numbers or other codes is not
> permissible since coupled CPU idle was merged.



> At least some of the mvebu_v7_cpu_suspend() implementations return the
> value from cpu_suspend(), which returns zero if the CPU vectors back
> into the kernel via cpu_resume() (the success case), or the non-zero
> return value of the suspend actor, or one (failure cases).
>
> We do not want to be returning the failure case value back to CPU idle
> as that indicates that we successfully entered one of the deeper idle
> states.  Always return zero instead, indicating that we slept for the
> shortest amount of time.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> Please think about this - having only an Armada 388 platform, I can't
> test this due to CPU idle being disabled there.  However, the code paths
> indicate that if we fail to sleep in armada_370_xp_pmsu_idle_enter() or
> armada_38x_do_cpu_suspend() fail to sleep, we end up reporting that we
> successfully slept in CPU idle state 1 in both cases.
>
> Rafael - the code in cpuidle.c prior to the addition of coupled CPU idle
> seems to allow the return of negative error numbers from the ->enter()
> method:
>
>          if (entered_state >= 0) {
>                  /* Update cpuidle counters */
>                  /* This can be moved to within driver enter routine
>                   * but that results in multiple copies of same code.
>                   */
>                  dev->states_usage[entered_state].time += dev->last_residency;
>                  dev->states_usage[entered_state].usage++;
>          } else {
>                  dev->last_residency = 0;
>          }
>
> Since coupled CPU idle, negative return codes will give a negative table
> index inside cpuidle_state_is_coupled() - which is obviously buggy.  Does
> this need fixing?  If so, this patch below is wrong, and we should be
> returning a negative errno rather than zero on failure to enter a low
> power state.

It is not possible since the commit 0b89e9aa2.

This change was to prevent to re-enable the local interrupt when we are 
in the process of coupling an idle state and let the coupled code to 
handle that.

If the backend driver changed the state in our back (for example to a 
shallower state) to a non coupled one, the local interrupt will be 
re-enabled. If at the contrary, the backend driver choose a deeper idle 
state which is coupled, we won't re-enable the interrupt. I don't know 
any driver doing this but the code allows it (not sure it is a good 
thing - anyway).

So the test:

if (!cpuidle_state_is_coupled(dev, drv, entered_state))

should be:

if (!cpuidle_state_is_coupled(dev, drv, *index*))

... in order to have the decisions made when entering this function 
symmetric to the ones which will be taken when exiting the function 
because they will be based on the same index.

It is the same behavior than the broadcast timer enter/exit in the same 
function.

To be clean, the above should be fixed.

But in any case, armada_370_xp_pmsu_idle_enter and 
armada_38x_do_cpu_suspend can return a negative value because they don't 
depend on the ARCH_NEEDS_CPU_IDLE_COUPLED, and cpuidle_state_is_coupled 
is an inline for 'return false'.

  -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog