| Message ID | 1569199517-5884-5-git-send-email-vincent.chen@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: correct the do_trap_break() | expand |
On Mon, Sep 23, 2019 at 08:45:17AM +0800, Vincent Chen wrote: > To make the code more straightforward, replacing the switch statement > with if statement. > > Suggested-by: Paul Walmsley <paul.walmsley@sifive.com> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > arch/riscv/kernel/traps.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > index dd13bc90aeb6..1ac75f7d0bff 100644 > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -124,23 +124,24 @@ static inline unsigned long get_break_insn_length(unsigned long pc) > > asmlinkage void do_trap_break(struct pt_regs *regs) > { > + if (user_mode(regs)) { > + force_sig_fault(SIGTRAP, TRAP_BRKPT, > + (void __user *)(regs->sepc)); > + return; > + } > +#ifdef CONFIG_GENERIC_BUG > + { > enum bug_trap_type type; > > type = report_bug(regs->sepc, regs); > + if (type == BUG_TRAP_TYPE_WARN) { > regs->sepc += get_break_insn_length(regs->sepc); > return; > } > +#endif /* CONFIG_GENERIC_BUG */ > + > + die(regs, "Kernel BUG"); I like where this is going, but I think this can be improved further given that fact that report_bug has a nice stub for the !CONFIG_GENERIC_BUG case. How about: if (user_mode(regs)) force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->sepc); else if (report_bug(regs->sepc, regs) == BUG_TRAP_TYPE_WARN) regs->sepc += get_break_insn_length(regs->sepc); else die(regs, "Kernel BUG");
Vincent, On Fri, 27 Sep 2019, Christoph Hellwig wrote: > On Mon, Sep 23, 2019 at 08:45:17AM +0800, Vincent Chen wrote: > > To make the code more straightforward, replacing the switch statement > > with if statement. > > > > Suggested-by: Paul Walmsley <paul.walmsley@sifive.com> > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> ... > I like where this is going, but I think this can be improved further > given that fact that report_bug has a nice stub for the > !CONFIG_GENERIC_BUG case. > > How about: > > if (user_mode(regs)) > force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->sepc); > else if (report_bug(regs->sepc, regs) == BUG_TRAP_TYPE_WARN) > regs->sepc += get_break_insn_length(regs->sepc); > else > die(regs, "Kernel BUG"); > Christoph's suggestion looks good to me. What do you think about this modification to your patch? - Paul From: Vincent Chen <vincent.chen@sifive.com> Date: Mon, 23 Sep 2019 08:45:17 +0800 Subject: [PATCH] riscv: remove the switch statement in do_trap_break() To make the code more straightforward, replace the switch statement with an if statement. Suggested-by: Paul Walmsley <paul.walmsley@sifive.com> Signed-off-by: Vincent Chen <vincent.chen@sifive.com> [paul.walmsley@sifive.com: removed CONFIG_GENERIC_BUG tests per Christoph's suggestion; cleaned up patch description] Cc: Christoph Hellwig <hch@lst.de> Link: https://lore.kernel.org/linux-riscv/20190927224711.GI4700@infradead.org/ Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com> --- arch/riscv/kernel/traps.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 93742df9067f..45b82be00714 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -124,24 +124,13 @@ static inline unsigned long get_break_insn_length(unsigned long pc) asmlinkage void do_trap_break(struct pt_regs *regs) { - if (!user_mode(regs)) { - enum bug_trap_type type; - - type = report_bug(regs->sepc, regs); - switch (type) { -#ifdef CONFIG_GENERIC_BUG - case BUG_TRAP_TYPE_WARN: - regs->sepc += get_break_insn_length(regs->sepc); - return; - case BUG_TRAP_TYPE_BUG: -#endif /* CONFIG_GENERIC_BUG */ - default: - die(regs, "Kernel BUG"); - } - } else { + if (user_mode(regs)) force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc)); - } + else if (report_bug(regs->sepc, regs) == BUG_TRAP_TYPE_WARN) + regs->sepc += get_break_insn_length(regs->sepc); + else + die(regs, "Kernel BUG"); } #ifdef CONFIG_GENERIC_BUG
On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote: > force_sig_fault(SIGTRAP, TRAP_BRKPT, > (void __user *)(regs->sepc)); No nee for the extra braces, which also means it all fits onto a single line. You could have just copied what I pasted..
On Mon, 7 Oct 2019, Christoph Hellwig wrote: > On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote: > > force_sig_fault(SIGTRAP, TRAP_BRKPT, > > (void __user *)(regs->sepc)); > > No nee for the extra braces, which also means it all fits onto a single > line. Good point, will drop the extra parens and remove the braces. - Paul
Sorry, I missed the comment. Christoph's suggestion is also good to me. I will modify it as you suggested. Thanks On Tue, Oct 8, 2019 at 12:31 AM Paul Walmsley <paul.walmsley@sifive.com> wrote: > > On Mon, 7 Oct 2019, Christoph Hellwig wrote: > > > On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote: > > > force_sig_fault(SIGTRAP, TRAP_BRKPT, > > > (void __user *)(regs->sepc)); > > > > No nee for the extra braces, which also means it all fits onto a single > > line. > > Good point, will drop the extra parens and remove the braces. > > > - Paul
On Tue, 8 Oct 2019, Vincent Chen wrote: > Sorry, I missed the comment. Christoph's suggestion is also good to me. > I will modify it as you suggested. Thanks - no need to resend, I'll queue the modified patch up here. - Paul
On Mon, 7 Oct 2019, Christoph Hellwig wrote: > On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote: > > force_sig_fault(SIGTRAP, TRAP_BRKPT, > > (void __user *)(regs->sepc)); > > No nee for the extra braces, which also means it all fits onto a single > line. You could have just copied what I pasted.. It turns out that the rewrite breaks allnoconfig: https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/VDCU2WOB6KQISREO4V5DTXEI2M7VOV55/ Am just going to pick up Vincent's original patch. Then we can do any subsequent cleanup in a separate patch. - Paul
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index dd13bc90aeb6..1ac75f7d0bff 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -124,23 +124,24 @@ static inline unsigned long get_break_insn_length(unsigned long pc) asmlinkage void do_trap_break(struct pt_regs *regs) { - if (!user_mode(regs)) { + if (user_mode(regs)) { + force_sig_fault(SIGTRAP, TRAP_BRKPT, + (void __user *)(regs->sepc)); + return; + } +#ifdef CONFIG_GENERIC_BUG + { enum bug_trap_type type; type = report_bug(regs->sepc, regs); - switch (type) { -#ifdef CONFIG_GENERIC_BUG - case BUG_TRAP_TYPE_WARN: + if (type == BUG_TRAP_TYPE_WARN) { regs->sepc += get_break_insn_length(regs->sepc); return; - case BUG_TRAP_TYPE_BUG: -#endif /* CONFIG_GENERIC_BUG */ - default: - die(regs, "Kernel BUG"); } - } else - force_sig_fault(SIGTRAP, TRAP_BRKPT, - (void __user *)(regs->sepc)); + } +#endif /* CONFIG_GENERIC_BUG */ + + die(regs, "Kernel BUG"); } #ifdef CONFIG_GENERIC_BUG
To make the code more straightforward, replacing the switch statement with if statement. Suggested-by: Paul Walmsley <paul.walmsley@sifive.com> Signed-off-by: Vincent Chen <vincent.chen@sifive.com> --- arch/riscv/kernel/traps.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)