* [PATCH v5] modules: introduce the MODULE_SCMVERSION config @ 2021-01-08 0:30 Will McVicker 2021-01-12 15:54 ` Jessica Yu ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Will McVicker @ 2021-01-08 0:30 UTC (permalink / raw) To: Jessica Yu, Masahiro Yamada, Michal Marek Cc: Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, linux-kernel, linux-kbuild, kernel-team, Will McVicker Config MODULE_SCMVERSION introduces a new module attribute -- `scmversion` -- which can be used to identify a given module's SCM version. This is very useful for developers that update their kernel independently from their kernel modules or vice-versa since the SCM version provided by UTS_RELEASE (`uname -r`) will now differ from the module's vermagic attribute. For example, we have a CI setup that tests new kernel changes on the hikey960 and db845c devices without updating their kernel modules. When these tests fail, we need to be able to identify the exact device configuration the test was using. By including MODULE_SCMVERSION, we can identify the exact kernel and modules' SCM versions for debugging the failures. Additionally, by exposing the SCM version via the sysfs node /sys/module/MODULENAME/scmversion, one can also verify the SCM versions of the modules loaded from the initramfs. Currently, modinfo can only retrieve module attributes from the module's ko on disk and not from the actual module that is loaded in RAM. You can retrieve the SCM version in two ways, 1) By using modinfo: > modinfo -F scmversion MODULENAME 2) By module sysfs node: > cat /sys/module/MODULENAME/scmversion Signed-off-by: Will McVicker <willmcvicker@google.com> --- Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++ include/linux/module.h | 1 + init/Kconfig | 14 ++++++++++++++ kernel/module.c | 2 ++ scripts/Makefile.modpost | 22 ++++++++++++++++++++++ scripts/mod/modpost.c | 24 +++++++++++++++++++++++- 6 files changed, 80 insertions(+), 1 deletion(-) diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module index 6272ae5fb366..a75d137e79f4 100644 --- a/Documentation/ABI/stable/sysfs-module +++ b/Documentation/ABI/stable/sysfs-module @@ -32,3 +32,21 @@ Description: Note: If the module is built into the kernel, or if the CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, this file will not be present. + +What: /sys/module/MODULENAME/scmversion +Date: November 2020 +KernelVersion: 5.12 +Contact: Will McVicker <willmcvicker@google.com> +Description: This read-only file will appear if modpost was supplied with an + SCM version for the module. It can be enabled with the config + MODULE_SCMVERSION. The SCM version is retrieved by + scripts/setlocalversion, which means that the presence of this + file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM + version that the module was compiled with is returned. The SCM + version is returned in the following format:: + + === + Git: g[a-f0-9]\+(-dirty)\? + Mercurial: hg[a-f0-9]\+(-dirty)\? + Subversion: svn[0-9]\+ + === diff --git a/include/linux/module.h b/include/linux/module.h index 7a0bcb5b1ffc..3b1612193cf9 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -372,6 +372,7 @@ struct module { struct module_attribute *modinfo_attrs; const char *version; const char *srcversion; + const char *scmversion; struct kobject *holders_dir; /* Exported symbols */ diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..3d9dac3c4e8f 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL the version). With this option, such a "srcversion" field will be created for all modules. If unsure, say N. +config MODULE_SCMVERSION + bool "SCM version for modules" + depends on LOCALVERSION_AUTO + help + This enables the module attribute "scmversion" which can be used + by developers to identify the SCM version of a given module, e.g. + git sha1 or hg sha1. The SCM version can be queried by modinfo or + via the sysfs node: /sys/modules/MODULENAME/scmversion. This is + useful when the kernel or kernel modules are updated separately + since that causes the vermagic of the kernel and the module to + differ. + + If unsure, say N. + config MODULE_SIG bool "Module signature verification" select MODULE_SIG_FORMAT diff --git a/kernel/module.c b/kernel/module.c index 4bf30e4b3eaa..d0b359c7e9c9 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ MODINFO_ATTR(version); MODINFO_ATTR(srcversion); +MODINFO_ATTR(scmversion); static char last_unloaded_module[MODULE_NAME_LEN+1]; @@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { &module_uevent, &modinfo_version, &modinfo_srcversion, + &modinfo_scmversion, &modinfo_initstate, &modinfo_coresize, &modinfo_initsize, diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index f54b6ac37ac2..f1126b60adb7 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) input-symdump := vmlinux.symvers output-symdump := Module.symvers +module_srcpath := $(srctree) else @@ -77,6 +78,17 @@ src := $(obj) include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \ $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile) +# Get the external module's source path. KBUILD_EXTMOD could either be an +# absolute path or relative path from $(srctree). This makes sure that we +# aren't using a relative path from a separate working directory (O= or +# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. +# So check the path relative to $(srctree) first. +ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) + module_srcpath := $(srctree)/$(KBUILD_EXTMOD) +else + module_srcpath := $(KBUILD_EXTMOD) +endif + # modpost option for external modules MODPOST += -e @@ -85,6 +97,16 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers endif +ifeq ($(CONFIG_MODULE_SCMVERSION),y) +# Get the SCM version of the module. Sed verifies setlocalversion returns +# a proper revision based on the SCM type, e.g. git, mercurial, or svn. +module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ + sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') +ifneq ($(module_scmversion),) +MODPOST += -v$(module_scmversion) +endif +endif + # modpost options for modules (both in-kernel and external) MODPOST += \ $(addprefix -i ,$(wildcard $(input-symdump))) \ diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index d6c81657d695..489b65bc37de 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -30,6 +30,8 @@ static int have_vmlinux = 0; static int all_versions = 0; /* If we are modposting external module set to 1 */ static int external_module = 0; +#define MODULE_SCMVERSION_SIZE 64 +static char module_scmversion[MODULE_SCMVERSION_SIZE]; /* Only warn about unresolved symbols */ static int warn_unresolved = 0; /* How a symbol is exported */ @@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); } +/** + * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. + * @b: Buffer to append to. + * + * This function fills in the module attribute `scmversion` for the kernel + * module. This is useful for determining a given module's SCM version on + * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. + */ +static void add_scmversion(struct buffer *b) +{ + if (module_scmversion[0] != '\0') + buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); +} + /* Cannot check for assembler */ static void add_retpoline(struct buffer *b) { @@ -2546,7 +2562,7 @@ int main(int argc, char **argv) struct dump_list *dump_read_start = NULL; struct dump_list **dump_read_iter = &dump_read_start; - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { + while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { switch (opt) { case 'e': external_module = 1; @@ -2584,6 +2600,11 @@ int main(int argc, char **argv) case 'd': missing_namespace_deps = optarg; break; + case 'v': + if (!optarg) + fatal("'-v' requires an argument defining the SCM version."); + strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); + break; default: exit(1); } @@ -2630,6 +2651,7 @@ int main(int argc, char **argv) add_depends(&buf, mod); add_moddevtable(&buf, mod); add_srcversion(&buf, mod); + add_scmversion(&buf); sprintf(fname, "%s.mod.c", mod->name); write_if_changed(&buf, fname); -- 2.30.0.284.gd98b1dd5eaa7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-08 0:30 [PATCH v5] modules: introduce the MODULE_SCMVERSION config Will McVicker @ 2021-01-12 15:54 ` Jessica Yu 2021-01-14 18:20 ` Will McVicker 2021-01-17 15:20 ` Masahiro Yamada 2021-01-20 14:25 ` Christoph Hellwig 2 siblings, 1 reply; 9+ messages in thread From: Jessica Yu @ 2021-01-12 15:54 UTC (permalink / raw) To: Will McVicker Cc: Masahiro Yamada, Michal Marek, Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, linux-kernel, linux-kbuild, kernel-team +++ Will McVicker [08/01/21 00:30 +0000]: >Config MODULE_SCMVERSION introduces a new module attribute -- >`scmversion` -- which can be used to identify a given module's SCM >version. This is very useful for developers that update their kernel >independently from their kernel modules or vice-versa since the SCM >version provided by UTS_RELEASE (`uname -r`) will now differ from the >module's vermagic attribute. > >For example, we have a CI setup that tests new kernel changes on the >hikey960 and db845c devices without updating their kernel modules. When >these tests fail, we need to be able to identify the exact device >configuration the test was using. By including MODULE_SCMVERSION, we can >identify the exact kernel and modules' SCM versions for debugging the >failures. > >Additionally, by exposing the SCM version via the sysfs node >/sys/module/MODULENAME/scmversion, one can also verify the SCM versions >of the modules loaded from the initramfs. Currently, modinfo can only >retrieve module attributes from the module's ko on disk and not from the >actual module that is loaded in RAM. > >You can retrieve the SCM version in two ways, > >1) By using modinfo: > > modinfo -F scmversion MODULENAME >2) By module sysfs node: > > cat /sys/module/MODULENAME/scmversion > >Signed-off-by: Will McVicker <willmcvicker@google.com> Hi Will, Thanks for v5, I'm fine with this patch now that we've made it a configurable developer/debug option to supply an scmversion field for in-tree modules (although, this currently works for out-of-tree modules too since we based module_srcpath on KBUILD_EXTMOD in the external module case). I basically see this option as an alternative to CONFIG_MODULE_SRCVERSION_ALL to aid distro developers debug issues when the kernel and in-tree modules are updated separately. In any case, since there was pushback in our earlier discussions, I'd like to ask if there are any remaining objections to this patch. Masahiro, are you fine with the Makefile and modpost changes? Thanks, Jessica >--- > Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++ > include/linux/module.h | 1 + > init/Kconfig | 14 ++++++++++++++ > kernel/module.c | 2 ++ > scripts/Makefile.modpost | 22 ++++++++++++++++++++++ > scripts/mod/modpost.c | 24 +++++++++++++++++++++++- > 6 files changed, 80 insertions(+), 1 deletion(-) > >diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module >index 6272ae5fb366..a75d137e79f4 100644 >--- a/Documentation/ABI/stable/sysfs-module >+++ b/Documentation/ABI/stable/sysfs-module >@@ -32,3 +32,21 @@ Description: > Note: If the module is built into the kernel, or if the > CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, > this file will not be present. >+ >+What: /sys/module/MODULENAME/scmversion >+Date: November 2020 >+KernelVersion: 5.12 >+Contact: Will McVicker <willmcvicker@google.com> >+Description: This read-only file will appear if modpost was supplied with an >+ SCM version for the module. It can be enabled with the config >+ MODULE_SCMVERSION. The SCM version is retrieved by >+ scripts/setlocalversion, which means that the presence of this >+ file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM >+ version that the module was compiled with is returned. The SCM >+ version is returned in the following format:: >+ >+ === >+ Git: g[a-f0-9]\+(-dirty)\? >+ Mercurial: hg[a-f0-9]\+(-dirty)\? >+ Subversion: svn[0-9]\+ >+ === >diff --git a/include/linux/module.h b/include/linux/module.h >index 7a0bcb5b1ffc..3b1612193cf9 100644 >--- a/include/linux/module.h >+++ b/include/linux/module.h >@@ -372,6 +372,7 @@ struct module { > struct module_attribute *modinfo_attrs; > const char *version; > const char *srcversion; >+ const char *scmversion; > struct kobject *holders_dir; > > /* Exported symbols */ >diff --git a/init/Kconfig b/init/Kconfig >index b77c60f8b963..3d9dac3c4e8f 100644 >--- a/init/Kconfig >+++ b/init/Kconfig >@@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL > the version). With this option, such a "srcversion" field > will be created for all modules. If unsure, say N. > >+config MODULE_SCMVERSION >+ bool "SCM version for modules" >+ depends on LOCALVERSION_AUTO >+ help >+ This enables the module attribute "scmversion" which can be used >+ by developers to identify the SCM version of a given module, e.g. >+ git sha1 or hg sha1. The SCM version can be queried by modinfo or >+ via the sysfs node: /sys/modules/MODULENAME/scmversion. This is >+ useful when the kernel or kernel modules are updated separately >+ since that causes the vermagic of the kernel and the module to >+ differ. >+ >+ If unsure, say N. >+ > config MODULE_SIG > bool "Module signature verification" > select MODULE_SIG_FORMAT >diff --git a/kernel/module.c b/kernel/module.c >index 4bf30e4b3eaa..d0b359c7e9c9 100644 >--- a/kernel/module.c >+++ b/kernel/module.c >@@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ > > MODINFO_ATTR(version); > MODINFO_ATTR(srcversion); >+MODINFO_ATTR(scmversion); > > static char last_unloaded_module[MODULE_NAME_LEN+1]; > >@@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { > &module_uevent, > &modinfo_version, > &modinfo_srcversion, >+ &modinfo_scmversion, > &modinfo_initstate, > &modinfo_coresize, > &modinfo_initsize, >diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost >index f54b6ac37ac2..f1126b60adb7 100644 >--- a/scripts/Makefile.modpost >+++ b/scripts/Makefile.modpost >@@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > input-symdump := vmlinux.symvers > output-symdump := Module.symvers >+module_srcpath := $(srctree) > > else > >@@ -77,6 +78,17 @@ src := $(obj) > include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \ > $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile) > >+# Get the external module's source path. KBUILD_EXTMOD could either be an >+# absolute path or relative path from $(srctree). This makes sure that we >+# aren't using a relative path from a separate working directory (O= or >+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. >+# So check the path relative to $(srctree) first. >+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) >+ module_srcpath := $(srctree)/$(KBUILD_EXTMOD) >+else >+ module_srcpath := $(KBUILD_EXTMOD) >+endif >+ > # modpost option for external modules > MODPOST += -e > >@@ -85,6 +97,16 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers > > endif > >+ifeq ($(CONFIG_MODULE_SCMVERSION),y) >+# Get the SCM version of the module. Sed verifies setlocalversion returns >+# a proper revision based on the SCM type, e.g. git, mercurial, or svn. >+module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ >+ sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') >+ifneq ($(module_scmversion),) >+MODPOST += -v$(module_scmversion) >+endif >+endif >+ > # modpost options for modules (both in-kernel and external) > MODPOST += \ > $(addprefix -i ,$(wildcard $(input-symdump))) \ >diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c >index d6c81657d695..489b65bc37de 100644 >--- a/scripts/mod/modpost.c >+++ b/scripts/mod/modpost.c >@@ -30,6 +30,8 @@ static int have_vmlinux = 0; > static int all_versions = 0; > /* If we are modposting external module set to 1 */ > static int external_module = 0; >+#define MODULE_SCMVERSION_SIZE 64 >+static char module_scmversion[MODULE_SCMVERSION_SIZE]; > /* Only warn about unresolved symbols */ > static int warn_unresolved = 0; > /* How a symbol is exported */ >@@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) > buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); > } > >+/** >+ * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. >+ * @b: Buffer to append to. >+ * >+ * This function fills in the module attribute `scmversion` for the kernel >+ * module. This is useful for determining a given module's SCM version on >+ * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. >+ */ >+static void add_scmversion(struct buffer *b) >+{ >+ if (module_scmversion[0] != '\0') >+ buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); >+} >+ > /* Cannot check for assembler */ > static void add_retpoline(struct buffer *b) > { >@@ -2546,7 +2562,7 @@ int main(int argc, char **argv) > struct dump_list *dump_read_start = NULL; > struct dump_list **dump_read_iter = &dump_read_start; > >- while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { >+ while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { > switch (opt) { > case 'e': > external_module = 1; >@@ -2584,6 +2600,11 @@ int main(int argc, char **argv) > case 'd': > missing_namespace_deps = optarg; > break; >+ case 'v': >+ if (!optarg) >+ fatal("'-v' requires an argument defining the SCM version."); >+ strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); >+ break; > default: > exit(1); > } >@@ -2630,6 +2651,7 @@ int main(int argc, char **argv) > add_depends(&buf, mod); > add_moddevtable(&buf, mod); > add_srcversion(&buf, mod); >+ add_scmversion(&buf); > > sprintf(fname, "%s.mod.c", mod->name); > write_if_changed(&buf, fname); >-- >2.30.0.284.gd98b1dd5eaa7-goog > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-12 15:54 ` Jessica Yu @ 2021-01-14 18:20 ` Will McVicker 0 siblings, 0 replies; 9+ messages in thread From: Will McVicker @ 2021-01-14 18:20 UTC (permalink / raw) To: Jessica Yu Cc: Masahiro Yamada, Michal Marek, Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, linux-kernel, linux-kbuild, kernel-team Thanks Jessica for the reviews! --Will On Tue, Jan 12, 2021 at 7:55 AM Jessica Yu <jeyu@kernel.org> wrote: > > +++ Will McVicker [08/01/21 00:30 +0000]: > >Config MODULE_SCMVERSION introduces a new module attribute -- > >`scmversion` -- which can be used to identify a given module's SCM > >version. This is very useful for developers that update their kernel > >independently from their kernel modules or vice-versa since the SCM > >version provided by UTS_RELEASE (`uname -r`) will now differ from the > >module's vermagic attribute. > > > >For example, we have a CI setup that tests new kernel changes on the > >hikey960 and db845c devices without updating their kernel modules. When > >these tests fail, we need to be able to identify the exact device > >configuration the test was using. By including MODULE_SCMVERSION, we can > >identify the exact kernel and modules' SCM versions for debugging the > >failures. > > > >Additionally, by exposing the SCM version via the sysfs node > >/sys/module/MODULENAME/scmversion, one can also verify the SCM versions > >of the modules loaded from the initramfs. Currently, modinfo can only > >retrieve module attributes from the module's ko on disk and not from the > >actual module that is loaded in RAM. > > > >You can retrieve the SCM version in two ways, > > > >1) By using modinfo: > > > modinfo -F scmversion MODULENAME > >2) By module sysfs node: > > > cat /sys/module/MODULENAME/scmversion > > > >Signed-off-by: Will McVicker <willmcvicker@google.com> > > Hi Will, > > Thanks for v5, I'm fine with this patch now that we've made it a > configurable developer/debug option to supply an scmversion field for > in-tree modules (although, this currently works for out-of-tree > modules too since we based module_srcpath on KBUILD_EXTMOD in the > external module case). I basically see this option as an alternative > to CONFIG_MODULE_SRCVERSION_ALL to aid distro developers debug issues > when the kernel and in-tree modules are updated separately. > > In any case, since there was pushback in our earlier discussions, I'd > like to ask if there are any remaining objections to this patch. > > Masahiro, are you fine with the Makefile and modpost changes? > > Thanks, > > Jessica > > >--- > > Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++ > > include/linux/module.h | 1 + > > init/Kconfig | 14 ++++++++++++++ > > kernel/module.c | 2 ++ > > scripts/Makefile.modpost | 22 ++++++++++++++++++++++ > > scripts/mod/modpost.c | 24 +++++++++++++++++++++++- > > 6 files changed, 80 insertions(+), 1 deletion(-) > > > >diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module > >index 6272ae5fb366..a75d137e79f4 100644 > >--- a/Documentation/ABI/stable/sysfs-module > >+++ b/Documentation/ABI/stable/sysfs-module > >@@ -32,3 +32,21 @@ Description: > > Note: If the module is built into the kernel, or if the > > CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, > > this file will not be present. > >+ > >+What: /sys/module/MODULENAME/scmversion > >+Date: November 2020 > >+KernelVersion: 5.12 > >+Contact: Will McVicker <willmcvicker@google.com> > >+Description: This read-only file will appear if modpost was supplied with an > >+ SCM version for the module. It can be enabled with the config > >+ MODULE_SCMVERSION. The SCM version is retrieved by > >+ scripts/setlocalversion, which means that the presence of this > >+ file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM > >+ version that the module was compiled with is returned. The SCM > >+ version is returned in the following format:: > >+ > >+ === > >+ Git: g[a-f0-9]\+(-dirty)\? > >+ Mercurial: hg[a-f0-9]\+(-dirty)\? > >+ Subversion: svn[0-9]\+ > >+ === > >diff --git a/include/linux/module.h b/include/linux/module.h > >index 7a0bcb5b1ffc..3b1612193cf9 100644 > >--- a/include/linux/module.h > >+++ b/include/linux/module.h > >@@ -372,6 +372,7 @@ struct module { > > struct module_attribute *modinfo_attrs; > > const char *version; > > const char *srcversion; > >+ const char *scmversion; > > struct kobject *holders_dir; > > > > /* Exported symbols */ > >diff --git a/init/Kconfig b/init/Kconfig > >index b77c60f8b963..3d9dac3c4e8f 100644 > >--- a/init/Kconfig > >+++ b/init/Kconfig > >@@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL > > the version). With this option, such a "srcversion" field > > will be created for all modules. If unsure, say N. > > > >+config MODULE_SCMVERSION > >+ bool "SCM version for modules" > >+ depends on LOCALVERSION_AUTO > >+ help > >+ This enables the module attribute "scmversion" which can be used > >+ by developers to identify the SCM version of a given module, e.g. > >+ git sha1 or hg sha1. The SCM version can be queried by modinfo or > >+ via the sysfs node: /sys/modules/MODULENAME/scmversion. This is > >+ useful when the kernel or kernel modules are updated separately > >+ since that causes the vermagic of the kernel and the module to > >+ differ. > >+ > >+ If unsure, say N. > >+ > > config MODULE_SIG > > bool "Module signature verification" > > select MODULE_SIG_FORMAT > >diff --git a/kernel/module.c b/kernel/module.c > >index 4bf30e4b3eaa..d0b359c7e9c9 100644 > >--- a/kernel/module.c > >+++ b/kernel/module.c > >@@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ > > > > MODINFO_ATTR(version); > > MODINFO_ATTR(srcversion); > >+MODINFO_ATTR(scmversion); > > > > static char last_unloaded_module[MODULE_NAME_LEN+1]; > > > >@@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { > > &module_uevent, > > &modinfo_version, > > &modinfo_srcversion, > >+ &modinfo_scmversion, > > &modinfo_initstate, > > &modinfo_coresize, > > &modinfo_initsize, > >diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > >index f54b6ac37ac2..f1126b60adb7 100644 > >--- a/scripts/Makefile.modpost > >+++ b/scripts/Makefile.modpost > >@@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > > > input-symdump := vmlinux.symvers > > output-symdump := Module.symvers > >+module_srcpath := $(srctree) > > > > else > > > >@@ -77,6 +78,17 @@ src := $(obj) > > include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \ > > $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile) > > > >+# Get the external module's source path. KBUILD_EXTMOD could either be an > >+# absolute path or relative path from $(srctree). This makes sure that we > >+# aren't using a relative path from a separate working directory (O= or > >+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. > >+# So check the path relative to $(srctree) first. > >+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) > >+ module_srcpath := $(srctree)/$(KBUILD_EXTMOD) > >+else > >+ module_srcpath := $(KBUILD_EXTMOD) > >+endif > >+ > > # modpost option for external modules > > MODPOST += -e > > > >@@ -85,6 +97,16 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers > > > > endif > > > >+ifeq ($(CONFIG_MODULE_SCMVERSION),y) > >+# Get the SCM version of the module. Sed verifies setlocalversion returns > >+# a proper revision based on the SCM type, e.g. git, mercurial, or svn. > >+module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ > >+ sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') > >+ifneq ($(module_scmversion),) > >+MODPOST += -v$(module_scmversion) > >+endif > >+endif > >+ > > # modpost options for modules (both in-kernel and external) > > MODPOST += \ > > $(addprefix -i ,$(wildcard $(input-symdump))) \ > >diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > >index d6c81657d695..489b65bc37de 100644 > >--- a/scripts/mod/modpost.c > >+++ b/scripts/mod/modpost.c > >@@ -30,6 +30,8 @@ static int have_vmlinux = 0; > > static int all_versions = 0; > > /* If we are modposting external module set to 1 */ > > static int external_module = 0; > >+#define MODULE_SCMVERSION_SIZE 64 > >+static char module_scmversion[MODULE_SCMVERSION_SIZE]; > > /* Only warn about unresolved symbols */ > > static int warn_unresolved = 0; > > /* How a symbol is exported */ > >@@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) > > buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); > > } > > > >+/** > >+ * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. > >+ * @b: Buffer to append to. > >+ * > >+ * This function fills in the module attribute `scmversion` for the kernel > >+ * module. This is useful for determining a given module's SCM version on > >+ * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. > >+ */ > >+static void add_scmversion(struct buffer *b) > >+{ > >+ if (module_scmversion[0] != '\0') > >+ buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); > >+} > >+ > > /* Cannot check for assembler */ > > static void add_retpoline(struct buffer *b) > > { > >@@ -2546,7 +2562,7 @@ int main(int argc, char **argv) > > struct dump_list *dump_read_start = NULL; > > struct dump_list **dump_read_iter = &dump_read_start; > > > >- while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { > >+ while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { > > switch (opt) { > > case 'e': > > external_module = 1; > >@@ -2584,6 +2600,11 @@ int main(int argc, char **argv) > > case 'd': > > missing_namespace_deps = optarg; > > break; > >+ case 'v': > >+ if (!optarg) > >+ fatal("'-v' requires an argument defining the SCM version."); > >+ strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); > >+ break; > > default: > > exit(1); > > } > >@@ -2630,6 +2651,7 @@ int main(int argc, char **argv) > > add_depends(&buf, mod); > > add_moddevtable(&buf, mod); > > add_srcversion(&buf, mod); > >+ add_scmversion(&buf); > > > > sprintf(fname, "%s.mod.c", mod->name); > > write_if_changed(&buf, fname); > >-- > >2.30.0.284.gd98b1dd5eaa7-goog > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-08 0:30 [PATCH v5] modules: introduce the MODULE_SCMVERSION config Will McVicker 2021-01-12 15:54 ` Jessica Yu @ 2021-01-17 15:20 ` Masahiro Yamada 2021-01-19 21:48 ` Will McVicker 2021-01-20 14:25 ` Christoph Hellwig 2 siblings, 1 reply; 9+ messages in thread From: Masahiro Yamada @ 2021-01-17 15:20 UTC (permalink / raw) To: Will McVicker Cc: Jessica Yu, Michal Marek, Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, Linux Kernel Mailing List, Linux Kbuild mailing list, Cc: Android Kernel On Fri, Jan 8, 2021 at 9:30 AM Will McVicker <willmcvicker@google.com> wrote: > > Config MODULE_SCMVERSION introduces a new module attribute -- > `scmversion` -- which can be used to identify a given module's SCM > version. This is very useful for developers that update their kernel > independently from their kernel modules or vice-versa since the SCM > version provided by UTS_RELEASE (`uname -r`) will now differ from the > module's vermagic attribute. > > For example, we have a CI setup that tests new kernel changes on the > hikey960 and db845c devices without updating their kernel modules. When > these tests fail, we need to be able to identify the exact device > configuration the test was using. By including MODULE_SCMVERSION, we can > identify the exact kernel and modules' SCM versions for debugging the > failures. > > Additionally, by exposing the SCM version via the sysfs node > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions > of the modules loaded from the initramfs. Currently, modinfo can only > retrieve module attributes from the module's ko on disk and not from the > actual module that is loaded in RAM. > > You can retrieve the SCM version in two ways, > > 1) By using modinfo: > > modinfo -F scmversion MODULENAME > 2) By module sysfs node: > > cat /sys/module/MODULENAME/scmversion > > Signed-off-by: Will McVicker <willmcvicker@google.com> > --- > Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++ > include/linux/module.h | 1 + > init/Kconfig | 14 ++++++++++++++ > kernel/module.c | 2 ++ > scripts/Makefile.modpost | 22 ++++++++++++++++++++++ > scripts/mod/modpost.c | 24 +++++++++++++++++++++++- > 6 files changed, 80 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module > index 6272ae5fb366..a75d137e79f4 100644 > --- a/Documentation/ABI/stable/sysfs-module > +++ b/Documentation/ABI/stable/sysfs-module > @@ -32,3 +32,21 @@ Description: > Note: If the module is built into the kernel, or if the > CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, > this file will not be present. > + > +What: /sys/module/MODULENAME/scmversion > +Date: November 2020 > +KernelVersion: 5.12 > +Contact: Will McVicker <willmcvicker@google.com> > +Description: This read-only file will appear if modpost was supplied with an > + SCM version for the module. It can be enabled with the config > + MODULE_SCMVERSION. The SCM version is retrieved by > + scripts/setlocalversion, which means that the presence of this > + file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM > + version that the module was compiled with is returned. The SCM > + version is returned in the following format:: > + > + === > + Git: g[a-f0-9]\+(-dirty)\? > + Mercurial: hg[a-f0-9]\+(-dirty)\? > + Subversion: svn[0-9]\+ > + === > diff --git a/include/linux/module.h b/include/linux/module.h > index 7a0bcb5b1ffc..3b1612193cf9 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -372,6 +372,7 @@ struct module { > struct module_attribute *modinfo_attrs; > const char *version; > const char *srcversion; > + const char *scmversion; > struct kobject *holders_dir; > > /* Exported symbols */ > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..3d9dac3c4e8f 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL > the version). With this option, such a "srcversion" field > will be created for all modules. If unsure, say N. > > +config MODULE_SCMVERSION > + bool "SCM version for modules" > + depends on LOCALVERSION_AUTO > + help > + This enables the module attribute "scmversion" which can be used > + by developers to identify the SCM version of a given module, e.g. > + git sha1 or hg sha1. The SCM version can be queried by modinfo or > + via the sysfs node: /sys/modules/MODULENAME/scmversion. This is > + useful when the kernel or kernel modules are updated separately > + since that causes the vermagic of the kernel and the module to > + differ. > + > + If unsure, say N. > + > config MODULE_SIG > bool "Module signature verification" > select MODULE_SIG_FORMAT > diff --git a/kernel/module.c b/kernel/module.c > index 4bf30e4b3eaa..d0b359c7e9c9 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ > > MODINFO_ATTR(version); > MODINFO_ATTR(srcversion); > +MODINFO_ATTR(scmversion); > > static char last_unloaded_module[MODULE_NAME_LEN+1]; > > @@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { > &module_uevent, > &modinfo_version, > &modinfo_srcversion, > + &modinfo_scmversion, > &modinfo_initstate, > &modinfo_coresize, > &modinfo_initsize, > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > index f54b6ac37ac2..f1126b60adb7 100644 > --- a/scripts/Makefile.modpost > +++ b/scripts/Makefile.modpost > @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > input-symdump := vmlinux.symvers > output-symdump := Module.symvers > +module_srcpath := $(srctree) > > else > > @@ -77,6 +78,17 @@ src := $(obj) > include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \ > $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile) > > +# Get the external module's source path. KBUILD_EXTMOD could either be an > +# absolute path or relative path from $(srctree). No. KBUILD_EXTMOD could either be an absolute or relative path from $(objtree). > This makes sure that we > +# aren't using a relative path from a separate working directory (O= or > +# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. > +# So check the path relative to $(srctree) first. > +ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) > + module_srcpath := $(srctree)/$(KBUILD_EXTMOD) > +else > + module_srcpath := $(KBUILD_EXTMOD) > +endif > + This hunk is pointless in my view. The code should be much simpler, as follows: --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) input-symdump := vmlinux.symvers output-symdump := Module.symvers +module_srcpath := $(srctree) else @@ -82,9 +83,20 @@ MODPOST += -e input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS) output-symdump := $(KBUILD_EXTMOD)/Module.symvers +module_srcpath := $(KBUILD_EXTMOD) endif > # modpost option for external modules > MODPOST += -e > > @@ -85,6 +97,16 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers > > endif > > +ifeq ($(CONFIG_MODULE_SCMVERSION),y) > +# Get the SCM version of the module. Sed verifies setlocalversion returns > +# a proper revision based on the SCM type, e.g. git, mercurial, or svn. > +module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ > + sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') > +ifneq ($(module_scmversion),) > +MODPOST += -v$(module_scmversion) A space after -v for consistency. > +endif > +endif > + > # modpost options for modules (both in-kernel and external) > MODPOST += \ > $(addprefix -i ,$(wildcard $(input-symdump))) \ > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index d6c81657d695..489b65bc37de 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -30,6 +30,8 @@ static int have_vmlinux = 0; > static int all_versions = 0; > /* If we are modposting external module set to 1 */ > static int external_module = 0; > +#define MODULE_SCMVERSION_SIZE 64 > +static char module_scmversion[MODULE_SCMVERSION_SIZE]; > /* Only warn about unresolved symbols */ > static int warn_unresolved = 0; > /* How a symbol is exported */ > @@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) > buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); > } > > +/** > + * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. > + * @b: Buffer to append to. > + * > + * This function fills in the module attribute `scmversion` for the kernel > + * module. This is useful for determining a given module's SCM version on > + * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. > + */ > +static void add_scmversion(struct buffer *b) > +{ > + if (module_scmversion[0] != '\0') > + buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); > +} > + > /* Cannot check for assembler */ > static void add_retpoline(struct buffer *b) > { > @@ -2546,7 +2562,7 @@ int main(int argc, char **argv) > struct dump_list *dump_read_start = NULL; > struct dump_list **dump_read_iter = &dump_read_start; > > - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { > + while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { > switch (opt) { > case 'e': > external_module = 1; > @@ -2584,6 +2600,11 @@ int main(int argc, char **argv) > case 'd': > missing_namespace_deps = optarg; > break; > + case 'v': > + if (!optarg) > + fatal("'-v' requires an argument defining the SCM version."); The existing -i, -T, -o, -d options take an argument as well. Why don't they have such a check as you added? The answer is because getopt() checks it. I applied your patch, and ran modpost directly with -v at the end of the command line. masahiro@oscar:~/workspace/linux$ ./scripts/mod/modpost -v ./scripts/mod/modpost: option requires an argument -- 'v' I see the error message from getopt() instead of your hand-made error check. > + strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); > + break; > default: > exit(1); > } > @@ -2630,6 +2651,7 @@ int main(int argc, char **argv) > add_depends(&buf, mod); > add_moddevtable(&buf, mod); > add_srcversion(&buf, mod); > + add_scmversion(&buf); > > sprintf(fname, "%s.mod.c", mod->name); > write_if_changed(&buf, fname); > -- > 2.30.0.284.gd98b1dd5eaa7-goog > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-17 15:20 ` Masahiro Yamada @ 2021-01-19 21:48 ` Will McVicker 2021-01-20 0:18 ` Will McVicker 0 siblings, 1 reply; 9+ messages in thread From: Will McVicker @ 2021-01-19 21:48 UTC (permalink / raw) To: Masahiro Yamada Cc: Jessica Yu, Michal Marek, Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, Linux Kernel Mailing List, Linux Kbuild mailing list, Cc: Android Kernel Thanks for the review Masahiro! I'll upload v6 shortly. --Will On Sun, Jan 17, 2021 at 7:21 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Fri, Jan 8, 2021 at 9:30 AM Will McVicker <willmcvicker@google.com> wrote: > > > > Config MODULE_SCMVERSION introduces a new module attribute -- > > `scmversion` -- which can be used to identify a given module's SCM > > version. This is very useful for developers that update their kernel > > independently from their kernel modules or vice-versa since the SCM > > version provided by UTS_RELEASE (`uname -r`) will now differ from the > > module's vermagic attribute. > > > > For example, we have a CI setup that tests new kernel changes on the > > hikey960 and db845c devices without updating their kernel modules. When > > these tests fail, we need to be able to identify the exact device > > configuration the test was using. By including MODULE_SCMVERSION, we can > > identify the exact kernel and modules' SCM versions for debugging the > > failures. > > > > Additionally, by exposing the SCM version via the sysfs node > > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions > > of the modules loaded from the initramfs. Currently, modinfo can only > > retrieve module attributes from the module's ko on disk and not from the > > actual module that is loaded in RAM. > > > > You can retrieve the SCM version in two ways, > > > > 1) By using modinfo: > > > modinfo -F scmversion MODULENAME > > 2) By module sysfs node: > > > cat /sys/module/MODULENAME/scmversion > > > > Signed-off-by: Will McVicker <willmcvicker@google.com> > > --- > > Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++ > > include/linux/module.h | 1 + > > init/Kconfig | 14 ++++++++++++++ > > kernel/module.c | 2 ++ > > scripts/Makefile.modpost | 22 ++++++++++++++++++++++ > > scripts/mod/modpost.c | 24 +++++++++++++++++++++++- > > 6 files changed, 80 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module > > index 6272ae5fb366..a75d137e79f4 100644 > > --- a/Documentation/ABI/stable/sysfs-module > > +++ b/Documentation/ABI/stable/sysfs-module > > @@ -32,3 +32,21 @@ Description: > > Note: If the module is built into the kernel, or if the > > CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, > > this file will not be present. > > + > > +What: /sys/module/MODULENAME/scmversion > > +Date: November 2020 > > +KernelVersion: 5.12 > > +Contact: Will McVicker <willmcvicker@google.com> > > +Description: This read-only file will appear if modpost was supplied with an > > + SCM version for the module. It can be enabled with the config > > + MODULE_SCMVERSION. The SCM version is retrieved by > > + scripts/setlocalversion, which means that the presence of this > > + file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM > > + version that the module was compiled with is returned. The SCM > > + version is returned in the following format:: > > + > > + === > > + Git: g[a-f0-9]\+(-dirty)\? > > + Mercurial: hg[a-f0-9]\+(-dirty)\? > > + Subversion: svn[0-9]\+ > > + === > > diff --git a/include/linux/module.h b/include/linux/module.h > > index 7a0bcb5b1ffc..3b1612193cf9 100644 > > --- a/include/linux/module.h > > +++ b/include/linux/module.h > > @@ -372,6 +372,7 @@ struct module { > > struct module_attribute *modinfo_attrs; > > const char *version; > > const char *srcversion; > > + const char *scmversion; > > struct kobject *holders_dir; > > > > /* Exported symbols */ > > diff --git a/init/Kconfig b/init/Kconfig > > index b77c60f8b963..3d9dac3c4e8f 100644 > > --- a/init/Kconfig > > +++ b/init/Kconfig > > @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL > > the version). With this option, such a "srcversion" field > > will be created for all modules. If unsure, say N. > > > > +config MODULE_SCMVERSION > > + bool "SCM version for modules" > > + depends on LOCALVERSION_AUTO > > + help > > + This enables the module attribute "scmversion" which can be used > > + by developers to identify the SCM version of a given module, e.g. > > + git sha1 or hg sha1. The SCM version can be queried by modinfo or > > + via the sysfs node: /sys/modules/MODULENAME/scmversion. This is > > + useful when the kernel or kernel modules are updated separately > > + since that causes the vermagic of the kernel and the module to > > + differ. > > + > > + If unsure, say N. > > + > > config MODULE_SIG > > bool "Module signature verification" > > select MODULE_SIG_FORMAT > > diff --git a/kernel/module.c b/kernel/module.c > > index 4bf30e4b3eaa..d0b359c7e9c9 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ > > > > MODINFO_ATTR(version); > > MODINFO_ATTR(srcversion); > > +MODINFO_ATTR(scmversion); > > > > static char last_unloaded_module[MODULE_NAME_LEN+1]; > > > > @@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { > > &module_uevent, > > &modinfo_version, > > &modinfo_srcversion, > > + &modinfo_scmversion, > > &modinfo_initstate, > > &modinfo_coresize, > > &modinfo_initsize, > > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > > index f54b6ac37ac2..f1126b60adb7 100644 > > --- a/scripts/Makefile.modpost > > +++ b/scripts/Makefile.modpost > > @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > > > input-symdump := vmlinux.symvers > > output-symdump := Module.symvers > > +module_srcpath := $(srctree) > > > > else > > > > @@ -77,6 +78,17 @@ src := $(obj) > > include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \ > > $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile) > > > > +# Get the external module's source path. KBUILD_EXTMOD could either be an > > +# absolute path or relative path from $(srctree). > > > No. > KBUILD_EXTMOD could either be an absolute or relative path from $(objtree). > > > > > This makes sure that we > > +# aren't using a relative path from a separate working directory (O= or > > +# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. > > +# So check the path relative to $(srctree) first. > > +ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) > > + module_srcpath := $(srctree)/$(KBUILD_EXTMOD) > > +else > > + module_srcpath := $(KBUILD_EXTMOD) > > +endif > > + > > This hunk is pointless in my view. > > The code should be much simpler, as follows: > > > > --- a/scripts/Makefile.modpost > +++ b/scripts/Makefile.modpost > @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > input-symdump := vmlinux.symvers > output-symdump := Module.symvers > +module_srcpath := $(srctree) > > else > > @@ -82,9 +83,20 @@ MODPOST += -e > > input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS) > output-symdump := $(KBUILD_EXTMOD)/Module.symvers > +module_srcpath := $(KBUILD_EXTMOD) > > endif > > > > > > > > > > # modpost option for external modules > > MODPOST += -e > > > > @@ -85,6 +97,16 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers > > > > endif > > > > +ifeq ($(CONFIG_MODULE_SCMVERSION),y) > > +# Get the SCM version of the module. Sed verifies setlocalversion returns > > +# a proper revision based on the SCM type, e.g. git, mercurial, or svn. > > +module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ > > + sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') > > +ifneq ($(module_scmversion),) > > +MODPOST += -v$(module_scmversion) > > > A space after -v for consistency. > > > > > +endif > > +endif > > + > > # modpost options for modules (both in-kernel and external) > > MODPOST += \ > > $(addprefix -i ,$(wildcard $(input-symdump))) \ > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > > index d6c81657d695..489b65bc37de 100644 > > --- a/scripts/mod/modpost.c > > +++ b/scripts/mod/modpost.c > > @@ -30,6 +30,8 @@ static int have_vmlinux = 0; > > static int all_versions = 0; > > /* If we are modposting external module set to 1 */ > > static int external_module = 0; > > +#define MODULE_SCMVERSION_SIZE 64 > > +static char module_scmversion[MODULE_SCMVERSION_SIZE]; > > /* Only warn about unresolved symbols */ > > static int warn_unresolved = 0; > > /* How a symbol is exported */ > > @@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) > > buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); > > } > > > > +/** > > + * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. > > + * @b: Buffer to append to. > > + * > > + * This function fills in the module attribute `scmversion` for the kernel > > + * module. This is useful for determining a given module's SCM version on > > + * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. > > + */ > > +static void add_scmversion(struct buffer *b) > > +{ > > + if (module_scmversion[0] != '\0') > > + buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); > > +} > > + > > /* Cannot check for assembler */ > > static void add_retpoline(struct buffer *b) > > { > > @@ -2546,7 +2562,7 @@ int main(int argc, char **argv) > > struct dump_list *dump_read_start = NULL; > > struct dump_list **dump_read_iter = &dump_read_start; > > > > - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { > > + while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { > > switch (opt) { > > case 'e': > > external_module = 1; > > @@ -2584,6 +2600,11 @@ int main(int argc, char **argv) > > case 'd': > > missing_namespace_deps = optarg; > > break; > > + case 'v': > > + if (!optarg) > > + fatal("'-v' requires an argument defining the SCM version."); > > > The existing -i, -T, -o, -d options take an argument as well. > Why don't they have such a check as you added? > The answer is because getopt() checks it. > > > I applied your patch, and ran modpost directly > with -v at the end of the command line. > > > masahiro@oscar:~/workspace/linux$ ./scripts/mod/modpost -v > ./scripts/mod/modpost: option requires an argument -- 'v' > > > I see the error message from getopt() instead of > your hand-made error check. > > > > > > > > > > + strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); > > + break; > > default: > > exit(1); > > } > > @@ -2630,6 +2651,7 @@ int main(int argc, char **argv) > > add_depends(&buf, mod); > > add_moddevtable(&buf, mod); > > add_srcversion(&buf, mod); > > + add_scmversion(&buf); > > > > sprintf(fname, "%s.mod.c", mod->name); > > write_if_changed(&buf, fname); > > -- > > 2.30.0.284.gd98b1dd5eaa7-goog > > > > > -- > Best Regards > Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-19 21:48 ` Will McVicker @ 2021-01-20 0:18 ` Will McVicker 2021-01-20 19:41 ` Masahiro Yamada 0 siblings, 1 reply; 9+ messages in thread From: Will McVicker @ 2021-01-20 0:18 UTC (permalink / raw) To: Masahiro Yamada Cc: Jessica Yu, Michal Marek, Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, Linux Kernel Mailing List, Linux Kbuild mailing list, Cc: Android Kernel Hi Masahiro, After testing your suggestions for module_srcpath, I remembered why I needed this: +ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) + module_srcpath := $(srctree)/$(KBUILD_EXTMOD) +else + module_srcpath := $(KBUILD_EXTMOD) +endif Basically KBUILD_EXTMOD actually has two uses when defined as a relative path: (1) defines the path to the external module's source relative to $(srctree) (2) defines the path to the external module's build artifacts relative to $(objtree) Since setlocalversion is run from $(objtree), we need to use $(srctree)/$(KBUILD_EXTMOD) when it's a relative path. Just to note, if I do just use KBUILD_EXTMOD, then the script setlocalversion fails to find the srctree and returns an empty string. Please correct me if I'm wrong. I messed around with this by adding many prints to Makefile.modpost and setlocalversion to make sure everything was defined as expected. Thanks, Will On Tue, Jan 19, 2021 at 1:48 PM Will McVicker <willmcvicker@google.com> wrote: > > Thanks for the review Masahiro! I'll upload v6 shortly. > > --Will > > On Sun, Jan 17, 2021 at 7:21 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Fri, Jan 8, 2021 at 9:30 AM Will McVicker <willmcvicker@google.com> wrote: > > > > > > Config MODULE_SCMVERSION introduces a new module attribute -- > > > `scmversion` -- which can be used to identify a given module's SCM > > > version. This is very useful for developers that update their kernel > > > independently from their kernel modules or vice-versa since the SCM > > > version provided by UTS_RELEASE (`uname -r`) will now differ from the > > > module's vermagic attribute. > > > > > > For example, we have a CI setup that tests new kernel changes on the > > > hikey960 and db845c devices without updating their kernel modules. When > > > these tests fail, we need to be able to identify the exact device > > > configuration the test was using. By including MODULE_SCMVERSION, we can > > > identify the exact kernel and modules' SCM versions for debugging the > > > failures. > > > > > > Additionally, by exposing the SCM version via the sysfs node > > > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions > > > of the modules loaded from the initramfs. Currently, modinfo can only > > > retrieve module attributes from the module's ko on disk and not from the > > > actual module that is loaded in RAM. > > > > > > You can retrieve the SCM version in two ways, > > > > > > 1) By using modinfo: > > > > modinfo -F scmversion MODULENAME > > > 2) By module sysfs node: > > > > cat /sys/module/MODULENAME/scmversion > > > > > > Signed-off-by: Will McVicker <willmcvicker@google.com> > > > --- > > > Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++ > > > include/linux/module.h | 1 + > > > init/Kconfig | 14 ++++++++++++++ > > > kernel/module.c | 2 ++ > > > scripts/Makefile.modpost | 22 ++++++++++++++++++++++ > > > scripts/mod/modpost.c | 24 +++++++++++++++++++++++- > > > 6 files changed, 80 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module > > > index 6272ae5fb366..a75d137e79f4 100644 > > > --- a/Documentation/ABI/stable/sysfs-module > > > +++ b/Documentation/ABI/stable/sysfs-module > > > @@ -32,3 +32,21 @@ Description: > > > Note: If the module is built into the kernel, or if the > > > CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, > > > this file will not be present. > > > + > > > +What: /sys/module/MODULENAME/scmversion > > > +Date: November 2020 > > > +KernelVersion: 5.12 > > > +Contact: Will McVicker <willmcvicker@google.com> > > > +Description: This read-only file will appear if modpost was supplied with an > > > + SCM version for the module. It can be enabled with the config > > > + MODULE_SCMVERSION. The SCM version is retrieved by > > > + scripts/setlocalversion, which means that the presence of this > > > + file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM > > > + version that the module was compiled with is returned. The SCM > > > + version is returned in the following format:: > > > + > > > + === > > > + Git: g[a-f0-9]\+(-dirty)\? > > > + Mercurial: hg[a-f0-9]\+(-dirty)\? > > > + Subversion: svn[0-9]\+ > > > + === > > > diff --git a/include/linux/module.h b/include/linux/module.h > > > index 7a0bcb5b1ffc..3b1612193cf9 100644 > > > --- a/include/linux/module.h > > > +++ b/include/linux/module.h > > > @@ -372,6 +372,7 @@ struct module { > > > struct module_attribute *modinfo_attrs; > > > const char *version; > > > const char *srcversion; > > > + const char *scmversion; > > > struct kobject *holders_dir; > > > > > > /* Exported symbols */ > > > diff --git a/init/Kconfig b/init/Kconfig > > > index b77c60f8b963..3d9dac3c4e8f 100644 > > > --- a/init/Kconfig > > > +++ b/init/Kconfig > > > @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL > > > the version). With this option, such a "srcversion" field > > > will be created for all modules. If unsure, say N. > > > > > > +config MODULE_SCMVERSION > > > + bool "SCM version for modules" > > > + depends on LOCALVERSION_AUTO > > > + help > > > + This enables the module attribute "scmversion" which can be used > > > + by developers to identify the SCM version of a given module, e.g. > > > + git sha1 or hg sha1. The SCM version can be queried by modinfo or > > > + via the sysfs node: /sys/modules/MODULENAME/scmversion. This is > > > + useful when the kernel or kernel modules are updated separately > > > + since that causes the vermagic of the kernel and the module to > > > + differ. > > > + > > > + If unsure, say N. > > > + > > > config MODULE_SIG > > > bool "Module signature verification" > > > select MODULE_SIG_FORMAT > > > diff --git a/kernel/module.c b/kernel/module.c > > > index 4bf30e4b3eaa..d0b359c7e9c9 100644 > > > --- a/kernel/module.c > > > +++ b/kernel/module.c > > > @@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ > > > > > > MODINFO_ATTR(version); > > > MODINFO_ATTR(srcversion); > > > +MODINFO_ATTR(scmversion); > > > > > > static char last_unloaded_module[MODULE_NAME_LEN+1]; > > > > > > @@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { > > > &module_uevent, > > > &modinfo_version, > > > &modinfo_srcversion, > > > + &modinfo_scmversion, > > > &modinfo_initstate, > > > &modinfo_coresize, > > > &modinfo_initsize, > > > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > > > index f54b6ac37ac2..f1126b60adb7 100644 > > > --- a/scripts/Makefile.modpost > > > +++ b/scripts/Makefile.modpost > > > @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > > > > > input-symdump := vmlinux.symvers > > > output-symdump := Module.symvers > > > +module_srcpath := $(srctree) > > > > > > else > > > > > > @@ -77,6 +78,17 @@ src := $(obj) > > > include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \ > > > $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile) > > > > > > +# Get the external module's source path. KBUILD_EXTMOD could either be an > > > +# absolute path or relative path from $(srctree). > > > > > > No. > > KBUILD_EXTMOD could either be an absolute or relative path from $(objtree). > > > > > > > > > This makes sure that we > > > +# aren't using a relative path from a separate working directory (O= or > > > +# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. > > > +# So check the path relative to $(srctree) first. > > > +ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) > > > + module_srcpath := $(srctree)/$(KBUILD_EXTMOD) > > > +else > > > + module_srcpath := $(KBUILD_EXTMOD) > > > +endif > > > + > > > > This hunk is pointless in my view. > > > > The code should be much simpler, as follows: > > > > > > > > --- a/scripts/Makefile.modpost > > +++ b/scripts/Makefile.modpost > > @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > > > input-symdump := vmlinux.symvers > > output-symdump := Module.symvers > > +module_srcpath := $(srctree) > > > > else > > > > @@ -82,9 +83,20 @@ MODPOST += -e > > > > input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS) > > output-symdump := $(KBUILD_EXTMOD)/Module.symvers > > +module_srcpath := $(KBUILD_EXTMOD) > > > > endif > > > > > > > > > > > > > > > > > > > # modpost option for external modules > > > MODPOST += -e > > > > > > @@ -85,6 +97,16 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers > > > > > > endif > > > > > > +ifeq ($(CONFIG_MODULE_SCMVERSION),y) > > > +# Get the SCM version of the module. Sed verifies setlocalversion returns > > > +# a proper revision based on the SCM type, e.g. git, mercurial, or svn. > > > +module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ > > > + sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') > > > +ifneq ($(module_scmversion),) > > > +MODPOST += -v$(module_scmversion) > > > > > > A space after -v for consistency. > > > > > > > > > +endif > > > +endif > > > + > > > # modpost options for modules (both in-kernel and external) > > > MODPOST += \ > > > $(addprefix -i ,$(wildcard $(input-symdump))) \ > > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > > > index d6c81657d695..489b65bc37de 100644 > > > --- a/scripts/mod/modpost.c > > > +++ b/scripts/mod/modpost.c > > > @@ -30,6 +30,8 @@ static int have_vmlinux = 0; > > > static int all_versions = 0; > > > /* If we are modposting external module set to 1 */ > > > static int external_module = 0; > > > +#define MODULE_SCMVERSION_SIZE 64 > > > +static char module_scmversion[MODULE_SCMVERSION_SIZE]; > > > /* Only warn about unresolved symbols */ > > > static int warn_unresolved = 0; > > > /* How a symbol is exported */ > > > @@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) > > > buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); > > > } > > > > > > +/** > > > + * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. > > > + * @b: Buffer to append to. > > > + * > > > + * This function fills in the module attribute `scmversion` for the kernel > > > + * module. This is useful for determining a given module's SCM version on > > > + * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. > > > + */ > > > +static void add_scmversion(struct buffer *b) > > > +{ > > > + if (module_scmversion[0] != '\0') > > > + buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); > > > +} > > > + > > > /* Cannot check for assembler */ > > > static void add_retpoline(struct buffer *b) > > > { > > > @@ -2546,7 +2562,7 @@ int main(int argc, char **argv) > > > struct dump_list *dump_read_start = NULL; > > > struct dump_list **dump_read_iter = &dump_read_start; > > > > > > - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { > > > + while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { > > > switch (opt) { > > > case 'e': > > > external_module = 1; > > > @@ -2584,6 +2600,11 @@ int main(int argc, char **argv) > > > case 'd': > > > missing_namespace_deps = optarg; > > > break; > > > + case 'v': > > > + if (!optarg) > > > + fatal("'-v' requires an argument defining the SCM version."); > > > > > > The existing -i, -T, -o, -d options take an argument as well. > > Why don't they have such a check as you added? > > The answer is because getopt() checks it. > > > > > > I applied your patch, and ran modpost directly > > with -v at the end of the command line. > > > > > > masahiro@oscar:~/workspace/linux$ ./scripts/mod/modpost -v > > ./scripts/mod/modpost: option requires an argument -- 'v' > > > > > > I see the error message from getopt() instead of > > your hand-made error check. > > > > > > > > > > > > > > > > > > > + strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); > > > + break; > > > default: > > > exit(1); > > > } > > > @@ -2630,6 +2651,7 @@ int main(int argc, char **argv) > > > add_depends(&buf, mod); > > > add_moddevtable(&buf, mod); > > > add_srcversion(&buf, mod); > > > + add_scmversion(&buf); > > > > > > sprintf(fname, "%s.mod.c", mod->name); > > > write_if_changed(&buf, fname); > > > -- > > > 2.30.0.284.gd98b1dd5eaa7-goog > > > > > > > > > -- > > Best Regards > > Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-20 0:18 ` Will McVicker @ 2021-01-20 19:41 ` Masahiro Yamada 0 siblings, 0 replies; 9+ messages in thread From: Masahiro Yamada @ 2021-01-20 19:41 UTC (permalink / raw) To: Will McVicker Cc: Jessica Yu, Michal Marek, Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, Linux Kernel Mailing List, Linux Kbuild mailing list, Cc: Android Kernel On Wed, Jan 20, 2021 at 9:19 AM Will McVicker <willmcvicker@google.com> wrote: > > Hi Masahiro, > > After testing your suggestions for module_srcpath, I remembered why I > needed this: > > +ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),) > + module_srcpath := $(srctree)/$(KBUILD_EXTMOD) > +else > + module_srcpath := $(KBUILD_EXTMOD) > +endif > > Basically KBUILD_EXTMOD actually has two uses when defined as a relative path: > > (1) defines the path to the external module's source relative to $(srctree) > (2) defines the path to the external module's build artifacts relative > to $(objtree) > > Since setlocalversion is run from $(objtree), we need to use > $(srctree)/$(KBUILD_EXTMOD) when it's a relative path. Just to note, > if I do just use KBUILD_EXTMOD, then the script setlocalversion fails > to find the srctree and returns an empty string. > > Please correct me if I'm wrong. I messed around with this by adding > many prints to Makefile.modpost and setlocalversion to make sure > everything was defined as expected. Hmm, you are right. I did not know the current Kbuild works like that. But, I do not want to uglify the code just for supporting this rare use-case. Rather, I want to apply this https://lore.kernel.org/patchwork/patch/1368277/ and, still want to see as simple code as the following: --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) input-symdump := vmlinux.symvers output-symdump := Module.symvers +module-srcpath := $(srctree) else @@ -82,9 +83,20 @@ MODPOST += -e input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS) output-symdump := $(KBUILD_EXTMOD)/Module.symvers +module-srcpath := $(KBUILD_EXTMOD) endif -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-08 0:30 [PATCH v5] modules: introduce the MODULE_SCMVERSION config Will McVicker 2021-01-12 15:54 ` Jessica Yu 2021-01-17 15:20 ` Masahiro Yamada @ 2021-01-20 14:25 ` Christoph Hellwig 2021-01-20 17:01 ` Saravana Kannan 2 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2021-01-20 14:25 UTC (permalink / raw) To: Will McVicker Cc: Jessica Yu, Masahiro Yamada, Michal Marek, Greg Kroah-Hartman, Christoph Hellwig, Saravana Kannan, linux-kernel, linux-kbuild, kernel-team On Fri, Jan 08, 2021 at 12:30:35AM +0000, Will McVicker wrote: > For example, we have a CI setup that tests new kernel changes on the > hikey960 and db845c devices without updating their kernel modules. When > these tests fail, we need to be able to identify the exact device > configuration the test was using. By including MODULE_SCMVERSION, we can > identify the exact kernel and modules' SCM versions for debugging the > failures. Sorry, but this still has no business in the upstream kernel as every change to the kernel is free to just change any API. That is whatever you test there is a completely unsupported setup. More importantly the "scmversion" of a module simply does not matter, as we only support modules from the kernel tree and it thus must be the kernel version. be supported. You are still trying to sneak out of tree module infrastructure in here with a bad cover story. Please stop doing that. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config 2021-01-20 14:25 ` Christoph Hellwig @ 2021-01-20 17:01 ` Saravana Kannan 0 siblings, 0 replies; 9+ messages in thread From: Saravana Kannan @ 2021-01-20 17:01 UTC (permalink / raw) To: Christoph Hellwig Cc: Will McVicker, Jessica Yu, Masahiro Yamada, Michal Marek, Greg Kroah-Hartman, LKML, Linux Kbuild mailing list, Android Kernel Team On Wed, Jan 20, 2021 at 6:26 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Fri, Jan 08, 2021 at 12:30:35AM +0000, Will McVicker wrote: > > For example, we have a CI setup that tests new kernel changes on the > > hikey960 and db845c devices without updating their kernel modules. When > > these tests fail, we need to be able to identify the exact device > > configuration the test was using. By including MODULE_SCMVERSION, we can > > identify the exact kernel and modules' SCM versions for debugging the > > failures. > > Sorry, but this still has no business in the upstream kernel as every > change to the kernel is free to just change any API. Sure, and this patch is making no claims one way or the other on that topic. > That is whatever > you test there is a completely unsupported setup. Plenty of distributions maintain stable kernels based on LTS. We've done that too and we are able to do LTS kernel binary updates (so better security) without waiting around for the modules to get updated. Keep in mind, that not all modules might be updated at the same time either. That's something that's definitely feasible and works. And if the API changes, MODVERSIONS catches + CI helps catch them. And if something slips in and things fail, we want to find out what kernel source was used vs what module source was used to debug the problem. This is all relevant even for in-tree modules. > More importantly the "scmversion" of a module simply does not matter, > as we only support modules from the kernel tree and it thus must be the > kernel version. > be supported. This is all talking about only in-tree modules. If you update the kernel vs the modules separately, the scmversion does matter. Also, by your argument, the vermagic or srcversion properties in a module shouldn't be there either. > You are still trying to sneak out of tree module infrastructure in here > with a bad cover story. Please stop doing that. If Will needs to maintain a downstream patch for adding out-of-tree module support, maintaining this additional patch isn't going to significantly increase his work. But he's trying to upstream at least the part that's useful for upstream. This is still a real problem for a device/board that's fully upstream. Please stop ignoring real upstream problems just because it can also be a problem for out of tree modules. This is not how we encourage folks to upstream their changes. -Saravana ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-20 19:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-08 0:30 [PATCH v5] modules: introduce the MODULE_SCMVERSION config Will McVicker 2021-01-12 15:54 ` Jessica Yu 2021-01-14 18:20 ` Will McVicker 2021-01-17 15:20 ` Masahiro Yamada 2021-01-19 21:48 ` Will McVicker 2021-01-20 0:18 ` Will McVicker 2021-01-20 19:41 ` Masahiro Yamada 2021-01-20 14:25 ` Christoph Hellwig 2021-01-20 17:01 ` Saravana Kannan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).