From patchwork Fri Oct 19 09:37:48 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Walleij X-Patchwork-Id: 1617291 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id 817E1DF2AB for ; Fri, 19 Oct 2012 09:40:06 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TP91h-0007HC-U3; Fri, 19 Oct 2012 09:37:58 +0000 Received: from mail-wi0-f177.google.com ([209.85.212.177]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TP91c-0007Gu-7N for linux-arm-kernel@lists.infradead.org; Fri, 19 Oct 2012 09:37:54 +0000 Received: by mail-wi0-f177.google.com with SMTP id hj13so14656wib.0 for ; Fri, 19 Oct 2012 02:37:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-gm-message-state; bh=GjZ4d27aRlQZ1Xup/+stbExcgXpI4usaTMesYA3EYRE=; b=QxSFklR5Nh5Xm9ezqImFUD2ziTBNuAw2GBIxOrSa/enB+WE+D1Wj/6BFuqf//vQXRG /V1D+wnfxFtcUu728aNI/X4gY4uSaD6kwmhXlKN9VPeYJ9jHrachaUj2nTCvBjqEoiUR 4a28ucGN1jL5goPjs5+z5HNV/ebLpao9UKhpTHbxYyTN/Nmb0hJBHTLw5mUm/5EKQS8z PmOp7Rco5HweDjpPYx6ZNLAVS7ykwnlW5RyW1UAQyqDzxfkQOH4CPI3FGUjHJcT9JTiz vX1lD6lOrLq4pMmoEvWyJRv8StSxsKQED9IGi8Y4OhK5WTGZajcuyJh26QldyE79vQUf RCCg== MIME-Version: 1.0 Received: by 10.180.101.230 with SMTP id fj6mr1829302wib.16.1350639468354; Fri, 19 Oct 2012 02:37:48 -0700 (PDT) Received: by 10.223.36.4 with HTTP; Fri, 19 Oct 2012 02:37:48 -0700 (PDT) In-Reply-To: <20121019045802.GA21078@S2101-09.ap.freescale.net> References: <20121019015621.GA17360@nchen-desktop> <20121019045802.GA21078@S2101-09.ap.freescale.net> Date: Fri, 19 Oct 2012 11:37:48 +0200 Message-ID: Subject: Re: Do we need to fix below dump during cpu hot plug operation? From: Linus Walleij To: Shawn Guo X-Gm-Message-State: ALoCoQlORr9RE7vP3P/cLKRKCSEL8VeZYncsvVbkmB4fRNA6FTwuBNhOsb3bj7deFxWTIAMO68ES X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.212.177 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Russell King , Peter Chen , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Fri, Oct 19, 2012 at 6:58 AM, Shawn Guo wrote: > Linus, Russell, > Does it seem to be a valid fix? Yes, but this patch is very incomplete. It only accounts for handling the fact that the *twd_clk has already been fetched. If the clock .setup() and .stop() is going to be called repeatedly, look over the entire .setup() and .stop() function pair and make it reentrant, right now it's pure luck if it works. There are obviously more things that need fixing here. You need to move this_cpu_clk = __this_cpu_ptr(twd_evt); and check that before trying to register the clockevent right, because if setup() is called repeatedly, you will add a new clockevent every time, which is a massive memory leak. Isn't the easiest and most straigt-forward fix something like this: From 202c411f3212f74a5d2525ca291b249e1599b64e Mon Sep 17 00:00:00 2001 From: Linus Walleij Date: Fri, 19 Oct 2012 11:30:47 +0200 Subject: [PATCH] ARM: SMP_TWD: make setup()/stop() reentrant This makes the SMP_TWD clock .setup()/.stop() pair reentrant by not re-fetching the clk and re-registering the clock event every time .setup() is called. We also make sure to call the clk_enable()/clk_disable() pair on subsequent calls. As it has been brought to my knowledge that this pair is going to be called from atomic contexts for CPU clusters coming and going, the clk_prepare()/clk_unprepare() calls cannot be called on subsequent .setup()/.stop() iterations. The patch assumes that the code will make sure that twd_set_mode() is called through .set_mode() on the clock event *after* the .setup() call, so that the timer registers are fully re-programmed after a new .setup() cycle. Reported-by: Peter Chen Signed-off-by: Linus Walleij --- arch/arm/kernel/smp_twd.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) twd_timer_rate = clk_get_rate(twd_clk); diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c index e1f9069..1ac637b 100644 --- a/arch/arm/kernel/smp_twd.c +++ b/arch/arm/kernel/smp_twd.c @@ -31,6 +31,7 @@ static void __iomem *twd_base; static struct clk *twd_clk; static unsigned long twd_timer_rate; +static bool initial_setup_called; static struct clock_event_device __percpu **twd_evt; static int twd_ppi; @@ -93,6 +94,8 @@ static void twd_timer_stop(struct clock_event_device *clk) { twd_set_mode(CLOCK_EVT_MODE_UNUSED, clk); disable_percpu_irq(clk->irq); + if (twd_clk) + clk_disable(twd_clk); } #ifdef CONFIG_COMMON_CLK @@ -273,8 +276,21 @@ static int __cpuinit twd_timer_setup(struct clock_event_device *clk) { struct clock_event_device **this_cpu_clk; - if (!twd_clk) - twd_clk = twd_get_clock(); + /* + * If the basic setup has been done before, don't bother + * with yet again looking up the clock and register the clock + * source. + */ + if (initial_setup_called) { + if (twd_clk) + clk_enable(twd_clk); + __raw_writel(0, twd_base + TWD_TIMER_CONTROL); + enable_percpu_irq(clk->irq, 0); + return 0; + } + initial_setup_called = true; + + twd_clk = twd_get_clock(); if (!IS_ERR_OR_NULL(twd_clk))