Re: [UPDATED PATCH 21/27] sparc32, smpboot: Use generic SMP bootinginfrastructure

From: Srivatsa S. Bhat
Date: Sat Jun 02 2012 - 12:00:20 EST

Next message: Witold Baryluk: "WARNING: arch/x86/built-in.o: Section mismatch in reference fromthe variable test_nmi_ipi_callback_na to function.ini.text:test_nmi_ipi_callback"
Previous message: BenoÃt ThÃbaudeau: "Re: init: multi param parsing regression since 3.4"
In reply to: Sam Ravnborg: "Re: [UPDATED PATCH 21/27] sparc32, smpboot: Use generic SMP bootinginfrastructure"
Next in thread: Sam Ravnborg: "Re: [UPDATED PATCH 21/27] sparc32, smpboot: Use generic SMPbooting infrastructure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 06/02/2012 08:43 PM, Sam Ravnborg wrote:

>>
>>> I will try to dive into this and see if I on the sparc32 side
>>> can make leon behave like the other platforms, and then
>>> unify some of the smp cpu boot stuff such that we then
>>> can introduce the generalization from your patch.
>
> This is what I came up with.
> I modelled this over your patch-set to make it
> simpler to convert to the generic scheme.
>

Thanks a lot for doing this :-)

> I realised a bit more how CPU's are started up on sparc32.
> So I think all of leon, sun4m and sun4d should work with this.
>
> But my SUN box is defunct atm. so I have not tested it.
> Comments appreciated!
>
> PS. I used the ugly __names for now.
> As soon as you have found a better set of names this should
> be fixed here too.
>

As I mentioned in my other mail, I am thinking of changing them to
arch_cpu_pre_starting(), arch_cpu_pre_online() and arch_cpu_post_online().
Let me know what you think of those names.

Would you kindly add a changelog and your sign-off to this patch?

>
> diff --git a/arch/sparc/kernel/kernel.h b/arch/sparc/kernel/kernel.h
> index 291bb5d..2bc1c6d 100644
> --- a/arch/sparc/kernel/kernel.h
> +++ b/arch/sparc/kernel/kernel.h
> @@ -48,6 +48,10 @@ extern void sun4m_init_IRQ(void);
> extern void sun4m_unmask_profile_irq(void);
> extern void sun4m_clear_profile_irq(int cpu);
>
> +/* sun4m_smp.c */
> +void sun4m_cpu_pre_starting(void);
> +void sun4m_cpu_pre_online(void);
> +
> /* sun4d_irq.c */
> extern spinlock_t sun4d_imsk_lock;
>
> @@ -60,6 +64,14 @@ extern int show_sun4d_interrupts(struct seq_file *, void *);
> extern void sun4d_distribute_irqs(void);
> extern void sun4d_free_irq(unsigned int irq, void *dev_id);
>
> +/* sun4d_smp.c */
> +void sun4d_cpu_pre_starting(void);
> +void sun4d_cpu_pre_online(void);
> +
> +/* leon_smp.c */
> +void leon_cpu_pre_starting(void);
> +void leon_cpu_pre_online(void);
> +
> /* head_32.S */
> extern unsigned int t_nmi[];
> extern unsigned int linux_trap_ipi15_sun4d[];
> diff --git a/arch/sparc/kernel/leon_smp.c b/arch/sparc/kernel/leon_smp.c
> index 0f3fb6d..0b028b3 100644
> --- a/arch/sparc/kernel/leon_smp.c
> +++ b/arch/sparc/kernel/leon_smp.c
> @@ -69,31 +69,20 @@ static inline unsigned long do_swap(volatile unsigned long *ptr,
> return val;
> }
>
> -void __cpuinit leon_callin(void)
> +void __cpuinit leon_cpu_pre_starting(void)
> {
> - int cpuid = hard_smp_processor_id();
> -
> - local_ops->cache_all();
> - local_ops->tlb_all();
> leon_configure_cache_smp();
> +}
>
> - notify_cpu_starting(cpuid);
> -
> - /* Get our local ticker going. */
> - register_percpu_ce(cpuid);
> -
> - calibrate_delay();
> - smp_store_cpu_info(cpuid);
>
> - local_ops->cache_all();
> - local_ops->tlb_all();
> +void __cpuinit cpu_cpu_pre_starting(void *unused)

Shouldn't this be leon_cpu_pre_online()? And the parameters
should be consistent too (here it expects a void*, but it is
called with no arguments...)

