diff mbox series

RISC-V: fix taking the text_mutex twice during sifive errata patching

Message ID 20230302174154.970746-1-conor@kernel.org (mailing list archive)
State Accepted
Commit bf89b7ee52af5a5944fa3539e86089f72475055b
Headers show
Series RISC-V: fix taking the text_mutex twice during sifive errata patching | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be for-next
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 1 and now 1
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 0 this patch: 0
conchuod/alphanumeric_selects success Out of order selects before the patch: 728 and now 728
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 11 this patch: 11
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch warning WARNING: Reported-by: should be immediately followed by Link: with a URL to the report
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success Fixes tag looks correct
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Conor Dooley March 2, 2023, 5:41 p.m. UTC 
From: Conor Dooley <conor.dooley@microchip.com>

Chris pointed out that some bonehead, *cough* me *cough*, added two
mutex_locks() to the SiFive errata patching. The second was meant to
have been a mutex_unlock().

Reported-by: Chris Hofstaedtler <zeha@debian.org>
Fixes: 9493e6f3ce02 ("RISC-V: take text_mutex during alternative patching")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Palmer, you want to take this one for PR#2, or is it too late?
---
 arch/riscv/errata/sifive/errata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Uytterhoeven March 7, 2023, 8:13 p.m. UTC | #1 
Hi Conor,

On Thu, Mar 2, 2023 at 6:45 PM Conor Dooley <conor@kernel.org> wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
>
> Chris pointed out that some bonehead, *cough* me *cough*, added two
> mutex_locks() to the SiFive errata patching. The second was meant to
> have been a mutex_unlock().
>
> Reported-by: Chris Hofstaedtler <zeha@debian.org>
> Fixes: 9493e6f3ce02 ("RISC-V: take text_mutex during alternative patching")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Thanks for your patch!

This fixes the crash I see (with "earlycon keep_bootcon") on StarLight,
and which I've just bisected to aforementioned commit, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

You may want to include (a part of) the crash log below in your
patch description, to make it easier for affected people to find
the fix.

