[patch] clockevents_notify() need to be called with irq's enabled

[patch] clockevents_notify() need to be called with irq's enabled

[Suresh Siddha]

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: clockevents_notify() need to be called with irq's enabled

Currently clockevents_notify() is called with interrupts enabled at some
places and interrupts disabled at some other places.

This results in a deadlock in this scenario.

cpu A holding the clockevents_lock in clockevents_notify() with irq enabled
cpu B waiting for the clockevents_lock in clockevents_notify() with irq disabled
cpu C doing set_mtrr() which will try to rendezvous of all the cpus.

This will result in C and A come to the rendezvous point and waiting for B.
B stuck forever waiting for the spinlock and thus not reaching rendezvous
point.

Fix the usage of clockevents_notify() so that it will always be called
with interrupts enabled and thus avoid the above deadlock.

This issue left us wondering if we need to change the MTRR rendezvous logic to
use stop machine logic (instead of smp_call_function) or add a check
in spinlock debug code to see if there are other spinlocks which gets
taken under both interrupts enabled/disabled conditions.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---

Index: tip/arch/x86/kernel/process.c
===================================================================
--- tip.orig/arch/x86/kernel/process.c
+++ tip/arch/x86/kernel/process.c
@@ -520,17 +520,13 @@ static void c1e_idle(void)
 			       cpu);
 			local_irq_disable();
 		}
+		local_irq_enable();
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
+		local_irq_disable();
 
 		default_idle();
 
-		/*
-		 * The switch back from broadcast mode needs to be
-		 * called with interrupts disabled.
-		 */
-		 local_irq_disable();
 		 clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
-		 local_irq_enable();
 	} else
 		default_idle();
 }
Index: tip/drivers/acpi/processor_idle.c
===================================================================
--- tip.orig/drivers/acpi/processor_idle.c
+++ tip/drivers/acpi/processor_idle.c
@@ -829,16 +829,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (unlikely(!pr))
 		return 0;
 
+	local_irq_enable();
+	lapic_timer_state_broadcast(pr, cx, 1);
 	local_irq_disable();
 
 	/* Do not access any ACPI IO ports in suspend path */
 	if (acpi_idle_suspend) {
 		local_irq_enable();
 		cpu_relax();
+		lapic_timer_state_broadcast(pr, cx, 0);
 		return 0;
 	}
 
-	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
 	kt2 = ktime_get_real();
@@ -873,6 +875,13 @@ static int acpi_idle_enter_simple(struct
 	if (acpi_idle_suspend)
 		return(acpi_idle_enter_c1(dev, state));
 
+	local_irq_enable();
+	/*
+	 * Must be done before busmaster disable as we might need to
+	 * access HPET !
+	 */
+	lapic_timer_state_broadcast(pr, cx, 1);
+
 	local_irq_disable();
 	current_thread_info()->status &= ~TS_POLLING;
 	/*
@@ -884,15 +893,10 @@ static int acpi_idle_enter_simple(struct
 	if (unlikely(need_resched())) {
 		current_thread_info()->status |= TS_POLLING;
 		local_irq_enable();
+		lapic_timer_state_broadcast(pr, cx, 0);
 		return 0;
 	}
 
-	/*
-	 * Must be done before busmaster disable as we might need to
-	 * access HPET !
-	 */
-	lapic_timer_state_broadcast(pr, cx, 1);
-
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
@@ -957,6 +961,12 @@ static int acpi_idle_enter_bm(struct cpu
 			return 0;
 		}
 	}
+	local_irq_enable();
+	/*
+	 * Must be done before busmaster disable as we might need to
+	 * access HPET !
+	 */
+	lapic_timer_state_broadcast(pr, cx, 1);
 
 	local_irq_disable();
 	current_thread_info()->status &= ~TS_POLLING;
