Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210422151022.17868-1-sxwjean@me.com> <15aac264-8626-ad15-7301-044c622d7f60@csgroup.eu>
In-Reply-To: <15aac264-8626-ad15-7301-044c622d7f60@csgroup.eu>
From:   Xiongwei Song <sxwjean@gmail.com>
Date:   Sat, 24 Apr 2021 22:36:53 +0800
Message-ID: <CAEVVKH8K84=DuQy370eVv4kx24nHiOqnputtERLuGoc4-MwYtg@mail.gmail.com>
Subject: Re: [PATCH 1/2] powerpc: Make the code in __show_regs nice-looking
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
Cc:     Xiongwei Song <sxwjean@me.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>, npiggin@gmail.com,
        ravi.bangoria@linux.ibm.com, mikey@neuling.org,
        aneesh.kumar@linux.ibm.com, 0x7f454c46@gmail.com,
        PowerPC <linuxppc-dev@lists.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Apr 22, 2021 at 11:27 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 22/04/2021 =C3=A0 17:10, Xiongwei Song a =C3=A9crit :
> > 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. N=
ow you have to go and look
> into a sub-function.

How about replace if statement with switch statement directly, like
the changes below:

@@ -1467,13 +1481,17 @@ static void __show_regs(struct pt_regs *regs)
        trap =3D TRAP(regs);
        if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
                pr_cont("CFAR: "REG" ", regs->orig_gpr3);
-       if (trap =3D=3D INTERRUPT_MACHINE_CHECK ||
-           trap =3D=3D INTERRUPT_DATA_STORAGE ||
-           trap =3D=3D INTERRUPT_ALIGNMENT) {
+       switch(trap){
+       case INTERRUPT_MACHINE_CHECK:
+       case INTERRUPT_DATA_STORAGE:
+       case INTERRUPT_ALIGNMENT:
                if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
                        pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar,
regs->dsisr);
                else
                        pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar,
regs->dsisr);
+               break;
+       default:
+               break;
        }

Thanks,
Xiongwei