Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201216220850.659584-1-willmcvicker@google.com>
 <20201218120138.GA5265@linux-8ccs> <CABYd82bf+RACxboKCxoV=N63ynKwPspGhb6G5yYPiCFKnOk9Hw@mail.gmail.com>
 <20210107143111.GA30412@linux-8ccs>
In-Reply-To: <20210107143111.GA30412@linux-8ccs>
From:   Will McVicker <willmcvicker@google.com>
Date:   Thu, 7 Jan 2021 16:30:25 -0800
Message-ID: <CABYd82bVo4+M18dZ1d2x6e6-_o8YMs50v-VV_Z_DUr2vdRb-Bg@mail.gmail.com>
Subject: Re: [PATCH v4] modules: introduce the MODULE_SCMVERSION config
To:     Jessica Yu <jeyu@kernel.org>
Cc:     Masahiro Yamada <masahiroy@kernel.org>,
        Michal Marek <michal.lkml@markovi.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Christoph Hellwig <hch@infradead.org>,
        Saravana Kannan <saravanak@google.com>,
        linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org,
        kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Jessica,

I looked into the LOCALVERSION= part more and finally figured out how
that part of the script works. I wasn't familiar with how the
parameter substitution worked -- ${parameter:+alt_value}. I verified
that we will only get an SCM version returned when LOCALVERSION_AUTO=y
and updated the Documentation in v5.

Thanks,
Will

On Thu, Jan 7, 2021 at 6:31 AM Jessica Yu <jeyu@kernel.org> wrote:
>
> +++ Will McVicker [06/01/21 10:44 -0800]:
> >Thanks for the vacation notice Jessica! I'm just letting you know I'm
> >back as well and am happy to respond to any concerns regarding v4 when
> >you get all caught up.
> >
> >I hope you had a relaxing holiday :)
>
> Hi Will - thanks, same to you!
>
> >On Fri, Dec 18, 2020 at 4:01 AM Jessica Yu <jeyu@kernel.org> wrote:
> >>
> >> +++ Will McVicker [16/12/20 22:08 +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>
> >> >---
> [ added back diff ]
> >> >Changelog since v3:
> >> >- Dropped [PATCH v2 1/2] scripts/setlocalversion: allow running in a subdir
> >> >
> >> > Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++
> >> > include/linux/module.h                |  1 +
> >> > init/Kconfig                          | 12 ++++++++++++
> >> > kernel/module.c                       |  2 ++
> >> > scripts/Makefile.modpost              | 22 ++++++++++++++++++++++
> >> > scripts/mod/modpost.c                 | 24 +++++++++++++++++++++++-
> >> > 6 files changed, 78 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module
> >> >index 6272ae5fb366..2ba731767737 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.11
>
> I guess we'll have to bump KernelVersion now (sorry about the timing!)
>
> >> >+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 or LOCALVERSION=.
>
> I think the "or LOCALVERSION=" part is inaccurate, right? We need
> just LOCALVERSION_AUTO for the full scm string for this to work.
>
> >> >+           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 c4e7a887f469..6bd710308863 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..d9ae12f16ba2 100644
> >> >--- a/init/Kconfig
> >> >+++ b/init/Kconfig
> >> >@@ -2131,6 +2131,18 @@ 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.
>
> Since I consider this a debug/developer option, let's add a "If
> unsure, say N." at the end of this, similar to the other
> module options.
>
> Thanks!
>
> Jessica