From patchwork Wed Feb 25 23:36:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 5885171 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id D0A3CBF440 for ; Wed, 25 Feb 2015 23:12:51 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id DA3A320304 for ; Wed, 25 Feb 2015 23:12:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9A79B200F3 for ; Wed, 25 Feb 2015 23:12:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752769AbbBYXMs (ORCPT ); Wed, 25 Feb 2015 18:12:48 -0500 Received: from v094114.home.net.pl ([79.96.170.134]:55076 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752617AbbBYXMs (ORCPT ); Wed, 25 Feb 2015 18:12:48 -0500 Received: from afcl80.neoplus.adsl.tpnet.pl (95.49.63.80) (HELO vostro.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer v0.80) id c67bfea85a644c59; Thu, 26 Feb 2015 00:12:43 +0100 From: "Rafael J. Wysocki" To: Lorenzo Pieralisi , Daniel Lezcano Cc: "linux-pm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Peter Zijlstra Subject: Re: [PATCH 1/2] drivers: cpuidle: remove stale irq disabling call in cpuidle_enter_freeze() Date: Thu, 26 Feb 2015 00:36:10 +0100 Message-ID: <1789709.hzh3CkisvM@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/3.19.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20150225143917.GC20214@red-moon> References: <1424800730-32059-1-git-send-email-lorenzo.pieralisi@arm.com> <54EDD883.30608@linaro.org> <20150225143917.GC20214@red-moon> MIME-Version: 1.0 Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wednesday, February 25, 2015 02:39:17 PM Lorenzo Pieralisi wrote: > On Wed, Feb 25, 2015 at 02:13:23PM +0000, Daniel Lezcano wrote: > > On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote: > > > On return from cpuidle_enter_freeze() irqs are re-enabled by the function > > > caller (ie cpuidle_idle_call) in the idle loop. This patch removes a stale > > > local_irq_disable() call and its stale comment in cpuidle_enter_freeze(), > > > since they disagree and do not serve a useful purpose. > > > > > > Cc: Rafael J. Wysocki > > > Cc: Daniel Lezcano > > > Signed-off-by: Lorenzo Pieralisi > > > --- > > > drivers/cpuidle/cpuidle.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > > > index 4d53458..f47edc6c 100644 > > > --- a/drivers/cpuidle/cpuidle.c > > > +++ b/drivers/cpuidle/cpuidle.c > > > @@ -144,9 +144,6 @@ void cpuidle_enter_freeze(void) > > > cpuidle_enter(drv, dev, index); > > > else > > > arch_cpu_idle(); > > > - > > > - /* Interrupts are enabled again here. */ > > > - local_irq_disable(); > > > } > > > > Hmm, I think Rafael added this prevent lockdep to raise a warning. > > Ok, so the comment is there to say "at this point of execution IRQs > are enabled", it does not refer to local_irq_disable() call effects, > that's misleading and not necessarily nice, at least it should > be explained. > > > Otherwise, cpuidle_enter or arch_cpu_idle enables the irq again and then > > when exiting the cpu_idle_call, we enable them again, so leading to a > > lockdep WARN in trace_hardirqs_on_caller. > > Would not it be better to enable irqs in cpuidle_enter_freeze() on > returning from enter_freeze_proper() and remove the local_irq_enable() > call in the cpuidle_idle_call() before jumping to exit_idle ? > > > That said, if we have to do this, it may reveal something is wrong in > > the code. > > I just spotted code through inspection, I have to say at the moment it > is not very clear what it is meant to achieve, so I put together this > patch. So there are two code paths in cpuidle_idle_call(), the enter_freeze_proper() one which does *not* re-enable interrupts and the one you modified which does that. The local_irq_disable() is to keep things consistent. I'm not entirely against of re-arranging things here, but a patch like the (untested) one below might be more appropriate. Rafael (who would appreciate it if people asked questions instead of sending patches on a hunch). --- drivers/cpuidle/cpuidle.c | 2 +- kernel/sched/idle.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: linux-pm/drivers/cpuidle/cpuidle.c =================================================================== --- linux-pm.orig/drivers/cpuidle/cpuidle.c +++ linux-pm/drivers/cpuidle/cpuidle.c @@ -132,6 +132,7 @@ void cpuidle_enter_freeze(void) index = cpuidle_find_deepest_state(drv, dev, true); if (index >= 0) { enter_freeze_proper(drv, dev, index); + local_irq_enable(); return; } @@ -146,7 +147,6 @@ void cpuidle_enter_freeze(void) arch_cpu_idle(); /* Interrupts are enabled again here. */ - local_irq_disable(); } /** Index: linux-pm/kernel/sched/idle.c =================================================================== --- linux-pm.orig/kernel/sched/idle.c +++ linux-pm/kernel/sched/idle.c @@ -116,7 +116,6 @@ static void cpuidle_idle_call(void) */ if (idle_should_freeze()) { cpuidle_enter_freeze(); - local_irq_enable(); goto exit_idle; }