PROBLEM: BUG appearing when trying to allocate interrupt on Exynos MCT after CPU hotplug
Stephen Boyd
sboyd at codeaurora.org
Mon Oct 27 13:16:16 PDT 2014
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 <sboyd at codeaurora.org>
>>> 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 <kgene.kim at samsung.com>
>>> Acked-by: Marc Zyngier <marc.zyngier at arm.com>
>>> Cc: Thomas Abraham <thomas.abraham at linaro.org>
>>> Signed-off-by: Stephen Boyd <sboyd at codeaurora.org>
>>
>> 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 <chander.kashyap at linaro.org>
>> 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 <chander.kashyap at linaro.org>
>> Acked-by: Mark Rutland <mark.rutland at arm.com>
>> Reviewed-by: Tomasz Figa <t.figa at samsung.com>
>> Signed-off-by: Kukjin Kim <kgene.kim at samsung.com>
>>
>> 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 <sboyd at codeaurora.org>
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 <sboyd at codeaurora.org>
---
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);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
More information about the linux-arm-kernel
mailing list