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

[David Daney <ddaney.cavm@gmail.com>]

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

>
> 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();
>>>   }
>>>
>>>   /**
>