LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
@ 2008-10-20 12:13 Ivan Vecera
  2008-10-20 14:39 ` Neil Horman
  2008-10-20 16:00 ` Ingo Molnar
  0 siblings, 2 replies; 11+ messages in thread
From: Ivan Vecera @ 2008-10-20 12:13 UTC (permalink / raw)
  To: linux-kernel; +Cc: tglx, mingo, hpa, nhorman, jmarchan, Ivan Vecera

Function machine_halt (resp. native_machine_halt) is empty for x86
architectures. When command 'halt -f' is invoked, the message
"System halted." is displayed but this is not really true because
all CPUs are still running.
There are also similar inconsistencies for other arches (some uses
power-off for halt or forever-loop with IRQs enabled/disabled).
IMO there should be used the same approach for all architectures
OR what does the message "System halted" really mean?

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 arch/x86/kernel/reboot.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index f4c93f1..15ad949 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -465,6 +465,14 @@ static void native_machine_restart(char *__unused)
 
 static void native_machine_halt(void)
 {
+	/* stop other cpus and apics */
+	machine_shutdown();
+
+	/* stop this cpu */
+	local_irq_disable();
+	if (hlt_works(smp_processor_id()))
+		for (;;) halt();
+	for (;;);
 }
 
 static void native_machine_power_off(void)
-- 
1.5.6.3


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-10-20 12:13 [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt Ivan Vecera
@ 2008-10-20 14:39 ` Neil Horman
  2008-10-20 16:00 ` Ingo Molnar
  1 sibling, 0 replies; 11+ messages in thread
From: Neil Horman @ 2008-10-20 14:39 UTC (permalink / raw)
  To: Ivan Vecera; +Cc: linux-kernel, tglx, mingo, hpa, nhorman, jmarchan

On Mon, Oct 20, 2008 at 02:13:07PM +0200, Ivan Vecera wrote:
> Function machine_halt (resp. native_machine_halt) is empty for x86
> architectures. When command 'halt -f' is invoked, the message
> "System halted." is displayed but this is not really true because
> all CPUs are still running.
> There are also similar inconsistencies for other arches (some uses
> power-off for halt or forever-loop with IRQs enabled/disabled).
> IMO there should be used the same approach for all architectures
> OR what does the message "System halted" really mean?
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  arch/x86/kernel/reboot.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f4c93f1..15ad949 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -465,6 +465,14 @@ static void native_machine_restart(char *__unused)
>  
>  static void native_machine_halt(void)
>  {
> +	/* stop other cpus and apics */
> +	machine_shutdown();
> +
> +	/* stop this cpu */
> +	local_irq_disable();
> +	if (hlt_works(smp_processor_id()))
> +		for (;;) halt();
> +	for (;;);
>  }
>  
>  static void native_machine_power_off(void)
> -- 
> 1.5.6.3
> 

Acked-by: Neil Horman <nhorman@tuxdriver.com>

-- 
/***************************************************
 *Neil Horman
 *Senior Software Engineer
 *Red Hat, Inc.
 *nhorman@redhat.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-10-20 12:13 [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt Ivan Vecera
  2008-10-20 14:39 ` Neil Horman
@ 2008-10-20 16:00 ` Ingo Molnar
  2008-10-21 12:36   ` Ivan Vecera
  1 sibling, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2008-10-20 16:00 UTC (permalink / raw)
  To: Ivan Vecera; +Cc: linux-kernel, tglx, mingo, hpa, nhorman, jmarchan


* Ivan Vecera <ivecera@redhat.com> wrote:

> Function machine_halt (resp. native_machine_halt) is empty for x86 
> architectures. When command 'halt -f' is invoked, the message "System 
> halted." is displayed but this is not really true because all CPUs are 
> still running. There are also similar inconsistencies for other arches 
> (some uses power-off for halt or forever-loop with IRQs 
> enabled/disabled). IMO there should be used the same approach for all 
> architectures OR what does the message "System halted" really mean?

no fundamental objections, but could you please do it a bit cleaner:

>  static void native_machine_halt(void)
>  {
> +	/* stop other cpus and apics */
> +	machine_shutdown();
> +
> +	/* stop this cpu */
> +	local_irq_disable();
> +	if (hlt_works(smp_processor_id()))
> +		for (;;) halt();
> +	for (;;);
>  }

