commit 431a84b1a4f7d1a0085d5b91330c5053cc8e8b12
("ARM: 8034/1: Disable preemption in iwmmxt_task_enable()")
introduced macros {inc,dec}_preempt_count to iwmmxt_task_enable
to make it run with preemption disabled.
Unfortunately, other functions in iwmmxt.S also use concan_{save,dump,load}
sections located in iwmmxt_task_enable() to deal with iWMMXt coprocessor.
This causes an unbalanced preempt_count due to excessive dec_preempt_count
and destroyed return addresses in callers of concan_ labels due to a register
collision:
Linux version 3.16.0-rc3-00062-gd92a333-dirty (jef@armhf) (gcc version 4.8.3 (Debian 4.8.3-4) ) #5 PREEMPT Thu Jul 3 19:46:39 CEST 2014
CPU: ARMv7 Processor [560f5815] revision 5 (ARMv7), cr=10c5387d
CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
Machine model: SolidRun CuBox
...
PJ4 iWMMXt v2 coprocessor enabled.
...
Unable to handle kernel paging request at virtual address fffffffe
pgd = bb25c000
[fffffffe] *pgd=3bfde821, *pte=00000000, *ppte=00000000
Internal error: Oops: 80000007 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 62 Comm: startpar Not tainted 3.16.0-rc3-00062-gd92a333-dirty #5
task: bb230b80 ti: bb256000 task.ti: bb256000
PC is at 0xfffffffe
LR is at iwmmxt_task_copy+0x44/0x4c
pc : [<fffffffe>] lr : [<800130ac>] psr: 40000033
sp : bb257de8 ip : 00000013 fp : bb257ea4
r10: bb256000 r9 : fffffdfe r8 : 76e898e6
r7 : bb257ec8 r6 : bb256000 r5 : 7ea12760 r4 : 000000a0
r3 : ffffffff r2 : 00000003 r1 : bb257df8 r0 : 00000000
Flags: nZcv IRQs on FIQs on Mode SVC_32 ISA Thumb Segment user
Control: 10c5387d Table: 3b25c019 DAC: 00000015
Process startpar (pid: 62, stack limit = 0xbb256248)
This patch fixes the issue by moving concan_{save,dump,load} into separate
code sections and make iwmmxt_task_enable() call them in the same way the
other functions use concan_ symbols. The test for valid ownership is moved
to concan_save and is safe for the other user of it, iwmmxt_task_disable().
The register collision is also resolved by moving concan_ symbols as
{inc,dec}_preempt_count are now local to iwmmxt_task_enable().
Signed-off-by: Sebastian Hesselbarth <[email protected]>
Reported-by: Jean-Francois Moine <[email protected]>
Fixes: 431a84b1a4f7 ("ARM: 8034/1: Disable preemption in iwmmxt_task_enable()")
---
The offending commit was intoduced past v3.15-rc1 and the corresponding fix
should also be queued up for stable v3.15+
Cc: Russell King <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Jean-Francois Moine <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/arm/kernel/iwmmxt.S | 16 +++++++++-------
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
index a5599cf..fc95b89 100644
--- a/arch/arm/kernel/iwmmxt.S
+++ b/arch/arm/kernel/iwmmxt.S
@@ -94,13 +94,19 @@ ENTRY(iwmmxt_task_enable)
mrc p15, 0, r2, c2, c0, 0
mov r2, r2 @ cpwait
+ bl concan_save
- teq r1, #0 @ test for last ownership
- mov lr, r9 @ normal exit from exception
- beq concan_load @ no owner, skip save
+#ifdef CONFIG_PREEMPT_COUNT
+ get_thread_info r10
+#endif
+4: dec_preempt_count r10, r3
+ mov pc, r9 @ normal exit from exception
concan_save:
+ teq r1, #0 @ test for last ownership
+ beq concan_load @ no owner, skip save
+
tmrc r2, wCon
@ CUP? wCx
@@ -175,10 +181,6 @@ concan_load:
tmcr wCon, r2
3:
-#ifdef CONFIG_PREEMPT_COUNT
- get_thread_info r10
-#endif
-4: dec_preempt_count r10, r3
mov pc, lr
/*
--
1.7.2.5
On Fri, Jul 11, 2014 at 10:10:13AM +0100, Sebastian Hesselbarth wrote:
> commit 431a84b1a4f7d1a0085d5b91330c5053cc8e8b12
> ("ARM: 8034/1: Disable preemption in iwmmxt_task_enable()")
> introduced macros {inc,dec}_preempt_count to iwmmxt_task_enable
> to make it run with preemption disabled.
>
> Unfortunately, other functions in iwmmxt.S also use concan_{save,dump,load}
> sections located in iwmmxt_task_enable() to deal with iWMMXt coprocessor.
> This causes an unbalanced preempt_count due to excessive dec_preempt_count
> and destroyed return addresses in callers of concan_ labels due to a register
> collision:
Indeed, I missed this part completely.
> --- a/arch/arm/kernel/iwmmxt.S
> +++ b/arch/arm/kernel/iwmmxt.S
> @@ -94,13 +94,19 @@ ENTRY(iwmmxt_task_enable)
>
> mrc p15, 0, r2, c2, c0, 0
> mov r2, r2 @ cpwait
> + bl concan_save
>
> - teq r1, #0 @ test for last ownership
> - mov lr, r9 @ normal exit from exception
> - beq concan_load @ no owner, skip save
> +#ifdef CONFIG_PREEMPT_COUNT
> + get_thread_info r10
> +#endif
> +4: dec_preempt_count r10, r3
> + mov pc, r9 @ normal exit from exception
>
> concan_save:
>
> + teq r1, #0 @ test for last ownership
> + beq concan_load @ no owner, skip save
> +
> tmrc r2, wCon
>
> @ CUP? wCx
> @@ -175,10 +181,6 @@ concan_load:
> tmcr wCon, r2
>
> 3:
> -#ifdef CONFIG_PREEMPT_COUNT
> - get_thread_info r10
> -#endif
> -4: dec_preempt_count r10, r3
> mov pc, lr
It looks fine to me. One optimisation you could do is to replace a
couple of beq 3f with moveq pc, lr.
--
Catalin
commit 431a84b1a4f7d1a0085d5b91330c5053cc8e8b12
("ARM: 8034/1: Disable preemption in iwmmxt_task_enable()")
introduced macros {inc,dec}_preempt_count to iwmmxt_task_enable
to make it run with preemption disabled.
Unfortunately, other functions in iwmmxt.S also use concan_{save,dump,load}
sections located in iwmmxt_task_enable() to deal with iWMMXt coprocessor.
This causes an unbalanced preempt_count due to excessive dec_preempt_count
and destroyed return addresses in callers of concan_ labels due to a register
collision:
Linux version 3.16.0-rc3-00062-gd92a333-dirty (jef@armhf) (gcc version 4.8.3 (Debian 4.8.3-4) ) #5 PREEMPT Thu Jul 3 19:46:39 CEST 2014
CPU: ARMv7 Processor [560f5815] revision 5 (ARMv7), cr=10c5387d
CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
Machine model: SolidRun CuBox
...
PJ4 iWMMXt v2 coprocessor enabled.
...
Unable to handle kernel paging request at virtual address fffffffe
pgd = bb25c000
[fffffffe] *pgd=3bfde821, *pte=00000000, *ppte=00000000
Internal error: Oops: 80000007 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 62 Comm: startpar Not tainted 3.16.0-rc3-00062-gd92a333-dirty #5
task: bb230b80 ti: bb256000 task.ti: bb256000
PC is at 0xfffffffe
LR is at iwmmxt_task_copy+0x44/0x4c
pc : [<fffffffe>] lr : [<800130ac>] psr: 40000033
sp : bb257de8 ip : 00000013 fp : bb257ea4
r10: bb256000 r9 : fffffdfe r8 : 76e898e6
r7 : bb257ec8 r6 : bb256000 r5 : 7ea12760 r4 : 000000a0
r3 : ffffffff r2 : 00000003 r1 : bb257df8 r0 : 00000000
Flags: nZcv IRQs on FIQs on Mode SVC_32 ISA Thumb Segment user
Control: 10c5387d Table: 3b25c019 DAC: 00000015
Process startpar (pid: 62, stack limit = 0xbb256248)
This patch fixes the issue by moving concan_{save,dump,load} into separate
code sections and make iwmmxt_task_enable() call them in the same way the
other functions use concan_ symbols. The test for valid ownership is moved
to concan_save and is safe for the other user of it, iwmmxt_task_disable().
The register collision is also resolved by moving concan_ symbols as
{inc,dec}_preempt_count are now local to iwmmxt_task_enable().
Signed-off-by: Sebastian Hesselbarth <[email protected]>
Reported-by: Jean-Francois Moine <[email protected]>
Fixes: 431a84b1a4f7 ("ARM: 8034/1: Disable preemption in iwmmxt_task_enable()")
---
The offending commit was intoduced past v3.15-rc1 and the corresponding fix
should also be queued up for stable v3.15+
Changelog
v1->v2:
- return immediately from concan_ instead of branch to 3f, i.e. replace
'beq 3f' with 'moveq pc, lr' (Suggested by Catalin Marinas)
Cc: Russell King <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Jean-Francois Moine <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/arm/kernel/iwmmxt.S | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
index a5599cfc43cb..2b32978ae905 100644
--- a/arch/arm/kernel/iwmmxt.S
+++ b/arch/arm/kernel/iwmmxt.S
@@ -94,13 +94,19 @@ ENTRY(iwmmxt_task_enable)
mrc p15, 0, r2, c2, c0, 0
mov r2, r2 @ cpwait
+ bl concan_save
- teq r1, #0 @ test for last ownership
- mov lr, r9 @ normal exit from exception
- beq concan_load @ no owner, skip save
+#ifdef CONFIG_PREEMPT_COUNT
+ get_thread_info r10
+#endif
+4: dec_preempt_count r10, r3
+ mov pc, r9 @ normal exit from exception
concan_save:
+ teq r1, #0 @ test for last ownership
+ beq concan_load @ no owner, skip save
+
tmrc r2, wCon
@ CUP? wCx
@@ -138,7 +144,7 @@ concan_dump:
wstrd wR15, [r1, #MMX_WR15]
2: teq r0, #0 @ anything to load?
- beq 3f
+ moveq pc, lr @ if not, return
concan_load:
@@ -171,14 +177,9 @@ concan_load:
@ clear CUP/MUP (only if r1 != 0)
teq r1, #0
mov r2, #0
- beq 3f
- tmcr wCon, r2
+ moveq pc, lr
-3:
-#ifdef CONFIG_PREEMPT_COUNT
- get_thread_info r10
-#endif
-4: dec_preempt_count r10, r3
+ tmcr wCon, r2
mov pc, lr
/*
--
2.0.0
On Sat, Jul 12, 2014 at 12:05:30PM +0100, Sebastian Hesselbarth wrote:
> commit 431a84b1a4f7d1a0085d5b91330c5053cc8e8b12
> ("ARM: 8034/1: Disable preemption in iwmmxt_task_enable()")
> introduced macros {inc,dec}_preempt_count to iwmmxt_task_enable
> to make it run with preemption disabled.
>
> Unfortunately, other functions in iwmmxt.S also use concan_{save,dump,load}
> sections located in iwmmxt_task_enable() to deal with iWMMXt coprocessor.
> This causes an unbalanced preempt_count due to excessive dec_preempt_count
> and destroyed return addresses in callers of concan_ labels due to a register
> collision:
>
> Linux version 3.16.0-rc3-00062-gd92a333-dirty (jef@armhf) (gcc version 4.8.3 (Debian 4.8.3-4) ) #5 PREEMPT Thu Jul 3 19:46:39 CEST 2014
> CPU: ARMv7 Processor [560f5815] revision 5 (ARMv7), cr=10c5387d
> CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
> Machine model: SolidRun CuBox
> ...
> PJ4 iWMMXt v2 coprocessor enabled.
> ...
> Unable to handle kernel paging request at virtual address fffffffe
> pgd = bb25c000
> [fffffffe] *pgd=3bfde821, *pte=00000000, *ppte=00000000
> Internal error: Oops: 80000007 [#1] PREEMPT ARM
> Modules linked in:
> CPU: 0 PID: 62 Comm: startpar Not tainted 3.16.0-rc3-00062-gd92a333-dirty #5
> task: bb230b80 ti: bb256000 task.ti: bb256000
> PC is at 0xfffffffe
> LR is at iwmmxt_task_copy+0x44/0x4c
> pc : [<fffffffe>] lr : [<800130ac>] psr: 40000033
> sp : bb257de8 ip : 00000013 fp : bb257ea4
> r10: bb256000 r9 : fffffdfe r8 : 76e898e6
> r7 : bb257ec8 r6 : bb256000 r5 : 7ea12760 r4 : 000000a0
> r3 : ffffffff r2 : 00000003 r1 : bb257df8 r0 : 00000000
> Flags: nZcv IRQs on FIQs on Mode SVC_32 ISA Thumb Segment user
> Control: 10c5387d Table: 3b25c019 DAC: 00000015
> Process startpar (pid: 62, stack limit = 0xbb256248)
>
> This patch fixes the issue by moving concan_{save,dump,load} into separate
> code sections and make iwmmxt_task_enable() call them in the same way the
> other functions use concan_ symbols. The test for valid ownership is moved
> to concan_save and is safe for the other user of it, iwmmxt_task_disable().
> The register collision is also resolved by moving concan_ symbols as
> {inc,dec}_preempt_count are now local to iwmmxt_task_enable().
>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> Reported-by: Jean-Francois Moine <[email protected]>
> Fixes: 431a84b1a4f7 ("ARM: 8034/1: Disable preemption in iwmmxt_task_enable()")
> ---
> The offending commit was intoduced past v3.15-rc1 and the corresponding fix
> should also be queued up for stable v3.15+
>
> Changelog
> v1->v2:
> - return immediately from concan_ instead of branch to 3f, i.e. replace
> 'beq 3f' with 'moveq pc, lr' (Suggested by Catalin Marinas)
>
> Cc: Russell King <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Jean-Francois Moine <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Acked-by: Catalin Marinas <[email protected]>