Re: [PATCH linux-next] power:pkeys: fix bugon.cocci warnings

[Arnd Bergmann]

On Sat, May 7, 2022 at 9:04 AM Christophe Leroy
<christophe.leroy@xxxxxxxxxx> wrote:
> Le 02/05/2022 à 15:24, Michael Ellerman a écrit :
> > CGEL <cgel.zte@xxxxxxxxx> writes:
> >> From: Jing Yangyang <jing.yangyang@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>
> >> Signed-off-by: Jing Yangyang <jing.yangyang@xxxxxxxxxx>
> >> ---
> >>   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