the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to 
this and could be shared. You could move the stop_this_cpu() function to 
arch/x86/kernel/process.c (out of smp.c), so that it can be used by 
reboot.c.

furthermore, native_machine_power_off() should probably fall back to 
native_machine_halt() as well - should pm_power_off() be disabled (or if 
it fails to stop the machine).

hm?

	Ingo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-10-20 16:00 ` Ingo Molnar
@ 2008-10-21 12:36   ` Ivan Vecera
  2008-11-06 16:10     ` Ivan Vecera
  2008-11-06 20:43     ` Ingo Molnar
  0 siblings, 2 replies; 11+ messages in thread
From: Ivan Vecera @ 2008-10-21 12:36 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: linux-kernel, tglx, mingo, hpa, nhorman, jmarchan

Ingo Molnar wrote:
> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to 
> this and could be shared. You could move the stop_this_cpu() function to 
> arch/x86/kernel/process.c (out of smp.c), so that it can be used by 
> reboot.c.
> 
Yes, this make sense. Here is the patch.

Ivan

---
 arch/x86/kernel/process.c |   16 ++++++++++++++++
 arch/x86/kernel/reboot.c  |    5 +++++
 arch/x86/kernel/smp.c     |   13 -------------
 include/asm-x86/system.h  |    2 ++
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c622772..af6904e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -8,6 +8,7 @@
 #include <linux/pm.h>
 #include <linux/clockchips.h>
 #include <asm/system.h>
+#include <asm/apic.h>
 
 unsigned long idle_halt;
 EXPORT_SYMBOL(idle_halt);
@@ -122,6 +123,21 @@ void default_idle(void)
 EXPORT_SYMBOL(default_idle);
 #endif
 
+void stop_this_cpu(void *dummy)
+{
+	local_irq_disable();
+	/*
+	 * Remove this CPU:
+	 */
+	cpu_clear(smp_processor_id(), cpu_online_map);
+#ifdef CONFIG_X86_LOCAL_APIC
+	disable_local_APIC();
+#endif
+	if (hlt_works(smp_processor_id()))
+		for (;;) halt();
+	for (;;);
+}
+
 static void do_nothing(void *unused)
 {
 }
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index f4c93f1..3113d9e 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -465,6 +465,11 @@ static void native_machine_restart(char *__unused)
 
 static void native_machine_halt(void)
 {
+	/* stop other cpus and apics */
+	machine_shutdown();
+
+	/* stop this cpu */
+	stop_this_cpu(NULL);
 }
 
 static void native_machine_power_off(void)
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 18f9b19..3f92b13 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
 		send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
 }
 
-static void stop_this_cpu(void *dummy)
-{
-	local_irq_disable();
-	/*
-	 * Remove this CPU:
-	 */
-	cpu_clear(smp_processor_id(), cpu_online_map);
-	disable_local_APIC();
-	if (hlt_works(smp_processor_id()))
-		for (;;) halt();
-	for (;;);
-}
-
 /*
  * this function calls the 'stop' function on all other CPUs in the system.
  */
diff --git a/include/asm-x86/system.h b/include/asm-x86/system.h
index b20c894..17ac753 100644
--- a/include/asm-x86/system.h
+++ b/include/asm-x86/system.h
@@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
 
 void default_idle(void);
 
