On Mon, Feb 27, 2012 at 6:49 PM, Turquette, Mike <mturque...@ti.com> wrote: > On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee <rob....@linaro.org> wrote: >> +/** >> + * cpuidle_enter_wrap - performing timekeeping and irq around enter function >> + * @dev: pointer to a valid cpuidle_device object >> + * @drv: pointer to a valid cpuidle_driver object >> + * @index: index of the target cpuidle state. >> + */ >> +static inline int cpuidle_wrap_enter(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index, >> + int (*enter)(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int >> index)) >> +{ >> + ktime_t time_start, time_end; >> + s64 diff; >> + >> + time_start = ktime_get(); >> + >> + index = enter(dev, drv, index); >> + >> + time_end = ktime_get(); >> + >> + local_irq_enable(); >> + >> + diff = ktime_to_us(ktime_sub(time_end, time_start)); >> + if (diff > INT_MAX) >> + diff = INT_MAX; >> + >> + dev->last_residency = (int) diff; >> + >> + return index; >> +} > > Hi Rob, > > In a previous series I brought up the idea of not accounting for time > if a C-state transition fails. My post on that thread can be found > here: > http://article.gmane.org/gmane.linux.ports.arm.kernel/149293/ > > How do you feel about adding something like the following? > > if (IS_ERR(index)) > dev->last_residency = 0; > return index; > > Obviously it will up to the platforms to figure out how to propagate > that error up from their respective low power code.
To be completely clear on what you're asking for, from cpuidle_idle_call in drivers/cpuidle/cpuidle.c: ... target_state = &drv->states[next_state]; trace_power_start(POWER_CSTATE, next_state, dev->cpu); trace_cpu_idle(next_state, dev->cpu); entered_state = target_state->enter(dev, drv, next_state); trace_power_end(dev->cpu); trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu); 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 += (unsigned long long)dev->last_residency; dev->states_usage[entered_state].usage++; } ... Note the "if (entered_state >= 0)". This ultimately prevents the cpuidle device time accounting upon an negative value being returned. So are you asking for the if(IS_ERR(index)) check to prevent the unnecessary last_residency time calculation in the wrapper, or to make sure a last_residency is zero upon failure? (or both?) It seems like a bug (or lack or documentation at best) in the code that exists today to not zero out dev->last_residency upon a negative return value as this value is used by the governors upon the next idle. So to ensure last_residency is 0 upon failure, I think it'd be best to add that to an new else statement immediately following the "if (entered_state >=))" so that any platform cpuidle driver that returns a negative will have the last_residency zeroed out, not just those that use en_core_tk_irqen. > > Regards, > Mike _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev