[PATCH 2/2] powerpc/papr_scm: Fix build failure when CONFIG_PERF_EVENTS is not set

[Aneesh Kumar K V]

On 3/22/22 10:02 PM, Dan Williams wrote:
> On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain at linux.ibm.com> wrote:
>>
>>
>>
>> On 3/22/22 03:09, Dan Williams wrote:
>>> On Fri, Mar 18, 2022 at 4:42 AM Kajol Jain <kjain at linux.ibm.com> wrote:
>>>>
>>>> The following build failure occures when CONFIG_PERF_EVENTS is not set
>>>> as generic pmu functions are not visible in that scenario.
>>>>
>>>> arch/powerpc/platforms/pseries/papr_scm.c:372:35: error: ‘struct perf_event’ has no member named ‘attr’
>>>>           p->nvdimm_events_map[event->attr.config],
>>>>                                     ^~
>>>> In file included from ./include/linux/list.h:5,
>>>>                   from ./include/linux/kobject.h:19,
>>>>                   from ./include/linux/of.h:17,
>>>>                   from arch/powerpc/platforms/pseries/papr_scm.c:5:
>>>> arch/powerpc/platforms/pseries/papr_scm.c: In function ‘papr_scm_pmu_event_init’:
>>>> arch/powerpc/platforms/pseries/papr_scm.c:389:49: error: ‘struct perf_event’ has no member named ‘pmu’
>>>>    struct nvdimm_pmu *nd_pmu = to_nvdimm_pmu(event->pmu);
>>>>                                                   ^~
>>>> ./include/linux/container_of.h:18:26: note: in definition of macro ‘container_of’
>>>>    void *__mptr = (void *)(ptr);     \
>>>>                            ^~~
>>>> arch/powerpc/platforms/pseries/papr_scm.c:389:30: note: in expansion of macro ‘to_nvdimm_pmu’
>>>>    struct nvdimm_pmu *nd_pmu = to_nvdimm_pmu(event->pmu);
>>>>                                ^~~~~~~~~~~~~
>>>> In file included from ./include/linux/bits.h:22,
>>>>                   from ./include/linux/bitops.h:6,
>>>>                   from ./include/linux/of.h:15,
>>>>                   from arch/powerpc/platforms/pseries/papr_scm.c:5:
>>>>
>>>> Fix the build issue by adding check for CONFIG_PERF_EVENTS config option
>>>> and disabling the papr_scm perf interface support incase this config
>>>> is not set
>>>>
>>>> Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support") (Commit id
>>>> based on linux-next tree)
>>>> Signed-off-by: Kajol Jain <kjain at linux.ibm.com>
>>>> ---
>>>>   arch/powerpc/platforms/pseries/papr_scm.c | 15 +++++++++++++++
>>>
>>> This is a bit messier than I would have liked mainly because it dumps
>>> a bunch of ifdefery into a C file contrary to coding style, "Wherever
>>> possible, don't use preprocessor conditionals (#if, #ifdef) in .c
>>> files". I would expect this all to move to an organization like:
>>
>> Hi Dan,
>>        Thanks for reviewing the patches. Inorder to avoid the multiple
>> ifdefs checks, we can also add stub function for papr_scm_pmu_register.
>> With that change we will just have one ifdef check for
>> CONFIG_PERF_EVENTS config in both papr_scm.c and nd.h file. Hence we can
>> avoid adding new files specific for papr_scm perf interface.
>>
>> Below is the code snippet for that change, let me know if looks fine to
>> you. I tested it
>> with set/unset PAPR_SCM config value and set/unset PERF_EVENTS config
>> value combinations.
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c
>> b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 4dd513d7c029..38fabb44d3c3 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -69,8 +69,6 @@
>>   #define PAPR_SCM_PERF_STATS_EYECATCHER __stringify(SCMSTATS)
>>   #define PAPR_SCM_PERF_STATS_VERSION 0x1
>>
>> -#define to_nvdimm_pmu(_pmu)    container_of(_pmu, struct nvdimm_pmu, pmu)
>> -
>>   /* Struct holding a single performance metric */
>>   struct papr_scm_perf_stat {
>>          u8 stat_id[8];
>> @@ -346,6 +344,9 @@ static ssize_t drc_pmem_query_stats(struct
>> papr_scm_priv *p,
>>          return 0;
>>   }
>>
>> +#ifdef CONFIG_PERF_EVENTS
>> +#define to_nvdimm_pmu(_pmu)    container_of(_pmu, struct nvdimm_pmu, pmu)
>> +
>>   static int papr_scm_pmu_get_value(struct perf_event *event, struct
>> device *dev, u64 *count)
>>   {
>>          struct papr_scm_perf_stat *stat;
>> @@ -558,6 +559,10 @@ static void papr_scm_pmu_register(struct
>> papr_scm_priv *p)
>>          dev_info(&p->pdev->dev, "nvdimm pmu didn't register rc=%d\n", rc);
>>   }
>>
>> +#else
>> +static inline void papr_scm_pmu_register(struct papr_scm_priv *p) { }
> 
> Since this isn't in a header file, it does not need to be marked
> "inline" the compiler will figure it out.
> 
>> +#endif
> 
> It might be time to create:
> 
> arch/powerpc/platforms/pseries/papr_scm.h
> 
> ...there is quite a bit of header material accrued in papr_scm.c and
> once the ifdefs start landing in it then it becomes a nominal coding
> style issue. That said, this is certainly more palatable than the
> previous version. So if you don't want to create papr_scm.h yet for
> this, at least make a note in the changelog that the first portion of
> arch/powerpc/platforms/pseries/papr_scm.c is effectively papr_scm.h
> content and may move there in the future, or something like that.

There was an attempt to do that earlier. We should get that reposted 
with further changes move more details to header.

https://lore.kernel.org/nvdimm/163454440296.431294.2368481747380790011.stgit@lep8c.aus.stglabs.ibm.com