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

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

[Yong Zhang]

From: Yong Zhang <yong.zhang@windriver.com>

Too early to enable irq will break some following action,
such as notify_cpu_starting().

I don't get side effect with this patch.

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

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

[David Daney]

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?

>
> I don't get side effect with this patch.

Without this, where do irqs get enabled on the secondary CPUs?

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

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

[Yong Zhang]

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

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

But it seems there is not suitable place to put local_irq_enable(),
though ->smp_finish() looks like a more suitable place.

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

-- 
Only stand for myself

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

[David Daney]

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

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

[Yong Zhang]

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