To make the code more straightforward, replacing the switch statement
with if statement.
Suggested-by: Paul Walmsley <[email protected]>
Signed-off-by: Vincent Chen <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Vincent Chen <[email protected]>
> ---
> 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 <[email protected]>
> > Signed-off-by: Vincent Chen <[email protected]>
...
> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Vincent Chen <[email protected]>
[[email protected]: removed CONFIG_GENERIC_BUG tests per
Christoph's suggestion; cleaned up patch description]
Cc: Christoph Hellwig <[email protected]>
Link: https://lore.kernel.org/linux-riscv/[email protected]/
Signed-off-by: Paul Walmsley <[email protected]>
---
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
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 <[email protected]> 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/[email protected]/thread/VDCU2WOB6KQISREO4V5DTXEI2M7VOV55/
Am just going to pick up Vincent's original patch. Then we can do
any subsequent cleanup in a separate patch.
- Paul