Re: [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary()

[Yong Zhang <yong.zhang0@gmail.com>]

On Tue, Apr 17, 2012 at 01:45:50PM -0700, David Daney wrote:
> On 04/16/2012 08:08 PM, Yong Zhang wrote:
> >On Mon, Apr 16, 2012 at 09:48:14AM -0700, David Daney wrote:
> >>On 04/16/2012 12:25 AM, Yong Zhang wrote:
> >>>From: Yong Zhang<yong.zhang@windriver.com>
> >>>
> >>>Too early to enable irq will break some following action,
> >>>such as notify_cpu_starting().
> >>
> >>Can you be more specific about what breaks?
> >
> >For example:
> >
> >	CPU1				CPU2
> >__cpu_up();
> >   mp_ops->boot_secondary();
> >     				start_secondary();
> >				  octeon_init_secondary();
> >				    raw_local_irq_enable();
> >				<IRQ>
> >				do something;
> >				      wake up softirqd;
> >				      try_to_wake_up();
> >				        select_fallback_rq();
> >					/* select wrong cpu */
> >     set_cpu_online();
> >
> 
> Yeah, that looks broken to me too.
> 
> >>
> >>>
> >>>I don't get side effect with this patch.
> >>
> >>Without this, where do irqs get enabled on the secondary CPUs?
> >
> >cpu_idle() will handle it. But in fact we should not depend on
> >cpu_idle().
> 
> It is not done in cpu_idle() itself.  If irqs are disabled upon
> entry to cpu_idle() *and* need_resched(), then we call into
> schedule().  The irqs are then enabled by the
> raw_spin_unlock_irq(&rq->lock) at the end of kernel/sched/core.c:
> __schedule().
> 
> >
> >But it seems there is not suitable place to put local_irq_enable(),
> >though ->smp_finish() looks like a more suitable place.
> 
> It would be better, but it seems like really it should be done in
> cpu_idle() immediately after rcu_idle_enter().

Hmm... seems we should put it before enter while (1) in cpu_idle(),
otherwise tick_nohz_idle_enter() will be unhappy if NO_HZ enabled.

But anyway, we still cann't survive from set_cpu_online() called on
other cpu. I'll recheck smp support for MIPS to see if things could
be improved ;-)

Thanks,
Yong
> 
> >
> >When looking more at smp support on MIPS, there is more things I find.
> >Such as set_cpu_online() is called on CPU1, so there will be another race
> >window like above scenario. Please take a look at what commit 2baab4e9
> >intend to resolve.
> >
> >Thanks,
> >Yong
> >
> >
> >
> >
> >>
> >>>
> >>>Signed-off-by: Yong Zhang<yong.zhang0@gmail.com>
> >>>Cc: David Daney<ddaney@caviumnetworks.com>
> >>>Cc: Ralf Baechle<ralf@linux-mips.org>
> >>>---
> >>>  arch/mips/cavium-octeon/smp.c |    1 -
> >>>  1 files changed, 0 insertions(+), 1 deletions(-)
> >>>
> >>>diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
> >>>index 97e7ce9..7e65c88 100644
> >>>--- a/arch/mips/cavium-octeon/smp.c
> >>>+++ b/arch/mips/cavium-octeon/smp.c
> >>>@@ -185,7 +185,6 @@ static void __cpuinit octeon_init_secondary(void)
> >>>  	octeon_init_cvmcount();
> >>>
> >>>  	octeon_irq_setup_secondary();
> >>>-	raw_local_irq_enable();
> >>>  }
> >>>
> >>>  /**
> >