Re: [PATCH 01/02] OMAP3 CPUidle driver

[Kevin Hilman <khilman@mvista.com>]

jouni.hogander@nokia.com (Högander Jouni) writes:

> "ext Kevin Hilman" <khilman@mvista.com> writes:
>
>> "Rajendra Nayak" <rnayak@ti.com> writes:
>>
>>>> "Rajendra Nayak" <rnayak@ti.com> writes:
>>>> 
>>>> > This patch adds the OMAP3 cpuidle driver. Irq enable/disable is done
>>>> > in the core cpuidle driver before it queries the governor for the
>>>> > next state.
>>>> 
>>>> Can you explain why you need the IRQ/FIQ disable added to 
>>>> cpuidle_idle_call()
>>>> 
>>>
>>> This was done to prevent any interrupts firing in between a 
>>> cpuidle_curr_governor->select() and target_state->enter().
>>
>> I understand that, but I still don't understand exactly what you're
>> trying to prevent.  Did you have a specific bug that this prevented?
>>
>>> An interrupt in between could end up with a previously selected 
>>> state to be programmed.
>>
>> Remember that this function _is_ the idle loop, meaning when this runs
>> nothing else is happening.  After the select, if other system activity
>> has happened (e.g. and interrupt, or thread wakeup etc.), it will run
>> before the target_state->enter() because of the check for
>> need_resched().
>
> What happens if this interrupt, or thread wakeup causes change on
> latency requirements? Then we are entering sleep state which was
> selected using wrong latency requirement data.

If a thread is awoken by an interrupt, then need_resched() will be
true, and the idle loop will exit without trying to enter the idle
state.

>>
>>> Any suggestions on a better way to handle this?
>>
>> Just drop the IRQ/FIQ disables altogether.
>
> At least these are needed at some point in idle loop. Otherwise we
> might stepout from idle and sram in a point where it is not acceptable.

Agreed, interrupt enable/disable is needed in the idle loop, but not
in the CPUidle code.  

The current OMAP idle loop code (omap2_pm_idle) already does this, but
the CPUidle "enter state" hook does not.  

The omap3_enter_idle() function should be the one who enables/disables
interrupts.  At a minimulm, the omap_sram_idle should be called with
interrupts disabled.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html