+void stop_this_cpu(void *dummy);
+
 /*
  * Force strict CPU ordering.
  * And yes, this is required on UP too when we're talking
-- 
1.5.6.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-10-21 12:36   ` Ivan Vecera
@ 2008-11-06 16:10     ` Ivan Vecera
  2008-11-06 16:15       ` Neil Horman
  2008-11-10 22:49       ` Pavel Machek
  2008-11-06 20:43     ` Ingo Molnar
  1 sibling, 2 replies; 11+ messages in thread
From: Ivan Vecera @ 2008-11-06 16:10 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: linux-kernel, tglx, mingo, hpa, nhorman, jmarchan

Any comments?

Ivan

---
Ivan Vecera wrote:
> Ingo Molnar wrote:
>> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to 
>> this and could be shared. You could move the stop_this_cpu() function to 
>> arch/x86/kernel/process.c (out of smp.c), so that it can be used by 
>> reboot.c.
>>
> Yes, this make sense. Here is the patch.
> 
> Ivan
> 
> ---
>  arch/x86/kernel/process.c |   16 ++++++++++++++++
>  arch/x86/kernel/reboot.c  |    5 +++++
>  arch/x86/kernel/smp.c     |   13 -------------
>  include/asm-x86/system.h  |    2 ++
>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index c622772..af6904e 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -8,6 +8,7 @@
>  #include <linux/pm.h>
>  #include <linux/clockchips.h>
>  #include <asm/system.h>
> +#include <asm/apic.h>
>  
>  unsigned long idle_halt;
>  EXPORT_SYMBOL(idle_halt);
> @@ -122,6 +123,21 @@ void default_idle(void)
>  EXPORT_SYMBOL(default_idle);
>  #endif
>  
> +void stop_this_cpu(void *dummy)
> +{
> +	local_irq_disable();
> +	/*
> +	 * Remove this CPU:
> +	 */
> +	cpu_clear(smp_processor_id(), cpu_online_map);
> +#ifdef CONFIG_X86_LOCAL_APIC
> +	disable_local_APIC();
> +#endif
> +	if (hlt_works(smp_processor_id()))
> +		for (;;) halt();
> +	for (;;);
> +}
> +
>  static void do_nothing(void *unused)
>  {
>  }
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f4c93f1..3113d9e 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -465,6 +465,11 @@ static void native_machine_restart(char *__unused)
>  
>  static void native_machine_halt(void)
>  {
> +	/* stop other cpus and apics */
> +	machine_shutdown();
> +
> +	/* stop this cpu */
> +	stop_this_cpu(NULL);
>  }
>  
>  static void native_machine_power_off(void)
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 18f9b19..3f92b13 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
>  		send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
>  }
>  
> -static void stop_this_cpu(void *dummy)
> -{
> -	local_irq_disable();
> -	/*
> -	 * Remove this CPU:
> -	 */
> -	cpu_clear(smp_processor_id(), cpu_online_map);
> -	disable_local_APIC();
> -	if (hlt_works(smp_processor_id()))
> -		for (;;) halt();
> -	for (;;);
> -}
> -
>  /*
>   * this function calls the 'stop' function on all other CPUs in the system.
>   */
> diff --git a/include/asm-x86/system.h b/include/asm-x86/system.h
> index b20c894..17ac753 100644
> --- a/include/asm-x86/system.h
> +++ b/include/asm-x86/system.h
> @@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
>  
>  void default_idle(void);
>  
> +void stop_this_cpu(void *dummy);
> +
>  /*
>   * Force strict CPU ordering.
>   * And yes, this is required on UP too when we're talking


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-11-06 16:10     ` Ivan Vecera
@ 2008-11-06 16:15       ` Neil Horman
  2008-11-10 22:49       ` Pavel Machek
  1 sibling, 0 replies; 11+ messages in thread
From: Neil Horman @ 2008-11-06 16:15 UTC (permalink / raw)
  To: Ivan Vecera
  Cc: Ingo Molnar, linux-kernel, tglx, mingo, hpa, nhorman, jmarchan

On Thu, Nov 06, 2008 at 05:10:01PM +0100, Ivan Vecera wrote:
> Any comments?
> 
> Ivan
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
> Ivan Vecera wrote:
> > Ingo Molnar wrote:
> >> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to 
> >> this and could be shared. You could move the stop_this_cpu() function to 
> >> arch/x86/kernel/process.c (out of smp.c), so that it can be used by 
> >> reboot.c.
> >>
> > Yes, this make sense. Here is the patch.
> > 
> > Ivan
> > 
> > ---
> >  arch/x86/kernel/process.c |   16 ++++++++++++++++
> >  arch/x86/kernel/reboot.c  |    5 +++++
> >  arch/x86/kernel/smp.c     |   13 -------------
> >  include/asm-x86/system.h  |    2 ++
> >  4 files changed, 23 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index c622772..af6904e 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/clockchips.h>
> >  #include <asm/system.h>
> > +#include <asm/apic.h>
> >  
> >  unsigned long idle_halt;
> >  EXPORT_SYMBOL(idle_halt);
> > @@ -122,6 +123,21 @@ void default_idle(void)
> >  EXPORT_SYMBOL(default_idle);
> >  #endif
> >  
> > +void stop_this_cpu(void *dummy)
> > +{
> > +	local_irq_disable();
> > +	/*
> > +	 * Remove this CPU:
> > +	 */
> > +	cpu_clear(smp_processor_id(), cpu_online_map);
> > +#ifdef CONFIG_X86_LOCAL_APIC
> > +	disable_local_APIC();
> > +#endif
> > +	if (hlt_works(smp_processor_id()))
> > +		for (;;) halt();
> > +	for (;;);
> > +}
> > +
> >  static void do_nothing(void *unused)
> >  {
> >  }
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index f4c93f1..3113d9e 100644
> > --- a/arch/x86/kernel/reboot.c
> > +++ b/arch/x86/kernel/reboot.c
> > @@ -465,6 +465,11 @@ static void native_machine_restart(char *__unused)
> >  
> >  static void native_machine_halt(void)
> >  {
> > +	/* stop other cpus and apics */
> > +	machine_shutdown();
> > +
> > +	/* stop this cpu */
> > +	stop_this_cpu(NULL);
> >  }
> >  
> >  static void native_machine_power_off(void)
> > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> > index 18f9b19..3f92b13 100644
> > --- a/arch/x86/kernel/smp.c
> > +++ b/arch/x86/kernel/smp.c
> > @@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
> >  		send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
> >  }
> >  
> > -static void stop_this_cpu(void *dummy)
> > -{
> > -	local_irq_disable();
> > -	/*
> > -	 * Remove this CPU:
> > -	 */
> > -	cpu_clear(smp_processor_id(), cpu_online_map);
> > -	disable_local_APIC();
> > -	if (hlt_works(smp_processor_id()))
> > -		for (;;) halt();
> > -	for (;;);
> > -}
> > -
> >  /*
> >   * this function calls the 'stop' function on all other CPUs in the system.
> >   */
> > diff --git a/include/asm-x86/system.h b/include/asm-x86/system.h
> > index b20c894..17ac753 100644
> > --- a/include/asm-x86/system.h
> > +++ b/include/asm-x86/system.h
> > @@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
> >  
> >  void default_idle(void);
> >  
> > +void stop_this_cpu(void *dummy);
> > +
> >  /*
> >   * Force strict CPU ordering.
> >   * And yes, this is required on UP too when we're talking
> 

-- 
/***************************************************
 *Neil Horman
 *Senior Software Engineer
 *Red Hat, Inc.
 *nhorman@redhat.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-10-21 12:36   ` Ivan Vecera
  2008-11-06 16:10     ` Ivan Vecera
@ 2008-11-06 20:43     ` Ingo Molnar
  2008-11-11 12:47       ` Ivan Vecera
  1 sibling, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2008-11-06 20:43 UTC (permalink / raw)
  To: Ivan Vecera; +Cc: linux-kernel, tglx, mingo, hpa, nhorman, jmarchan


* Ivan Vecera <ivecera@redhat.com> wrote:

> Ingo Molnar wrote:
> > the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to 
> > this and could be shared. You could move the stop_this_cpu() function to 
> > arch/x86/kernel/process.c (out of smp.c), so that it can be used by 
> > reboot.c.
> > 
> Yes, this make sense. Here is the patch.

looks good. One small detail:

> +#ifdef CONFIG_X86_LOCAL_APIC
> +	disable_local_APIC();
> +#endif

could you please avoid this #ifdef by putting an inline void function 
for disable_local_APIC() into arch/x86/include/asm/apic.h for the 
!CONFIG_X86_LOCAL_APIC case?

	Ingo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-11-06 16:10     ` Ivan Vecera
  2008-11-06 16:15       ` Neil Horman
@ 2008-11-10 22:49       ` Pavel Machek
  2008-11-11 12:48         ` Ivan Vecera
  1 sibling, 1 reply; 11+ messages in thread
From: Pavel Machek @ 2008-11-10 22:49 UTC (permalink / raw)
  To: Ivan Vecera
  Cc: Ingo Molnar, linux-kernel, tglx, mingo, hpa, nhorman, jmarchan

On Thu 2008-11-06 17:10:01, Ivan Vecera wrote:
> Any comments?
> 
> Ivan
> 
> ---
> Ivan Vecera wrote:
> > Ingo Molnar wrote:
> >> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to 
> >> this and could be shared. You could move the stop_this_cpu() function to 
> >> arch/x86/kernel/process.c (out of smp.c), so that it can be used by 
> >> reboot.c.
> >>
> > Yes, this make sense. Here is the patch.
> > 
> > Ivan
> > 
> > ---
> >  arch/x86/kernel/process.c |   16 ++++++++++++++++
> >  arch/x86/kernel/reboot.c  |    5 +++++
> >  arch/x86/kernel/smp.c     |   13 -------------
> >  include/asm-x86/system.h  |    2 ++
> >  4 files changed, 23 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index c622772..af6904e 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/clockchips.h>
> >  #include <asm/system.h>
> > +#include <asm/apic.h>
> >  
> >  unsigned long idle_halt;
> >  EXPORT_SYMBOL(idle_halt);
> > @@ -122,6 +123,21 @@ void default_idle(void)
> >  EXPORT_SYMBOL(default_idle);
> >  #endif
> >  
> > +void stop_this_cpu(void *dummy)
> > +{
> > +	local_irq_disable();
> > +	/*
> > +	 * Remove this CPU:
> > +	 */
> > +	cpu_clear(smp_processor_id(), cpu_online_map);
> > +#ifdef CONFIG_X86_LOCAL_APIC
> > +	disable_local_APIC();
> > +#endif
> > +	if (hlt_works(smp_processor_id()))
> > +		for (;;) halt();
> > +	for (;;);
> > +}

