jouni.hogander@xxxxxxxxx (Högander Jouni) writes: > "ext Kevin Hilman" <khilman@xxxxxxxxxx> writes: > >> jouni.hogander@xxxxxxxxx (Högander Jouni) writes: >> >>> "ext Kevin Hilman" <khilman@xxxxxxxxxx> writes: >>> >>>> "Rajendra Nayak" <rnayak@xxxxxx> writes: >>>> >>>>>> "Rajendra Nayak" <rnayak@xxxxxx> 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. > > Even in that case there is still period in idle loop, were it is possible > that latency requirement changes and cpuidle doesn't know this on frist > enter state. This is after need_resched. > > I don't know this area too good, but isn't it also possible that > interrupt which is handled, has caused latency requirement change and > there is no NEED_RESCHED flag set? IMHO, these things should be handled on a per-driver/subsystem basis, not by a global interrupt lock. Remember that this patchset is still missing a very important piece which is the omap3_idle_bm_check(). This is an of how these special cases are handled. For example, if an interrupt fires which triggers driver activity, this function is supposed to look for clock activity and potentially adjust the depth of sleep that can be entered. >> >>>> >>>>> 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. > > If done this way, it should be checked that target state is still > valid before entering omap_sram_idle. How could CPUidle "enter state" > know wether it is still valid? > Again, this is the job of the OMAP specific idle loop, not the global CPUidle code. For example, the current idle loop simply does: if (omap_irq_pending()) goto out; so it doesn't enter idle if there's an interrupt pending. So, I'm not arguing that your concerns aren't valid, because indeed they are valid corner cases. I'm just arguing that these special cases should be handled in the OMAP code, not the global CPUidle code. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
