Skip to contentSkip to conversation

FAILED: patch "[PATCH] riscv: Avoid interrupts being erroneously enabled in" failed to apply to 5.3-stable tree

PATCH [email protected] 3 participants 5 messages Oct 8, 2019 – Oct 9, 2019

Subsystems (2)
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 5.3-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From c82dd6d078a2bb29d41eda032bb96d05699a524d Mon Sep 17 00:00:00 2001
From: Vincent Chen <[email protected]>
Date: Mon, 16 Sep 2019 16:47:41 +0800
Subject: [PATCH] riscv: Avoid interrupts being erroneously enabled in
handle_exception()

When the handle_exception function addresses an exception, the interrupts will be unconditionally enabled after finishing the context save. However, It may erroneously enable the interrupts if the interrupts are disabled before entering the handle_exception.

For example, one of the WARN_ON() condition is satisfied in the scheduling where the interrupt is disabled and rq.lock is locked. The WARN_ON will trigger a break exception and the handle_exception function will enable the interrupts before entering do_trap_break function. During the procedure, if a timer interrupt is pending, it will be taken when interrupts are enabled. In this case, it may cause a deadlock problem if the rq.lock is locked again in the timer ISR.

Hence, the handle_exception() can only enable interrupts when the state of sstatus.SPIE is 1.

This patch is tested on HiFive Unleashed board.

[[email protected]: updated to apply]
Fixes bcae803a21317 ("RISC-V: Enable IRQ during exception handling")
Cc David Abdurachmanov <[email protected]>
arch/riscv/kernel/entry.S +5 -1
index 74ccfd464071..da7aa88113c2 100644
@@ -166,9 +166,13 @@ ENTRY(handle_exception)
 	move a0, sp /* pt_regs */
 	tail do_IRQ
 1:
-	/* Exceptions run with interrupts enabled */
+	/* Exceptions run with interrupts enabled or disabled
+	   depending on the state of sstatus.SR_SPIE */
+	andi t0, s1, SR_SPIE
+	beqz t0, 1f
 	csrs CSR_SSTATUS, SR_SIE
 
+1:
 	/* Handle syscalls */
 	li t0, EXC_SYSCALL
 	beq s4, t0, handle_syscall
maintainer
#1 lore raw
Quoted 59 lines · On Tue, Oct 08, 2019 at 07:27:44PM +0200, [email protected] wrote:

The patch below does not apply to the 5.3-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From c82dd6d078a2bb29d41eda032bb96d05699a524d Mon Sep 17 00:00:00 2001
From: Vincent Chen <[email protected]>
Date: Mon, 16 Sep 2019 16:47:41 +0800
Subject: [PATCH] riscv: Avoid interrupts being erroneously enabled in handle_exception()

When the handle_exception function addresses an exception, the interrupts will be unconditionally enabled after finishing the context save. However, It may erroneously enable the interrupts if the interrupts are disabled before entering the handle_exception.

For example, one of the WARN_ON() condition is satisfied in the scheduling where the interrupt is disabled and rq.lock is locked. The WARN_ON will trigger a break exception and the handle_exception function will enable the interrupts before entering do_trap_break function. During the procedure, if a timer interrupt is pending, it will be taken when interrupts are enabled. In this case, it may cause a deadlock problem if the rq.lock is locked again in the timer ISR.

Hence, the handle_exception() can only enable interrupts when the state of sstatus.SPIE is 1.

This patch is tested on HiFive Unleashed board.

[[email protected]: updated to apply]
Fixes bcae803a21317 ("RISC-V: Enable IRQ during exception handling")
Cc David Abdurachmanov <[email protected]>
arch/riscv/kernel/entry.S +0 -0
index 74ccfd464071..da7aa88113c2 100644
@@ -166,9 +166,13 @@ ENTRY(handle_exception)
 	move a0, sp /* pt_regs */
 	tail do_IRQ

1:

arch/riscv/kernel/entry.S +4 -1
-	/* Exceptions run with interrupts enabled */
+	/* Exceptions run with interrupts enabled or disabled
+	   depending on the state of sstatus.SR_SPIE */
+	andi t0, s1, SR_SPIE
+	beqz t0, 1f

csrs CSR_SSTATUS, SR_SIE

Quoted 1 line

+1:

I might be missing something here, but wasn't the label "1" already declared a few lines above?

--
Thanks,
Sasha

maintainer
#3 lore raw
Quoted 8 lines · Paul Walmsley
Quoted 2 lines · Sasha Levin

I might be missing something here, but wasn't the label "1" already declared a few lines above?

See the "Local Labels" section of

https://sourceware.org/binutils/docs-2.24/as/Symbol-Names.html#Symbol-Names

Thanks!

I've fixed the patch to compensate for not having 4f3f90084673f ("riscv:
Using CSR numbers to access CSRs") and queued it for 5.4 and 4.19.

Signature · 2 lines
Thanks,
Sasha
#4 lore raw
Quoted 6 lines · Sasha Levin
Quoted 5 lines · Paul Walmsley
Quoted 2 lines · Sasha Levin

I might be missing something here, but wasn't the label "1" already declared a few lines above?

Quoted 8 lines
Quoted 3 lines

See the "Local Labels" section of

https://sourceware.org/binutils/docs-2.24/as/Symbol-Names.html#Symbol-Names

Thanks!

I've fixed the patch to compensate for not having 4f3f90084673f ("riscv:
Using CSR numbers to access CSRs") and queued it for 5.4 and 4.19.

Thanks Sasha! You just saved me some time :-)

- Paul