[PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set

[PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set

[Kajol Jain]

The following build failure occures when CONFIG_PERF_EVENTS is not set
as generic pmu functions are not visible in that scenario.

|-- s390-randconfig-r044-20220313
|   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_migrate_context
|   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_register
|   `-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_unregister

Similar build failure in nds32 architecture:
nd_perf.c:(.text+0x21e): undefined reference to `perf_pmu_migrate_context'
nd_perf.c:(.text+0x434): undefined reference to `perf_pmu_register'
nd_perf.c:(.text+0x57c): undefined reference to `perf_pmu_unregister'

Fix this issue by adding check for CONFIG_PERF_EVENTS config option
and disabling the nvdimm perf interface incase this config is not set.

Also removed function declaration of perf_pmu_migrate_context,
perf_pmu_register, perf_pmu_unregister functions from nd.h as these are
common pmu functions which are part of perf_event.h and since we
are disabling nvdimm perf interface incase CONFIG_PERF_EVENTS option
is not set, we not need to declare them in nd.h

Fixes: 0fab1ba6ad6b ("drivers/nvdimm: Add perf interface to expose
nvdimm performance stats") (Commit id based on linux-next tree)
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Link: https://lore.kernel.org/all/62317124.YBQFU33+s%2FwdvWGj%25lkp@intel.com/
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/nvdimm/Makefile | 2 +-
 include/linux/nd.h      | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

---
- This fix patch changes are added and tested on top of linux-next tree
  on the 'next-20220315' branch.
---
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..a4265eaf5ae8 100644
--- a/include/linux/nd.h
+++ b/include/linux/nd.h
@@ -8,8 +8,10 @@
 #include <linux/ndctl.h>
 #include <linux/device.h>
 #include <linux/badblocks.h>
+#ifdef CONFIG_PERF_EVENTS
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
+#endif
 
 enum nvdimm_event {
 	NVDIMM_REVALIDATE_POISON,
@@ -25,6 +27,7 @@ enum nvdimm_claim_class {
 	NVDIMM_CCLASS_UNKNOWN,
 };
 
+#ifdef CONFIG_PERF_EVENTS
 #define NVDIMM_EVENT_VAR(_id)  event_attr_##_id
 #define NVDIMM_EVENT_PTR(_id)  (&event_attr_##_id.attr.attr)
 
@@ -63,9 +66,7 @@ extern ssize_t nvdimm_events_sysfs_show(struct device *dev,
 
 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);
+#endif
 
 struct nd_device_driver {
 	struct device_driver drv;
-- 
2.31.1

[PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set

[Kajol Jain]

The following build failure occures when CONFIG_PERF_EVENTS is not set
as generic pmu functions are not visible in that scenario.

|-- s390-randconfig-r044-20220313
|   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_migrate_context
|   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_register
|   `-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_unregister

Similar build failure in nds32 architecture:
nd_perf.c:(.text+0x21e): undefined reference to `perf_pmu_migrate_context'
nd_perf.c:(.text+0x434): undefined reference to `perf_pmu_register'
nd_perf.c:(.text+0x57c): undefined reference to `perf_pmu_unregister'

Fix this issue by adding check for CONFIG_PERF_EVENTS config option
and disabling the nvdimm perf interface incase this config is not set.

Also removed function declaration of perf_pmu_migrate_context,
perf_pmu_register, perf_pmu_unregister functions from nd.h as these are
common pmu functions which are part of perf_event.h and since we
are disabling nvdimm perf interface incase CONFIG_PERF_EVENTS option
is not set, we not need to declare them in nd.h

Fixes: 0fab1ba6ad6b ("drivers/nvdimm: Add perf interface to expose
nvdimm performance stats") (Commit id based on linux-next tree)
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Link: https://lore.kernel.org/all/62317124.YBQFU33+s%2FwdvWGj%25lkp@intel.com/
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/nvdimm/Makefile | 2 +-
 include/linux/nd.h      | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

---
- This fix patch changes are added and tested on top of linux-next tree
  on the 'next-20220315' branch.
---
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..a4265eaf5ae8 100644
--- a/include/linux/nd.h
+++ b/include/linux/nd.h
@@ -8,8 +8,10 @@
 #include <linux/ndctl.h>
 #include <linux/device.h>
 #include <linux/badblocks.h>
+#ifdef CONFIG_PERF_EVENTS
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
+#endif
 
 enum nvdimm_event {
 	NVDIMM_REVALIDATE_POISON,
@@ -25,6 +27,7 @@ enum nvdimm_claim_class {
 	NVDIMM_CCLASS_UNKNOWN,
 };
 
+#ifdef CONFIG_PERF_EVENTS
 #define NVDIMM_EVENT_VAR(_id)  event_attr_##_id
 #define NVDIMM_EVENT_PTR(_id)  (&event_attr_##_id.attr.attr)
 
@@ -63,9 +66,7 @@ extern ssize_t nvdimm_events_sysfs_show(struct device *dev,
 
 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);
+#endif
 
 struct nd_device_driver {
 	struct device_driver drv;
-- 
2.31.1

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

[Kajol Jain]

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 +++++++++++++++
 1 file changed, 15 insertions(+)

---
- This fix patch changes are added and tested on top of linux-next tree
  on the 'next-20220315' branch.
---
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 4f65bc0fb861..ffe85aeb4127 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -19,7 +19,10 @@
 #include <asm/papr_pdsm.h>
 #include <asm/mce.h>
 #include <asm/unaligned.h>
+
+#ifdef CONFIG_PERF_EVENTS
 #include <linux/perf_event.h>
+#endif
 
 #define BIND_ANY_ADDR (~0ul)
 
@@ -69,7 +72,9 @@
 #define PAPR_SCM_PERF_STATS_EYECATCHER __stringify(SCMSTATS)
 #define PAPR_SCM_PERF_STATS_VERSION 0x1
 
+#ifdef CONFIG_PERF_EVENTS
 #define to_nvdimm_pmu(_pmu)	container_of(_pmu, struct nvdimm_pmu, pmu)
+#endif
 
 /* Struct holding a single performance metric */
 struct papr_scm_perf_stat {
@@ -127,8 +132,10 @@ struct papr_scm_priv {
 	/* The bits which needs to be overridden */
 	u64 health_bitmap_inject_mask;
 
+#ifdef CONFIG_PERF_EVENTS
 	 /* array to have event_code and stat_id mappings */
 	char **nvdimm_events_map;
+#endif
 };
 
 static int papr_scm_pmem_flush(struct nd_region *nd_region,
@@ -349,6 +356,7 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
 	return 0;
 }
 
+#ifdef CONFIG_PERF_EVENTS
 static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev, u64 *count)
 {
 	struct papr_scm_perf_stat *stat;
@@ -560,6 +568,7 @@ static void papr_scm_pmu_register(struct papr_scm_priv *p)
 pmu_err_print:
 	dev_info(&p->pdev->dev, "nvdimm pmu didn't register rc=%d\n", rc);
 }
+#endif
 
 /*
  * Issue hcall to retrieve dimm health info and populate papr_scm_priv with the
@@ -1537,7 +1546,10 @@ static int papr_scm_probe(struct platform_device *pdev)
 		goto err2;
 
 	platform_set_drvdata(pdev, p);
+
+#ifdef CONFIG_PERF_EVENTS
 	papr_scm_pmu_register(p);
+#endif
 
 	return 0;
 
@@ -1557,11 +1569,14 @@ static int papr_scm_remove(struct platform_device *pdev)
 	nvdimm_bus_unregister(p->bus);
 	drc_pmem_unbind(p);
 
+#ifdef CONFIG_PERF_EVENTS
 	if (pdev->archdata.priv)
 		unregister_nvdimm_pmu(pdev->archdata.priv);
 
 	pdev->archdata.priv = NULL;
 	kfree(p->nvdimm_events_map);
+#endif
+
 	kfree(p->bus_desc.provider_name);
 	kfree(p);
 
-- 
2.31.1

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

[Kajol Jain]

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 +++++++++++++++
 1 file changed, 15 insertions(+)

---
- This fix patch changes are added and tested on top of linux-next tree
  on the 'next-20220315' branch.
---
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 4f65bc0fb861..ffe85aeb4127 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -19,7 +19,10 @@
 #include <asm/papr_pdsm.h>
 #include <asm/mce.h>
 #include <asm/unaligned.h>
+
+#ifdef CONFIG_PERF_EVENTS
 #include <linux/perf_event.h>
+#endif
 
 #define BIND_ANY_ADDR (~0ul)
 
@@ -69,7 +72,9 @@
 #define PAPR_SCM_PERF_STATS_EYECATCHER __stringify(SCMSTATS)
 #define PAPR_SCM_PERF_STATS_VERSION 0x1
 
+#ifdef CONFIG_PERF_EVENTS
 #define to_nvdimm_pmu(_pmu)	container_of(_pmu, struct nvdimm_pmu, pmu)
+#endif
 
 /* Struct holding a single performance metric */
 struct papr_scm_perf_stat {
@@ -127,8 +132,10 @@ struct papr_scm_priv {
 	/* The bits which needs to be overridden */
 	u64 health_bitmap_inject_mask;
 
+#ifdef CONFIG_PERF_EVENTS
 	 /* array to have event_code and stat_id mappings */
 	char **nvdimm_events_map;
+#endif
 };
 
 static int papr_scm_pmem_flush(struct nd_region *nd_region,
@@ -349,6 +356,7 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
 	return 0;
 }
 
+#ifdef CONFIG_PERF_EVENTS
 static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev, u64 *count)
 {
 	struct papr_scm_perf_stat *stat;
@@ -560,6 +568,7 @@ static void papr_scm_pmu_register(struct papr_scm_priv *p)
 pmu_err_print:
 	dev_info(&p->pdev->dev, "nvdimm pmu didn't register rc=%d\n", rc);
 }
+#endif
 
 /*
  * Issue hcall to retrieve dimm health info and populate papr_scm_priv with the
@@ -1537,7 +1546,10 @@ static int papr_scm_probe(struct platform_device *pdev)
 		goto err2;
 
 	platform_set_drvdata(pdev, p);
+
+#ifdef CONFIG_PERF_EVENTS
 	papr_scm_pmu_register(p);
+#endif
 
 	return 0;
 
@@ -1557,11 +1569,14 @@ static int papr_scm_remove(struct platform_device *pdev)
 	nvdimm_bus_unregister(p->bus);
 	drc_pmem_unbind(p);
 
+#ifdef CONFIG_PERF_EVENTS
 	if (pdev->archdata.priv)
 		unregister_nvdimm_pmu(pdev->archdata.priv);
 
 	pdev->archdata.priv = NULL;
 	kfree(p->nvdimm_events_map);
+#endif
+
 	kfree(p->bus_desc.provider_name);
 	kfree(p);
 
-- 
2.31.1

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

[Dan Williams]

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:

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.

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

[Dan Williams]

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:

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.

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

[Dan Williams]

On Mon, Mar 21, 2022 at 2:39 PM Dan Williams <dan.j.williams@intel.com> 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:
>
> 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.

Also, please base it on the current state of the libnvdimm-for-next
branch as -next includes some of the SMART health changes leading to
at least one conflict.

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

[Dan Williams]

On Mon, Mar 21, 2022 at 2:39 PM Dan Williams <dan.j.williams@intel.com> 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:
>
> 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.

Also, please base it on the current state of the libnvdimm-for-next
branch as -next includes some of the SMART health changes leading to
at least one conflict.

Re: [PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set

[Dan Williams]

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.
>
> |-- s390-randconfig-r044-20220313
> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_migrate_context
> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_register
> |   `-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_unregister
>
> Similar build failure in nds32 architecture:
> nd_perf.c:(.text+0x21e): undefined reference to `perf_pmu_migrate_context'
> nd_perf.c:(.text+0x434): undefined reference to `perf_pmu_register'
> nd_perf.c:(.text+0x57c): undefined reference to `perf_pmu_unregister'
>
> Fix this issue by adding check for CONFIG_PERF_EVENTS config option
> and disabling the nvdimm perf interface incase this config is not set.
>
> Also removed function declaration of perf_pmu_migrate_context,
> perf_pmu_register, perf_pmu_unregister functions from nd.h as these are
> common pmu functions which are part of perf_event.h and since we
> are disabling nvdimm perf interface incase CONFIG_PERF_EVENTS option
> is not set, we not need to declare them in nd.h
>
> Fixes: 0fab1ba6ad6b ("drivers/nvdimm: Add perf interface to expose
> nvdimm performance stats") (Commit id based on linux-next tree)
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> Link: https://lore.kernel.org/all/62317124.YBQFU33+s%2FwdvWGj%25lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/nvdimm/Makefile | 2 +-
>  include/linux/nd.h      | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> ---
> - This fix patch changes are added and tested on top of linux-next tree
>   on the 'next-20220315' branch.
> ---
> 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..a4265eaf5ae8 100644
> --- a/include/linux/nd.h
> +++ b/include/linux/nd.h
> @@ -8,8 +8,10 @@
>  #include <linux/ndctl.h>
>  #include <linux/device.h>
>  #include <linux/badblocks.h>
> +#ifdef CONFIG_PERF_EVENTS
>  #include <linux/perf_event.h>

Why must this not be included? Doesn't it already handle the
CONFIG_PERF_EVENTS=n case internally?

>  #include <linux/platform_device.h>

I notice now that this platform-device header should have never been
added in the first place, just forward declare:

struct platform_device;

> +#endif
>
>  enum nvdimm_event {
>         NVDIMM_REVALIDATE_POISON,
> @@ -25,6 +27,7 @@ enum nvdimm_claim_class {
>         NVDIMM_CCLASS_UNKNOWN,
>  };
>
> +#ifdef CONFIG_PERF_EVENTS
>  #define NVDIMM_EVENT_VAR(_id)  event_attr_##_id
>  #define NVDIMM_EVENT_PTR(_id)  (&event_attr_##_id.attr.attr)

Why must these be inside the ifdef guard?

>
> @@ -63,9 +66,7 @@ extern ssize_t nvdimm_events_sysfs_show(struct device *dev,
>
>  int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm, struct platform_device *pdev);
>  void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu);

Shouldn't there also be stub functions in the CONFIG_PERF_EVENTS=n case?

static inline int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm,
struct platform_device *pdev)
{
    return -ENXIO;
}

static inline 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);

Yeah, I should have caught these earlier.

> +#endif
>
>  struct nd_device_driver {
>         struct device_driver drv;
> --
> 2.31.1
>

Re: [PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set

[Dan Williams]

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.
>
> |-- s390-randconfig-r044-20220313
> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_migrate_context
> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_register
> |   `-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_unregister
>
> Similar build failure in nds32 architecture:
> nd_perf.c:(.text+0x21e): undefined reference to `perf_pmu_migrate_context'
> nd_perf.c:(.text+0x434): undefined reference to `perf_pmu_register'
> nd_perf.c:(.text+0x57c): undefined reference to `perf_pmu_unregister'
>
> Fix this issue by adding check for CONFIG_PERF_EVENTS config option
> and disabling the nvdimm perf interface incase this config is not set.
>
> Also removed function declaration of perf_pmu_migrate_context,
> perf_pmu_register, perf_pmu_unregister functions from nd.h as these are
> common pmu functions which are part of perf_event.h and since we
> are disabling nvdimm perf interface incase CONFIG_PERF_EVENTS option
> is not set, we not need to declare them in nd.h
>
> Fixes: 0fab1ba6ad6b ("drivers/nvdimm: Add perf interface to expose
> nvdimm performance stats") (Commit id based on linux-next tree)
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> Link: https://lore.kernel.org/all/62317124.YBQFU33+s%2FwdvWGj%25lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/nvdimm/Makefile | 2 +-
>  include/linux/nd.h      | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> ---
> - This fix patch changes are added and tested on top of linux-next tree
>   on the 'next-20220315' branch.
> ---
> 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..a4265eaf5ae8 100644
> --- a/include/linux/nd.h
> +++ b/include/linux/nd.h
> @@ -8,8 +8,10 @@
>  #include <linux/ndctl.h>
>  #include <linux/device.h>
>  #include <linux/badblocks.h>
> +#ifdef CONFIG_PERF_EVENTS
>  #include <linux/perf_event.h>

Why must this not be included? Doesn't it already handle the
CONFIG_PERF_EVENTS=n case internally?

>  #include <linux/platform_device.h>

I notice now that this platform-device header should have never been
added in the first place, just forward declare:

struct platform_device;

> +#endif
>
>  enum nvdimm_event {
>         NVDIMM_REVALIDATE_POISON,
> @@ -25,6 +27,7 @@ enum nvdimm_claim_class {
>         NVDIMM_CCLASS_UNKNOWN,
>  };
>
> +#ifdef CONFIG_PERF_EVENTS
>  #define NVDIMM_EVENT_VAR(_id)  event_attr_##_id
>  #define NVDIMM_EVENT_PTR(_id)  (&event_attr_##_id.attr.attr)

Why must these be inside the ifdef guard?

>
> @@ -63,9 +66,7 @@ extern ssize_t nvdimm_events_sysfs_show(struct device *dev,
>
>  int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm, struct platform_device *pdev);
>  void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu);

Shouldn't there also be stub functions in the CONFIG_PERF_EVENTS=n case?

static inline int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm,
struct platform_device *pdev)
{
    return -ENXIO;
}

static inline 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);

Yeah, I should have caught these earlier.

> +#endif
>
>  struct nd_device_driver {
>         struct device_driver drv;
> --
> 2.31.1
>

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

[kajoljain]

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

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

[kajoljain]

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

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

[kajoljain]

On 3/22/22 07:15, Dan Williams wrote:
> On Mon, Mar 21, 2022 at 2:39 PM Dan Williams <dan.j.williams@intel.com> 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:
>>
>> 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.
> 
> Also, please base it on the current state of the libnvdimm-for-next
> branch as -next includes some of the SMART health changes leading to
> at least one conflict.

Ok Dan, I will rebase my changes on top of libnvdimm-for-next branch.

Thanks,
Kajol Jain

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

[kajoljain]

On 3/22/22 07:15, Dan Williams wrote:
> On Mon, Mar 21, 2022 at 2:39 PM Dan Williams <dan.j.williams@intel.com> 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:
>>
>> 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.
> 
> Also, please base it on the current state of the libnvdimm-for-next
> branch as -next includes some of the SMART health changes leading to
> at least one conflict.

Ok Dan, I will rebase my changes on top of libnvdimm-for-next branch.

Thanks,
Kajol Jain

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

[Dan Williams]

On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }

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.

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

[Dan Williams]

On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }

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.

Re: [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@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@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) { }
> 
> 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 

Re: [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@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@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) { }
> 
> 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 

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

[Vaibhav Jain]

Dan Williams <dan.j.williams@intel.com> writes:

> On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
>
> 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.

Great suggestion Dan and incidently we are already working on a patchset
to reconcile this by moving a bunch of these defines from papr_scm.c to a
header. That work is primarily done to towards remove the redundancy
between papr_scm and generic ndtest module that we have today. We will
post the patches in next few days.

-- 
Cheers
~ Vaibhav

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

[Vaibhav Jain]

Dan Williams <dan.j.williams@intel.com> writes:

> On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
>
> 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.

Great suggestion Dan and incidently we are already working on a patchset
to reconcile this by moving a bunch of these defines from papr_scm.c to a
header. That work is primarily done to towards remove the redundancy
between papr_scm and generic ndtest module that we have today. We will
post the patches in next few days.

-- 
Cheers
~ Vaibhav

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

[Michael Ellerman]

Dan Williams <dan.j.williams@intel.com> writes:
> On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
>
> 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.

IMHO the only thing that belongs in a header is content that's needed by
other C files. As long as those types/declarations are only used in
papr_scm.c then they should stay in the C file, and there's no need for
a header.

I know the coding style rule is "avoid ifdefs in .c files", but I'd
argue that rule should be ignored if you're creating a header file
purely so that you can use an ifdef :)

Coding style also says:

  Prefer to compile out entire functions, rather than portions of functions or
  portions of expressions.  Rather than putting an ifdef in an expression, factor
  out part or all of the expression into a separate helper function and apply the
  conditional to that function.

Which is what we're doing here with eg. papr_scm_pmu_register().

Certainly for this merge window I think introducing a header is likely
to cause more problems than it solves, so let's not do that for now. We
can revisit it for the next merge window.

cheers

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

[Michael Ellerman]

Dan Williams <dan.j.williams@intel.com> writes:
> On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
>
> 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.

IMHO the only thing that belongs in a header is content that's needed by
other C files. As long as those types/declarations are only used in
papr_scm.c then they should stay in the C file, and there's no need for
a header.

I know the coding style rule is "avoid ifdefs in .c files", but I'd
argue that rule should be ignored if you're creating a header file
purely so that you can use an ifdef :)

Coding style also says:

  Prefer to compile out entire functions, rather than portions of functions or
  portions of expressions.  Rather than putting an ifdef in an expression, factor
  out part or all of the expression into a separate helper function and apply the
  conditional to that function.

Which is what we're doing here with eg. papr_scm_pmu_register().

Certainly for this merge window I think introducing a header is likely
to cause more problems than it solves, so let's not do that for now. We
can revisit it for the next merge window.

cheers

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

[Dan Williams]

On Wed, Mar 23, 2022 at 3:05 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
> > On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
> >
> > 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.
>
> IMHO the only thing that belongs in a header is content that's needed by
> other C files. As long as those types/declarations are only used in
> papr_scm.c then they should stay in the C file, and there's no need for
> a header.
>
> I know the coding style rule is "avoid ifdefs in .c files", but I'd
> argue that rule should be ignored if you're creating a header file
> purely so that you can use an ifdef :)
>
> Coding style also says:
>
>   Prefer to compile out entire functions, rather than portions of functions or
>   portions of expressions.  Rather than putting an ifdef in an expression, factor
>   out part or all of the expression into a separate helper function and apply the
>   conditional to that function.
>
> Which is what we're doing here with eg. papr_scm_pmu_register().
>
> Certainly for this merge window I think introducing a header is likely
> to cause more problems than it solves, so let's not do that for now. We
> can revisit it for the next merge window.

Fair enough. Kajol, please turn that snippet proposal into a formal patch.

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

[Dan Williams]

On Wed, Mar 23, 2022 at 3:05 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
> > On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
> >
> > 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.
>
> IMHO the only thing that belongs in a header is content that's needed by
> other C files. As long as those types/declarations are only used in
> papr_scm.c then they should stay in the C file, and there's no need for
> a header.
>
> I know the coding style rule is "avoid ifdefs in .c files", but I'd
> argue that rule should be ignored if you're creating a header file
> purely so that you can use an ifdef :)
>
> Coding style also says:
>
>   Prefer to compile out entire functions, rather than portions of functions or
>   portions of expressions.  Rather than putting an ifdef in an expression, factor
>   out part or all of the expression into a separate helper function and apply the
>   conditional to that function.
>
> Which is what we're doing here with eg. papr_scm_pmu_register().
>
> Certainly for this merge window I think introducing a header is likely
> to cause more problems than it solves, so let's not do that for now. We
> can revisit it for the next merge window.

Fair enough. Kajol, please turn that snippet proposal into a formal patch.

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

[kajoljain]

On 3/23/22 21:02, Dan Williams wrote:
> On Wed, Mar 23, 2022 at 3:05 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>> On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
>>>
>>> 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.
>>
>> IMHO the only thing that belongs in a header is content that's needed by
>> other C files. As long as those types/declarations are only used in
>> papr_scm.c then they should stay in the C file, and there's no need for
>> a header.
>>
>> I know the coding style rule is "avoid ifdefs in .c files", but I'd
>> argue that rule should be ignored if you're creating a header file
>> purely so that you can use an ifdef :)
>>
>> Coding style also says:
>>
>>   Prefer to compile out entire functions, rather than portions of functions or
>>   portions of expressions.  Rather than putting an ifdef in an expression, factor
>>   out part or all of the expression into a separate helper function and apply the
>>   conditional to that function.
>>
>> Which is what we're doing here with eg. papr_scm_pmu_register().
>>
>> Certainly for this merge window I think introducing a header is likely
>> to cause more problems than it solves, so let's not do that for now. We
>> can revisit it for the next merge window.
> 
> Fair enough. Kajol, please turn that snippet proposal into a formal patch.

Sure, I will send patchset for the same.

Thanks,
Kajol Jain

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

[kajoljain]

On 3/23/22 21:02, Dan Williams wrote:
> On Wed, Mar 23, 2022 at 3:05 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>> On Tue, Mar 22, 2022 at 7:30 AM kajoljain <kjain@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@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) { }
>>>
>>> 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.
>>
>> IMHO the only thing that belongs in a header is content that's needed by
>> other C files. As long as those types/declarations are only used in
>> papr_scm.c then they should stay in the C file, and there's no need for
>> a header.
>>
>> I know the coding style rule is "avoid ifdefs in .c files", but I'd
>> argue that rule should be ignored if you're creating a header file
>> purely so that you can use an ifdef :)
>>
>> Coding style also says:
>>
>>   Prefer to compile out entire functions, rather than portions of functions or
>>   portions of expressions.  Rather than putting an ifdef in an expression, factor
>>   out part or all of the expression into a separate helper function and apply the
>>   conditional to that function.
>>
>> Which is what we're doing here with eg. papr_scm_pmu_register().
>>
>> Certainly for this merge window I think introducing a header is likely
>> to cause more problems than it solves, so let's not do that for now. We
>> can revisit it for the next merge window.
> 
> Fair enough. Kajol, please turn that snippet proposal into a formal patch.

Sure, I will send patchset for the same.

Thanks,
Kajol Jain

Re: [PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set

[kajoljain]

On 3/22/22 07:40, 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.
>>
>> |-- s390-randconfig-r044-20220313
>> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_migrate_context
>> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_register
>> |   `-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_unregister
>>
>> Similar build failure in nds32 architecture:
>> nd_perf.c:(.text+0x21e): undefined reference to `perf_pmu_migrate_context'
>> nd_perf.c:(.text+0x434): undefined reference to `perf_pmu_register'
>> nd_perf.c:(.text+0x57c): undefined reference to `perf_pmu_unregister'
>>
>> Fix this issue by adding check for CONFIG_PERF_EVENTS config option
>> and disabling the nvdimm perf interface incase this config is not set.
>>
>> Also removed function declaration of perf_pmu_migrate_context,
>> perf_pmu_register, perf_pmu_unregister functions from nd.h as these are
>> common pmu functions which are part of perf_event.h and since we
>> are disabling nvdimm perf interface incase CONFIG_PERF_EVENTS option
>> is not set, we not need to declare them in nd.h
>>
>> Fixes: 0fab1ba6ad6b ("drivers/nvdimm: Add perf interface to expose
>> nvdimm performance stats") (Commit id based on linux-next tree)
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> Link: https://lore.kernel.org/all/62317124.YBQFU33+s%2FwdvWGj%25lkp@intel.com/
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>>  drivers/nvdimm/Makefile | 2 +-
>>  include/linux/nd.h      | 7 ++++---
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> ---
>> - This fix patch changes are added and tested on top of linux-next tree
>>   on the 'next-20220315' branch.
>> ---
>> 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..a4265eaf5ae8 100644
>> --- a/include/linux/nd.h
>> +++ b/include/linux/nd.h
>> @@ -8,8 +8,10 @@
>>  #include <linux/ndctl.h>
>>  #include <linux/device.h>
>>  #include <linux/badblocks.h>
>> +#ifdef CONFIG_PERF_EVENTS
>>  #include <linux/perf_event.h>
> 
> Why must this not be included? Doesn't it already handle the
> CONFIG_PERF_EVENTS=n case internally?
> 
>>  #include <linux/platform_device.h>
> 
> I notice now that this platform-device header should have never been
> added in the first place, just forward declare:
> 
> struct platform_device;

Hi Dan,
    Sure I will do the required changes.

Thanks,
Kajol Jain
> 
>> +#endif
>>
>>  enum nvdimm_event {
>>         NVDIMM_REVALIDATE_POISON,
>> @@ -25,6 +27,7 @@ enum nvdimm_claim_class {
>>         NVDIMM_CCLASS_UNKNOWN,
>>  };
>>
>> +#ifdef CONFIG_PERF_EVENTS
>>  #define NVDIMM_EVENT_VAR(_id)  event_attr_##_id
>>  #define NVDIMM_EVENT_PTR(_id)  (&event_attr_##_id.attr.attr)
> 
> Why must these be inside the ifdef guard?
> 
>>
>> @@ -63,9 +66,7 @@ extern ssize_t nvdimm_events_sysfs_show(struct device *dev,
>>
>>  int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm, struct platform_device *pdev);
>>  void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu);
> 
> Shouldn't there also be stub functions in the CONFIG_PERF_EVENTS=n case?
> 
> static inline int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm,
> struct platform_device *pdev)
> {
>     return -ENXIO;
> }
> 
> static inline 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);
> 
> Yeah, I should have caught these earlier.
> 
>> +#endif
>>
>>  struct nd_device_driver {
>>         struct device_driver drv;
>> --
>> 2.31.1
>>

Re: [PATCH 1/2] drivers/nvdimm: Fix build failure when CONFIG_PERF_EVENTS is not set

[kajoljain]

On 3/22/22 07:40, 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.
>>
>> |-- s390-randconfig-r044-20220313
>> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_migrate_context
>> |   |-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_register
>> |   `-- nd_perf.c:(.text):undefined-reference-to-perf_pmu_unregister
>>
>> Similar build failure in nds32 architecture:
>> nd_perf.c:(.text+0x21e): undefined reference to `perf_pmu_migrate_context'
>> nd_perf.c:(.text+0x434): undefined reference to `perf_pmu_register'
>> nd_perf.c:(.text+0x57c): undefined reference to `perf_pmu_unregister'
>>
>> Fix this issue by adding check for CONFIG_PERF_EVENTS config option
>> and disabling the nvdimm perf interface incase this config is not set.
>>
>> Also removed function declaration of perf_pmu_migrate_context,
>> perf_pmu_register, perf_pmu_unregister functions from nd.h as these are
>> common pmu functions which are part of perf_event.h and since we
>> are disabling nvdimm perf interface incase CONFIG_PERF_EVENTS option
>> is not set, we not need to declare them in nd.h
>>
>> Fixes: 0fab1ba6ad6b ("drivers/nvdimm: Add perf interface to expose
>> nvdimm performance stats") (Commit id based on linux-next tree)
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> Link: https://lore.kernel.org/all/62317124.YBQFU33+s%2FwdvWGj%25lkp@intel.com/
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>>  drivers/nvdimm/Makefile | 2 +-
>>  include/linux/nd.h      | 7 ++++---
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> ---
>> - This fix patch changes are added and tested on top of linux-next tree
>>   on the 'next-20220315' branch.
>> ---
>> 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..a4265eaf5ae8 100644
>> --- a/include/linux/nd.h
>> +++ b/include/linux/nd.h
>> @@ -8,8 +8,10 @@
>>  #include <linux/ndctl.h>
>>  #include <linux/device.h>
>>  #include <linux/badblocks.h>
>> +#ifdef CONFIG_PERF_EVENTS
>>  #include <linux/perf_event.h>
> 
> Why must this not be included? Doesn't it already handle the
> CONFIG_PERF_EVENTS=n case internally?
> 
>>  #include <linux/platform_device.h>
> 
> I notice now that this platform-device header should have never been
> added in the first place, just forward declare:
> 
> struct platform_device;

Hi Dan,
    Sure I will do the required changes.

Thanks,
Kajol Jain
> 
>> +#endif
>>
>>  enum nvdimm_event {
>>         NVDIMM_REVALIDATE_POISON,
>> @@ -25,6 +27,7 @@ enum nvdimm_claim_class {
>>         NVDIMM_CCLASS_UNKNOWN,
>>  };
>>
>> +#ifdef CONFIG_PERF_EVENTS
>>  #define NVDIMM_EVENT_VAR(_id)  event_attr_##_id
>>  #define NVDIMM_EVENT_PTR(_id)  (&event_attr_##_id.attr.attr)
> 
> Why must these be inside the ifdef guard?
> 
>>
>> @@ -63,9 +66,7 @@ extern ssize_t nvdimm_events_sysfs_show(struct device *dev,
>>
>>  int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm, struct platform_device *pdev);
>>  void unregister_nvdimm_pmu(struct nvdimm_pmu *nd_pmu);
> 
> Shouldn't there also be stub functions in the CONFIG_PERF_EVENTS=n case?
> 
> static inline int register_nvdimm_pmu(struct nvdimm_pmu *nvdimm,
> struct platform_device *pdev)
> {
>     return -ENXIO;
> }
> 
> static inline 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);
> 
> Yeah, I should have caught these earlier.
> 
>> +#endif
>>
>>  struct nd_device_driver {
>>         struct device_driver drv;
>> --
>> 2.31.1
>>