Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20210825064228.70487-1-deng.changcheng@zte.com.cn>
 <8735hsvzig.fsf@mpe.ellerman.id.au> <e2310273-8bc8-deac-9a80-0d1587acecb1@csgroup.eu>
In-Reply-To: <e2310273-8bc8-deac-9a80-0d1587acecb1@csgroup.eu>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Sat, 7 May 2022 10:49:31 +0200
Message-ID: <CAK8P3a3s8GUtUUt-7BvUEajEDDAJMH7yJNCaUF8hdhjfmf_f7Q@mail.gmail.com>
Subject: Re: [PATCH linux-next] power:pkeys: fix bugon.cocci warnings
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
Cc:     Michael Ellerman <mpe@ellerman.id.au>, CGEL <cgel.zte@gmail.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Sandipan Das <sandipan@linux.ibm.com>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Zeal Robot <zealci@zte.com.cn>,
        Jing Yangyang <jing.yangyang@zte.com.cn>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Sat, May 7, 2022 at 9:04 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> Le 02/05/2022 à 15:24, Michael Ellerman a écrit :
> > CGEL <cgel.zte@gmail.com> writes:
> >> From: Jing Yangyang <jing.yangyang@zte.com.cn>
> >>
> >> Use BUG_ON instead of a if condition followed by BUG.
> >>
> >> ./arch/powerpc/include/asm/book3s/64/pkeys.h:21:2-5:WARNING
> >> Use BUG_ON instead of if condition followed by BUG.
> >> ./arch/powerpc/include/asm/book3s/64/pkeys.h:14:2-5:WARNING
> >> Use BUG_ON instead of if condition followed by BUG.
> >>
> >> Generated by: scripts/coccinelle/misc/bugon.cocci
> >>
> >> Reported-by: Zeal Robot <zealci@zte.com.cn>
> >> Signed-off-by: Jing Yangyang <jing.yangyang@zte.com.cn>
> >> ---
> >>   arch/powerpc/include/asm/book3s/64/pkeys.h | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/book3s/64/pkeys.h b/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> index 5b17813..5f74f0c 100644
> >> --- a/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> +++ b/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> @@ -10,15 +10,13 @@ static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
> >>      if (!mmu_has_feature(MMU_FTR_PKEY))
> >>              return 0x0UL;
> >>
> >> -    if (radix_enabled())
> >> -            BUG();
> >> +    BUG_ON(radix_enabled());
> >>      return hash__vmflag_to_pte_pkey_bits(vm_flags);
> >>   }
> >>
> >>   static inline u16 pte_to_pkey_bits(u64 pteflags)
> >>   {
> >> -    if (radix_enabled())
> >> -            BUG();
> >> +    BUG_ON(radix_enabled());
> >>      return hash__pte_to_pkey_bits(pteflags);
> >>   }
> >
> > Have you checked how this changes the generated code?
> >
> > radix_enabled() is a jump label, via mmu_feature().
> >
> > Possibly the compiler just works it all out and generates the same code,
> > but I'd want some evidence of that before merging this.
>
> Seems like the compiler is not that good, the generated code for test1()
> is much better than the one for test2(), see below.
>
> void test1(void)
> {
>         if (radix_enabled())
>                 BUG();
> }
>
> void test2(void)
> {
>         BUG_ON(radix_enabled());
> }
>
> 0000000000000900 <.test1>:
>   900:  60 00 00 00     nop
>   904:  0f e0 00 00     twui    r0,0
>   908:  60 00 00 00     nop
>   90c:  60 00 00 00     nop
>   910:  4e 80 00 20     blr
>   914:  60 00 00 00     nop
>   918:  60 00 00 00     nop
>   91c:  60 00 00 00     nop
>
> 0000000000000920 <.test2>:
>   920:  60 00 00 00     nop
>   924:  39 20 00 01     li      r9,1
>   928:  0b 09 00 00     tdnei   r9,0
>   92c:  4e 80 00 20     blr
>   930:  39 20 00 00     li      r9,0
>   934:  0b 09 00 00     tdnei   r9,0
>   938:  4e 80 00 20     blr
>
>
> We should keep things as they are and change the coccinelle script.

Maybe just drop the custom ppc64 BUG_ON() then if it creates worse
code? The default BUG_ON() should be equivalent to the open-coded
version.

        Arnd