Messages in this thread | | | Subject | Re: [PATCH 1/2] powerpc: Make the code in __show_regs nice-looking | From | Christophe Leroy <> | Date | Thu, 22 Apr 2021 17:27:14 +0200 |
| |
Le 22/04/2021 à 17:10, Xiongwei Song a écrit : > From: Xiongwei Song <sxwjean@gmail.com> > > 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@gmail.com> > --- > 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 >
| |