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

[David Daney <ddaney.cavm@xxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > >
> > > 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().

> 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@xxxxxxxxx>
> > > Cc: David Daney<ddaney@xxxxxxxxxxxxxxxxxx>
> > > Cc: Ralf Baechle<ralf@xxxxxxxxxxxxxx>
> > > ---
> > >   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();
> > >   }
> > >
> > >   /**