ARM: remove redundant irq disable at halt and restart

PATCH In review Johan Hovold 2 participants 8 messages Oct 24, 2014 – Nov 26, 2014 Arm (32-bit)

lore

Share & tools

lore

Johan Hovold

author

2014-10-24 19:06 UTC #0 lore raw

PATCH 1/1 ARM: remove redundant irq disable at halt and restart

Remove redundant local_irq_disable() at machine halt and restart.

Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.

Signed-off-by Johan Hovold <[email protected]>

---
 arch/arm/kernel/process.c | 2 --
 1 file changed, 2 deletions(-)

arch/arm/kernel/process.c +0 -2

index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }

@@ -237,7 +236,6 @@ void machine_restart(char *cmd)
 
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();
 	while (1);
 }

Signature · 1 line

2.0.4

Felipe Balbi

2014-10-24 19:16 UTC #1 lore raw

Quoted 5 lines · Johan Hovold

Remove redundant local_irq_disable() at machine halt and restart.
Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.

Assuming this is correct, you should have:

Fixes 44424c3 (ARM: 7803/1: Fix deadlock scenario with smp_send_stop())

Cc <[email protected]> # v3.12+

Quoted 18 lines

Signed-off-by Johan Hovold <[email protected]>

---
 arch/arm/kernel/process.c | 2 --
 1 file changed, 2 deletions(-)

arch/arm/kernel/process.c +0 -1

index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }

arch/arm/kernel/process.c void machine_restart(char *cmd) · line 240

Quoted 4 lines

arch/arm/kernel/process.c +0 -1

@@ -237,7 +236,6 @@ void machine_restart(char *cmd)
 
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();

... but wouldn't this reintroduce the the buck which that commit fixed ?

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/2526e9cd/attachment.sig>

Felipe Balbi

2014-10-24 19:21 UTC #2 lore raw

Quoted 29 lines · Felipe Balbi

Quoted 5 lines · Johan Hovold
Remove redundant local_irq_disable() at machine halt and restart.
Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.
Assuming this is correct, you should have:
Fixes 44424c3 (ARM: 7803/1: Fix deadlock scenario with smp_send_stop())
Cc <[email protected]> # v3.12+
Quoted 17 lines
Signed-off-by Johan Hovold <[email protected]>
---
 arch/arm/kernel/process.c | 2 --
 1 file changed, 2 deletions(-)
arch/arm/kernel/process.c +0 -1
index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }
 

Quoted 7 lines

arch/arm/kernel/process.c void machine_restart(char *cmd) · line 240
Quoted 5 lines
arch/arm/kernel/process.c +0 -1
@@ -237,7 +236,6 @@ void machine_restart(char *cmd)
 
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();
... but wouldn't this reintroduce the the buck which that commit fixed ?

s/buck/bug :-) my fingers have a mind of their own, aparently.

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/fce47236/attachment.sig>

Johan Hovold

author

2014-10-24 19:28 UTC #3 lore raw

Quoted 4 lines · Felipe Balbi

Quoted 3 lines · Felipe Balbi
Quoted 2 lines · Johan Hovold
Remove redundant local_irq_disable() at machine halt and restart.

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.
Assuming this is correct, you should have:
Fixes 44424c3 (ARM: 7803/1: Fix deadlock scenario with smp_send_stop())
Cc <[email protected]> # v3.12+

It's not a bug. Just a redundant disabling of already disabled interrupts, something which could possibly lead someone to believe that interrupts could be re-enabled by the power-off handler.

And if that was the case, wouldn't that introduce the bug that 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") was trying to fix?

Quoted 19 lines

Signed-off-by Johan Hovold <[email protected]>

---
 arch/arm/kernel/process.c | 2 --
 1 file changed, 2 deletions(-)

arch/arm/kernel/process.c +0 -1

index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }

@@ -237,7 +236,6 @@ void machine_restart(char *cmd)

Quoted 7 lines

Quoted 5 lines
arch/arm/kernel/process.c void machine_restart(char *cmd) · line 240
Quoted 3 lines
arch/arm/kernel/process.c +0 -1
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();
... but wouldn't this reintroduce the the buck which that commit fixed ?
s/buck/bug :-) my fingers have a mind of their own, aparently.

No, the interrupts would still be disabled.

Thanks,
Johan

Felipe Balbi

2014-10-24 19:42 UTC #4 lore raw

Quoted 9 lines · Johan Hovold

Quoted 8 lines · Felipe Balbi
Quoted 7 lines · Felipe Balbi
Quoted 5 lines · Johan Hovold
Remove redundant local_irq_disable() at machine halt and restart.
Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Assuming this is correct, you should have:
Fixes 44424c3 (ARM: 7803/1: Fix deadlock scenario with smp_send_stop())
Cc <[email protected]> # v3.12+
It's not a bug. Just a redundant disabling of already disabled interrupts, something which could possibly lead someone to believe that interrupts could be re-enabled by the power-off handler.

I didn't dig any of this out but I'll assume you did :-) So I withdraw my comment ;-)

Quoted 20 lines

Quoted 20 lines
Quoted 19 lines
Quoted 13 lines
arch/arm/kernel/process.c +0 -1
index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }
 
arch/arm/kernel/process.c void machine_restart(char *cmd) · line 240
Quoted 4 lines
arch/arm/kernel/process.c +0 -1
@@ -237,7 +236,6 @@ void machine_restart(char *cmd)
 
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();
... but wouldn't this reintroduce the the buck which that commit fixed ?

Quoted 5 lines

Quoted 1 line
s/buck/bug :-) my fingers have a mind of their own, aparently.
:)
No, the interrupts would still be disabled.

alright... so far I couldn't find where IRQs are disable before machine_power_off() is called. Starting a do_poweroff(), couldn't find it... Oh well, I'll keep digging.