> +{
> + int cpuid = hard_smp_processor_id();
>
> - /*
> - * Unblock the master CPU _only_ when the scheduler state
> - * of all secondary CPUs will be up-to-date, so after
> - * the SMP initialization the master will be just allowed
> - * to call the scheduler code.
> - * Allow master to continue.
> + /* Allow master to continue. The master will then give us the
> + * go-ahead by setting the smp_commenced_mask and will wait without
> + * timeouts until our setup is completed fully (signified by
> + * our bit being set in the cpu_online_mask).
> */
> do_swap(&cpu_callin_map[cpuid], 1);
>
> @@ -110,9 +99,6 @@ void __cpuinit leon_callin(void)
>
> while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
> mb();
> -
> - local_irq_enable();
> - set_cpu_online(cpuid, true);
> }
>
> /*
> diff --git a/arch/sparc/kernel/smp_32.c b/arch/sparc/kernel/smp_32.c
> index 79db45e..2e2990f 100644
> --- a/arch/sparc/kernel/smp_32.c
> +++ b/arch/sparc/kernel/smp_32.c
> @@ -20,6 +20,7 @@
> #include <linux/seq_file.h>
> #include <linux/cache.h>
> #include <linux/delay.h>
> +#include <linux/cpu.h>
>
> #include <asm/ptrace.h>
> #include <linux/atomic.h>
> @@ -32,8 +33,10 @@
> #include <asm/cacheflush.h>
> #include <asm/tlbflush.h>
> #include <asm/cpudata.h>
> +#include <asm/timer.h>
> #include <asm/leon.h>
>
> +#include "kernel.h"
> #include "irq.h"
>
> volatile unsigned long cpu_callin_map[NR_CPUS] __cpuinitdata = {0,};
> @@ -294,6 +297,89 @@ int __cpuinit __cpu_up(unsigned int cpu, struct task_struct *tidle)
> return ret;
> }
>
> +void __cpuinit __cpu_pre_starting(void *unused)
> +{
> + local_ops->cache_all();
> + local_ops->tlb_all();
> +
> + switch(sparc_cpu_model) {
> + case sun4m:
> + sun4m_cpu_pre_starting();

Wouldn't it be better to have a provision to pass the
pointer down to these functions as well?

> + break;
> + case sun4d:
> + sun4d_cpu_pre_starting();
> + break;
> + case sparc_leon:
> + leon_cpu_pre_starting();
> + break;
> + default:
> + BUG();
> + }
> +}
> +
> +void __cpuinit __cpu_pre_online(void *unused)
> +{
> + unsigned int cpuid = hard_smp_processor_id();
> +
> + register_percpu_ce(cpuid);
> +
> + calibrate_delay();
> + smp_store_cpu_info(cpuid);
> +
> + local_ops->cache_all();
> + local_ops->tlb_all();
> +
> + switch(sparc_cpu_model) {
> + case sun4m:
> + sun4m_cpu_pre_online();
> + break;
> + case sun4d:
> + sun4d_cpu_pre_online();
> + break;
> + case sparc_leon:
> + leon_cpu_pre_starting();

Shouldn't this be leon_cpu_pre_online()?

> + break;
> + default:
> + BUG();
> + }
> +}
> +
> +void __cpuinit sparc_start_secondary(void *unused)
> +{
> + unsigned int cpu;
> +
> + /*
> + * SMP booting is extremely fragile in some architectures. So run
> + * the cpu initialization code first before anything else.
> + */
> + __cpu_pre_starting(NULL);
> +

Why not pass the pointer to __cpu_pre_starting() as well as the rest of
the functions?

