From: Thomas Gleixner <[email protected]>
Instead of making the lock raw, remove it as it protects nothing.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
Signed-off-by: Steven Rostedt <[email protected]>
---
arch/mips/cavium-octeon/smp.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
index efcfff4..86fce15 100644
--- a/arch/mips/cavium-octeon/smp.c
+++ b/arch/mips/cavium-octeon/smp.c
@@ -257,8 +257,6 @@ DEFINE_PER_CPU(int, cpu_state);
extern void fixup_irqs(void);
-static DEFINE_SPINLOCK(smp_reserve_lock);
-
static int octeon_cpu_disable(void)
{
unsigned int cpu = smp_processor_id();
@@ -266,8 +264,6 @@ static int octeon_cpu_disable(void)
if (cpu == 0)
return -EBUSY;
- spin_lock(&smp_reserve_lock);
-
cpu_clear(cpu, cpu_online_map);
cpu_clear(cpu, cpu_callin_map);
local_irq_disable();
@@ -277,8 +273,6 @@ static int octeon_cpu_disable(void)
flush_cache_all();
local_flush_tlb_all();
- spin_unlock(&smp_reserve_lock);
-
return 0;
}
--
1.7.10
On 06/07/2012 08:51 AM, Steven Rostedt wrote:
> From: Thomas Gleixner<[email protected]>
>
> Instead of making the lock raw, remove it as it protects nothing.
I don't know how you guys are managing the RT branch, but this seems
quite similar to:
a3c8b4faeeccb33dbad6969bc9e50bf409f167e7 (MIPS: Cavium: Remove
smp_reserve_lock.)
David Daney
>
> Signed-off-by: Thomas Gleixner<[email protected]>
> Cc: [email protected]
> Signed-off-by: Steven Rostedt<[email protected]>
> ---
> arch/mips/cavium-octeon/smp.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
> index efcfff4..86fce15 100644
> --- a/arch/mips/cavium-octeon/smp.c
> +++ b/arch/mips/cavium-octeon/smp.c
> @@ -257,8 +257,6 @@ DEFINE_PER_CPU(int, cpu_state);
>
> extern void fixup_irqs(void);
>
> -static DEFINE_SPINLOCK(smp_reserve_lock);
> -
> static int octeon_cpu_disable(void)
> {
> unsigned int cpu = smp_processor_id();
> @@ -266,8 +264,6 @@ static int octeon_cpu_disable(void)
> if (cpu == 0)
> return -EBUSY;
>
> - spin_lock(&smp_reserve_lock);
> -
> cpu_clear(cpu, cpu_online_map);
> cpu_clear(cpu, cpu_callin_map);
> local_irq_disable();
> @@ -277,8 +273,6 @@ static int octeon_cpu_disable(void)
> flush_cache_all();
> local_flush_tlb_all();
>
> - spin_unlock(&smp_reserve_lock);
> -
> return 0;
> }
>
On Thu, 2012-06-07 at 10:50 -0700, David Daney wrote:
> On 06/07/2012 08:51 AM, Steven Rostedt wrote:
> > From: Thomas Gleixner<[email protected]>
> >
> > Instead of making the lock raw, remove it as it protects nothing.
>
> I don't know how you guys are managing the RT branch, but this seems
> quite similar to:
>
> a3c8b4faeeccb33dbad6969bc9e50bf409f167e7 (MIPS: Cavium: Remove
> smp_reserve_lock.)
Great! Then we don't need to worry about it :-)
But as it doesn't seem that this patch was marked as stable, we will be
carrying it in -rt where we support older kernels.
Should it go to mainline stable?
-- Steve
>
> David Daney
>
On 06/07/2012 11:56 AM, Steven Rostedt wrote:
> On Thu, 2012-06-07 at 10:50 -0700, David Daney wrote:
>> On 06/07/2012 08:51 AM, Steven Rostedt wrote:
>>> From: Thomas Gleixner<[email protected]>
>>>
>>> Instead of making the lock raw, remove it as it protects nothing.
>>
>> I don't know how you guys are managing the RT branch, but this seems
>> quite similar to:
>>
>> a3c8b4faeeccb33dbad6969bc9e50bf409f167e7 (MIPS: Cavium: Remove
>> smp_reserve_lock.)
>
> Great! Then we don't need to worry about it :-)
>
> But as it doesn't seem that this patch was marked as stable, we will be
> carrying it in -rt where we support older kernels.
>
> Should it go to mainline stable?
>
I don't think it is necessary. As far as I know, RT may be the only
thing that needs it.
David Daney
> -- Steve
>
>>
>> David Daney
>>
>
>
On Thu, 2012-06-07 at 12:08 -0700, David Daney wrote:
> > Should it go to mainline stable?
> >
>
> I don't think it is necessary. As far as I know, RT may be the only
> thing that needs it.
Ah, you're right. As this is just an issue because it is called with
interrupts disabled (from stop_machine). Although it's interesting that
the mips code, re-enables interrupts from that function.
>From kernel/cpu.c:
_cpu_down() {
__stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
take_cpu_down() {
err = __cpu_disable();
kernel/stop_machine.c:
__stop_machine(int (*fn)(void *) ...) {
local_irq_save(flags);
hard_irq_disable();
ret = (*fn)(data);
local_irq_restore(flags);
arch/mips/include/asm/smp.h:
static inline int __cpu_disable(void)
{
extern struct plat_smp_ops *mp_ops; /* private */
return mp_ops->cpu_disable();
}
arch/mips/cavium-octeon/smp.c:
octeon_cpu_disable(void) {
local_irq_disable();
fixup_irqs();
local_irq_enable();
struct plat_smp_ops octeon_smp_ops = {
.cpu_disable = octeon_cpu_disable,
Is this expected? It causes the cpu notifiers to be called with
interrupts enabled. Not sure if that's a problem or not.
-- Steve
On 06/07/2012 12:32 PM, Steven Rostedt wrote:
> On Thu, 2012-06-07 at 12:08 -0700, David Daney wrote:
>
>>> Should it go to mainline stable?
>>>
>>
>> I don't think it is necessary. As far as I know, RT may be the only
>> thing that needs it.
>
> Ah, you're right. As this is just an issue because it is called with
> interrupts disabled (from stop_machine). Although it's interesting that
> the mips code, re-enables interrupts from that function.
>
> From kernel/cpu.c:
>
> _cpu_down() {
> __stop_machine(take_cpu_down,&tcd_param, cpumask_of(cpu));
>
> take_cpu_down() {
> err = __cpu_disable();
>
> kernel/stop_machine.c:
>
> __stop_machine(int (*fn)(void *) ...) {
> local_irq_save(flags);
> hard_irq_disable();
> ret = (*fn)(data);
> local_irq_restore(flags);
>
>
> arch/mips/include/asm/smp.h:
>
> static inline int __cpu_disable(void)
> {
> extern struct plat_smp_ops *mp_ops; /* private */
>
> return mp_ops->cpu_disable();
> }
>
> arch/mips/cavium-octeon/smp.c:
>
> octeon_cpu_disable(void) {
> local_irq_disable();
> fixup_irqs();
> local_irq_enable();
>
> struct plat_smp_ops octeon_smp_ops = {
> .cpu_disable = octeon_cpu_disable,
>
>
> Is this expected? It causes the cpu notifiers to be called with
> interrupts enabled. Not sure if that's a problem or not.
I am inclined to go with your instinct here. Probably we shouldn't
unconditionally local_irq_enable() here.
Perhaps {,raw}_local_irq_save/{,raw}local_irq_restore would be better.
Or even no local irq enable manipulation...
In any event, I may let Ralf sort it out.
David Daney