lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
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