Patchwork EDAC, {skx|i10nm}_edac: Fix randconfig build error

login
register
mail settings
Submitter Luck, Tony
Date March 15, 2019, 5:49 p.m.
Message ID <20190315174956.GA20831@agluck-desk>
Download mbox | patch
Permalink /patch/749871/
State New
Headers show

Comments

Luck, Tony - March 15, 2019, 5:49 p.m.
On Fri, Mar 15, 2019 at 06:37:20PM +0100, Borislav Petkov wrote:
> I think the shared code should not have any reference to modules because
> it is supposed to be a library. Can you move the THIS_MODULE out of the
> common file and into the actual module?


Yes - Qiuxu did that already ... patch reposted below.

-Tony


From f9c656177e07fe5e978b09e5795a2a84a27bd54c Mon Sep 17 00:00:00 2001
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Date: Wed, 6 Mar 2019 09:59:14 +0800
Subject: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error

Kbuild failed on the kernel configurations below:

  CONFIG_ACPI_NFIT=y
  CONFIG_EDAC_DEBUG=y
  CONFIG_EDAC_SKX=m
  CONFIG_EDAC_I10NM=y
         or
  CONFIG_ACPI_NFIT=y
  CONFIG_EDAC_DEBUG=y
  CONFIG_EDAC_SKX=y
  CONFIG_EDAC_I10NM=m