Unable to handle kernel NULL pointer dereference at virtual address
0000000000000030
Oops [#1]
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted
6.2.0-rc1-starlight-00079-g9493e6f3ce02 #229
Hardware name: BeagleV Starlight Beta (DT)
epc : __schedule+0x42/0x500
 ra : schedule+0x46/0xce
epc : ffffffff8065957c ra : ffffffff80659a80 sp : ffffffff81203c80
 gp : ffffffff812d50a0 tp : ffffffff8120db40 t0 : ffffffff81203d68
 t1 : 0000000000000001 t2 : 4c45203a76637369 s0 : ffffffff81203cf0
 s1 : ffffffff8120db40 a0 : 0000000000000000 a1 : ffffffff81213958
 a2 : ffffffff81213958 a3 : 0000000000000000 a4 : 0000000000000000
 a5 : ffffffff80a1bd00 a6 : 0000000000000000 a7 : 0000000052464e43
 s2 : ffffffff8120db41 s3 : ffffffff80a1ad00 s4 : 0000000000000000
 s5 : 0000000000000002 s6 : ffffffff81213938 s7 : 0000000000000000
 s8 : 0000000000000000 s9 : 0000000000000001 s10: ffffffff812d7204
 s11: ffffffff80d3c920 t3 : 0000000000000001 t4 : ffffffff812e6dd7
 t5 : ffffffff812e6dd8 t6 : ffffffff81203bb8
status: 0000000200000100 badaddr: 0000000000000030 cause: 000000000000000d
[<ffffffff80659a80>] schedule+0x46/0xce
[<ffffffff80659dce>] schedule_preempt_disabled+0x16/0x28
[<ffffffff8065ae0c>] __mutex_lock.constprop.0+0x3fe/0x652
[<ffffffff8065b138>] __mutex_lock_slowpath+0xe/0x16
[<ffffffff8065b182>] mutex_lock+0x42/0x4c
[<ffffffff8000ad94>] sifive_errata_patch_func+0xf6/0x18c
[<ffffffff80002b92>] _apply_alternatives+0x74/0x76
[<ffffffff80802ee8>] apply_boot_alternatives+0x3c/0xfa
[<ffffffff80803cb0>] setup_arch+0x60c/0x640
[<ffffffff80800926>] start_kernel+0x8e/0x99c
---[ end trace 0000000000000000 ]---

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Conor Dooley March 7, 2023, 8:17 p.m. UTC | #2 
On Tue, Mar 07, 2023 at 09:13:01PM +0100, Geert Uytterhoeven wrote:
> Hi Conor,
> 
> On Thu, Mar 2, 2023 at 6:45 PM Conor Dooley <conor@kernel.org> wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> >
> > Chris pointed out that some bonehead, *cough* me *cough*, added two
> > mutex_locks() to the SiFive errata patching. The second was meant to
> > have been a mutex_unlock().
> >
> > Reported-by: Chris Hofstaedtler <zeha@debian.org>
> > Fixes: 9493e6f3ce02 ("RISC-V: take text_mutex during alternative patching")
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Thanks for your patch!
> 
> This fixes the crash I see (with "earlycon keep_bootcon") on StarLight,
> and which I've just bisected to aforementioned commit, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> You may want to include (a part of) the crash log below in your
> patch description, to make it easier for affected people to find
> the fix.

There was no crash log when Chris pointed it out on IRC & I never asked
if they had hit it in a running system.
If there's a v2 for whatever reason, I'll incorporate the splat.
Apologies for the inconvenience caused by my boneheadedness!
I'd hoped that this would make Palmer's second PR, but it was not to
be :(
Palmer Dabbelt March 7, 2023, 8:38 p.m. UTC | #3 
On Tue, 07 Mar 2023 12:17:22 PST (-0800), Conor Dooley wrote:
> On Tue, Mar 07, 2023 at 09:13:01PM +0100, Geert Uytterhoeven wrote:
>> Hi Conor,
>> 
>> On Thu, Mar 2, 2023 at 6:45 PM Conor Dooley <conor@kernel.org> wrote:
>> > From: Conor Dooley <conor.dooley@microchip.com>
>> >
>> > Chris pointed out that some bonehead, *cough* me *cough*, added two
>> > mutex_locks() to the SiFive errata patching. The second was meant to
>> > have been a mutex_unlock().
>> >
>> > Reported-by: Chris Hofstaedtler <zeha@debian.org>
>> > Fixes: 9493e6f3ce02 ("RISC-V: take text_mutex during alternative patching")
>> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> 
>> Thanks for your patch!
>> 
>> This fixes the crash I see (with "earlycon keep_bootcon") on StarLight,
>> and which I've just bisected to aforementioned commit, so
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> 
>> You may want to include (a part of) the crash log below in your
>> patch description, to make it easier for affected people to find
>> the fix.
>
> There was no crash log when Chris pointed it out on IRC & I never asked
> if they had hit it in a running system.
> If there's a v2 for whatever reason, I'll incorporate the splat.
> Apologies for the inconvenience caused by my boneheadedness!
> I'd hoped that this would make Palmer's second PR, but it was not to
> be :(

Sorry about that, there was just too much going on in the second half of 
the merge window and I was trying to dig through stuff.  I'd actually 
though you were talking about the other mutex patch (the text_mutex one) 
when you pinged it, I forgot there were two.

I just queued this up, it's testing but I doubt it'll trip anything.  It 
should be in fixes soon.
Palmer Dabbelt March 7, 2023, 8:52 p.m. UTC | #4 
On Tue, 07 Mar 2023 12:13:01 PST (-0800), geert@linux-m68k.org wrote:
> Hi Conor,
>
> On Thu, Mar 2, 2023 at 6:45 PM Conor Dooley <conor@kernel.org> wrote:
>> From: Conor Dooley <conor.dooley@microchip.com>
>>
>> Chris pointed out that some bonehead, *cough* me *cough*, added two
>> mutex_locks() to the SiFive errata patching. The second was meant to
>> have been a mutex_unlock().
>>
>> Reported-by: Chris Hofstaedtler <zeha@debian.org>
>> Fixes: 9493e6f3ce02 ("RISC-V: take text_mutex during alternative patching")
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>
> Thanks for your patch!
>
> This fixes the crash I see (with "earlycon keep_bootcon") on StarLight,
> and which I've just bisected to aforementioned commit, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> You may want to include (a part of) the crash log below in your
> patch description, to make it easier for affected people to find
> the fix.

I added it to the commit, thanks.  This is in fixes.

>
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000030
> Oops [#1]
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted
> 6.2.0-rc1-starlight-00079-g9493e6f3ce02 #229
> Hardware name: BeagleV Starlight Beta (DT)
> epc : __schedule+0x42/0x500
>  ra : schedule+0x46/0xce
> epc : ffffffff8065957c ra : ffffffff80659a80 sp : ffffffff81203c80
>  gp : ffffffff812d50a0 tp : ffffffff8120db40 t0 : ffffffff81203d68
>  t1 : 0000000000000001 t2 : 4c45203a76637369 s0 : ffffffff81203cf0
>  s1 : ffffffff8120db40 a0 : 0000000000000000 a1 : ffffffff81213958
>  a2 : ffffffff81213958 a3 : 0000000000000000 a4 : 0000000000000000
>  a5 : ffffffff80a1bd00 a6 : 0000000000000000 a7 : 0000000052464e43
>  s2 : ffffffff8120db41 s3 : ffffffff80a1ad00 s4 : 0000000000000000
>  s5 : 0000000000000002 s6 : ffffffff81213938 s7 : 0000000000000000
>  s8 : 0000000000000000 s9 : 0000000000000001 s10: ffffffff812d7204
>  s11: ffffffff80d3c920 t3 : 0000000000000001 t4 : ffffffff812e6dd7
>  t5 : ffffffff812e6dd8 t6 : ffffffff81203bb8
> status: 0000000200000100 badaddr: 0000000000000030 cause: 000000000000000d
> [<ffffffff80659a80>] schedule+0x46/0xce
> [<ffffffff80659dce>] schedule_preempt_disabled+0x16/0x28
> [<ffffffff8065ae0c>] __mutex_lock.constprop.0+0x3fe/0x652
> [<ffffffff8065b138>] __mutex_lock_slowpath+0xe/0x16
> [<ffffffff8065b182>] mutex_lock+0x42/0x4c
> [<ffffffff8000ad94>] sifive_errata_patch_func+0xf6/0x18c
> [<ffffffff80002b92>] _apply_alternatives+0x74/0x76
> [<ffffffff80802ee8>] apply_boot_alternatives+0x3c/0xfa
> [<ffffffff80803cb0>] setup_arch+0x60c/0x640
> [<ffffffff80800926>] start_kernel+0x8e/0x99c
> ---[ end trace 0000000000000000 ]---
>
> Gr{oetje,eeting}s,
>
>                         Geert
patchwork-bot+linux-riscv@kernel.org March 7, 2023, 9 p.m. UTC | #5 
Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Thu,  2 Mar 2023 17:41:55 +0000 you wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Chris pointed out that some bonehead, *cough* me *cough*, added two
> mutex_locks() to the SiFive errata patching. The second was meant to
> have been a mutex_unlock().
> 
> Reported-by: Chris Hofstaedtler <zeha@debian.org>
> Fixes: 9493e6f3ce02 ("RISC-V: take text_mutex during alternative patching")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> [...]

Here is the summary with links:
  - RISC-V: fix taking the text_mutex twice during sifive errata patching
    https://git.kernel.org/riscv/c/bf89b7ee52af

You are awesome, thank you!
diff mbox series

Patch

diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c
index da55cb247e89..31d2ebea4286 100644
--- a/arch/riscv/errata/sifive/errata.c
+++ b/arch/riscv/errata/sifive/errata.c
@@ -111,7 +111,7 @@  void __init_or_module sifive_errata_patch_func(struct alt_entry *begin,
 			mutex_lock(&text_mutex);
 			patch_text_nosync(ALT_OLD_PTR(alt), ALT_ALT_PTR(alt),
 					  alt->alt_len);
-			mutex_lock(&text_mutex);
+			mutex_unlock(&text_mutex);
 			cpu_apply_errata |= tmp;
 		}
 	}