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;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>,
        Xiongwei Song <sxwjean@me.com>, benh@kernel.crashing.org,
        paulus@samba.org, oleg@redhat.com, npiggin@gmail.com,
        aneesh.kumar@linux.ibm.com, ravi.bangoria@linux.ibm.com,
        mikey@neuling.org, haren@linux.ibm.com, akpm@linux-foundation.org,
        rppt@kernel.org, jniethe5@gmail.com, atrajeev@linux.vnet.ibm.com,
        maddy@linux.ibm.com, peterz@infradead.org, kjain@linux.ibm.com,
        kan.liang@linux.intel.com, aik@ozlabs.ru, alistair@popple.id.au,
        pmladek@suse.com, john.ogness@linutronix.de
Cc:     linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        Xiongwei Song <sxwjean@gmail.com>
Subject: Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
In-Reply-To: <70ece993-12bd-335c-d246-914564eb51dd@csgroup.eu>
References: <20210408140750.26832-1-sxwjean@me.com>
 <70ece993-12bd-335c-d246-914564eb51dd@csgroup.eu>
Date:   Sat, 10 Apr 2021 10:04:49 +1000
Message-ID: <874kgfdmxq.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 08/04/2021 =C3=A0 16:07, Xiongwei Song a =C3=A9crit=C2=A0:
>> From: Xiongwei Song <sxwjean@gmail.com>
>>=20
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
...
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include=
/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>   #include <asm/ftrace.h>
>>   #include <asm/kprobes.h>
>>   #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>=20=20=20
>>   struct interrupt_state {
>>   #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_r=
egs *regs, struct interrup
>>   		 * CT_WARN_ON comes here via program_check_exception,
>>   		 * so avoid recursion.
>>   		 */
>> -		if (TRAP(regs) !=3D 0x700)
>> +		if (TRAP(regs) !=3D INTERRUPT_PROGRAM)
>>   			CT_WARN_ON(ct_state() !=3D CONTEXT_KERNEL);
>>   	}
>>   #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struc=
t pt_regs *regs, struct inte
>>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>>   #endif
>>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) !=3D 0x900 && TRAP(regs) !=3D 0xf00 && TRAP(regs) !=3D =
0x260) {
>> +	if (TRAP(regs) !=3D INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) !=3D INTERRUPT_PERFMON) {
>
> I think too long names hinder readability, see later for suggestions.

I asked for the longer names :)

I think they make it easier for people who are less familiar with the
architecture than us to make sense of the names.

And there's only a couple of cases where it requires splitting a line,
and they could be converted to use switch if we think it's a problem.

>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int si=
g)
>>   	/* kernel has accessed a bad area */
>>=20=20=20
>>   	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
>
> It would be better to avoid #ifdefs when none where necessary before.

Yes I agree.

I think these can all be avoided by defining most of the values
regardless of what platform we're building for. Only the values that
overlap need to be kept behind an ifdef.

cheers