Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
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-...@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
Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
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
Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
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 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
Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
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
Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
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..
Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
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
> > Signed-off-by: Vincent Chen
...
> 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
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
Signed-off-by: Vincent Chen
[paul.walms...@sifive.com: removed CONFIG_GENERIC_BUG tests per
Christoph's suggestion; cleaned up patch description]
Cc: Christoph Hellwig
Link: https://lore.kernel.org/linux-riscv/20190927224711.gi4...@infradead.org/
Signed-off-by: Paul Walmsley
---
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
--
2.23.0
Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
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
> Signed-off-by: Vincent Chen
> ---
> 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");
[PATCH 4/4] riscv: remove the switch statement in do_trap_break()
To make the code more straightforward, replacing the switch statement
with if statement.
Suggested-by: Paul Walmsley
Signed-off-by: Vincent Chen
---
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)) {
+ 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
--
2.7.4