cheers

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/6c01e9a1/attachment-0001.sig>

Johan Hovold

author

2014-10-24 19:50 UTC #5 lore raw

Quoted 10 lines · Felipe Balbi

Quoted 9 lines · Johan Hovold
Quoted 8 lines · Felipe Balbi
Quoted 7 lines · Felipe Balbi
Quoted 5 lines · Johan Hovold
Remove redundant local_irq_disable() at machine halt and restart.
Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Assuming this is correct, you should have:
Fixes 44424c3 (ARM: 7803/1: Fix deadlock scenario with smp_send_stop())
Cc <[email protected]> # v3.12+
It's not a bug. Just a redundant disabling of already disabled interrupts, something which could possibly lead someone to believe that interrupts could be re-enabled by the power-off handler.

I meant re-enabled by arm_pm_restart().

Quoted 25 lines

I didn't dig any of this out but I'll assume you did :-) So I withdraw my comment ;-)
Quoted 22 lines
Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
arch/arm/kernel/process.c +0 -1
index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }
 
Quoted 7 lines
arch/arm/kernel/process.c void machine_restart(char *cmd) · line 240
Quoted 5 lines
arch/arm/kernel/process.c +0 -1
@@ -237,7 +236,6 @@ void machine_restart(char *cmd)
 
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();
... but wouldn't this reintroduce the the buck which that commit fixed ?
s/buck/bug :-) my fingers have a mind of their own, aparently.

Quoted 7 lines

Quoted 3 lines
:)
No, the interrupts would still be disabled.
alright... so far I couldn't find where IRQs are disable before machine_power_off() is called. Starting a do_poweroff(), couldn't find it... Oh well, I'll keep digging.

It's done a few lines above in the same function. ;)

void machine_restart(char *cmd)

{
        local_irq_disable();
	^^^
        smp_send_stop();

        arm_pm_restart(reboot_mode, cmd);

        /* Give a grace period for failure to restart of 1s */
        mdelay(1000);

        /* Whoops - the platform was unable to reboot. Tell the user! */
        printk("Reboot failed -- System halted\n");
        local_irq_disable();
        while (1);
}

[ and similarly in machine_power_off(). ]

Johan

Felipe Balbi

Reviewed-by

2014-10-24 20:07 UTC #6 lore raw

Quoted 74 lines · Johan Hovold

Quoted 10 lines · Felipe Balbi
Quoted 9 lines · Johan Hovold
Quoted 8 lines · Felipe Balbi
Quoted 7 lines · Felipe Balbi
Quoted 5 lines · Johan Hovold
Remove redundant local_irq_disable() at machine halt and restart.
Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Assuming this is correct, you should have:
Fixes 44424c3 (ARM: 7803/1: Fix deadlock scenario with smp_send_stop())
Cc <[email protected]> # v3.12+
It's not a bug. Just a redundant disabling of already disabled interrupts, something which could possibly lead someone to believe that interrupts could be re-enabled by the power-off handler.
I meant re-enabled by arm_pm_restart().
Quoted 25 lines
I didn't dig any of this out but I'll assume you did :-) So I withdraw my comment ;-)
Quoted 22 lines
Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
arch/arm/kernel/process.c +0 -1
index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }
 
Quoted 7 lines
arch/arm/kernel/process.c void machine_restart(char *cmd) · line 240
Quoted 5 lines
arch/arm/kernel/process.c +0 -1
@@ -237,7 +236,6 @@ void machine_restart(char *cmd)
 
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();
... but wouldn't this reintroduce the the buck which that commit fixed ?
s/buck/bug :-) my fingers have a mind of their own, aparently.
Quoted 7 lines
Quoted 3 lines
:)
No, the interrupts would still be disabled.
alright... so far I couldn't find where IRQs are disable before machine_power_off() is called. Starting a do_poweroff(), couldn't find it... Oh well, I'll keep digging.
It's done a few lines above in the same function. ;)
void machine_restart(char *cmd)
{
        local_irq_disable();
	^^^
        smp_send_stop();

        arm_pm_restart(reboot_mode, cmd);

        /* Give a grace period for failure to restart of 1s */
        mdelay(1000);

        /* Whoops - the platform was unable to reboot. Tell the user! */
        printk("Reboot failed -- System halted\n");
        local_irq_disable();
        while (1);
}

Quoted 1 line

[ and similarly in machine_power_off(). ]

oh, now I get it :-) Nevermind, I completely missed that it was duplicated. My bad.

Reviewed-by Felipe Balbi <[email protected]>

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/2fe28c40/attachment-0001.sig>

Johan Hovold

author

2014-11-26 15:13 UTC #7 lore raw

In reply to Johan Hovold

Quoted 7 lines · Johan Hovold

Remove redundant local_irq_disable() at machine halt and restart.
Since commit 44424c34049f ("ARM: 7803/1: Fix deadlock scenario with smp_send_stop()") interrupts are disabled before stopping secondary CPUs.
Signed-off-by Johan Hovold <[email protected]>

Russell, have you had a chance to look at this clean up?

Thanks,
Johan

Quoted 23 lines

---
 arch/arm/kernel/process.c | 2 --
 1 file changed, 2 deletions(-)

arch/arm/kernel/process.c +0 -2

index a35f6ebbd2c2..5663ab57cf07 100644
@@ -195,7 +195,6 @@ void machine_halt(void)
 	local_irq_disable();
 	smp_send_stop();
 
-	local_irq_disable();
 	while (1);
 }

@@ -237,7 +236,6 @@ void machine_restart(char *cmd)
 
 	/* Whoops - the platform was unable to reboot. Tell the user! */
 	printk("Reboot failed -- System halted\n");
-	local_irq_disable();
 	while (1);
 }