> + preempt_disable();
> + cpu = smp_processor_id();
> +
> + /* Invoke the CPU_STARTING notifier callbacks */
> + notify_cpu_starting(cpu);
> +
> + __cpu_pre_online(NULL);
> +
> + /* Set the CPU in the cpu_online_mask */
> + set_cpu_online(cpu, true);
> +
> + /* Enable local interrupts now */
> + local_irq_enable();
> +
> + wmb();
> + cpu_idle();
> +
> + /* We should never reach here! */
> + BUG();
> +}
> +
> +void __cpuinit smp_callin(void)
> +{
> + sparc_start_secondary(NULL);
> +}
> +
> void smp_bogo(struct seq_file *m)
> {
> int i;
> diff --git a/arch/sparc/kernel/sun4d_smp.c b/arch/sparc/kernel/sun4d_smp.c
> index ddaea31..bfbbc46 100644
> --- a/arch/sparc/kernel/sun4d_smp.c
> +++ b/arch/sparc/kernel/sun4d_smp.c
> @@ -50,10 +50,9 @@ static inline void show_leds(int cpuid)
> "i" (ASI_M_CTL));
> }
>
> -void __cpuinit smp4d_callin(void)
> +void __cpuinit sun4d_cpu_pre_starting(void)
> {
> int cpuid = hard_smp_processor_id();
> - unsigned long flags;
>
> /* Show we are alive */
> cpu_leds[cpuid] = 0x6;
> @@ -61,24 +60,14 @@ void __cpuinit smp4d_callin(void)
>
> /* Enable level15 interrupt, disable level14 interrupt for now */
> cc_set_imsk((cc_get_imsk() & ~0x8000) | 0x4000);
> +}
>
> - local_ops->cache_all();
> - local_ops->tlb_all();
> -
> - notify_cpu_starting(cpuid);
> - /*
> - * Unblock the master CPU _only_ when the scheduler state
> - * of all secondary CPUs will be up-to-date, so after
> - * the SMP initialization the master will be just allowed
> - * to call the scheduler code.
> - */
> - /* Get our local ticker going. */
> - register_percpu_ce(cpuid);
> +void __cpuinit sun4d_cpu_pre_online(void)
> +{
> + unsigned long flags;
> + int cpuid;
>
> - calibrate_delay();
> - smp_store_cpu_info(cpuid);
> - local_ops->cache_all();
> - local_ops->tlb_all();
> + cpuid = hard_smp_processor_id();
>
> /* Allow master to continue. */
> sun4d_swap((unsigned long *)&cpu_callin_map[cpuid], 1);
> @@ -106,16 +95,12 @@ void __cpuinit smp4d_callin(void)
> local_ops->cache_all();
> local_ops->tlb_all();
>
> - local_irq_enable(); /* We don't allow PIL 14 yet */
> -
> while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
> barrier();
>
> spin_lock_irqsave(&sun4d_imsk_lock, flags);
> cc_set_imsk(cc_get_imsk() & ~0x4000); /* Allow PIL 14 as well */
> spin_unlock_irqrestore(&sun4d_imsk_lock, flags);
> - set_cpu_online(cpuid, true);
> -
> }
>
> /*
> diff --git a/arch/sparc/kernel/sun4m_smp.c b/arch/sparc/kernel/sun4m_smp.c
> index 128af73..0cf6b4f 100644
> --- a/arch/sparc/kernel/sun4m_smp.c
> +++ b/arch/sparc/kernel/sun4m_smp.c
> @@ -34,30 +34,19 @@ swap_ulong(volatile unsigned long *ptr, unsigned long val)
> return val;
> }
>
> -void __cpuinit smp4m_callin(void)
> +void __cpuinit sun4m_cpu_pre_starting(void)
> {
> - int cpuid = hard_smp_processor_id();
> -
> - local_ops->cache_all();
> - local_ops->tlb_all();
> -
> - notify_cpu_starting(cpuid);
> -
> - register_percpu_ce(cpuid);
> -
> - calibrate_delay();
> - smp_store_cpu_info(cpuid);
> +}
>
> - local_ops->cache_all();
> - local_ops->tlb_all();
> +void __cpuinit sun4m_cpu_pre_online(void)
> +{
> + int cpuid = hard_smp_processor_id();
>
> - /*
> - * Unblock the master CPU _only_ when the scheduler state
> - * of all secondary CPUs will be up-to-date, so after
> - * the SMP initialization the master will be just allowed
> - * to call the scheduler code.
> + /* Allow master to continue. The master will then give us the
> + * go-ahead by setting the smp_commenced_mask and will wait without
> + * timeouts until our setup is completed fully (signified by
> + * our bit being set in the cpu_online_mask).
> */
> - /* Allow master to continue. */
> swap_ulong(&cpu_callin_map[cpuid], 1);
>
> /* XXX: What's up with all the flushes? */
> @@ -75,10 +64,6 @@ void __cpuinit smp4m_callin(void)
>
> while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
> mb();
> -
> - local_irq_enable();
> -
> - set_cpu_online(cpuid, true);
> }
>
> /*
> diff --git a/arch/sparc/kernel/trampoline_32.S b/arch/sparc/kernel/trampoline_32.S
> index af27aca..6cdb08c 100644
> --- a/arch/sparc/kernel/trampoline_32.S
> +++ b/arch/sparc/kernel/trampoline_32.S
> @@ -79,18 +79,15 @@ cpu3_startup:
> nop
>
> /* Start this processor. */
> - call smp4m_callin
> + call smp_callin
> nop
>
> - b,a smp_do_cpu_idle
> + b,a smp_panic
>
> .text
> .align 4
>
> -smp_do_cpu_idle:
> - call cpu_idle
> - mov 0, %o0
> -
> +smp_panic:
> call cpu_panic
> nop
>
> @@ -144,10 +141,10 @@ sun4d_cpu_startup:
> nop
>
> /* Start this processor. */
> - call smp4d_callin
> + call smp_callin
> nop
>
> - b,a smp_do_cpu_idle
> + b,a smp_panic
>
> __CPUINIT
> .align 4
> @@ -201,7 +198,7 @@ leon_smp_cpu_startup:
> nop
>
> /* Start this processor. */
> - call leon_callin
> + call smp_callin

I still didn't get how this solves the original problem of
not having sparc_cpu_model set to sparc_leon. You mentioned
that by the time we reach leon_smp_cpu_startup, that variable
is not set. Even inside leon_smp_cpu_startup, I don't immediately
see where it is set. Am I missing something?

> nop
>
> - b,a smp_do_cpu_idle
> + b,a smp_panic
>

Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Witold Baryluk: "WARNING: arch/x86/built-in.o: Section mismatch in reference fromthe variable test_nmi_ipi_callback_na to function.ini.text:test_nmi_ipi_callback"
Previous message: BenoÃt ThÃbaudeau: "Re: init: multi param parsing regression since 3.4"
In reply to: Sam Ravnborg: "Re: [UPDATED PATCH 21/27] sparc32, smpboot: Use generic SMP bootinginfrastructure"
Next in thread: Sam Ravnborg: "Re: [UPDATED PATCH 21/27] sparc32, smpboot: Use generic SMPbooting infrastructure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]