Re: [PATCH V2 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 06/13/2013 04:32 PM, Joseph Lo wrote:
> On Thu, 2013-06-13 at 17:30 +0800, Daniel Lezcano wrote:
>> On 06/13/2013 04:13 AM, Joseph Lo wrote:
>>> On Thu, 2013-06-13 at 01:41 +0800, Stephen Warren wrote:
>>>> On 06/04/2013 03:40 PM, Daniel Lezcano wrote:
>>>>> On 06/04/2013 12:48 PM, Joseph Lo wrote:
>>>>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>>>>>> into this state, it saves it's context and needs a proper configuration
>>>>>> in flow controller to power gate the CPU when CPU runs into WFI
>>>>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>>>>>> wake up event in flow controller.
>>>>>>
>>>>>> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> I would like to understand why there is a WARN with the
>>>>> CPUIDLE_FLAG_TIMER_STOP flag set before queuing this patch and ensure it
>>>>> is not the tree hiding the forest.
>>>>
>>>> Joseph, are you planning to post an updated series or respond to resolve
>>>> Daniel's question?
>>>
>>> I need more time to investigate the detail about what caused the WARN
>>> when apply the flag. And what's the difference if we didn't enable
>>> CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, then it only applies
>>> CLOCK_EVT_NOTIFY_BROADCAST_ON on CPU0.
>>
>> I think I got it for this one. It is a bug in the cpuidle driver's code.
>>
>> cpuidle_register_driver does get_cpu => CPU0
>>
>> Then __cpuidle_register_driver(drv, <CPU0>)
>>
>>  => __cpuidle_driver_init(drv, <CPU0>)
>>
>> Hence, the timer broadcast is initialized only for cpu0.
>>
>> That should have been fixed with:
>>
> OK, thanks.
> 
>> commit 82467a5a885ddd9f80309682159da8db510e7832
>> Author: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Date:   Fri Jun 7 21:53:09 2013 +0000
>>
>>     cpuidle: simplify multiple driver support
>>
>>     Commit bf4d1b5 (cpuidle: support multiple drivers) introduced support
>>     for using multiple cpuidle drivers at the same time.  It added a
>>     couple of new APIs to register the driver per CPU, but that led to
>>     some unnecessary code complexity related to the kernel config options
>>     deciding whether or not the multiple driver support is enabled.  The
>>     code has to work as it did before when the multiple driver support is
>>     not enabled and the multiple driver support has to be compatible with
>>     the previously existing API.
>>
>>     Remove the new API, not used by any driver in the tree yet (but
>>     needed for the HMP cpuidle drivers that will be submitted soon), and
>>     add a new cpumask pointer to the cpuidle driver structure that will
>>     point to the mask of CPUs handled by the given driver.  That will
>>     allow the cpuidle_[un]register_driver() API to be used for the
>>     multiple driver support along with the cpuidle_[un]register()
>>     functions added recently.
>>
>>     [rjw: Changelog]
>>     Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>>
>>> Why if I am moving the
>>> clockevent_notify of CLOCK_EVT_NOTIFY_BROADCAST_EXIT before
>>> "local_irq_enable" in the
>>> "cpuidle_enter_state" (drivers/cpuidle/cpuidle.c), then the warning
>>> message gone?
>>
>> Is the warning coming immediately, at the first time
>> CLOCK_EVT_NOTIFY_BROADCAST_EXIT is invoked or can it occur at different
>> moment ?
>>
> Not exactly at the 1st time, it may happen in the 3rd to 10th time. But
> it only happens once. I mean I did check the
> tick_broadcast_pending_mask, it only happens once the CPU didn't clear
> the tick_broadcast_pending_mask after CLOCK_EVT_NOTIFY_BROADCAST_EXIT.
> And cause the warning message. Then I didn't see it happen anymore.

I suspect when the CLOCK_EVENT_NOTIFY_EXIT is after the
local_irq_enable, we have the timer handler executed before where it
sets the tick_broadcast_pending_mask.

Hence in the tick_broadcast_oneshot_control function when we fall into
the condition:

 if (dev->next_event.tv64 == KTIME_MAX)
	goto out;

The tick_broadcast_pending_mask is not cleared and when re-entering with
CLOCK_EVENT_NOTIFY_ENTER, this mask is set and that raises the warning.

So, if I am not wrong, the condition to raise this warning would be:

 1. use the flag (of course), because the
CLOCK_EVT_NOTIFY_BROADCAST_EXIT is called after the local_irq_enabled

 2. there is no timer planned after the expired one.

 3. a new timer is created and the cpu enters the low power state again

The first step would be to create a simple program to reproduce easily
the warning.

First boot the kernel with init=/bin/bash (or whatever, the init process
is replaced by a shell), thus reducing considerably the number of timers.

Then try to raise the warning with cpu2:

	taskset 4 /bin/sleep 0.5

... several times until the warning appears.

As I don't have the hardware, this is based on assumptions, so may be I
am wrong.

-- 
 <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