Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Thu, Mar 21, 2019 at 03:13:39PM -0700, Luck, Tony wrote:
>
> From: Qiuxu Zhuo
>
> Kbuild failed on the kernel configurations below:
...
I've massaged this into the below and running randconfigs now:
---
From: Qiuxu Zhuo
Date: Thu, 21 Mar 2019 15:13:39 -0700
Subject: [PATCH] EDAC, skx, i10nm: Make skx_common.c a pure library
The following Kconfig constellations fail randconfig builds:
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
with:
...
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_edac or i10nm_edac - is
built-in and the other one is a module, the shared file skx_common.c
gets linked into a module object by kbuild. Therefore, when linking that
same file into vmlinux, the '__this_module' symbol used in debugfs isn't
defined, leading to the above error.
Fix it by moving all debugfs code from skx_common.c to both skx_base.c
and i10nm_base.c respectively. Thus, skx_common.c doesn't refer to
'__this_module' symbol anymore.
Clarify its purpose at the top of the file for future reference, while
at it.
[ bp: Make text more readable. ]
Fixes: d4dc89d069aa ("EDAC, i10nm: Add a driver for Intel 10nm server
processors")
Reported-by: Arnd Bergmann
Signed-off-by: Qiuxu Zhuo
Signed-off-by: Tony Luck
Signed-off-by: Borislav Petkov
Cc: James Morse
Cc: Mauro Carvalho Chehab
Cc: linux-edac
Link: https://lkml.kernel.org/r/20190321221339.GA32323@agluck-desk
---
drivers/edac/i10nm_base.c | 52 +--
drivers/edac/skx_base.c | 50 +-
drivers/edac/skx_common.c | 57 +++
drivers/edac/skx_common.h | 8 --
4 files changed, 109 insertions(+), 58 deletions(-)
diff --git a/drivers/edac/i10nm_base.c b/drivers/edac/i10nm_base.c
index c334fb7c63df..6f06aec4877c 100644
--- a/drivers/edac/i10nm_base.c
+++ b/drivers/edac/i10nm_base.c
@@ -181,6 +181,54 @@ static struct notifier_block i10nm_mce_dec = {
.priority = MCE_PRIO_EDAC,
};
+#ifdef CONFIG_EDAC_DEBUG
+/*
+ * Debug feature.
+ * Exercise the address decode logic by writing an address to
+ * /sys/kernel/debug/edac/i10nm_test/addr.
+ */
+static struct dentry *i10nm_test;
+
+static int debugfs_u64_set(void *data, u64 val)
+{
+ struct mce m;
+
+ pr_warn_once("Fake error to 0x%llx injected via debugfs\n", val);
+
+ memset(, 0, sizeof(m));
+ /* ADDRV + MemRd + Unknown channel */
+ m.status = MCI_STATUS_ADDRV + 0x90;
+ /* One corrected error */
+ m.status |= BIT_ULL(MCI_STATUS_CEC_SHIFT);
+ m.addr = val;
+ skx_mce_check_error(NULL, 0, );
+
+ return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
+
+static void setup_i10nm_debug(void)
+{
+ i10nm_test = edac_debugfs_create_dir("i10nm_test");
+ if (!i10nm_test)
+ return;
+
+ if (!edac_debugfs_create_file("addr", 0200, i10nm_test,
+ NULL, _u64_wo)) {
+ debugfs_remove(i10nm_test);
+ i10nm_test = NULL;
+ }
+}
+
+static void teardown_i10nm_debug(void)
+{
+ debugfs_remove_recursive(i10nm_test);
+}
+#else
+static inline void setup_i10nm_debug(void) {}
+static inline void teardown_i10nm_debug(void) {}
+#endif /*CONFIG_EDAC_DEBUG*/
+
static int __init i10nm_init(void)
{
u8 mc = 0, src_id = 0, node_id = 0;
@@ -249,7 +297,7 @@ static int __init i10nm_init(void)
opstate_init();
mce_register_decode_chain(_mce_dec);
- setup_skx_debug("i10nm_test");
+ setup_i10nm_debug();
i10nm_printk(KERN_INFO, "%s\n", I10NM_REVISION);
@@ -262,7 +310,7 @@ static int __init i10nm_init(void)
static void __exit i10nm_exit(void)
{
edac_dbg(2, "\n");
- teardown_skx_debug();
+ teardown_i10nm_debug();
mce_unregister_decode_chain(_mce_dec);
skx_adxl_put();
skx_remove();
diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c
index adae4c848ca1..a5c8fa3a249a 100644
--- a/drivers/edac/skx_base.c
+++ b/drivers/edac/skx_base.c
@@ -540,6 +540,54 @@ static struct notifier_block skx_mce_dec = {
.priority = MCE_PRIO_EDAC,
};
+#ifdef CONFIG_EDAC_DEBUG
+/*
+ * Debug feature.
+ * Exercise the address decode logic by writing an address to
+ * /sys/kernel/debug/edac/skx_test/addr.
+ */
+static struct dentry *skx_test;
+
+static int debugfs_u64_set(void *data, u64 val)
+{
+ struct mce m;
+
+ pr_warn_once("Fake error to 0x%llx injected via debugfs\n", val);
+
+ memset(, 0, sizeof(m));
+ /* ADDRV + MemRd + Unknown channel */
+ m.status = MCI_STATUS_ADDRV + 0x90;
+ /* One corrected error */
+ m.status |= BIT_ULL(MCI_STATUS_CEC_SHIFT);
+
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Fri, Mar 22, 2019 at 6:55 PM Luck, Tony wrote: > > On Fri, Mar 22, 2019 at 03:00:25PM +0100, Arnd Bergmann wrote: > > Sorry, this was my mistake, my email was garbled. The patch was > > correct though: the idea here is not to change the Kconfig symbols > > but to change the Makefile to do the right thing even when Kconfig > > is set wrong. > > Well this does seem like a "clever" way out of the randconfig > problem. New patch applies and when I set .config to have: > > CONFIG_EDAC_DEBUG=y > CONFIG_EDAC_SKX=y > CONFIG_EDAC_I10NM=m > CONFIG_EDAC_SKX_COMMON=y > > I don't see any build errors. > > There are lots of "skx_" symbols in System.map > > But I'm not at all sure what happened to the I10NM driver. > > I don't see it mentioned in the output from "make". > > It didn't get built as a module (no ".ko" file for it). > > It doesn't seem to be built in (no ".o" in drivers/edac/built-in.a) > > So I added a #error line to the start of i10nm_edac.c and > ran "make" again. Nothing. Oops, I guess my testing method was also insufficient, I only checked that the bug was gone, not that it actually worked. > So, I don't think this is doing what you think it should > do. Even if it did, it would seem very confusing to a user > that asked for "one module, one built-in" in Kconfig, but > found both built-in. > > Boris: I'm voting for Qiuxu's most recent solution (moving > all the EDAC_DEBUG bits out of skx_common.c). Yes, that's probably best then. My patch was likely close to another correct solution, but I've screwed it up twice now ;-) Arnd
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Fri, Mar 22, 2019 at 03:00:25PM +0100, Arnd Bergmann wrote: > Sorry, this was my mistake, my email was garbled. The patch was > correct though: the idea here is not to change the Kconfig symbols > but to change the Makefile to do the right thing even when Kconfig > is set wrong. Well this does seem like a "clever" way out of the randconfig problem. New patch applies and when I set .config to have: CONFIG_EDAC_DEBUG=y CONFIG_EDAC_SKX=y CONFIG_EDAC_I10NM=m CONFIG_EDAC_SKX_COMMON=y I don't see any build errors. There are lots of "skx_" symbols in System.map But I'm not at all sure what happened to the I10NM driver. I don't see it mentioned in the output from "make". It didn't get built as a module (no ".ko" file for it). It doesn't seem to be built in (no ".o" in drivers/edac/built-in.a) So I added a #error line to the start of i10nm_edac.c and ran "make" again. Nothing. So, I don't think this is doing what you think it should do. Even if it did, it would seem very confusing to a user that asked for "one module, one built-in" in Kconfig, but found both built-in. Boris: I'm voting for Qiuxu's most recent solution (moving all the EDAC_DEBUG bits out of skx_common.c). -Tony
[PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
Configurations with CONFIG_EDAC_SKX and CONFIG_EDAC_I10NM both enabled, but only one of them built-in and the other being a loadable module fail to link because the common file is built the wrong way: skx_common.c:672: undefined reference to `__this_module' This overrides the way the modules are built, building both into the zImage file if that happens. Signed-off-by: Arnd Bergmann --- drivers/edac/Kconfig | 9 + drivers/edac/Makefile | 8 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index 47eb4d13ed5f..70080926329f 100644 --- a/drivers/edac/Kconfig +++ b/drivers/edac/Kconfig @@ -235,6 +235,7 @@ config EDAC_SKX depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_SKX can't be y select DMI select ACPI_ADXL + select EDAC_SKX_COMMON help Support for error detection and correction the Intel Skylake server Integrated Memory Controllers. If your @@ -247,12 +248,20 @@ config EDAC_I10NM depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_I10NM can't be y select DMI select ACPI_ADXL + select EDAC_SKX_COMMON help Support for error detection and correction the Intel 10nm server Integrated Memory Controllers. If your system has non-volatile DIMMs you should also manually select CONFIG_ACPI_NFIT. +config EDAC_SKX_COMMON + tristate + help + This is an internal helper symbol to ensure that all variants + of the EDAC_SKX driver are either built-in or modular, as mixing + the two causes link time problems. + config EDAC_PND2 tristate "Intel Pondicherry2" depends on PCI && X86_64 && X86_MCE_INTEL diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index 89ad4a84a0f6..0f363309f662 100644 --- a/drivers/edac/Makefile +++ b/drivers/edac/Makefile @@ -58,10 +58,14 @@ layerscape_edac_mod-y := fsl_ddr_edac.o layerscape_edac.o obj-$(CONFIG_EDAC_LAYERSCAPE) += layerscape_edac_mod.o skx_edac-y := skx_common.o skx_base.o -obj-$(CONFIG_EDAC_SKX) += skx_edac.o +ifdef CONFIG_EDAC_SKX +obj-$(CONFIG_EDAC_SKX_COMMON) += skx_edac.o +endif i10nm_edac-y := skx_common.o i10nm_base.o -obj-$(CONFIG_EDAC_I10NM) += i10nm_edac.o +ifdef CONFIG_EDAC_I10NM +obj-$(CONFIG_EDAC_SKX_COMMON)) += i10nm_edac.o +endif obj-$(CONFIG_EDAC_MV64X60) += mv64x60_edac.o obj-$(CONFIG_EDAC_CELL)+= cell_edac.o -- 2.20.0
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Fri, Mar 15, 2019 at 10:28 PM Luck, Tony wrote: > > > Basically I cheat Kconfig, so if one driver is built-in and > > the other is a loadable module, we compile both as built-in. > > My mail client may have munged that path. Both "git am" and "patch -p1" > barf when I try to apply it. Since it was small I tried to replicate > manually, > but I must have messed up something because Kconfig still seems content > to let me have CONFIG_EDAC_SKX=y with CONFIG_EDAC_I10NM=m > > :-( Sorry, this was my mistake, my email was garbled. The patch was correct though: the idea here is not to change the Kconfig symbols but to change the Makefile to do the right thing even when Kconfig is set wrong. Arnd
[PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
From: Qiuxu Zhuo
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
built to an object in module style by kbuild. Therefore, when linking for
vmlinux, the '__this_module' symbol used in debugfs isn't defined.
Fix it by moving all debugfs code from skx_common.c to {skx|i10nm}_base.c
respectively. Thus, skx_common.c is a module independent file which doesn't
refer to '__this_module' symbol anymore.
Fixes: d4dc89d069aa ("EDAC, i10nm: Add a driver for Intel 10nm server
processors")
Reported-by: Arnd Bergmann
Signed-off-by: Qiuxu Zhuo
Signed-off-by: Tony Luck
---
Arnd: I couldn't get your Kconfig trick (to disallow mixing module with
builtin) working. Qiuxu made this better cleanup of the skx_common.c file that
moves out all of the debugfs bits, and adds a comment to the top of the
file warning future developers of the mixed mode issue.
drivers/edac/i10nm_base.c | 52 +--
drivers/edac/skx_base.c | 50 -
drivers/edac/skx_common.c | 51 +-
drivers/edac/skx_common.h | 8 --
4 files changed, 105 insertions(+), 56 deletions(-)
diff --git a/drivers/edac/i10nm_base.c b/drivers/edac/i10nm_base.c
index c334fb7c63df..6f06aec4877c 100644
--- a/drivers/edac/i10nm_base.c
+++ b/drivers/edac/i10nm_base.c
@@ -181,6 +181,54 @@ static struct notifier_block i10nm_mce_dec = {
.priority = MCE_PRIO_EDAC,
};
+#ifdef CONFIG_EDAC_DEBUG
+/*
+ * Debug feature.
+ * Exercise the address decode logic by writing an address to
+ * /sys/kernel/debug/edac/i10nm_test/addr.
+ */
+static struct dentry *i10nm_test;
+
+static int debugfs_u64_set(void *data, u64 val)
+{
+ struct mce m;
+
+ pr_warn_once("Fake error to 0x%llx injected via debugfs\n", val);
+
+ memset(, 0, sizeof(m));
+ /* ADDRV + MemRd + Unknown channel */
+ m.status = MCI_STATUS_ADDRV + 0x90;
+ /* One corrected error */
+ m.status |= BIT_ULL(MCI_STATUS_CEC_SHIFT);
+ m.addr = val;
+ skx_mce_check_error(NULL, 0, );
+
+ return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
+
+static void setup_i10nm_debug(void)
+{
+ i10nm_test = edac_debugfs_create_dir("i10nm_test");
+ if (!i10nm_test)
+ return;
+
+ if (!edac_debugfs_create_file("addr", 0200, i10nm_test,
+ NULL, _u64_wo)) {
+ debugfs_remove(i10nm_test);
+ i10nm_test = NULL;
+ }
+}
+
+static void teardown_i10nm_debug(void)
+{
+ debugfs_remove_recursive(i10nm_test);
+}
+#else
+static inline void setup_i10nm_debug(void) {}
+static inline void teardown_i10nm_debug(void) {}
+#endif /*CONFIG_EDAC_DEBUG*/
+
static int __init i10nm_init(void)
{
u8 mc = 0, src_id = 0, node_id = 0;
@@ -249,7 +297,7 @@ static int __init i10nm_init(void)
opstate_init();
mce_register_decode_chain(_mce_dec);
- setup_skx_debug("i10nm_test");
+ setup_i10nm_debug();
i10nm_printk(KERN_INFO, "%s\n", I10NM_REVISION);
@@ -262,7 +310,7 @@ static int __init i10nm_init(void)
static void __exit i10nm_exit(void)
{
edac_dbg(2, "\n");
- teardown_skx_debug();
+ teardown_i10nm_debug();
mce_unregister_decode_chain(_mce_dec);
skx_adxl_put();
skx_remove();
diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c
index adae4c848ca1..a5c8fa3a249a 100644
--- a/drivers/edac/skx_base.c
+++ b/drivers/edac/skx_base.c
@@ -540,6 +540,54 @@ static struct notifier_block skx_mce_dec = {
.priority = MCE_PRIO_EDAC,
};
+#ifdef CONFIG_EDAC_DEBUG
+/*
+ * Debug feature.
+ * Exercise the address decode logic by writing an address to
+ * /sys/kernel/debug/edac/skx_test/addr.
+ */
+static struct dentry *skx_test;
+
+static int debugfs_u64_set(void *data, u64 val)
+{
+ struct mce m;
+
+ pr_warn_once("Fake error to 0x%llx injected via debugfs\n", val);
+
+ memset(, 0, sizeof(m));
+ /* ADDRV + MemRd + Unknown channel */
+ m.status = MCI_STATUS_ADDRV + 0x90;
+ /* One corrected error */
+ m.status |= BIT_ULL(MCI_STATUS_CEC_SHIFT);
+ m.addr = val;
+ skx_mce_check_error(NULL, 0, );
+
+ return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
+
+static void setup_skx_debug(void)
+{
+ skx_test = edac_debugfs_create_dir("skx_test");
+ if (!skx_test)
+ return;
+
+ if
RE: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
> Basically I cheat Kconfig, so if one driver is built-in and > the other is a loadable module, we compile both as built-in. My mail client may have munged that path. Both "git am" and "patch -p1" barf when I try to apply it. Since it was small I tried to replicate manually, but I must have messed up something because Kconfig still seems content to let me have CONFIG_EDAC_SKX=y with CONFIG_EDAC_I10NM=m :-( -Tony
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Fri, Mar 15, 2019 at 7:11 PM Luck, Tony wrote: > > 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. I think they are a bit safer because CONFIG_EDAC_LAYERSCAPE and CONFIG_EDAC_MPC85XX are mutually exclusive (one is only on powerpc, the other is only on ARM). It would break though if one were to make them build with CONFIG_COMPILE_TEST, or if another driver gets added that for ARM. I just thought about a possible Kconfig solution some more, and had this idea: diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index 47eb4d13ed5f..70080926329f 100644 --- a/drivers/edac/Kconfig +++ b/drivers/edac/Kconfig @@ -235,6 +235,7 @@ config EDAC_SKX depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_SKX can't be y select DMI select ACPI_ADXL + select EDAC_SKX_COMMON help Support for error detection and correction the Intel Skylake server Integrated Memory Controllers. If your @@ -247,12 +248,20 @@ config EDAC_I10NM depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_I10NM can't be y select DMI select ACPI_ADXL + select EDAC_SKX_COMMON help Support for error detection and correction the Intel 10nm server Integrated Memory Controllers. If your system has non-volatile DIMMs you should also manually select CONFIG_ACPI_NFIT. +config EDAC_SKX_COMMON + tristate + help + This is an internal helper symbol to ensure that all variants + of the EDAC_SKX driver are either built-in or modular, as mixing + the two causes link time problems. + config EDAC_PND2 tristate "Intel Pondicherry2" depends on PCI && X86_64 && X86_MCE_INTEL diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index 89ad4a84a0f6..01134051f5bf 100644 --- a/drivers/edac/Makefile +++ b/drivers/edac/Makefile @@ -58,10 +58,14 @@ layerscape_edac_mod-y := fsl_ddr_edac.o layerscape_edac.o obj-$(CONFIG_EDAC_LAYERSCAPE) += layerscape_edac_mod.o skx_edac-y := skx_common.o skx_base.o -obj-$(CONFIG_EDAC_SKX) += skx_edac.o +ifdef CONFIG_EDAC_SKX +obj-$(CONFIG_EDAC_SKX_COMMON) += skx_edac.o +endif i10nm_edac-y := skx_common.o i10nm_base.o +ifdef CONFIG_EDAC_I10NM obj-$(CONFIG_EDAC_SKX_COMMON) += i10nm_edac.o +endif obj-$(CONFIG_EDAC_MV64X60) += mv64x60_edac.o obj-$(CONFIG_EDAC_CELL)+= cell_edac.o Basically I cheat Kconfig, so if one driver is built-in and the other is a loadable module, we compile both as built-in. Arnd
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
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
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
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? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
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
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
Signed-off-by: Qiuxu Zhuo
Signed-off-by: Tony Luck
---
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(-)
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(_mce_dec);
- setup_skx_debug("i10nm_test");
+ setup_skx_debug("i10nm_test", _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", _u64_wo);
mce_register_decode_chain(_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, _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
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Fri, Mar 15, 2019 at 08:57:18AM -0700, Luck, Tony wrote: > fsl_ddr_edac.c looks to be doing exactly what we are doing with > skx_common.c. They just get away with it for now because they don't > have a reference to THIS_MODULE since they don't set up anything > in sysfs. > > If this is your goal, then Qiuxu's patch that moves the problem > piece out of skx_common.c does what you are asking for. My goal is to not make it too complex and this way burden future maintainability, without introducing the craziest randconfig build fixes. 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? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Fri, Mar 15, 2019 at 10:43:42AM +0100, Borislav Petkov wrote: > On Thu, Mar 14, 2019 at 02:59:52PM -0700, Luck, Tony wrote: > > I made a patch based on option #3. Rough steps were: > > > > $ cat skx_common.c >> skx_common.h > > That doesn't look real clean to me. So we have fsl_ddr_edac.c which > gets linked in in two drivers and I think you could librarize that > skx_common.c the same way and have the function exports in the wrapper > drivers skx_edac and i10nm_edac calling those "library" functions in > skx_common.c. IMNSVHO. fsl_ddr_edac.c looks to be doing exactly what we are doing with skx_common.c. They just get away with it for now because they don't have a reference to THIS_MODULE since they don't set up anything in sysfs. If this is your goal, then Qiuxu's patch that moves the problem piece out of skx_common.c does what you are asking for. -Tony
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Thu, Mar 14, 2019 at 02:59:52PM -0700, Luck, Tony wrote: > I made a patch based on option #3. Rough steps were: > > $ cat skx_common.c >> skx_common.h That doesn't look real clean to me. So we have fsl_ddr_edac.c which gets linked in in two drivers and I think you could librarize that skx_common.c the same way and have the function exports in the wrapper drivers skx_edac and i10nm_edac calling those "library" functions in skx_common.c. IMNSVHO. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Thu, Mar 14, 2019 at 12:04:13PM +0100, Borislav Petkov wrote:
> 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).
There's a lot of common code. Patching the same thing in two
different files seems like make-work (and a recipe for things
to be forgotten/missed).
I made a patch based on option #3. Rough steps were:
$ cat skx_common.c >> skx_common.h
$ git rm skx_common.c
$ git mv skx_base.c skx_edac.c
$ git mv i10nm_base.c i10nm_edac.c
$ edit Makefile for changes of names
$ edit skx_common.h to add "static" everywhere
$ edit to fix the build issues
Not entirely happy with some of the bits in that last step.
Patch looks like this:
>From b9e9723c95b667d28d81ea53b35447e1ce8f3244 Mon Sep 17 00:00:00 2001
From: Tony Luck
Date: Thu, 14 Mar 2019 10:45:34 -0700
Subject: [PATCH] fix corner cases for randconfig
---
drivers/edac/Makefile | 2 -
drivers/edac/{i10nm_base.c => i10nm_edac.c} | 5 +
drivers/edac/skx_common.c | 691
drivers/edac/skx_common.h | 685 ++-
drivers/edac/{skx_base.c => skx_edac.c} | 9 +-
5 files changed, 676 insertions(+), 716 deletions(-)
rename drivers/edac/{i10nm_base.c => i10nm_edac.c} (98%)
delete mode 100644 drivers/edac/skx_common.c
rename drivers/edac/{skx_base.c => skx_edac.c} (98%)
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 89ad4a84a0f6..5d8d88574d3c 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -57,10 +57,8 @@ obj-$(CONFIG_EDAC_MPC85XX) += mpc85xx_edac_mod.o
layerscape_edac_mod-y := fsl_ddr_edac.o layerscape_edac.o
obj-$(CONFIG_EDAC_LAYERSCAPE) += layerscape_edac_mod.o
-skx_edac-y := skx_common.o skx_base.o
obj-$(CONFIG_EDAC_SKX) += skx_edac.o
-i10nm_edac-y := skx_common.o i10nm_base.o
obj-$(CONFIG_EDAC_I10NM) += i10nm_edac.o
obj-$(CONFIG_EDAC_MV64X60) += mv64x60_edac.o
diff --git a/drivers/edac/i10nm_base.c b/drivers/edac/i10nm_edac.c
similarity index 98%
rename from drivers/edac/i10nm_base.c
rename to drivers/edac/i10nm_edac.c
index c334fb7c63df..45c6497f35c5 100644
--- a/drivers/edac/i10nm_base.c
+++ b/drivers/edac/i10nm_edac.c
@@ -10,6 +10,11 @@
#include
#include
#include "edac_module.h"
+
+struct decoded_addr;
+typedef bool (*skx_decode_f)(struct decoded_addr *res);
+static skx_decode_f skx_decode;
+
#include "skx_common.h"
#define I10NM_REVISION "v0.0.3"
diff --git a/drivers/edac/skx_common.c b/drivers/edac/skx_common.c
deleted file mode 100644
index 0e96e7b5b0a7..
--- a/drivers/edac/skx_common.c
+++ /dev/null
@@ -1,691 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Common codes for both the skx_edac driver and Intel 10nm server EDAC driver.
- * Originally split out from the skx_edac driver.
- *
- * Copyright (c) 2018, Intel Corporation.
- */
-
-#include
-#include
-#include
-#include
-#include
-#include "edac_module.h"
-#include "skx_common.h"
-
-static const char * const component_names[] = {
- [INDEX_SOCKET] = "ProcessorSocketId",
- [INDEX_MEMCTRL] = "MemoryControllerId",
- [INDEX_CHANNEL] = "ChannelId",
- [INDEX_DIMM]= "DimmSlotId",
-};
-
-static int component_indices[ARRAY_SIZE(component_names)];
-static int adxl_component_count;
-static const char * const *adxl_component_names;
-static u64 *adxl_values;
-static char *adxl_msg;
-
-static char skx_msg[MSG_SIZE];
-static skx_decode_f skx_decode;
-static u64 skx_tolm, skx_tohm;
-static LIST_HEAD(dev_edac_list);
-
-int __init skx_adxl_get(void)
-{
-
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
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.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Thu, Mar 14, 2019 at 12:01 AM Luck, Tony 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 wrote:
> > > From: Qiuxu Zhuo
> > >
> > > 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
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Wed, Mar 06, 2019 at 09:15:13PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 6, 2019 at 6:58 PM Luck, Tony wrote:
> > From: Qiuxu Zhuo
> >
> > 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
Re: [PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
On Wed, Mar 6, 2019 at 6:58 PM Luck, Tony wrote: > From: Qiuxu Zhuo > > 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
[PATCH] EDAC, {skx|i10nm}_edac: Fix randconfig build error
From: Qiuxu Zhuo
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
Signed-off-by: Qiuxu Zhuo
Signed-off-by: Tony Luck
---
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(-)
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(_mce_dec);
- setup_skx_debug("i10nm_test");
+ setup_skx_debug("i10nm_test", _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", _u64_wo);
mce_register_decode_chain(_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, _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) {
