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

login
register
mail settings
Submitter Luck, Tony
Date March 6, 2019, 5:58 p.m.
Message ID <20190306175808.GA30016@agluck-desk>
Download mbox | patch
Permalink /patch/742731/
State New
Headers show

Comments

Luck, Tony - March 6, 2019, 5:58 p.m.
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

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>
---

This seems cleaner than adding all the EXPORTs to skx_common.c
I also tried a build with the 0x8A152468-config.gz that Arnd
supplied.

 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(-)
Arnd Bergmann - March 6, 2019, 8:15 p.m.
On Wed, Mar 6, 2019 at 6:58 PM Luck, Tony <tony.luck@intel.com> wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>
> This seems cleaner than adding all the EXPORTs to skx_common.c
> I also tried a build with the 0x8A152468-config.gz that Arnd
> supplied.

It's still a bit fragile since you do something that kbuild doesn't
expect with having a file in both a module and built-in code
in some configurations. I'm fairly sure this version works today,
but it would break the next time that file gets changed to include
a reference to THIS_MODULE, or anything else that is different
between built-in and modular code.

Another alternative would be to mark all symbols in this file
'static' and then include skx_common.c from the other two files.
Of course that is also ugly and it increases the overall code size,
so I don't see a way to win this.

     Arnd
Luck, Tony - March 13, 2019, 11:01 p.m.
On Wed, Mar 06, 2019 at 09:15:13PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 6, 2019 at 6:58 PM Luck, Tony <tony.luck@intel.com> wrote:
> > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >
> > This seems cleaner than adding all the EXPORTs to skx_common.c
> > I also tried a build with the 0x8A152468-config.gz that Arnd
> > supplied.
> 
> It's still a bit fragile since you do something that kbuild doesn't
> expect with having a file in both a module and built-in code
> in some configurations. I'm fairly sure this version works today,
> but it would break the next time that file gets changed to include
> a reference to THIS_MODULE, or anything else that is different
> between built-in and modular code.
> 
> Another alternative would be to mark all symbols in this file
> 'static' and then include skx_common.c from the other two files.
> Of course that is also ugly and it increases the overall code size,
> so I don't see a way to win this.

Boris,

So where should we go. Proposed solutions are piling up:

1) Make skx_common a module
	[downside: have to EXPORT everything in it]
2) Move module-ish bits out of skx_common
	[downside: perhaps fragile]
3) #include skx_common.c into {skx,i10nm}_edac.c
	[downside: no patch yet, bigger code size]

-Tony
Arnd Bergmann - March 14, 2019, 7:09 a.m.
On Thu, Mar 14, 2019 at 12:01 AM Luck, Tony <tony.luck@intel.com> wrote:
>
> On Wed, Mar 06, 2019 at 09:15:13PM +0100, Arnd Bergmann wrote:
> > On Wed, Mar 6, 2019 at 6:58 PM Luck, Tony <tony.luck@intel.com> wrote:
> > > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > >
> > > This seems cleaner than adding all the EXPORTs to skx_common.c
> > > I also tried a build with the 0x8A152468-config.gz that Arnd
> > > supplied.
> >
> > It's still a bit fragile since you do something that kbuild doesn't
> > expect with having a file in both a module and built-in code
> > in some configurations. I'm fairly sure this version works today,
> > but it would break the next time that file gets changed to include
> > a reference to THIS_MODULE, or anything else that is different
> > between built-in and modular code.
> >
> > Another alternative would be to mark all symbols in this file
> > 'static' and then include skx_common.c from the other two files.
> > Of course that is also ugly and it increases the overall code size,
> > so I don't see a way to win this.
>
> Boris,
>
> So where should we go. Proposed solutions are piling up:
>
> 1) Make skx_common a module
>         [downside: have to EXPORT everything in it]
> 2) Move module-ish bits out of skx_common
>         [downside: perhaps fragile]
> 3) #include skx_common.c into {skx,i10nm}_edac.c
>         [downside: no patch yet, bigger code size]

Sorry to add on to the already long list, but one more idea after
looking at the two files:

4) Have a single driver module, with the skx_common.c containing
    the top-level module_init/module_exit functions that call into the
    hardware specific versions.

This is the opposite of the usual recommendation (the driver
is already structured nicely otherwise), but it would be cheap
way out here.

      Arnd
Borislav Petkov - March 14, 2019, 11:04 a.m.
On Thu, Mar 14, 2019 at 08:09:06AM +0100, Arnd Bergmann wrote:
> > So where should we go. Proposed solutions are piling up:
> >
> > 1) Make skx_common a module
> >         [downside: have to EXPORT everything in it]
> > 2) Move module-ish bits out of skx_common
> >         [downside: perhaps fragile]
> > 3) #include skx_common.c into {skx,i10nm}_edac.c
> >         [downside: no patch yet, bigger code size]
> 
> Sorry to add on to the already long list, but one more idea after
> looking at the two files:
> 
> 4) Have a single driver module, with the skx_common.c containing
>     the top-level module_init/module_exit functions that call into the
>     hardware specific versions.
> 
> This is the opposite of the usual recommendation (the driver
> is already structured nicely otherwise), but it would be cheap
> way out here.

This is what I think right now: if this is going to become such a pain,
then we better keep those two drivers completely separate. Who cares if
there's some code duplication?! Better that than some obscure randconfig
build breakages and fixes introducing more ugliness. IOW, we should keep
it simple.

Unless, Tony, you want to be able to make changes to the common code in
one place and don't want to patch both. Then I'd librarize the common
functionality, i.e., do something along the lines of 2).

But having two completely separate drivers is the most robust variant,
IMO.

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*/