Failed log:
  ...
  CC [M]  drivers/edac/skx_common.o
  ...
  .../skx_common.o:.../skx_common.c:672: undefined reference to `__this_module'

That is because if one of the two drivers {skx|i10nm}_edac is built-in
and the other one is built as a module, the shared file skx_common.c is
always built to an object in module style by kbuild. Therefore, when
linking for vmlinux, the '__this_module' symbol isn't defined.

Fix it by moving the DEFINE_SIMPLE_ATTRIBUTE() from skx_common.c to
skx_base.c and i10nm_base.c, where the '__this_module' is always defined
whatever it's built-in or built as a module.

Test the patch with following configurations:

  CONFIG_ACPI_NFIT=y
  CONFIG_EDAC_DEBUG=[y|n]

  +------------------------------------+
  |  skx_edac  |  i10nm_edac  | Build  |
  |------------|--------------|--------|
  |     m      |      m       |   ok   |
  |------------|--------------|--------|
  |     m      |      y       |   ok   |
  |------------|--------------|--------|
  |     y      |      m       |   ok   |
  |------------|--------------|--------|
  |     y      |      y       |   ok   |
  +------------------------------------+

Fixes: d4dc89d069aa ("EDAC, i10nm: Add a driver for Intel 10nm server processors")
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/i10nm_base.c | 4 +++-
 drivers/edac/skx_base.c   | 4 +++-
 drivers/edac/skx_common.c | 7 +++----
 drivers/edac/skx_common.h | 7 +++++--
 4 files changed, 14 insertions(+), 8 deletions(-)
Borislav Petkov - March 15, 2019, 6:02 p.m.
On Fri, Mar 15, 2019 at 10:49:56AM -0700, Luck, Tony wrote:
> Yes - Qiuxu did that already ... patch reposted below.

... to which Arnd said that it were fragile because it might break if it
includes a THIS_MODULE reference. So I'd say we won't do that then. And
keep it strictly a library.

Right?
Luck, Tony - March 15, 2019, 6:11 p.m.
On Fri, Mar 15, 2019 at 07:02:06PM +0100, Borislav Petkov wrote:
> On Fri, Mar 15, 2019 at 10:49:56AM -0700, Luck, Tony wrote:
> > Yes - Qiuxu did that already ... patch reposted below.
> 
> ... to which Arnd said that it were fragile because it might break if it
> includes a THIS_MODULE reference. So I'd say we won't do that then. And
> keep it strictly a library.
> 
> Right?

What is your definition of "library"?

fsl_ddr_edac.c seems to have the same potential for breakage
if someone makes a change to that, then it will hit the same
problem.

-Tony

Patch

diff --git a/drivers/edac/i10nm_base.c b/drivers/edac/i10nm_base.c
index c334fb7c63df..57ae2c6d5958 100644
--- a/drivers/edac/i10nm_base.c
+++ b/drivers/edac/i10nm_base.c
@@ -181,6 +181,8 @@  static struct notifier_block i10nm_mce_dec = {
 	.priority	= MCE_PRIO_EDAC,
 };
 
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
+
 static int __init i10nm_init(void)
 {
 	u8 mc = 0, src_id = 0, node_id = 0;
@@ -249,7 +251,7 @@  static int __init i10nm_init(void)
 
 	opstate_init();
 	mce_register_decode_chain(&i10nm_mce_dec);
-	setup_skx_debug("i10nm_test");
+	setup_skx_debug("i10nm_test", &fops_u64_wo);
 
 	i10nm_printk(KERN_INFO, "%s\n", I10NM_REVISION);
 
diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c
index adae4c848ca1..1748f627ca6c 100644
--- a/drivers/edac/skx_base.c
+++ b/drivers/edac/skx_base.c
@@ -540,6 +540,8 @@  static struct notifier_block skx_mce_dec = {
 	.priority	= MCE_PRIO_EDAC,
 };
 
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
+
 /*
  * skx_init:
  *	make sure we are running on the correct cpu model
@@ -619,7 +621,7 @@  static int __init skx_init(void)
 	/* Ensure that the OPSTATE is set correctly for POLL or NMI */
 	opstate_init();
 
-	setup_skx_debug("skx_test");
+	setup_skx_debug("skx_test", &fops_u64_wo);
 
 	mce_register_decode_chain(&skx_mce_dec);
 
diff --git a/drivers/edac/skx_common.c b/drivers/edac/skx_common.c
index 0e96e7b5b0a7..f75af7ff5515 100644
--- a/drivers/edac/skx_common.c
+++ b/drivers/edac/skx_common.c
@@ -653,7 +653,7 @@  void skx_remove(void)
  */
 static struct dentry *skx_test;
 
-static int debugfs_u64_set(void *data, u64 val)
+int debugfs_u64_set(void *data, u64 val)
 {
 	struct mce m;
 
@@ -669,16 +669,15 @@  static int debugfs_u64_set(void *data, u64 val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
 
-void setup_skx_debug(const char *dirname)
+void setup_skx_debug(const char *dirname, const struct file_operations *fops)
 {
 	skx_test = edac_debugfs_create_dir(dirname);
 	if (!skx_test)
 		return;
 
 	if (!edac_debugfs_create_file("addr", 0200, skx_test,
-				      NULL, &fops_u64_wo)) {
+				      NULL, fops)) {
 		debugfs_remove(skx_test);
 		skx_test = NULL;
 	}
diff --git a/drivers/edac/skx_common.h b/drivers/edac/skx_common.h
index d25374e34d4f..637867e0952c 100644
--- a/drivers/edac/skx_common.h
+++ b/drivers/edac/skx_common.h
@@ -142,10 +142,13 @@  int skx_mce_check_error(struct notifier_block *nb, unsigned long val,
 void skx_remove(void);
 
 #ifdef CONFIG_EDAC_DEBUG
-void setup_skx_debug(const char *dirname);
+int debugfs_u64_set(void *data, u64 val);
+void setup_skx_debug(const char *dirname, const struct file_operations *fops);
 void teardown_skx_debug(void);
 #else
-static inline void setup_skx_debug(const char *dirname) {}
+static inline int debugfs_u64_set(void *data, u64 val) { return -ENOENT; }
+static inline void setup_skx_debug(const char *dirname,
+				   const struct file_operations *fops) {}
 static inline void teardown_skx_debug(void) {}
 #endif /*CONFIG_EDAC_DEBUG*/