@@ -969,6 +979,7 @@ static int acpi_idle_enter_bm(struct cpu
 	if (unlikely(need_resched())) {
 		current_thread_info()->status |= TS_POLLING;
 		local_irq_enable();
+		lapic_timer_state_broadcast(pr, cx, 0);
 		return 0;
 	}
 
@@ -976,11 +987,6 @@ static int acpi_idle_enter_bm(struct cpu
 
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
-	/*
-	 * Must be done before busmaster disable as we might need to
-	 * access HPET !
-	 */
-	lapic_timer_state_broadcast(pr, cx, 1);
 
 	kt1 = ktime_get_real();
 	/*

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Thomas Gleixner]

On Thu, 13 Aug 2009, Suresh Siddha wrote:

> From: Suresh Siddha <suresh.b.siddha@intel.com>
> Subject: clockevents_notify() need to be called with irq's enabled
> 
> Currently clockevents_notify() is called with interrupts enabled at some
> places and interrupts disabled at some other places.

The only place I can see which calls clockevents_notify with
interrupts enabled is the hrtimer cpu hotplug code.

I'm a bit wary to enable interrupts all over the place in sensitive
corners like ACPI idle code ...

Why don't we just do the obvious and take clockevents_lock irqsave ?

Thanks,

	tglx
---
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index a6dcd67..e43c4b6 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -137,11 +137,12 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
  */
 int clockevents_register_notifier(struct notifier_block *nb)
 {
+	unsigned long flags;
 	int ret;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	ret = raw_notifier_chain_register(&clockevents_chain, nb);
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 
 	return ret;
 }
@@ -181,13 +182,13 @@ void clockevents_register_device(struct clock_event_device *dev)
 	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
 	BUG_ON(!dev->cpumask);
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 
 	list_add(&dev->list, &clockevent_devices);
 	clockevents_do_notify(CLOCK_EVT_NOTIFY_ADD, dev);
 	clockevents_notify_released();
 
-	spin_unlock(&clockevents_lock);
+	spin_lock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_register_device);
 
@@ -235,8 +236,9 @@ void clockevents_exchange_device(struct clock_event_device *old,
 void clockevents_notify(unsigned long reason, void *arg)
 {
 	struct list_head *node, *tmp;
+	unsigned long flags;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	clockevents_do_notify(reason, arg);
 
 	switch (reason) {
@@ -251,7 +253,7 @@ void clockevents_notify(unsigned long reason, void *arg)
 	default:
 		break;
 	}
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_notify);
 #endif



 

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Suresh Siddha]

On Thu, 2009-08-13 at 23:05 -0700, Thomas Gleixner wrote:
> On Thu, 13 Aug 2009, Suresh Siddha wrote:
> 
> > From: Suresh Siddha <suresh.b.siddha@intel.com>
> > Subject: clockevents_notify() need to be called with irq's enabled
> > 
> > Currently clockevents_notify() is called with interrupts enabled at some
> > places and interrupts disabled at some other places.
> 
> The only place I can see which calls clockevents_notify with
> interrupts enabled is the hrtimer cpu hotplug code.
> 
> I'm a bit wary to enable interrupts all over the place in sensitive
> corners like ACPI idle code ...
> 
> Why don't we just do the obvious and take clockevents_lock irqsave ?

We didn't go that route because of the smp_call_function() in the cpu
hotplug code. So we can't disable interrupts in that path.

ACPI idle code changes were ok'd by Venki. We did 4 or so hours testing
with our patch. Will do more testing in the coming days.

thanks,
suresh

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Thomas Gleixner]

On Fri, 14 Aug 2009, Suresh Siddha wrote:

> On Thu, 2009-08-13 at 23:05 -0700, Thomas Gleixner wrote:
> > On Thu, 13 Aug 2009, Suresh Siddha wrote:
> > 
> > > From: Suresh Siddha <suresh.b.siddha@intel.com>
> > > Subject: clockevents_notify() need to be called with irq's enabled
> > > 
> > > Currently clockevents_notify() is called with interrupts enabled at some
> > > places and interrupts disabled at some other places.
> > 
> > The only place I can see which calls clockevents_notify with
> > interrupts enabled is the hrtimer cpu hotplug code.
> > 
> > I'm a bit wary to enable interrupts all over the place in sensitive
> > corners like ACPI idle code ...
> > 
> > Why don't we just do the obvious and take clockevents_lock irqsave ?
> 
> We didn't go that route because of the smp_call_function() in the cpu
> hotplug code. So we can't disable interrupts in that path.

