Do we need to fix below dump during cpu hot plug operation?

Commit Message

Linus Walleij Oct. 19, 2012, 9:37 a.m. UTC

On Fri, Oct 19, 2012 at 6:58 AM, Shawn Guo <[e-mail]> 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 <[e-mail]>
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 <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
 arch/arm/kernel/smp_twd.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

 		twd_timer_rate = clk_get_rate(twd_clk);

Comments

Russell King - ARM Linux Oct. 19, 2012, 9:40 a.m. UTC | #1

On Fri, Oct 19, 2012 at 11:37:48AM +0200, Linus Walleij wrote:
> @@ -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);

While we're here, can we please ensure that the CLK API is properly
respected, and use IS_ERR(twd_clk) to determine of the clock is valid
or not (and not use IS_ERR_OR_NULL()).

Linus Walleij Oct. 19, 2012, 9:50 a.m. UTC | #2

On Fri, Oct 19, 2012 at 11:40 AM, Russell King - ARM Linux
<[e-mail]> wrote:
> On Fri, Oct 19, 2012 at 11:37:48AM +0200, Linus Walleij wrote:
>> @@ -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);
>
> While we're here, can we please ensure that the CLK API is properly
> respected, and use IS_ERR(twd_clk) to determine of the clock is valid
> or not (and not use IS_ERR_OR_NULL()).

OK I'll refine this thing if it works for Freescale.

I'm under the impression that the use of IS_ERR_OR_NULL() in the
setup() section below the changed code is correct however, since it'll
catch the situation where the clock API is disabled (and returns rate
0) and then proceed to calibrate the rate properly?

Yours,
Linus Walleij

Patch

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))