| Message ID | 20240529095132.1929397-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | EDAC, i10nm: make skx_common.o a separate module | expand |
> Linking an object file into multiple modules causes a warning: > > scripts/Makefile.build:236: drivers/edac/Makefile: skx_common.o is added to multiple modules: i10nm_edac skx_edac In this case there is no practical problem from this double link. The two modules created: skx_edac.ko and i10nm_edac.ko are mutually exclusive. A system may load neither, either, but not both (enforced by the x86_match_cpu() check in each modules init function). > Make this a separate module instead. > +EXPORT_SYMBOL_GPL(skx_adxl_get); > +EXPORT_SYMBOL_GPL(skx_adxl_put); > +EXPORT_SYMBOL_GPL(skx_set_mem_cfg); > +EXPORT_SYMBOL_GPL(skx_set_decode); > +EXPORT_SYMBOL_GPL(skx_get_src_id); > +EXPORT_SYMBOL_GPL(skx_get_node_id); > +EXPORT_SYMBOL_GPL(skx_get_all_bus_mappings); > +EXPORT_SYMBOL_GPL(skx_get_hi_lo); > +EXPORT_SYMBOL_GPL(skx_get_dimm_info); > +EXPORT_SYMBOL_GPL(skx_get_nvdimm_info); > +EXPORT_SYMBOL_GPL(skx_register_mci); > +EXPORT_SYMBOL_GPL(skx_mce_check_error); > +EXPORT_SYMBOL_GPL(skx_remove); With all these new EXPORTs ... perhaps drivers/edac should start using a local export name space? -DDEFAULT_SYMBOL_NAMESPACE=EDAC ? But maybe a better fix might be to somehow tag skx_common.o to tell the checker script "It's OK. I meant to do that." -Tony
On Wed, May 29, 2024, at 18:14, Luck, Tony wrote: >> Linking an object file into multiple modules causes a warning: >> >> scripts/Makefile.build:236: drivers/edac/Makefile: skx_common.o is added to multiple modules: i10nm_edac skx_edac > > In this case there is no practical problem from this double link. The > two modules created: skx_edac.ko and i10nm_edac.ko > are mutually exclusive. A system may load neither, either, but not both > (enforced by the x86_match_cpu() check in each > modules init function). One of the problems here is that each compilation unit implicitly knows the name of the module it gets linked into, via the KBUILD_MODNAME macro. If it gets linked twice, the macro is wrong for at least one of the two, and this can lead to incorrect printk formats and other macro expansions using that as an identifier. A particularly bad case happens when one of the two is built-in while the other one is a loadable module. In this case, the module infrastructure assumes it's always built-in, which can mess up e.g. __exit annotations and THIS_MODULE references. Arnd
> One of the problems here is that each compilation unit implicitly > knows the name of the module it gets linked into, via the > KBUILD_MODNAME macro. If it gets linked twice, the macro is > wrong for at least one of the two, and this can lead to > incorrect printk formats and other macro expansions using > that as an identifier. Hardly any edac drivers use KBUILD_MODNAME. These two don't. > A particularly bad case happens when one of the two is > built-in while the other one is a loadable module. In > this case, the module infrastructure assumes it's always > built-in, which can mess up e.g. __exit annotations and > THIS_MODULE references. But that does seem like a real problem. I took your patch for a spin on a system that uses the i10nm_edac module. Works ok both when built as a module (the skx_edac_common module was autoloaded) and as a built-in. If Boris has no other thoughts about this I will apply your patch. -Tony
On Wed, May 29, 2024 at 11:51:11AM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Linking an object file into multiple modules causes a warning: > > scripts/Makefile.build:236: drivers/edac/Makefile: skx_common.o is added to multiple modules: i10nm_edac skx_edac What changed? This wasn't an issue until now-ish... > Make this a separate module instead. ...and I'm not crazy about the exported functions either.
On Wed, May 29, 2024, at 21:31, Borislav Petkov wrote: > On Wed, May 29, 2024 at 11:51:11AM +0200, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> Linking an object file into multiple modules causes a warning: >> >> scripts/Makefile.build:236: drivers/edac/Makefile: skx_common.o is added to multiple modules: i10nm_edac skx_edac > > What changed? > > This wasn't an issue until now-ish... It has caused problems in enough other drivers that we now have a warning for it, it was added in commit 598afa050403 ("kbuild: warn objects shared among multiple modules"). Most modules are modified already, and we are now down to the last handful. Since the bugs are fairly hard to understand when they happen, it would be nice to enable the warning by default. Arnd
On Wed, May 29, 2024 at 09:55:04PM +0200, Arnd Bergmann wrote: > It has caused problems in enough other drivers that we now > have a warning for it, it was added in commit 598afa050403 > ("kbuild: warn objects shared among multiple modules"). > > Most modules are modified already, and we are now down to > the last handful. Since the bugs are fairly hard to understand > when they happen, it would be nice to enable the warning > by default. Yah, this should've been in the commit message: it's not like I've caught a public announcement that we're doing this now... :) Tony, please add it before committing. Thx.
> From: Arnd Bergmann <arnd@arndb.de> > [ ...] > A particularly bad case happens when one of the two is built-in while the > other one is a loadable module. In this case, the module infrastructure > assumes it's always built-in, which can mess up e.g. __exit annotations and > THIS_MODULE references. Hi Arnd, Thanks for looking into this. I re-verified the following configuration w/o your patch, it worked well, and I didn't see __exit annotations and THIS_MODULE references got messed up. This was because skx_common.o was built into separate two copied texts (one is built-in, and the other is in the module). 1. Kernel configuration (one built-in, one module) CONFIG_EDAC_SKX=y CONFIG_EDAC_I10NM=m 2. Build OK. Loaded/unloaded i10nm_edac.ko on an Intel Sapphire Rapids server successfully. 3. Symbol in skx_common.o example: [root@fl31ca103as0104 ~]# cat /proc/kallsyms | grep skx_adxl_put ffffffff832c7da0 T skx_adxl_put <--- symbol in built-in kernel ffffffffc094a350 t skx_adxl_put [i10nm_edac] <--- symbol in 'i10nm_edac.ko' module Thanks! -Qiuxu
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index 9c09893695b7..4edfb83ffbee 100644 --- a/drivers/edac/Makefile +++ b/drivers/edac/Makefile @@ -54,11 +54,13 @@ 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 +skx_edac_common-y := skx_common.o -i10nm_edac-y := skx_common.o i10nm_base.o -obj-$(CONFIG_EDAC_I10NM) += i10nm_edac.o +skx_edac-y := skx_base.o +obj-$(CONFIG_EDAC_SKX) += skx_edac.o skx_edac_common.o + +i10nm_edac-y := i10nm_base.o +obj-$(CONFIG_EDAC_I10NM) += i10nm_edac.o skx_edac_common.o obj-$(CONFIG_EDAC_CELL) += cell_edac.o obj-$(CONFIG_EDAC_PPC4XX) += ppc4xx_edac.o diff --git a/drivers/edac/skx_common.c b/drivers/edac/skx_common.c index 27996b7924c8..8d18099fd528 100644 --- a/drivers/edac/skx_common.c +++ b/drivers/edac/skx_common.c @@ -48,7 +48,7 @@ static u64 skx_tolm, skx_tohm; static LIST_HEAD(dev_edac_list); static bool skx_mem_cfg_2lm; -int __init skx_adxl_get(void) +int skx_adxl_get(void) { const char * const *names; int i, j; @@ -110,12 +110,14 @@ int __init skx_adxl_get(void) return -ENODEV; } +EXPORT_SYMBOL_GPL(skx_adxl_get); -void __exit skx_adxl_put(void) +void skx_adxl_put(void) { kfree(adxl_values); kfree(adxl_msg); } +EXPORT_SYMBOL_GPL(skx_adxl_put); static bool skx_adxl_decode(struct decoded_addr *res, bool error_in_1st_level_mem) { @@ -187,12 +189,14 @@ void skx_set_mem_cfg(bool mem_cfg_2lm) { skx_mem_cfg_2lm = mem_cfg_2lm; } +EXPORT_SYMBOL_GPL(skx_set_mem_cfg); void skx_set_decode(skx_decode_f decode, skx_show_retry_log_f show_retry_log) { driver_decode = decode; skx_show_retry_rd_err_log = show_retry_log; } +EXPORT_SYMBOL_GPL(skx_set_decode); int skx_get_src_id(struct skx_dev *d, int off, u8 *id) { @@ -206,6 +210,7 @@ int skx_get_src_id(struct skx_dev *d, int off, u8 *id) *id = GET_BITFIELD(reg, 12, 14); return 0; } +EXPORT_SYMBOL_GPL(skx_get_src_id); int skx_get_node_id(struct skx_dev *d, u8 *id) { @@ -219,6 +224,7 @@ int skx_get_node_id(struct skx_dev *d, u8 *id) *id = GET_BITFIELD(reg, 0, 2); return 0; } +EXPORT_SYMBOL_GPL(skx_get_node_id); static int get_width(u32 mtr) { @@ -284,6 +290,7 @@ int skx_get_all_bus_mappings(struct res_config *cfg, struct list_head **list) *list = &dev_edac_list; return ndev; } +EXPORT_SYMBOL_GPL(skx_get_all_bus_mappings); int skx_get_hi_lo(unsigned int did, int off[], u64 *tolm, u64 *tohm) { @@ -323,6 +330,7 @@ int skx_get_hi_lo(unsigned int did, int off[], u64 *tolm, u64 *tohm) pci_dev_put(pdev); return -ENODEV; } +EXPORT_SYMBOL_GPL(skx_get_hi_lo); static int skx_get_dimm_attr(u32 reg, int lobit, int hibit, int add, int minval, int maxval, const char *name) @@ -394,6 +402,7 @@ int skx_get_dimm_info(u32 mtr, u32 mcmtr, u32 amap, struct dimm_info *dimm, return 1; } +EXPORT_SYMBOL_GPL(skx_get_dimm_info); int skx_get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc, int chan, int dimmno, const char *mod_str) @@ -442,6 +451,7 @@ int skx_get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc, return (size == 0 || size == ~0ull) ? 0 : 1; } +EXPORT_SYMBOL_GPL(skx_get_nvdimm_info); int skx_register_mci(struct skx_imc *imc, struct pci_dev *pdev, const char *ctl_name, const char *mod_str, @@ -512,6 +522,7 @@ int skx_register_mci(struct skx_imc *imc, struct pci_dev *pdev, imc->mci = NULL; return rc; } +EXPORT_SYMBOL_GPL(skx_register_mci); static void skx_unregister_mci(struct skx_imc *imc) { @@ -688,6 +699,7 @@ int skx_mce_check_error(struct notifier_block *nb, unsigned long val, mce->kflags |= MCE_HANDLED_EDAC; return NOTIFY_DONE; } +EXPORT_SYMBOL_GPL(skx_mce_check_error); void skx_remove(void) { @@ -725,3 +737,8 @@ void skx_remove(void) kfree(d); } } +EXPORT_SYMBOL_GPL(skx_remove); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Tony Luck"); +MODULE_DESCRIPTION("MC Driver for Intel server processors"); diff --git a/drivers/edac/skx_common.h b/drivers/edac/skx_common.h index b6d3607dffe2..11faf1db4fa4 100644 --- a/drivers/edac/skx_common.h +++ b/drivers/edac/skx_common.h @@ -231,8 +231,8 @@ typedef int (*get_dimm_config_f)(struct mem_ctl_info *mci, typedef bool (*skx_decode_f)(struct decoded_addr *res); typedef void (*skx_show_retry_log_f)(struct decoded_addr *res, char *msg, int len, bool scrub_err); -int __init skx_adxl_get(void); -void __exit skx_adxl_put(void); +int skx_adxl_get(void); +void skx_adxl_put(void); void skx_set_decode(skx_decode_f decode, skx_show_retry_log_f show_retry_log); void skx_set_mem_cfg(bool mem_cfg_2lm);