Re: [PATCH 1/2] powerpc: Make the code in __show_regs nice-looking

From: Christophe Leroy
Date: Thu Apr 22 2021 - 11:27:37 EST




Le 22/04/2021 à 17:10, Xiongwei Song a écrit :
From: Xiongwei Song <sxwjean@xxxxxxxxx>

Create a new function named interrupt_detail_printable to judge which
interrupts can print esr/dsisr register.

What is the benefit of that function ? It may be interesting if the test was done at several places, but as it is done at only one place, I don't thing it is an improvement.

Until know, you new immediately what was the traps that would print it. Now you have to go and look into a sub-function.

And the name is not nice either. All other functions testing anything on the trap type are called trap_is_something()

Here your function would be better called something like trap_sets_dsisr()


Signed-off-by: Xiongwei Song <sxwjean@xxxxxxxxx>
---
arch/powerpc/kernel/process.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 89e34aa273e2..5c3830837f3a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1453,9 +1453,23 @@ static void print_msr_bits(unsigned long val)
#define REGS_PER_LINE 8
#endif
+static bool interrupt_detail_printable(int trap)
+{
+ switch (trap) {
+ case INTERRUPT_MACHINE_CHECK:
+ case INTERRUPT_DATA_STORAGE:
+ case INTERRUPT_ALIGNMENT:
+ return true;
+ default:
+ return false;
+ }
+
+ return false;

That's redundant with the default case inside the switch.

+}
+
static void __show_regs(struct pt_regs *regs)
{
- int i, trap;
+ int i;
printk("NIP: "REG" LR: "REG" CTR: "REG"\n",
regs->nip, regs->link, regs->ctr);
@@ -1464,12 +1478,9 @@ static void __show_regs(struct pt_regs *regs)
printk("MSR: "REG" ", regs->msr);
print_msr_bits(regs->msr);
pr_cont(" CR: %08lx XER: %08lx\n", regs->ccr, regs->xer);
- trap = TRAP(regs);
if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
pr_cont("CFAR: "REG" ", regs->orig_gpr3);
- if (trap == INTERRUPT_MACHINE_CHECK ||
- trap == INTERRUPT_DATA_STORAGE ||
- trap == INTERRUPT_ALIGNMENT) {
+ if (interrupt_detail_printable(TRAP(regs))) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
else