Hmm, that's the tick_broadcast_on_off() stuff, right ?

Thanks,

	tglx

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Suresh Siddha]

On Fri, 2009-08-14 at 01:28 -0700, Thomas Gleixner wrote:
> On Fri, 14 Aug 2009, Suresh Siddha wrote:
> 
> > We didn't go that route because of the smp_call_function() in the cpu
> > hotplug code. So we can't disable interrupts in that path.
> 
> Hmm, that's the tick_broadcast_on_off() stuff, right ?

Yes Thomas.

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Thomas Gleixner]

On Fri, 14 Aug 2009, Suresh Siddha wrote:

> On Fri, 2009-08-14 at 01:28 -0700, Thomas Gleixner wrote:
> > On Fri, 14 Aug 2009, Suresh Siddha wrote:
> > 
> > > We didn't go that route because of the smp_call_function() in the cpu
> > > hotplug code. So we can't disable interrupts in that path.
> > 
> > Hmm, that's the tick_broadcast_on_off() stuff, right ?
> 
> Yes Thomas.

What would it take to move the smp_call_function into the calling code ?

Thanks,

	tglx

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Suresh Siddha]

On Fri, 2009-08-14 at 13:01 -0700, Thomas Gleixner wrote:
> What would it take to move the smp_call_function into the calling code ?

Here is the patch doing that. Thanks.

---

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: take clockevents_lock with interrupts disabled

Currently clockevents_notify() is called with interrupts enabled at some
places and interrupts disabled at some other places.

This results in a deadlock in this scenario.

cpu A holding the clockevents_lock in clockevents_notify() with irq enabled
cpu B waiting for the clockevents_lock in clockevents_notify() with irq disabled
cpu C doing set_mtrr() which will try to rendezvous of all the cpus.

This will result in C and A come to the rendezvous point and waiting for B.
B stuck forever waiting for the spinlock and thus not reaching rendezvous
point.

Fix the clockevents code so that clockevents_lock is taken
with interrupts disabled and thus avoid the above deadlock.

Also call lapic_timer_propagate_broadcast() on the destination cpu so that
we avoid calling smp_call_function() in the clockevents notifier chain.

This issue left us wondering if we need to change the MTRR rendezvous logic to
use stop machine logic (instead of smp_call_function) or add a check
in spinlock debug code to see if there are other spinlocks which gets
taken under both interrupts enabled/disabled conditions.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---

Index: tip/arch/x86/kernel/process.c
===================================================================
--- tip.orig/arch/x86/kernel/process.c
+++ tip/arch/x86/kernel/process.c
@@ -508,17 +508,10 @@ static void c1e_idle(void)
 
 		if (!cpumask_test_cpu(cpu, c1e_mask)) {
 			cpumask_set_cpu(cpu, c1e_mask);
-			/*
-			 * Force broadcast so ACPI can not interfere. Needs
-			 * to run with interrupts enabled as it uses
-			 * smp_function_call.
-			 */
-			local_irq_enable();
 			clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_FORCE,
 					   &cpu);
 			printk(KERN_INFO "Switch to broadcast mode on CPU%d\n",
 			       cpu);
-			local_irq_disable();
 		}
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
 
Index: tip/drivers/acpi/processor_idle.c
===================================================================
--- tip.orig/drivers/acpi/processor_idle.c
+++ tip/drivers/acpi/processor_idle.c
@@ -162,8 +162,9 @@ static void lapic_timer_check_state(int 
 		pr->power.timer_broadcast_on_state = state;
 }
 
