From patchwork Mon Oct 27 20:16:16 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 5166261 Return-Path: X-Original-To: patchwork-linux-samsung-soc@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 3D6459F318 for ; Mon, 27 Oct 2014 20:16:23 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 16FFC201DD for ; Mon, 27 Oct 2014 20:16:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E9050201D3 for ; Mon, 27 Oct 2014 20:16:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751890AbaJ0UQU (ORCPT ); Mon, 27 Oct 2014 16:16:20 -0400 Received: from smtp.codeaurora.org ([198.145.11.231]:40333 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751605AbaJ0UQT (ORCPT ); Mon, 27 Oct 2014 16:16:19 -0400 Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id AC5A513F7B1; Mon, 27 Oct 2014 20:16:18 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 486) id 8B0D513F7B2; Mon, 27 Oct 2014 20:16:18 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Received: from [10.46.167.8] (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: sboyd@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 49CA713F7A9; Mon, 27 Oct 2014 20:16:17 +0000 (UTC) Message-ID: <544EA810.6090502@codeaurora.org> Date: Mon, 27 Oct 2014 13:16:16 -0700 From: Stephen Boyd User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Marcin Jabrzyk , Russell King - ARM Linux CC: Kukjin Kim , Bartlomiej Zolnierkiewicz , Daniel Lezcano , linux-kernel@vger.kernel.org, kyungmin.park@samsung.com, linux-samsung-soc@vger.kernel.org, Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Mark Rutland , Chander Kashyap Subject: Re: =?windows-1252?Q?PROBLEM=3A=A0BUG__appearing_when_tr?= =?windows-1252?Q?ying_to_allocate_interrupt_on_Exynos_MCT_?= =?windows-1252?Q?after_CPU_hotplug?= References: <544907D4.1020409@samsung.com> <20141023140644.GI27405@n2100.arm.linux.org.uk> <54494BEE.9020702@codeaurora.org> <544A52B0.9050901@samsung.com> In-Reply-To: <544A52B0.9050901@samsung.com> X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-samsung-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 10/24/2014 06:22 AM, Marcin Jabrzyk wrote: > > > On 23/10/14 20:41, Stephen Boyd wrote: >> On 10/23/2014 07:06 AM, Russell King - ARM Linux wrote: >>> The CPU notifier is called via notify_cpu_starting(), which is called >>> with interrupts disabled, and a reason code of CPU_STARTING. >>> Interrupts >>> at this point /must/ remain disabled. >>> >>> The Exynos code then goes on to call exynos4_local_timer_setup() which >>> tries to reverse the free_irq() in exynos4_local_timer_stop() by >>> calling >>> request_irq(). Calling request_irq() with interrupts off has never >>> been >>> permissible. >>> >>> So, this code is wrong today, and it was also wrong when it was >>> written. >>> It /couldn't/ have been tested. It looks like this commit added this >>> buggy code: >>> >>> commit ee98d27df6827b5ba4bd99cb7d5cb1239b6a1a31 >>> Author: Stephen Boyd >>> Date: Fri Feb 15 16:40:51 2013 -0800 >>> >>> ARM: EXYNOS4: Divorce mct from local timer API >>> >>> Separate the mct local timers from the local timer API. This will >>> allow us to remove ARM local timer support in the near future and >>> gets us closer to moving this driver to drivers/clocksource. >>> >>> Acked-by: Kukjin Kim >>> Acked-by: Marc Zyngier >>> Cc: Thomas Abraham >>> Signed-off-by: Stephen Boyd >> >> I'm not so sure. It looks like in that patch I didn't change anything >> with respect to when things are called. In fact, it looks like we were >> calling setup_irq() there, but another patch around the same time >> changed that to request_irq() >> >> commit 7114cd749a12ff9fd64a2f6f04919760f45ab183 >> Author: Chander Kashyap >> Date: Wed Jun 19 00:29:35 2013 +0900 >> >> clocksource: exynos_mct: use (request/free)_irq calls for local >> timer registration >> >> Replace the (setup/remove)_irq calls for local timer >> registration with >> (request/free)_irq calls. This generalizes the local timer >> registration API. >> Suggested by Mark Rutland. >> >> Signed-off-by: Chander Kashyap >> Acked-by: Mark Rutland >> Reviewed-by: Tomasz Figa >> Signed-off-by: Kukjin Kim >> >> I don't believe setup_irq() allocates anything so we should probably go >> back to using that over request_irq() or explore requesting the irqs >> once and then enabling/disabling instead. >> > > So what would be a better way to handle this? Going back to setup_irq > or trying to enable/disable irqs on CPU hotplug? As this touched low > level things and it's rare case for setting/enabling irqs just after > CPU is coming back to life again. > The safest thing is setup_irq(), but do you care to try this patch? Doing the enable/disable is not as robust because request_irq() returns with the irq enabled and then we have to disable the irq to make things symmetric. This whole driver doesn't look like it's prepared for such a situation where the interrupt triggers before the clockevent is registered so this doesn't look like a problem in practice. Doing the disable right after request is typically bad though, and may not pass review. ----8<----- From: Stephen Boyd Subject: [PATCH] clocksource: exynos_mct: Avoid scheduling while atomic If we call request_irq() during the CPU_STARTING notifier we'll try to allocate an irq descriptor with GFP_KERNEL while we're running with irqs disabled. Just request the irqs at boot time and enable/disable them when a CPU comes up or goes down to avoid such problems. Signed-off-by: Stephen Boyd --- drivers/clocksource/exynos_mct.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c index 9403061a2acc..1800053b4644 100644 --- a/drivers/clocksource/exynos_mct.c +++ b/drivers/clocksource/exynos_mct.c @@ -467,13 +467,7 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt) if (mct_int_type == MCT_INT_SPI) { evt->irq = mct_irqs[MCT_L0_IRQ + cpu]; - if (request_irq(evt->irq, exynos4_mct_tick_isr, - IRQF_TIMER | IRQF_NOBALANCING, - evt->name, mevt)) { - pr_err("exynos-mct: cannot register IRQ %d\n", - evt->irq); - return -EIO; - } + enable_irq(evt->irq); irq_force_affinity(mct_irqs[MCT_L0_IRQ + cpu], cpumask_of(cpu)); } else { enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); @@ -488,7 +482,7 @@ static void exynos4_local_timer_stop(struct clock_event_device *evt) { evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); if (mct_int_type == MCT_INT_SPI) - free_irq(evt->irq, this_cpu_ptr(&percpu_mct_tick)); + disable_irq(evt->irq); else disable_percpu_irq(mct_irqs[MCT_L0_IRQ]); } @@ -522,8 +516,9 @@ static struct notifier_block exynos4_mct_cpu_nb = { static void __init exynos4_timer_resources(struct device_node *np, void __iomem *base) { - int err; + int err, cpu; struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick); + struct mct_clock_event_device *evt; struct clk *mct_clk, *tick_clk; tick_clk = np ? of_clk_get_by_name(np, "fin_pll") : @@ -549,7 +544,15 @@ static void __init exynos4_timer_resources(struct device_node *np, void __iomem WARN(err, "MCT: can't request IRQ %d (%d)\n", mct_irqs[MCT_L0_IRQ], err); } else { - irq_set_affinity(mct_irqs[MCT_L0_IRQ], cpumask_of(0)); + for_each_present_cpu(cpu) { + evt = per_cpu_ptr(&percpu_mct_tick, cpu); + if (request_irq(mct_irqs[MCT_L0_IRQ + cpu], + exynos4_mct_tick_isr, + IRQF_TIMER | IRQF_NOBALANCING, + "MCT", evt)) + pr_err("exynos-mct: cannot register IRQ\n"); + disable_irq(mct_irqs[MCT_L0_IRQ + cpu]); + } } err = register_cpu_notifier(&exynos4_mct_cpu_nb);