Message-ID: <CAMQu2gw0gd6HBVWRRLATH=BfLwgJjgr-pwfQk-HJxuK3yovzgQ@mail.gmail.com>
Date:	Wed, 18 Apr 2012 11:11:53 +0530
From:	"Shilimkar, Santosh" <santosh.shilimkar@...com>
To:	Suresh Siddha <suresh.b.siddha@...el.com>
Cc:	tglx@...utronix.de, linux-kernel@...r.kernel.org,
	johnstul@...ibm.com
Subject: Re: [PATCH] clockevents: Leave broadcast device shtudown only if the
 current device is always running.

On Wed, Apr 18, 2012 at 12:41 AM, Suresh Siddha
<suresh.b.siddha@...el.com> 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.

Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists