Re: [PATCH] clockevents: Leave broadcast device shtudown only if thecurrent device is always running.

From: Shilimkar, Santosh
Date: Wed Apr 18 2012 - 01:42:15 EST

On Wed, Apr 18, 2012 at 12:41 AM, Suresh Siddha
<suresh.b.siddha@xxxxxxxxx> wrote:
> On Tue, 2012-04-17 at 13:44 +0530, Santosh Shilimkar wrote:
>> On Tuesday 17 April 2012 03:33 AM, Suresh Siddha wrote:
>> > On Sun, 2012-04-15 at 18:27 +0530, Santosh Shilimkar wrote:
>> >> The main issue is, you are not letting the one time setup
>> >> needed for the broadcast device. Even with above change,
>> >> the 'tick_broadcast_device.evtdev' is still in SHUTDOWN
>> >> mode and the event handler is pointing to clockevents_handle_noop()
> If you see this, handler was not even set to the periodic mode handler.
> So the periodic mode for the broadcast device also was not working
> before (even with out the recent change).
> In tick_check_broadcast_device(), we do this:
>        if (!cpumask_empty(tick_get_broadcast_mask()))
>                tick_broadcast_start_periodic(dev);
> So with out the CLOCK_EVT_NOTIFY_BROADCAST_ON notification, we won't be
> even setting up the periodic mode correctly.
> I just extended this to the oneshot mode too, exposing the issue in your
> cpuidle code.
Periodic mode for broad-cast device was any way not much important and hence
the issue was hidden I guess.

>> >
>> > On x86, idle driver (like drivers/idle/intel_idle.c:
>> > __setup_broadcast_timer()) calls clockevents_notify() with
>> > CLOCK_EVT_NOTIFY_BROADCAST_ON, during which we do
>> > tick_broadcast_setup_oneshot(). See tick_do_broadcast_on_off() which
>> > does this. That should setup the evtdev handler, mode etc.
>> >
>> > And during the next CLOCK_EVT_NOTIFY_BROADCAST_ENTER we program the next
>> > broadcast event.
>> >
>> I see.
>> >
>> > Anyways, can you add CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF notifications in
>> > your idle driver to see if it addresses the problem. That is the correct
>> > thing to do here.
>> >
>> Before your commit 77b0d60c{clockevents: Leave the broadcast device..},
>> my idle driver was working without CLOCK_EVT_NOTIFY_BROADCAST_ON
>> notifier. Now it's clear that it was relying on the default
>> 'tick_broadcast_setup_oneshot()' which was happening during boot.
>> And ofcource notifying explicitly with BROADCAST_ON does the
>> tick setup. Will update my idle driver accordingly but can
>> you please explain me why the proposed patch [1] is not correct
>> approach ? It should be also do what you intended to do
>> with minimal change, right ?
> Please see the above. Essentially irrespective of the recent change,
> periodic mode was also broken. Probably no one cared about that mode and
> we didn't notice that issue so far. Anyhow, the correct thing to do here
> is to add those notifications to the cpuidle driver. Can you please
> check if the appended/untested patch fixes your issue? If so, please
> push this to your driver. thanks.
As I mentioned earlier, I have already tried my driver with
CLOCK_EVT_NOTIFY_BROADCAST_ON update yesterday and pushed
a patch for the same.
Thanks for the periodic mode point which i missed previously.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at