Why the new ifdef? And while we are at it, why is it neccessary to
disable APIC for stopping CPU? (comment in code would be nice)

> > -static void stop_this_cpu(void *dummy)
> > -{
> > -	local_irq_disable();
> > -	/*
> > -	 * Remove this CPU:
> > -	 */
> > -	cpu_clear(smp_processor_id(), cpu_online_map);
> > -	disable_local_APIC();
> > -	if (hlt_works(smp_processor_id()))
> > -		for (;;) halt();
> > -	for (;;);
> > -}
> > -
> >  /*
> >   * this function calls the 'stop' function on all other CPUs in the system.
> >   */

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-11-06 20:43     ` Ingo Molnar
@ 2008-11-11 12:47       ` Ivan Vecera
  2008-11-11 13:47         ` Ingo Molnar
  0 siblings, 1 reply; 11+ messages in thread
From: Ivan Vecera @ 2008-11-11 12:47 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: linux-kernel, tglx, mingo, hpa, nhorman, jmarchan, pavel

Ingo Molnar wrote:
> looks good. One small detail:
> 
>> +#ifdef CONFIG_X86_LOCAL_APIC
>> +	disable_local_APIC();
>> +#endif
> 
> could you please avoid this #ifdef by putting an inline void function 
> for disable_local_APIC() into arch/x86/include/asm/apic.h for the 
> !CONFIG_X86_LOCAL_APIC case?
> 
> 	Ingo
OK, here is the result.

Ivan

---
 arch/x86/include/asm/apic.h   |    1 +
 arch/x86/include/asm/system.h |    2 ++
 arch/x86/kernel/process.c     |   14 ++++++++++++++
 arch/x86/kernel/reboot.c      |    5 +++++
 arch/x86/kernel/smp.c         |   13 -------------
 5 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 3b1510b..25caa07 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -193,6 +193,7 @@ extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
 static inline void lapic_shutdown(void) { }
 #define local_apic_timer_c2_ok		1
 static inline void init_apic_mappings(void) { }
+static inline void disable_local_APIC(void) { }
 
 #endif /* !CONFIG_X86_LOCAL_APIC */
 
diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h
index 2ed3f0f..07c3e40 100644
--- a/arch/x86/include/asm/system.h
+++ b/arch/x86/include/asm/system.h
@@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
 
 void default_idle(void);
 
+void stop_this_cpu(void *dummy);
+
 /*
  * Force strict CPU ordering.
  * And yes, this is required on UP too when we're talking
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c622772..3380458 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -8,6 +8,7 @@
 #include <linux/pm.h>
 #include <linux/clockchips.h>
 #include <asm/system.h>
+#include <asm/apic.h>
 
 unsigned long idle_halt;
 EXPORT_SYMBOL(idle_halt);
@@ -122,6 +123,19 @@ void default_idle(void)
 EXPORT_SYMBOL(default_idle);
 #endif
 
+void stop_this_cpu(void *dummy)
+{
+	local_irq_disable();
+	/*
+	 * Remove this CPU:
+	 */
+	cpu_clear(smp_processor_id(), cpu_online_map);
+	disable_local_APIC();
+	if (hlt_works(smp_processor_id()))
+		for (;;) halt();
+	for (;;);
+}
+
 static void do_nothing(void *unused)
 {
 }
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 724adfc..34f8d37 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -461,6 +461,11 @@ static void native_machine_restart(char *__unused)
 
 static void native_machine_halt(void)
 {
+	/* stop other cpus and apics */
+	machine_shutdown();
+
+	/* stop this cpu */
+	stop_this_cpu(NULL);
 }
 
 static void native_machine_power_off(void)
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 18f9b19..3f92b13 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
 		send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
 }
 
-static void stop_this_cpu(void *dummy)
-{
-	local_irq_disable();
-	/*
-	 * Remove this CPU:
-	 */
-	cpu_clear(smp_processor_id(), cpu_online_map);
-	disable_local_APIC();
-	if (hlt_works(smp_processor_id()))
-		for (;;) halt();
-	for (;;);
-}
-
 /*
  * this function calls the 'stop' function on all other CPUs in the system.
  */
-- 
1.5.6.3


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-11-10 22:49       ` Pavel Machek
@ 2008-11-11 12:48         ` Ivan Vecera
  0 siblings, 0 replies; 11+ messages in thread
From: Ivan Vecera @ 2008-11-11 12:48 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Ingo Molnar, linux-kernel, tglx, mingo, hpa, nhorman, jmarchan

Pavel Machek wrote:
> On Thu 2008-11-06 17:10:01, Ivan Vecera wrote:
>> Any comments?
>>
>> Ivan
>>
>> ---
>> Ivan Vecera wrote:
>>> Ingo Molnar wrote:
>>>> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to 
>>>> this and could be shared. You could move the stop_this_cpu() function to 
>>>> arch/x86/kernel/process.c (out of smp.c), so that it can be used by 
>>>> reboot.c.
>>>>
>>> Yes, this make sense. Here is the patch.
>>>
>>> Ivan
>>>
>>> ---
>>>  arch/x86/kernel/process.c |   16 ++++++++++++++++
>>>  arch/x86/kernel/reboot.c  |    5 +++++
>>>  arch/x86/kernel/smp.c     |   13 -------------
>>>  include/asm-x86/system.h  |    2 ++
>>>  4 files changed, 23 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>>> index c622772..af6904e 100644
>>> --- a/arch/x86/kernel/process.c
>>> +++ b/arch/x86/kernel/process.c
>>> @@ -8,6 +8,7 @@
>>>  #include <linux/pm.h>
>>>  #include <linux/clockchips.h>
>>>  #include <asm/system.h>
>>> +#include <asm/apic.h>
>>>  
>>>  unsigned long idle_halt;
>>>  EXPORT_SYMBOL(idle_halt);
>>> @@ -122,6 +123,21 @@ void default_idle(void)
>>>  EXPORT_SYMBOL(default_idle);
>>>  #endif
>>>  
>>> +void stop_this_cpu(void *dummy)
>>> +{
>>> +	local_irq_disable();
>>> +	/*
>>> +	 * Remove this CPU:
>>> +	 */
>>> +	cpu_clear(smp_processor_id(), cpu_online_map);
>>> +#ifdef CONFIG_X86_LOCAL_APIC
>>> +	disable_local_APIC();
>>> +#endif
>>> +	if (hlt_works(smp_processor_id()))
>>> +		for (;;) halt();
>>> +	for (;;);
>>> +}
> 
> Why the new ifdef? And while we are at it, why is it neccessary to
> disable APIC for stopping CPU? (comment in code would be nice)
Because stop_this_cpu is shared between native_machine_halt and
native_smp_send_stop. It was only moved from smp.c to process.c.
native_machine_halt doesn't require disable APIC for stopping the
current CPU because at this time is already disabled.
The ifdef is another thing and I sent already corrected patch.
> 
>>> -static void stop_this_cpu(void *dummy)
>>> -{
>>> -	local_irq_disable();
>>> -	/*
>>> -	 * Remove this CPU:
>>> -	 */
>>> -	cpu_clear(smp_processor_id(), cpu_online_map);
>>> -	disable_local_APIC();
>>> -	if (hlt_works(smp_processor_id()))
>>> -		for (;;) halt();
>>> -	for (;;);
>>> -}
>>> -
>>>  /*
>>>   * this function calls the 'stop' function on all other CPUs in the system.
>>>   */
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt
  2008-11-11 12:47       ` Ivan Vecera
