Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752290AbbFYN4C (ORCPT ); Thu, 25 Jun 2015 09:56:02 -0400 Received: from www.linutronix.de ([62.245.132.108]:48597 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751573AbbFYNz7 (ORCPT ); Thu, 25 Jun 2015 09:55:59 -0400 Date: Thu, 25 Jun 2015 15:55:56 +0200 (CEST) From: Thomas Gleixner To: Sudeep Holla cc: linux-kernel@vger.kernel.org, Suzuki Poulose , Lorenzo Pieralisi , Catalin Marinas , "Rafael J. Wysocki" , Preeti U Murthy Subject: Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control if !GENERIC_CLOCKEVENTS_BROADCAST In-Reply-To: <1435228065-2428-1-git-send-email-sudeep.holla@arm.com> Message-ID: References: <1435228065-2428-1-git-send-email-sudeep.holla@arm.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5908 Lines: 179 On Thu, 25 Jun 2015, Sudeep Holla wrote: > tick_broadcast_enter returns 0 when CPU can switch to broadcast > timer and non-zero otherwise. However when GENERIC_CLOCKEVENTS_BROADCAST > and TICK_ONESHOT are disabled, tick_broadcast_oneshot_control returns 0 > which indicates to the CPUIdle framework that the CPU can enter deeper > idle states even when the CPU local timer will be shutdown. If the > target state needs broadcast but not broadcast timer is available, then > the CPU can not resume back from that idle state. > > This patch returns error when there's no broadcast timer support > available so that CPUIdle framework prevents the CPU from entering any > idle states losing the local timer. That's wrong and breaks stuff which does not require the broadcast nonsense. If TICK_ONESHOT is disabled, then everything is in periodic mode and tick_broadcast_enter() rightfully returns 0. Ditto for 'highres=off' on the command line. But there is a case which is not correctly handled right now. That's what you are trying to solve in the wrong way. If GENERIC_CLOCKEVENTS_BROADCAST=n or GENERIC_CLOCKEVENTS_BROADCAST=y and no broadcast device is available, AND cpu local tick device has the C3STOP flag set, then we have no way to tell the idle code that going deep is not allowed. So we need to be smarter than blindly changing a return value. Completely untested patch below. Thanks, tglx --- diff --git a/include/linux/tick.h b/include/linux/tick.h index 4191b5623a28..8731a58dd747 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -63,11 +63,7 @@ extern void tick_broadcast_control(enum tick_broadcast_mode mode); static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { } #endif /* BROADCAST */ -#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT) extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state); -#else -static inline int tick_broadcast_oneshot_control(enum tick_broadcast_state state) { return 0; } -#endif static inline void tick_broadcast_enable(void) { diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index d39f32cdd1b5..52c8d01b956b 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -662,24 +662,15 @@ static void broadcast_shutdown_local(struct clock_event_device *bc, clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN); } -/** - * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode - * @state: The target state (enter/exit) - * - * The system enters/leaves a state, where affected devices might stop - * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups. - * - * Called with interrupts disabled, so clockevents_lock is not - * required here because the local clock event device cannot go away - * under us. - */ -int tick_broadcast_oneshot_control(enum tick_broadcast_state state) +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state) { struct clock_event_device *bc, *dev; - struct tick_device *td; int cpu, ret = 0; ktime_t now; + if (!tick_broadcast_device.evtdev) + return -EBUSY; + /* * Periodic mode does not care about the enter/exit of power * states @@ -687,15 +678,7 @@ int tick_broadcast_oneshot_control(enum tick_broadcast_state state) if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) return 0; - /* - * We are called with preemtion disabled from the depth of the - * idle code, so we can't be moved away. - */ - td = this_cpu_ptr(&tick_cpu_device); - dev = td->evtdev; - - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) - return 0; + dev = this_cpu_ptr(&tick_cpu_device)->evtdev; raw_spin_lock(&tick_broadcast_lock); bc = tick_broadcast_device.evtdev; @@ -938,6 +921,13 @@ bool tick_broadcast_oneshot_available(void) return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false; } +#else +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state) +{ + if (!tick_broadcast_device.evtdev) + return -EBUSY; + return 0; +} #endif void __init tick_broadcast_init(void) diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 17f144450050..f66c11a30348 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -342,6 +342,27 @@ out_bc: tick_install_broadcast_device(newdev); } +/** + * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode + * @state: The target state (enter/exit) + * + * The system enters/leaves a state, where affected devices might stop + * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups. + * + * Called with interrupts disabled, so clockevents_lock is not + * required here because the local clock event device cannot go away + * under us. + */ +int tick_broadcast_oneshot_control(enum tick_broadcast_state state) +{ + struct tick_device *td = this_cpu_ptr(&tick_cpu_device); + + if (!(td->evtdev->features & CLOCK_EVT_FEAT_C3STOP)) + return 0; + + return __tick_broadcast_oneshot_control(state); +} + #ifdef CONFIG_HOTPLUG_CPU /* * Transfer the do_timer job away from a dying cpu. diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h index 42fdf4958bcc..a4a8d4e9baa1 100644 --- a/kernel/time/tick-sched.h +++ b/kernel/time/tick-sched.h @@ -71,4 +71,14 @@ extern void tick_cancel_sched_timer(int cpu); static inline void tick_cancel_sched_timer(int cpu) { } #endif +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST +extern int __tick_broadcast_oneshot_control(enum tick_broadcast_state state); +#else +static inline int +__tick_broadcast_oneshot_control(enum tick_broadcast_state state) +{ + return -EBUSY; +} +#endif + #endif -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/