-static void lapic_timer_propagate_broadcast(struct acpi_processor *pr)
+static void lapic_timer_propagate_broadcast(void *arg)
 {
+	struct acpi_processor *pr = (struct acpi_processor *) arg;
 	unsigned long reason;
 
 	reason = pr->power.timer_broadcast_on_state < INT_MAX ?
@@ -635,7 +636,8 @@ static int acpi_processor_power_verify(s
 		working++;
 	}
 
-	lapic_timer_propagate_broadcast(pr);
+	smp_call_function_single(pr->id, lapic_timer_propagate_broadcast,
+				 pr, 1);
 
 	return (working);
 }
Index: tip/kernel/time/clockevents.c
===================================================================
--- tip.orig/kernel/time/clockevents.c
+++ tip/kernel/time/clockevents.c
@@ -137,11 +137,12 @@ int clockevents_program_event(struct clo
  */
 int clockevents_register_notifier(struct notifier_block *nb)
 {
+	unsigned long flags;
 	int ret;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	ret = raw_notifier_chain_register(&clockevents_chain, nb);
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 
 	return ret;
 }
@@ -178,16 +179,18 @@ static void clockevents_notify_released(
  */
 void clockevents_register_device(struct clock_event_device *dev)
 {
+	unsigned long flags;
+
 	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
 	BUG_ON(!dev->cpumask);
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 
 	list_add(&dev->list, &clockevent_devices);
 	clockevents_do_notify(CLOCK_EVT_NOTIFY_ADD, dev);
 	clockevents_notify_released();
 
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_register_device);
 
@@ -235,8 +238,9 @@ void clockevents_exchange_device(struct 
 void clockevents_notify(unsigned long reason, void *arg)
 {
 	struct list_head *node, *tmp;
+	unsigned long flags;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	clockevents_do_notify(reason, arg);
 
 	switch (reason) {
@@ -251,7 +255,7 @@ void clockevents_notify(unsigned long re
 	default:
 		break;
 	}
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_notify);
 #endif
Index: tip/kernel/time/tick-broadcast.c
===================================================================
--- tip.orig/kernel/time/tick-broadcast.c
+++ tip/kernel/time/tick-broadcast.c
@@ -205,11 +205,11 @@ static void tick_handle_periodic_broadca
  * Powerstate information: The system enters/leaves a state, where
  * affected devices might stop
  */
-static void tick_do_broadcast_on_off(void *why)
+static void tick_do_broadcast_on_off(unsigned long *reason)
 {
 	struct clock_event_device *bc, *dev;
 	struct tick_device *td;
-	unsigned long flags, *reason = why;
+	unsigned long flags;
 	int cpu, bc_stopped;
 
 	spin_lock_irqsave(&tick_broadcast_lock, flags);
@@ -276,8 +276,7 @@ void tick_broadcast_on_off(unsigned long
 		printk(KERN_ERR "tick-broadcast: ignoring broadcast for "
 		       "offline CPU #%d\n", *oncpu);
 	else
-		smp_call_function_single(*oncpu, tick_do_broadcast_on_off,
-					 &reason, 1);
+		tick_do_broadcast_on_off(&reason);
 }
 
 /*

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Thomas Gleixner]

Suresh,

On Mon, 17 Aug 2009, Suresh Siddha wrote:

> --- tip.orig/arch/x86/kernel/process.c
> +++ tip/arch/x86/kernel/process.c
> @@ -508,17 +508,10 @@ static void c1e_idle(void)
>  
>  		if (!cpumask_test_cpu(cpu, c1e_mask)) {
>  			cpumask_set_cpu(cpu, c1e_mask);
> -			/*
> -			 * Force broadcast so ACPI can not interfere. Needs
> -			 * to run with interrupts enabled as it uses
> -			 * smp_function_call.
> -			 */

Please keep the first sentence of the comment intact. Everything else
looks fine.

Thanks,

	tglx

Re: [patch] clockevents_notify() need to be called with irq's enabled

[Suresh Siddha]

On Mon, 2009-08-17 at 14:27 -0700, Thomas Gleixner wrote:
> Suresh,
> 
> On Mon, 17 Aug 2009, Suresh Siddha wrote:
> 
> > --- tip.orig/arch/x86/kernel/process.c
> > +++ tip/arch/x86/kernel/process.c
> > @@ -508,17 +508,10 @@ static void c1e_idle(void)
> >  
> >  		if (!cpumask_test_cpu(cpu, c1e_mask)) {
> >  			cpumask_set_cpu(cpu, c1e_mask);
> > -			/*
> > -			 * Force broadcast so ACPI can not interfere. Needs
> > -			 * to run with interrupts enabled as it uses
> > -			 * smp_function_call.
> > -			 */
> 
> Please keep the first sentence of the comment intact. Everything else
> looks fine.

Oops. Patch appended.
---

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: take clockevents_lock with interrupts disabled

Currently clockevents_notify() is called with interrupts enabled at some
places and interrupts disabled at some other places.

This results in a deadlock in this scenario.

cpu A holding the clockevents_lock in clockevents_notify() with irq enabled
cpu B waiting for the clockevents_lock in clockevents_notify() with irq disabled
cpu C doing set_mtrr() which will try to rendezvous of all the cpus.

This will result in C and A come to the rendezvous point and waiting for B.
B stuck forever waiting for the spinlock and thus not reaching rendezvous
point.

Fix the clockevents code so that clockevents_lock is taken
with interrupts disabled and thus avoid the above deadlock.

Also call lapic_timer_propagate_broadcast() on the destination cpu so that
we avoid calling smp_call_function() in the clockevents notifier chain.

This issue left us wondering if we need to change the MTRR rendezvous logic to
use stop machine logic (instead of smp_call_function) or add a check
in spinlock debug code to see if there are other spinlocks which gets
taken under both interrupts enabled/disabled conditions.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---

Index: tip/arch/x86/kernel/process.c
===================================================================
--- tip.orig/arch/x86/kernel/process.c
+++ tip/arch/x86/kernel/process.c
@@ -509,16 +509,12 @@ static void c1e_idle(void)
 		if (!cpumask_test_cpu(cpu, c1e_mask)) {
 			cpumask_set_cpu(cpu, c1e_mask);
 			/*
-			 * Force broadcast so ACPI can not interfere. Needs
-			 * to run with interrupts enabled as it uses
-			 * smp_function_call.
-			 */
-			local_irq_enable();
+ 			 * Force broadcast so ACPI can not interfere.
+ 			 */
 			clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_FORCE,
 					   &cpu);
 			printk(KERN_INFO "Switch to broadcast mode on CPU%d\n",
 			       cpu);
-			local_irq_disable();
 		}
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
 
Index: tip/drivers/acpi/processor_idle.c
===================================================================
--- tip.orig/drivers/acpi/processor_idle.c
+++ tip/drivers/acpi/processor_idle.c
@@ -162,8 +162,9 @@ static void lapic_timer_check_state(int 
 		pr->power.timer_broadcast_on_state = state;
 }
 
-static void lapic_timer_propagate_broadcast(struct acpi_processor *pr)
+static void lapic_timer_propagate_broadcast(void *arg)
 {
+	struct acpi_processor *pr = (struct acpi_processor *) arg;
 	unsigned long reason;
 
 	reason = pr->power.timer_broadcast_on_state < INT_MAX ?
@@ -635,7 +636,8 @@ static int acpi_processor_power_verify(s
 		working++;
 	}
 
-	lapic_timer_propagate_broadcast(pr);
+	smp_call_function_single(pr->id, lapic_timer_propagate_broadcast,
+				 pr, 1);
 
 	return (working);
 }
Index: tip/kernel/time/clockevents.c
===================================================================
--- tip.orig/kernel/time/clockevents.c
+++ tip/kernel/time/clockevents.c
@@ -137,11 +137,12 @@ int clockevents_program_event(struct clo
  */
 int clockevents_register_notifier(struct notifier_block *nb)
 {
+	unsigned long flags;
 	int ret;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	ret = raw_notifier_chain_register(&clockevents_chain, nb);
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 
 	return ret;
 }
@@ -178,16 +179,18 @@ static void clockevents_notify_released(
  */
 void clockevents_register_device(struct clock_event_device *dev)
 {
+	unsigned long flags;
+
 	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
 	BUG_ON(!dev->cpumask);
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 
 	list_add(&dev->list, &clockevent_devices);
 	clockevents_do_notify(CLOCK_EVT_NOTIFY_ADD, dev);
 	clockevents_notify_released();
 
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_register_device);
 
@@ -235,8 +238,9 @@ void clockevents_exchange_device(struct 
 void clockevents_notify(unsigned long reason, void *arg)
 {
 	struct list_head *node, *tmp;
+	unsigned long flags;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	clockevents_do_notify(reason, arg);
 
 	switch (reason) {
@@ -251,7 +255,7 @@ void clockevents_notify(unsigned long re
 	default:
 		break;
 	}
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_notify);
 #endif
Index: tip/kernel/time/tick-broadcast.c
===================================================================
--- tip.orig/kernel/time/tick-broadcast.c
+++ tip/kernel/time/tick-broadcast.c
@@ -205,11 +205,11 @@ static void tick_handle_periodic_broadca
  * Powerstate information: The system enters/leaves a state, where
  * affected devices might stop
  */
-static void tick_do_broadcast_on_off(void *why)
+static void tick_do_broadcast_on_off(unsigned long *reason)
 {
 	struct clock_event_device *bc, *dev;
 	struct tick_device *td;
-	unsigned long flags, *reason = why;
+	unsigned long flags;
 	int cpu, bc_stopped;
 
 	spin_lock_irqsave(&tick_broadcast_lock, flags);
@@ -276,8 +276,7 @@ void tick_broadcast_on_off(unsigned long
 		printk(KERN_ERR "tick-broadcast: ignoring broadcast for "
 		       "offline CPU #%d\n", *oncpu);
 	else
-		smp_call_function_single(*oncpu, tick_do_broadcast_on_off,
-					 &reason, 1);
+		tick_do_broadcast_on_off(&reason);
 }
 
 /*

[tip:timers/urgent] clockevent: Prevent dead lock on clockevents_lock

[tip-bot for Suresh Siddha]

Commit-ID:  f833bab87fca5c3ce13778421b1365845843b976
Gitweb:     http://git.kernel.org/tip/f833bab87fca5c3ce13778421b1365845843b976
Author:     Suresh Siddha <suresh.b.siddha@intel.com>
AuthorDate: Mon, 17 Aug 2009 14:34:59 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 19 Aug 2009 18:15:10 +0200

clockevent: Prevent dead lock on clockevents_lock

Currently clockevents_notify() is called with interrupts enabled at
some places and interrupts disabled at some other places.

This results in a deadlock in this scenario.

cpu A holds clockevents_lock in clockevents_notify() with irqs enabled
cpu B waits for clockevents_lock in clockevents_notify() with irqs disabled
cpu C doing set_mtrr() which will try to rendezvous of all the cpus.

This will result in C and A come to the rendezvous point and waiting
for B. B is stuck forever waiting for the spinlock and thus not
reaching the rendezvous point.

Fix the clockevents code so that clockevents_lock is taken with
interrupts disabled and thus avoid the above deadlock.

Also call lapic_timer_propagate_broadcast() on the destination cpu so
that we avoid calling smp_call_function() in the clockevents notifier
chain.

This issue left us wondering if we need to change the MTRR rendezvous
logic to use stop machine logic (instead of smp_call_function) or add
a check in spinlock debug code to see if there are other spinlocks
which gets taken under both interrupts enabled/disabled conditions.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: "Pallipadi Venkatesh" <venkatesh.pallipadi@intel.com>
Cc: "Brown Len" <len.brown@intel.com>
LKML-Reference: <1250544899.2709.210.camel@sbs-t61.sc.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>


---
 arch/x86/kernel/process.c     |    6 +-----
 drivers/acpi/processor_idle.c |    6 ++++--
 kernel/time/clockevents.c     |   16 ++++++++++------
 kernel/time/tick-broadcast.c  |    7 +++----
 4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 994dd6a..071166a 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -519,16 +519,12 @@ static void c1e_idle(void)
 		if (!cpumask_test_cpu(cpu, c1e_mask)) {
 			cpumask_set_cpu(cpu, c1e_mask);
 			/*
-			 * Force broadcast so ACPI can not interfere. Needs
-			 * to run with interrupts enabled as it uses
-			 * smp_function_call.
+			 * Force broadcast so ACPI can not interfere.
 			 */
-			local_irq_enable();
 			clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_FORCE,
 					   &cpu);
 			printk(KERN_INFO "Switch to broadcast mode on CPU%d\n",
 			       cpu);
-			local_irq_disable();
 		}
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 0efa59e..66393d5 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -162,8 +162,9 @@ static void lapic_timer_check_state(int state, struct acpi_processor *pr,
 		pr->power.timer_broadcast_on_state = state;
 }
 
-static void lapic_timer_propagate_broadcast(struct acpi_processor *pr)
+static void lapic_timer_propagate_broadcast(void *arg)
 {
+	struct acpi_processor *pr = (struct acpi_processor *) arg;
 	unsigned long reason;
 
 	reason = pr->power.timer_broadcast_on_state < INT_MAX ?
@@ -635,7 +636,8 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
 		working++;
 	}
 
-	lapic_timer_propagate_broadcast(pr);
+	smp_call_function_single(pr->id, lapic_timer_propagate_broadcast,
+				 pr, 1);
 
 	return (working);
 }
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index a6dcd67..620b58a 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -137,11 +137,12 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
  */
 int clockevents_register_notifier(struct notifier_block *nb)
 {
+	unsigned long flags;
 	int ret;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	ret = raw_notifier_chain_register(&clockevents_chain, nb);
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 
 	return ret;
 }
@@ -178,16 +179,18 @@ static void clockevents_notify_released(void)
  */
 void clockevents_register_device(struct clock_event_device *dev)
 {
+	unsigned long flags;
+
 	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
 	BUG_ON(!dev->cpumask);
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 
 	list_add(&dev->list, &clockevent_devices);
 	clockevents_do_notify(CLOCK_EVT_NOTIFY_ADD, dev);
 	clockevents_notify_released();
 
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_register_device);
 
@@ -235,8 +238,9 @@ void clockevents_exchange_device(struct clock_event_device *old,
 void clockevents_notify(unsigned long reason, void *arg)
 {
 	struct list_head *node, *tmp;
+	unsigned long flags;
 
-	spin_lock(&clockevents_lock);
+	spin_lock_irqsave(&clockevents_lock, flags);
 	clockevents_do_notify(reason, arg);
 
 	switch (reason) {
@@ -251,7 +255,7 @@ void clockevents_notify(unsigned long reason, void *arg)
 	default:
 		break;
 	}
-	spin_unlock(&clockevents_lock);
+	spin_unlock_irqrestore(&clockevents_lock, flags);
 }
 EXPORT_SYMBOL_GPL(clockevents_notify);
 #endif
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 877dbed..c2ec250 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -205,11 +205,11 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev)
  * Powerstate information: The system enters/leaves a state, where
  * affected devices might stop
  */
-static void tick_do_broadcast_on_off(void *why)
+static void tick_do_broadcast_on_off(unsigned long *reason)
 {
 	struct clock_event_device *bc, *dev;
 	struct tick_device *td;
-	unsigned long flags, *reason = why;
+	unsigned long flags;
 	int cpu, bc_stopped;
 
 	spin_lock_irqsave(&tick_broadcast_lock, flags);
@@ -276,8 +276,7 @@ void tick_broadcast_on_off(unsigned long reason, int *oncpu)
 		printk(KERN_ERR "tick-broadcast: ignoring broadcast for "
 		       "offline CPU #%d\n", *oncpu);
 	else
-		smp_call_function_single(*oncpu, tick_do_broadcast_on_off,
-					 &reason, 1);
+		tick_do_broadcast_on_off(&reason);
 }
 
 /*