From: kajoljain <kjain@linux.ibm.com> To: Dan Williams <dan.j.williams@intel.com> Cc: Michael Ellerman <mpe@ellerman.id.au>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Linux NVDIMM <nvdimm@lists.linux.dev>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Linux-Next Mailing List <linux-next@vger.kernel.org>, Peter Zijlstra <peterz@infradead.org>, "Weiny, Ira" <ira.weiny@intel.com>, Vishal L Verma <vishal.l.verma@intel.com>, Stephen Rothwell <sfr@canb.auug.org.au>, Santosh Sivaraj <santosh@fossix.org>, maddy@linux.ibm.com, rnsastry@linux.ibm.com, "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>, atrajeev@linux.vnet.ibm.com, Vaibhav Jain <vaibhav@linux.ibm.com>, Thomas Gleixner <tglx@linutronix.de>, Linux MM <linux-mm@kvack.org> Subject: Re: [PATCH 2/2] powerpc/papr_scm: Fix build failure when CONFIG_PERF_EVENTS is not set Date: Tue, 22 Mar 2022 19:59:33 +0530 [thread overview] Message-ID: <c198a1b5-cc7e-4e51-533b-a5794f506b17@linux.ibm.com> (raw) In-Reply-To: <CAPcyv4iqpTn89WLOW1XjFXGZEYG_MmPg+VQbcDJ9ygJ4Jaybtw@mail.gmail.com> On 3/22/22 03:09, Dan Williams wrote: > On Fri, Mar 18, 2022 at 4:42 AM Kajol Jain <kjain@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@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) { } +#endif + /* * Issue hcall to retrieve dimm health info and populate papr_scm_priv with the * health information. diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile index 3fb806748716..ba0296dca9db 100644 --- a/drivers/nvdimm/Makefile +++ b/drivers/nvdimm/Makefile @@ -15,7 +15,7 @@ nd_e820-y := e820.o libnvdimm-y := core.o libnvdimm-y += bus.o libnvdimm-y += dimm_devs.o -libnvdimm-y += nd_perf.o +libnvdimm-$(CONFIG_PERF_EVENTS) += nd_perf.o libnvdimm-y += dimm.o libnvdimm-y += region_devs.o libnvdimm-y += region.o diff --git a/include/linux/nd.h b/include/linux/nd.h index 7b2ccbdc1cbc..a387e2b630ad 100644 --- a/include/linux/nd.h +++ b/include/linux/nd.h @@ -57,15 +57,22 @@ struct nvdimm_pmu { struct cpumask arch_cpumask; }; +#ifdef CONFIG_PERF_EVENTS extern ssize_t nvdimm_events_sysfs_show(struct device *dev, struct device_attribute *attr, char *page); int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm, struct platform_device *pdev); void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu); -void perf_pmu_migrate_context(struct pmu *pmu, int src_cpu, int dst_cpu); -int perf_pmu_register(struct pmu *pmu, const char *name, int type); -void perf_pmu_unregister(struct pmu *pmu); + +#else +static inline int register_nvdimm_pmu(struct nvdimm_pmu *nd_pmu, struct platform_device *pdev) +{ + return -ENXIO; +} + +static inline void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu) { } +#endif struct nd_device_driver { struct device_driver drv; > > arch/powerpc/platforms/pseries/papr_scm/main.c > arch/powerpc/platforms/pseries/papr_scm/perf.c > > ...and a new config symbol like: > > config PAPR_SCM_PERF > depends on PAPR_SCM && PERF_EVENTS > def_bool y > > ...with wrappers in header files to make everything compile away > without any need for main.c to carry an ifdef. > > Can you turn a patch like that in the next couple days? Otherwise, I > think if Linus saw me sending a late breaking compile fix that threw > coding style out the window he'd have cause to just drop the pull > request entirely. Sure Dan, I will work on the patches and send it asap once we finalized the changes. Thanks, Kajol Jain
WARNING: multiple messages have this Message-ID (diff)
From: kajoljain <kjain@linux.ibm.com> To: Dan Williams <dan.j.williams@intel.com> Cc: Linux NVDIMM <nvdimm@lists.linux.dev>, Santosh Sivaraj <santosh@fossix.org>, maddy@linux.ibm.com, "Weiny, Ira" <ira.weiny@intel.com>, Stephen Rothwell <sfr@canb.auug.org.au>, Peter Zijlstra <peterz@infradead.org>, rnsastry@linux.ibm.com, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Linux MM <linux-mm@kvack.org>, Linux-Next Mailing List <linux-next@vger.kernel.org>, "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>, Vishal L Verma <vishal.l.verma@intel.com>, Vaibhav Jain <vaibhav@linux.ibm.com>, atrajeev@linux.vnet.ibm.com, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Thomas Gleixner <tglx@linutronix.de> Subject: Re: [PATCH 2/2] powerpc/papr_scm: Fix build failure when CONFIG_PERF_EVENTS is not set Date: Tue, 22 Mar 2022 19:59:33 +0530 [thread overview] Message-ID: <c198a1b5-cc7e-4e51-533b-a5794f506b17@linux.ibm.com> (raw) In-Reply-To: <CAPcyv4iqpTn89WLOW1XjFXGZEYG_MmPg+VQbcDJ9ygJ4Jaybtw@mail.gmail.com> On 3/22/22 03:09, Dan Williams wrote: > On Fri, Mar 18, 2022 at 4:42 AM Kajol Jain <kjain@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@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) { } +#endif + /* * Issue hcall to retrieve dimm health info and populate papr_scm_priv with the * health information. diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile index 3fb806748716..ba0296dca9db 100644 --- a/drivers/nvdimm/Makefile +++ b/drivers/nvdimm/Makefile @@ -15,7 +15,7 @@ nd_e820-y := e820.o libnvdimm-y := core.o libnvdimm-y += bus.o libnvdimm-y += dimm_devs.o -libnvdimm-y += nd_perf.o +libnvdimm-$(CONFIG_PERF_EVENTS) += nd_perf.o libnvdimm-y += dimm.o libnvdimm-y += region_devs.o libnvdimm-y += region.o diff --git a/include/linux/nd.h b/include/linux/nd.h index 7b2ccbdc1cbc..a387e2b630ad 100644 --- a/include/linux/nd.h +++ b/include/linux/nd.h @@ -57,15 +57,22 @@ struct nvdimm_pmu { struct cpumask arch_cpumask; }; +#ifdef CONFIG_PERF_EVENTS extern ssize_t nvdimm_events_sysfs_show(struct device *dev, struct device_attribute *attr, char *page); int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm, struct platform_device *pdev); void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu); -void perf_pmu_migrate_context(struct pmu *pmu, int src_cpu, int dst_cpu); -int perf_pmu_register(struct pmu *pmu, const char *name, int type); -void perf_pmu_unregister(struct pmu *pmu); + +#else +static inline int register_nvdimm_pmu(struct nvdimm_pmu *nd_pmu, struct platform_device *pdev) +{ + return -ENXIO; +} + +static inline void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu) { } +#endif struct nd_device_driver { struct device_driver drv; > > arch/powerpc/platforms/pseries/papr_scm/main.c > arch/powerpc/platforms/pseries/papr_scm/perf.c > > ...and a new config symbol like: > > config PAPR_SCM_PERF > depends on PAPR_SCM && PERF_EVENTS > def_bool y > > ...with wrappers in header files to make everything compile away > without any need for main.c to carry an ifdef. > > Can you turn a patch like that in the next couple days? Otherwise, I > think if Linus saw me sending a late breaking compile fix that threw > coding style out the window he'd have cause to just drop the pull > request entirely. Sure Dan, I will work on the patches and send it asap once we finalized the changes. Thanks, Kajol Jain
next prev parent reply other threads:[~2022-03-22 14:30 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-18 11:41 [PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set Kajol Jain 2022-03-18 11:41 ` Kajol Jain 2022-03-18 11:41 ` [PATCH 2/2] powerpc/papr_scm: " Kajol Jain 2022-03-18 11:41 ` Kajol Jain 2022-03-21 21:39 ` Dan Williams 2022-03-21 21:39 ` Dan Williams 2022-03-22 1:45 ` Dan Williams 2022-03-22 1:45 ` Dan Williams 2022-03-22 14:30 ` kajoljain 2022-03-22 14:30 ` kajoljain 2022-03-22 14:29 ` kajoljain [this message] 2022-03-22 14:29 ` kajoljain 2022-03-22 16:32 ` Dan Williams 2022-03-22 16:32 ` Dan Williams 2022-03-23 7:43 ` Aneesh Kumar K V 2022-03-23 7:43 ` Aneesh Kumar K V 2022-03-23 7:48 ` Vaibhav Jain 2022-03-23 7:48 ` Vaibhav Jain 2022-03-23 10:04 ` Michael Ellerman 2022-03-23 10:04 ` Michael Ellerman 2022-03-23 15:32 ` Dan Williams 2022-03-23 15:32 ` Dan Williams 2022-03-23 16:40 ` kajoljain 2022-03-23 16:40 ` kajoljain 2022-03-22 2:10 ` [PATCH 1/2] drivers/nvdimm: " Dan Williams 2022-03-22 2:10 ` Dan Williams 2022-03-23 16:43 ` kajoljain 2022-03-23 16:43 ` kajoljain
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c198a1b5-cc7e-4e51-533b-a5794f506b17@linux.ibm.com \ --to=kjain@linux.ibm.com \ --cc=aneesh.kumar@linux.ibm.com \ --cc=atrajeev@linux.vnet.ibm.com \ --cc=dan.j.williams@intel.com \ --cc=ira.weiny@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-next@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=maddy@linux.ibm.com \ --cc=mpe@ellerman.id.au \ --cc=nvdimm@lists.linux.dev \ --cc=peterz@infradead.org \ --cc=rnsastry@linux.ibm.com \ --cc=santosh@fossix.org \ --cc=sfr@canb.auug.org.au \ --cc=tglx@linutronix.de \ --cc=vaibhav@linux.ibm.com \ --cc=vishal.l.verma@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.