Remove unnecessary irq disabling

RR asks us if it is really necessary to disable interrupts in setup_secondary_APIC_clock(). The answer is no, since setup_APIC_timer() starts by saving irq flags, which also disables them.

Signed-off-by Glauber de Oliveira Costa <[email protected]>

Quoted 17 lines · Glauber de Oliveira Costa

RR asks us if it is really necessary to disable interrupts in setup_secondary_APIC_clock(). The answer is no, since setup_APIC_timer() starts by saving irq flags, which also disables them.

Signed-off-by Glauber de Oliveira Costa <[email protected]>

arch/x86_64/kernel/apic.c +0 -2

@@ -875,9 +875,7 @@ void __init setup_boot_APIC_clock (void)
 
 void __cpuinit setup_secondary_APIC_clock(void)
 {
-	local_irq_disable(); /* FIXME: Do we need this? --RR */
 	setup_APIC_timer(calibration_result);
-	local_irq_enable();
 }
 

arch/x86_64/kernel/apic.c void __init setup_boot_APIC_clock (void) · line 881

Quoted 1 line

arch/x86_64/kernel/apic.c +0 -0

 void disable_APIC_timer(void)

Okay, I'll bite: before the patch, this code would exit
with interrupts *enabled*, always. Now it does not.

What does that break, or was it already broken and this fixes it?

Quoted 2 lines

What does that break, or was it already broken and this fixes it?

I think neither. This function is only called at early bootup, (start_secondary() ), and most of its callees have interrupts off anyway. But maybe we do lose something. Andi, do you have a word on this?

Quoted 4 lines

Quoted 1 line

What does that break, or was it already broken and this fixes it?

I think neither. This function is only called at early bootup, (start_secondary() ), and most of its callees have interrupts off anyway. But maybe we do lose something. Andi, do you have a word on this?

We need to enable them somewhere, but cpu_idle will do it in the end. So it should be safe to drop it. I guess keeping them disabled the whole time will be a little safer against potential races.

I added the patch, but might want some cooking in -mm first because this is always fragile code.

Quoted 5 lines

Quoted 2 lines

What does that break, or was it already broken and this fixes it?

I think neither. This function is only called at early bootup, (start_secondary() ), and most of its callees have interrupts off anyway. But maybe we do lose something. Andi, do you have a word on this?

We need to enable them somewhere, but cpu_idle will do it in the end. So it should be safe to drop it. I guess keeping them disabled the whole time will be a little safer against potential races.