Re: [PATCH v1 16/22] powerpc/ftrace: Minimise number of #ifdefs

[Christophe Leroy]

Le 18/04/2022 à 09:59, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> A lot of #ifdefs can be replaced by IS_ENABLED()
>>
>> Do so.
>>
>> This requires to have kernel_toc_addr() defined at all time
>> and PPC_INST_LD_TOC as well.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
>> ---
>>  arch/powerpc/include/asm/code-patching.h |   2 -
>>  arch/powerpc/include/asm/module.h        |   2 -
>>  arch/powerpc/include/asm/sections.h      |  24 +--
>>  arch/powerpc/kernel/trace/ftrace.c       | 201 ++++++++++++-----------
>>  4 files changed, 113 insertions(+), 116 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/code-patching.h 
>> b/arch/powerpc/include/asm/code-patching.h
>> index 4260e89f62b1..071fcbec31c5 100644
>> --- a/arch/powerpc/include/asm/code-patching.h
>> +++ b/arch/powerpc/include/asm/code-patching.h
>> @@ -217,7 +217,6 @@ static inline unsigned long 
>> ppc_kallsyms_lookup_name(const char *name)
>>      return addr;
>>  }
>>
>> -#ifdef CONFIG_PPC64
>>  /*
>>   * Some instruction encodings commonly used in dynamic ftracing
>>   * and function live patching.
>> @@ -234,6 +233,5 @@ static inline unsigned long 
>> ppc_kallsyms_lookup_name(const char *name)
>>
>>  /* usually preceded by a mflr r0 */
>>  #define PPC_INST_STD_LR        PPC_RAW_STD(_R0, _R1, PPC_LR_STKOFF)
>> -#endif /* CONFIG_PPC64 */
>>
>>  #endif /* _ASM_POWERPC_CODE_PATCHING_H */
>> diff --git a/arch/powerpc/include/asm/module.h 
>> b/arch/powerpc/include/asm/module.h
>> index e6f5963fd96e..700d7ecd9012 100644
>> --- a/arch/powerpc/include/asm/module.h
>> +++ b/arch/powerpc/include/asm/module.h
>> @@ -41,9 +41,7 @@ struct mod_arch_specific {
>>
>>  #ifdef CONFIG_FUNCTION_TRACER
>>      unsigned long tramp;
>> -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>>      unsigned long tramp_regs;
>> -#endif
>>  #endif
>>
>>      /* List of BUG addresses, source line numbers and filenames */
>> diff --git a/arch/powerpc/include/asm/sections.h 
>> b/arch/powerpc/include/asm/sections.h
>> index 8be2c491c733..6980eaeb16fe 100644
>> --- a/arch/powerpc/include/asm/sections.h
>> +++ b/arch/powerpc/include/asm/sections.h
>> @@ -29,18 +29,6 @@ extern char start_virt_trampolines[];
>>  extern char end_virt_trampolines[];
>>  #endif
>>
>> -/*
>> - * This assumes the kernel is never compiled -mcmodel=small or
>> - * the total .toc is always less than 64k.
>> - */
>> -static inline unsigned long kernel_toc_addr(void)
>> -{
>> -    unsigned long toc_ptr;
>> -
>> -    asm volatile("mr %0, 2" : "=r" (toc_ptr));
>> -    return toc_ptr;
>> -}
>> -
>>  static inline int overlaps_interrupt_vector_text(unsigned long start,
>>                              unsigned long end)
>>  {
>> @@ -60,5 +48,17 @@ static inline int overlaps_kernel_text(unsigned 
>> long start, unsigned long end)
>>
>>  #endif
>>
>> +/*
>> + * This assumes the kernel is never compiled -mcmodel=small or
>> + * the total .toc is always less than 64k.
>> + */
>> +static inline unsigned long kernel_toc_addr(void)
>> +{
>> +    unsigned long toc_ptr;
>> +
>> +    asm volatile("mr %0, 2" : "=r" (toc_ptr));
>> +    return toc_ptr;
>> +}
>> +
>>  #endif /* __KERNEL__ */
>>  #endif    /* _ASM_POWERPC_SECTIONS_H */
>> diff --git a/arch/powerpc/kernel/trace/ftrace.c 
>> b/arch/powerpc/kernel/trace/ftrace.c
>> index ffedf8c82ea8..4dd30e396026 100644
>> --- a/arch/powerpc/kernel/trace/ftrace.c
>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>> @@ -150,55 +150,55 @@ __ftrace_make_nop(struct module *mod,
>>          return -EINVAL;
>>      }
>>
>> -    /* When using -mkernel_profile or PPC32 there is no load to jump 
>> over */
>> -    pop = ppc_inst(PPC_RAW_NOP());
>> +    if (IS_ENABLED(CONFIG_MPROFILE_KERNEL)) {
>> +        /* When using -mkernel_profile or PPC32 there is no load to 
>> jump over */
>> +        pop = ppc_inst(PPC_RAW_NOP());
> 
> Why move this inside the if() statement? You can keep this out and drop 
> the else clause.

I find it less error prone that way.

Putting a bad value and then replace it later with the good one can be 
misleading, and if some day someone removes that second set by error, it 
will go unnoticed. When you set it only once, GCC will complain in case 
that setting disappear by error.

Christophe