@ 2008-11-11 13:47         ` Ingo Molnar
  0 siblings, 0 replies; 11+ messages in thread
From: Ingo Molnar @ 2008-11-11 13:47 UTC (permalink / raw)
  To: Ivan Vecera; +Cc: linux-kernel, tglx, mingo, hpa, nhorman, jmarchan, pavel


* Ivan Vecera <ivecera@redhat.com> wrote:

> Ingo Molnar wrote:
> > looks good. One small detail:
> > 
> >> +#ifdef CONFIG_X86_LOCAL_APIC
> >> +	disable_local_APIC();
> >> +#endif
> > 
> > could you please avoid this #ifdef by putting an inline void function 
> > for disable_local_APIC() into arch/x86/include/asm/apic.h for the 
> > !CONFIG_X86_LOCAL_APIC case?
> > 
> > 	Ingo
> OK, here is the result.
> 
> Ivan
> 
> ---
>  arch/x86/include/asm/apic.h   |    1 +
>  arch/x86/include/asm/system.h |    2 ++
>  arch/x86/kernel/process.c     |   14 ++++++++++++++
>  arch/x86/kernel/reboot.c      |    5 +++++
>  arch/x86/kernel/smp.c         |   13 -------------
>  5 files changed, 22 insertions(+), 13 deletions(-)

applied to tip/x86/reboot, thanks Ivan!

	Ingo

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2008-11-11 13:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-20 12:13 [PATCH] x86: call machine_shutdown and stop all CPUs in native_machine_halt Ivan Vecera
2008-10-20 14:39 ` Neil Horman
2008-10-20 16:00 ` Ingo Molnar
2008-10-21 12:36   ` Ivan Vecera
2008-11-06 16:10     ` Ivan Vecera
2008-11-06 16:15       ` Neil Horman
2008-11-10 22:49       ` Pavel Machek
2008-11-11 12:48         ` Ivan Vecera
2008-11-06 20:43     ` Ingo Molnar
2008-11-11 12:47       ` Ivan Vecera
2008-11-11 13:47         ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).