[PATCH RFC 0/4] Introduce uts_release

[PATCH RFC 0/4] Introduce uts_release

[John Garry]

When hacking it is a waste of time and compute energy that we need to
rebuild much kernel code just for changing the head git commit, like this:

> touch include/generated/utsrelease.h 
> time make  -j3
mkdir -p /home/john/mnt_sda4/john/kernel-dev2/tools/objtool && make O=/home/john/mnt_sda4/john/kernel-dev2 subdir=tools/objtool --no-print-directory -C objtool 
  INSTALL libsubcmd_headers
  CALL    scripts/checksyscalls.sh
  CC      init/version.o
  AR      init/built-in.a
  CC      kernel/sys.o
  CC      kernel/module/main.o
  AR      kernel/module/built-in.a
  CC      drivers/base/firmware_loader/main.o
  CC      kernel/trace/trace.o
  AR      drivers/base/firmware_loader/built-in.a
  AR      drivers/base/built-in.a
  CC      net/ethtool/ioctl.o
  AR      kernel/trace/built-in.a
  AR      kernel/built-in.a
  AR      net/ethtool/built-in.a
  AR      net/built-in.a
  AR      drivers/built-in.a
  AR      built-in.a
  ...

Files like drivers/base/firmware_loader/main.c needs to be recompiled as
it includes generated/utsrelease.h for UTS_RELEASE macro, and utsrelease.h
is regenerated when the head commit changes.

Introduce global char uts_release[] in init/version.c, which this
mentioned code can use instead of UTS_RELEASE, meaning that we don't need
to rebuild for changing the head commit - only init/version.c needs to be
rebuilt. Whether all the references to UTS_RELEASE in the codebase are
proper is a different matter.

For an x86_64 defconfig build for this series on my old laptop, here is
before and after rebuild time:

before:
real    0m53.591s
user    1m1.842s
sys     0m9.161s

after:
real    0m37.481s
user    0m46.461s
sys     0m7.199s

Sending as an RFC as I need to test more of the conversions and I would
like to also convert more UTS_RELEASE users to prove this is proper
approach.

John Garry (4):
  init: Add uts_release
  tracing: Use uts_release
  net: ethtool: Use uts_release
  firmware_loader: Use uts_release

 drivers/base/firmware_loader/main.c | 39 +++++++++++++++++++++++------
 include/linux/utsname.h             |  1 +
 init/version.c                      |  3 +++
 kernel/trace/trace.c                |  4 +--
 net/ethtool/ioctl.c                 |  4 +--
 5 files changed, 39 insertions(+), 12 deletions(-)

-- 
2.35.3

[PATCH RFC 1/4] init: Add uts_release

[John Garry]

Add a char [] for UTS_RELEASE so that we don't need to rebuild code which
references UTS_RELEASE.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 include/linux/utsname.h | 1 +
 init/version.c          | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/utsname.h b/include/linux/utsname.h
index bf7613ba412b..15b0b1c9a9ee 100644
--- a/include/linux/utsname.h
+++ b/include/linux/utsname.h
@@ -88,5 +88,6 @@ static inline struct new_utsname *init_utsname(void)
 }
 
 extern struct rw_semaphore uts_sem;
+extern const char uts_release[];
 
 #endif /* _LINUX_UTSNAME_H */
diff --git a/init/version.c b/init/version.c
index 94c96f6fbfe6..87fecdd4fbfb 100644
--- a/init/version.c
+++ b/init/version.c
@@ -16,6 +16,7 @@
 #include <linux/uts.h>
 #include <linux/utsname.h>
 #include <linux/proc_ns.h>
+#include <generated/utsrelease.h>
 
 static int __init early_hostname(char *arg)
 {
@@ -48,6 +49,8 @@ BUILD_LTO_INFO;
 
 struct uts_namespace init_uts_ns __weak;
 const char linux_banner[] __weak;
+const char uts_release[] = UTS_RELEASE;
+EXPORT_SYMBOL_GPL(uts_release);
 
 #include "version-timestamp.c"
 
-- 
2.35.3

[PATCH RFC 2/4] tracing: Use uts_release

[John Garry]

Instead of using UTS_RELEASE, use uts_release, which means that we don't
need to rebuild the code just for the git head commit changing.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 kernel/trace/trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2a7c6fd934e9..68513924beb4 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -13,7 +13,7 @@
  *  Copyright (C) 2004 Nadia Yvette Chambers
  */
 #include <linux/ring_buffer.h>
-#include <generated/utsrelease.h>
+#include <linux/utsname.h>
 #include <linux/stacktrace.h>
 #include <linux/writeback.h>
 #include <linux/kallsyms.h>
@@ -4354,7 +4354,7 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter)
 	get_total_entries(buf, &total, &entries);
 
 	seq_printf(m, "# %s latency trace v1.1.5 on %s\n",
-		   name, UTS_RELEASE);
+		   name, uts_release);
 	seq_puts(m, "# -----------------------------------"
 		 "---------------------------------\n");
 	seq_printf(m, "# latency: %lu us, #%lu/%lu, CPU#%d |"
-- 
2.35.3

Re: [PATCH RFC 2/4] tracing: Use uts_release

[Steven Rostedt]

On Wed, 31 Jan 2024 10:48:49 +0000
John Garry <john.g.garry@oracle.com> wrote:

> Instead of using UTS_RELEASE, use uts_release, which means that we don't
> need to rebuild the code just for the git head commit changing.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

> ---
>  kernel/trace/trace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 2a7c6fd934e9..68513924beb4 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -13,7 +13,7 @@
>   *  Copyright (C) 2004 Nadia Yvette Chambers
>   */
>  #include <linux/ring_buffer.h>
> -#include <generated/utsrelease.h>
> +#include <linux/utsname.h>
>  #include <linux/stacktrace.h>
>  #include <linux/writeback.h>
>  #include <linux/kallsyms.h>
> @@ -4354,7 +4354,7 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter)
>  	get_total_entries(buf, &total, &entries);
>  
>  	seq_printf(m, "# %s latency trace v1.1.5 on %s\n",
> -		   name, UTS_RELEASE);
> +		   name, uts_release);
>  	seq_puts(m, "# -----------------------------------"
>  		 "---------------------------------\n");
>  	seq_printf(m, "# latency: %lu us, #%lu/%lu, CPU#%d |"

[PATCH RFC 3/4] net: ethtool: Use uts_release

[John Garry]

Instead of using UTS_RELEASE, use uts_release, which means that we don't
need to rebuild the code just for the git head commit changing.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 net/ethtool/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 7519b0818b91..81d052505f67 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -31,7 +31,7 @@
 #include <net/xdp_sock_drv.h>
 #include <net/flow_offload.h>
 #include <linux/ethtool_netlink.h>
-#include <generated/utsrelease.h>
+#include <linux/utsname.h>
 #include "common.h"
 
 /* State held across locks and calls for commands which have devlink fallback */
@@ -713,7 +713,7 @@ ethtool_get_drvinfo(struct net_device *dev, struct ethtool_devlink_compat *rsp)
 	struct device *parent = dev->dev.parent;
 
 	rsp->info.cmd = ETHTOOL_GDRVINFO;
-	strscpy(rsp->info.version, UTS_RELEASE, sizeof(rsp->info.version));
+	strscpy(rsp->info.version, uts_release, sizeof(rsp->info.version));
 	if (ops->get_drvinfo) {
 		ops->get_drvinfo(dev, &rsp->info);
 		if (!rsp->info.bus_info[0] && parent)
-- 
2.35.3

Re: [PATCH RFC 3/4] net: ethtool: Use uts_release

[Jakub Kicinski]

On Wed, 31 Jan 2024 10:48:50 +0000 John Garry wrote:
> Instead of using UTS_RELEASE, use uts_release, which means that we don't
> need to rebuild the code just for the git head commit changing.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Yes, please!

Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH RFC 3/4] net: ethtool: Use uts_release

[John Garry]

On 31/01/2024 19:24, Jakub Kicinski wrote:
> On Wed, 31 Jan 2024 10:48:50 +0000 John Garry wrote:
>> Instead of using UTS_RELEASE, use uts_release, which means that we don't
>> need to rebuild the code just for the git head commit changing.
>>
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
> Yes, please!
> 
> Acked-by: Jakub Kicinski<kuba@kernel.org>

Cheers

BTW, I assume that changes like this are also ok:

--------8<---------

    net: team: Don't bother filling in ethtool driver version

    The version is same as the default, as don't bother.

    Signed-off-by: John Garry <john.g.garry@oracle.com>

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index f575f225d417..0a44bbdcfb7b 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -25,7 +25,6 @@
#include <net/genetlink.h>
#include <net/netlink.h>
#include <net/sch_generic.h>
-#include <generated/utsrelease.h>
#include <linux/if_team.h>

#define DRV_NAME "team"
@@ -2074,7 +2073,6 @@ static void team_ethtool_get_drvinfo(struct
net_device *dev,
                                     struct ethtool_drvinfo *drvinfo)
{
        strscpy(drvinfo->driver, DRV_NAME, sizeof(drvinfo->driver));
-       strscpy(drvinfo->version, UTS_RELEASE, sizeof(drvinfo->version));
}

-------->8---------

right?

John

Re: [PATCH RFC 3/4] net: ethtool: Use uts_release

[Jiri Pirko]

Thu, Feb 01, 2024 at 01:57:16PM CET, john.g.garry@oracle.com wrote:
>On 31/01/2024 19:24, Jakub Kicinski wrote:
>> On Wed, 31 Jan 2024 10:48:50 +0000 John Garry wrote:
>> > Instead of using UTS_RELEASE, use uts_release, which means that we don't
>> > need to rebuild the code just for the git head commit changing.
>> > 
>> > Signed-off-by: John Garry<john.g.garry@oracle.com>
>> Yes, please!
>> 
>> Acked-by: Jakub Kicinski<kuba@kernel.org>
>
>Cheers
>
>BTW, I assume that changes like this are also ok:
>
>--------8<---------
>
>   net: team: Don't bother filling in ethtool driver version
>
>   The version is same as the default, as don't bother.
>
>   Signed-off-by: John Garry <john.g.garry@oracle.com>
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index f575f225d417..0a44bbdcfb7b 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -25,7 +25,6 @@
>#include <net/genetlink.h>
>#include <net/netlink.h>
>#include <net/sch_generic.h>
>-#include <generated/utsrelease.h>
>#include <linux/if_team.h>
>
>#define DRV_NAME "team"
>@@ -2074,7 +2073,6 @@ static void team_ethtool_get_drvinfo(struct
>net_device *dev,
>                                    struct ethtool_drvinfo *drvinfo)
>{
>       strscpy(drvinfo->driver, DRV_NAME, sizeof(drvinfo->driver));
>-       strscpy(drvinfo->version, UTS_RELEASE, sizeof(drvinfo->version));

Yeah. I don't see why should anyone care about this "version"...
Let's remove it.

>}
>
>-------->8---------
>
>right?
>
>John
>
>
>
>
>

Re: [PATCH RFC 3/4] net: ethtool: Use uts_release

[Jakub Kicinski]

On Thu, 1 Feb 2024 14:20:23 +0100 Jiri Pirko wrote:
> >BTW, I assume that changes like this are also ok:
> >
> >--------8<---------
> >
> >   net: team: Don't bother filling in ethtool driver version

Yup, just to be clear - you can send this independently from the series,
tag is as 

 [PATCH net-next]

we'll take it via the networking tree. I'm not sure which tree the other
patches will go thru..

Re: [PATCH RFC 3/4] net: ethtool: Use uts_release

[John Garry]

On 01/02/2024 16:09, Jakub Kicinski wrote:
> On Thu, 1 Feb 2024 14:20:23 +0100 Jiri Pirko wrote:
>>> BTW, I assume that changes like this are also ok:
>>>
>>> --------8<---------
>>>
>>>    net: team: Don't bother filling in ethtool driver version
> 
> Yup, just to be clear - you can send this independently from the series,

Sure, and I think rocker and staging/octeon also have this unnecessary 
code also.

> tag is as
> 
>   [PATCH net-next]

ah, yes

> 
> we'll take it via the networking tree.

Thanks. I assume Greg - the staging maintainer - would take the octeon 
patch.

> I'm not sure which tree the other
> patches will go thru..

I think that the best thing to do is get a minimal set in for 6.9 and 
then merge the rest in the next cycle. I've got about 22 patches in 
total now, but I think that there will be more. We'll see who can pick 
up the first set when I send it officially.

Thanks,
John

[PATCH RFC 4/4] firmware_loader: Use uts_release

[John Garry]

Instead of using UTS_RELEASE, use uts_release, which means that we don't
need to rebuild the code just for the git head commit changing.

Since UTS_RELEASE was used for fw_path and this points to const data,
append uts_release dynamically to an intermediate string.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/base/firmware_loader/main.c | 39 +++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ea28102d421e..87da7be61a29 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -38,7 +38,7 @@
 #include <linux/zstd.h>
 #include <linux/xz.h>
 
-#include <generated/utsrelease.h>
+#include <linux/utsname.h>
 
 #include "../base.h"
 #include "firmware.h"
@@ -471,9 +471,9 @@ static int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
 static char fw_path_para[256];
 static const char * const fw_path[] = {
 	fw_path_para,
-	"/lib/firmware/updates/" UTS_RELEASE,
+	"/lib/firmware/updates/", /* UTS_RELEASE is appended later */
 	"/lib/firmware/updates",
-	"/lib/firmware/" UTS_RELEASE,
+	"/lib/firmware/", /* UTS_RELEASE is appended later */
 	"/lib/firmware"
 };
 
@@ -496,7 +496,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 	size_t size;
 	int i, len, maxlen = 0;
 	int rc = -ENOENT;
-	char *path, *nt = NULL;
+	char *path, *fw_path_string, *nt = NULL;
 	size_t msize = INT_MAX;
 	void *buffer = NULL;
 
@@ -510,6 +510,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 	if (!path)
 		return -ENOMEM;
 
+	fw_path_string = __getname();
+	if (!fw_path_string) {
+		__putname(path);
+		return -ENOMEM;
+	}
+
 	wait_for_initramfs();
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		size_t file_size = 0;
@@ -519,16 +525,32 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 		if (!fw_path[i][0])
 			continue;
 
+		len = snprintf(fw_path_string, PATH_MAX, "%s", fw_path[i]);
+		if (len >= PATH_MAX) {
+			rc = -ENAMETOOLONG;
+			break;
+		}
+
+		/* Special handling to append UTS_RELEASE */
+		if (fw_path[i][len - 1] == '/') {
+			len = snprintf(fw_path_string, PATH_MAX, "%s%s",
+					fw_path[i], uts_release);
+			if (len >= PATH_MAX) {
+				rc = -ENAMETOOLONG;
+				break;
+			}
+		}
+
 		/* strip off \n from customized path */
-		maxlen = strlen(fw_path[i]);
+		maxlen = strlen(fw_path_string);
 		if (i == 0) {
-			nt = strchr(fw_path[i], '\n');
+			nt = strchr(fw_path_string, '\n');
 			if (nt)
-				maxlen = nt - fw_path[i];
+				maxlen = nt - fw_path_string;
 		}
 
 		len = snprintf(path, PATH_MAX, "%.*s/%s%s",
-			       maxlen, fw_path[i],
+			       maxlen, fw_path_string,
 			       fw_priv->fw_name, suffix);
 		if (len >= PATH_MAX) {
 			rc = -ENAMETOOLONG;
@@ -585,6 +607,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 		break;
 	}
 	__putname(path);
+	__putname(fw_path_string);
 
 	return rc;
 }
-- 
2.35.3

Re: [PATCH RFC 0/4] Introduce uts_release

[Greg KH]

On Wed, Jan 31, 2024 at 10:48:47AM +0000, John Garry wrote:
> When hacking it is a waste of time and compute energy that we need to
> rebuild much kernel code just for changing the head git commit, like this:
> 
> > touch include/generated/utsrelease.h 
> > time make  -j3
> mkdir -p /home/john/mnt_sda4/john/kernel-dev2/tools/objtool && make O=/home/john/mnt_sda4/john/kernel-dev2 subdir=tools/objtool --no-print-directory -C objtool 
>   INSTALL libsubcmd_headers
>   CALL    scripts/checksyscalls.sh
>   CC      init/version.o
>   AR      init/built-in.a
>   CC      kernel/sys.o
>   CC      kernel/module/main.o
>   AR      kernel/module/built-in.a
>   CC      drivers/base/firmware_loader/main.o
>   CC      kernel/trace/trace.o
>   AR      drivers/base/firmware_loader/built-in.a
>   AR      drivers/base/built-in.a
>   CC      net/ethtool/ioctl.o
>   AR      kernel/trace/built-in.a
>   AR      kernel/built-in.a
>   AR      net/ethtool/built-in.a
>   AR      net/built-in.a
>   AR      drivers/built-in.a
>   AR      built-in.a
>   ...
> 
> Files like drivers/base/firmware_loader/main.c needs to be recompiled as
> it includes generated/utsrelease.h for UTS_RELEASE macro, and utsrelease.h
> is regenerated when the head commit changes.
> 
> Introduce global char uts_release[] in init/version.c, which this
> mentioned code can use instead of UTS_RELEASE, meaning that we don't need
> to rebuild for changing the head commit - only init/version.c needs to be
> rebuilt. Whether all the references to UTS_RELEASE in the codebase are
> proper is a different matter.
> 
> For an x86_64 defconfig build for this series on my old laptop, here is
> before and after rebuild time:
> 
> before:
> real    0m53.591s
> user    1m1.842s
> sys     0m9.161s
> 
> after:
> real    0m37.481s
> user    0m46.461s
> sys     0m7.199s
> 
> Sending as an RFC as I need to test more of the conversions and I would
> like to also convert more UTS_RELEASE users to prove this is proper
> approach.

I like it, I also think that v4l2 includes this as well as all of those
drivers seem to rebuild when this changes, does that not happen for you
too?

Anyway, if the firmware changes work, I'm all for this, thanks for
taking it on!

thanks,

greg k-h

Re: [PATCH RFC 0/4] Introduce uts_release

[John Garry]

On 31/01/2024 16:22, Greg KH wrote:
>> before:
>> real    0m53.591s
>> user    1m1.842s
>> sys     0m9.161s
>>
>> after:
>> real    0m37.481s
>> user    0m46.461s
>> sys     0m7.199s
>>
>> Sending as an RFC as I need to test more of the conversions and I would
>> like to also convert more UTS_RELEASE users to prove this is proper
>> approach.
> I like it, I also think that v4l2 includes this as well as all of those
> drivers seem to rebuild when this changes, does that not happen for you
> too?

I didn't see that. Were you were building for arm64? I can see some v4l2 
configs enabled there for the vanilla defconfig (but none for x86-64).

> 
> Anyway, if the firmware changes work, I'm all for this, thanks for
> taking it on!

cheers. BTW, I just noticed that utsrelase.h might not be updated for my 
allmodconfig build when I change the head commit - I'll investigate why ...

John

Re: [PATCH RFC 0/4] Introduce uts_release

[Greg KH]

On Wed, Jan 31, 2024 at 05:16:09PM +0000, John Garry wrote:
> On 31/01/2024 16:22, Greg KH wrote:
> > > before:
> > > real    0m53.591s
> > > user    1m1.842s
> > > sys     0m9.161s
> > > 
> > > after:
> > > real    0m37.481s
> > > user    0m46.461s
> > > sys     0m7.199s
> > > 
> > > Sending as an RFC as I need to test more of the conversions and I would
> > > like to also convert more UTS_RELEASE users to prove this is proper
> > > approach.
> > I like it, I also think that v4l2 includes this as well as all of those
> > drivers seem to rebuild when this changes, does that not happen for you
> > too?
> 
> I didn't see that. Were you were building for arm64? I can see some v4l2
> configs enabled there for the vanilla defconfig (but none for x86-64).

Building for x86, maybe it's one of the other LINUX_VERSION type defines
we have, sorry, can't remember, it's been a long time since I looked
into it.

thanks,

greg k-h

Re: [PATCH RFC 0/4] Introduce uts_release

[Masahiro Yamada]

On Wed, Jan 31, 2024 at 7:49 PM John Garry <john.g.garry@oracle.com> wrote:
>
> When hacking it is a waste of time and compute energy that we need to
> rebuild much kernel code just for changing the head git commit, like this:
>
> > touch include/generated/utsrelease.h
> > time make  -j3
> mkdir -p /home/john/mnt_sda4/john/kernel-dev2/tools/objtool && make O=/home/john/mnt_sda4/john/kernel-dev2 subdir=tools/objtool --no-print-directory -C objtool
>   INSTALL libsubcmd_headers
>   CALL    scripts/checksyscalls.sh
>   CC      init/version.o
>   AR      init/built-in.a
>   CC      kernel/sys.o
>   CC      kernel/module/main.o
>   AR      kernel/module/built-in.a
>   CC      drivers/base/firmware_loader/main.o
>   CC      kernel/trace/trace.o
>   AR      drivers/base/firmware_loader/built-in.a
>   AR      drivers/base/built-in.a
>   CC      net/ethtool/ioctl.o
>   AR      kernel/trace/built-in.a
>   AR      kernel/built-in.a
>   AR      net/ethtool/built-in.a
>   AR      net/built-in.a
>   AR      drivers/built-in.a
>   AR      built-in.a
>   ...
>
> Files like drivers/base/firmware_loader/main.c needs to be recompiled as
> it includes generated/utsrelease.h for UTS_RELEASE macro, and utsrelease.h
> is regenerated when the head commit changes.
>
> Introduce global char uts_release[] in init/version.c, which this
> mentioned code can use instead of UTS_RELEASE, meaning that we don't need
> to rebuild for changing the head commit - only init/version.c needs to be
> rebuilt. Whether all the references to UTS_RELEASE in the codebase are
> proper is a different matter.
>
> For an x86_64 defconfig build for this series on my old laptop, here is
> before and after rebuild time:
>
> before:
> real    0m53.591s
> user    1m1.842s
> sys     0m9.161s
>
> after:
> real    0m37.481s
> user    0m46.461s
> sys     0m7.199s
>
> Sending as an RFC as I need to test more of the conversions and I would
> like to also convert more UTS_RELEASE users to prove this is proper
> approach.
>
> John Garry (4):
>   init: Add uts_release
>   tracing: Use uts_release
>   net: ethtool: Use uts_release
>   firmware_loader: Use uts_release
>
>  drivers/base/firmware_loader/main.c | 39 +++++++++++++++++++++++------
>  include/linux/utsname.h             |  1 +
>  init/version.c                      |  3 +++
>  kernel/trace/trace.c                |  4 +--
>  net/ethtool/ioctl.c                 |  4 +--
>  5 files changed, 39 insertions(+), 12 deletions(-)
>
> --
> 2.35.3
>





As you see, several drivers store UTS_RELEASE in their driver data,
and even print it in debug print.


I do not see why it is useful.
As you discussed in 3/4, if UTS_RELEASE is unneeded,
it is better to get rid of it.


If such version information is useful for drivers, the intention is
whether the version of the module, or the version of vmlinux.
That is a question.
They differ when CONFIG_MODVERSION.


When module developers intend to printk the git version
from which the module was compiled from,
presumably they want to use UTS_RELEASE, which
was expanded at the compile time of the module.

If you replace it with uts_release, it is the git version
of vmlinux.


Of course, the replacement is safe for always-builtin code.



Lastly, we can avoid using UTS_RELEASE without relying
on your patch.



For example, commit 3a3a11e6e5a2bc0595c7e36ae33c861c9e8c75b1
replaced  UTS_RELEASE with init_uts_ns.name.release


So, is your uts_release a shorthand of init_uts_ns.name.release?



I think what you can contribute are:

 - Explore the UTS_RELEASE users, and check if you can get rid of it.

 - Where UTS_RELEASE is useful, consider if it is possible
   to replace it with init_uts_ns.name.release



-- 
Best Regards
Masahiro Yamada

Re: [PATCH RFC 0/4] Introduce uts_release

[Jakub Kicinski]

On Sat, 3 Feb 2024 00:01:26 +0900 Masahiro Yamada wrote:
> I do not see why it is useful.
> As you discussed in 3/4, if UTS_RELEASE is unneeded,
> it is better to get rid of it.

To be clear - the discussion on 3/4 was about the fact that netdev
already prints UTS_RELEASE into the version member of driver info
struct, as a default. So the drivers no longer have to. But there's 
no user-observable change there.

Re: [PATCH RFC 0/4] Introduce uts_release

[John Garry]

On 02/02/2024 15:01, Masahiro Yamada wrote:
>> --
>> 2.35.3
> 
> As you see, several drivers store UTS_RELEASE in their driver data,
> and even print it in debug print.
> 
> 
> I do not see why it is useful.

I would tend to agree, and mentioned that earlier.

> As you discussed in 3/4, if UTS_RELEASE is unneeded,
> it is better to get rid of it.

Jakub replied about this.

> 
> 
> If such version information is useful for drivers, the intention is
> whether the version of the module, or the version of vmlinux.
> That is a question.
> They differ when CONFIG_MODVERSION.
> 

I think often this information in UTS_RELEASE is shared as informative 
only, so the user can conveniently know the specific kernel git version.

> 
> When module developers intend to printk the git version
> from which the module was compiled from,
> presumably they want to use UTS_RELEASE, which
> was expanded at the compile time of the module.
> 
> If you replace it with uts_release, it is the git version
> of vmlinux.
> 
> 
> Of course, the replacement is safe for always-builtin code.
> 
> 
> 
> Lastly, we can avoid using UTS_RELEASE without relying
> on your patch.
> 
> 
> 
> For example, commit 3a3a11e6e5a2bc0595c7e36ae33c861c9e8c75b1
> replaced  UTS_RELEASE with init_uts_ns.name.release
> 
> 
> So, is your uts_release a shorthand of init_uts_ns.name.release?

Yes - well that both are strings containing UTS_RELEASE. Using a struct 
sub-member is bit ungainly, but I suppose that we should not be making 
life easy for people using this.

However we already have init_utsname in:

static inline struct new_utsname *init_utsname(void)
{
	return &init_uts_ns.name;
}

So could use init_utsname()->release, which is a bit nicer.

> 
> 
> 
> I think what you can contribute are:
> 
>   - Explore the UTS_RELEASE users, and check if you can get rid of it.

Unfortunately I expect resistance for this. I also expect places like FW 
loader it is necessary. And when this is used in sysfs, people will say 
that it is part of the ABI now.

How about I send the patch to update to use init_uts_ns and mention also 
that it would be better to not use at all, if possible? I can cc you.

> 
>   - Where UTS_RELEASE is useful, consider if it is possible
>     to replace it with init_uts_ns.name.release

ok, but, as above, could use init_utsname()->release also

Thanks,
John

Re: [PATCH RFC 0/4] Introduce uts_release

[Masahiro Yamada]

On Mon, Feb 5, 2024 at 5:25 PM John Garry <john.g.garry@oracle.com> wrote:
>
> On 02/02/2024 15:01, Masahiro Yamada wrote:
> >> --
> >> 2.35.3
> >
> > As you see, several drivers store UTS_RELEASE in their driver data,
> > and even print it in debug print.
> >
> >
> > I do not see why it is useful.
>
> I would tend to agree, and mentioned that earlier.
>
> > As you discussed in 3/4, if UTS_RELEASE is unneeded,
> > it is better to get rid of it.
>
> Jakub replied about this.
>
> >
> >
> > If such version information is useful for drivers, the intention is
> > whether the version of the module, or the version of vmlinux.
> > That is a question.
> > They differ when CONFIG_MODVERSION.
> >
>
> I think often this information in UTS_RELEASE is shared as informative
> only, so the user can conveniently know the specific kernel git version.
>
> >
> > When module developers intend to printk the git version
> > from which the module was compiled from,
> > presumably they want to use UTS_RELEASE, which
> > was expanded at the compile time of the module.
> >
> > If you replace it with uts_release, it is the git version
> > of vmlinux.
> >
> >
> > Of course, the replacement is safe for always-builtin code.
> >
> >
> >
> > Lastly, we can avoid using UTS_RELEASE without relying
> > on your patch.
> >
> >
> >
> > For example, commit 3a3a11e6e5a2bc0595c7e36ae33c861c9e8c75b1
> > replaced  UTS_RELEASE with init_uts_ns.name.release
> >
> >
> > So, is your uts_release a shorthand of init_uts_ns.name.release?
>
> Yes - well that both are strings containing UTS_RELEASE. Using a struct
> sub-member is bit ungainly, but I suppose that we should not be making
> life easy for people using this.
>
> However we already have init_utsname in:
>
> static inline struct new_utsname *init_utsname(void)
> {
>         return &init_uts_ns.name;
> }
>
> So could use init_utsname()->release, which is a bit nicer.
>
> >
> >
> >
> > I think what you can contribute are:
> >
> >   - Explore the UTS_RELEASE users, and check if you can get rid of it.
>
> Unfortunately I expect resistance for this. I also expect places like FW
> loader it is necessary. And when this is used in sysfs, people will say
> that it is part of the ABI now.
>
> How about I send the patch to update to use init_uts_ns and mention also
> that it would be better to not use at all, if possible? I can cc you.


OK.


As I mentioned in the previous reply, the replacement is safe
for builtin code.

When you touch modular code, please pay a little more care,
because UTS_RELEASE and init_utsname()->release
may differ when CONFIG_MODVERSIONS=y.







>
> >
> >   - Where UTS_RELEASE is useful, consider if it is possible
> >     to replace it with init_uts_ns.name.release
>
> ok, but, as above, could use init_utsname()->release also


I am fine with it.


init_utsname()->release is more commonly used
(but less common than UTS_RELEASE)



$ git grep   'init_utsname()->release' | wc
     28      92    2065
$ git grep   'init_uts_ns.name.release' | wc
      7      34     587
$ git grep   'UTS_RELEASE' | wc
     57     304    4741




-- 
Best Regards
Masahiro Yamada

Re: [PATCH RFC 0/4] Introduce uts_release

[John Garry]

On 05/02/2024 23:10, Masahiro Yamada wrote:
>>> I think what you can contribute are:
>>>
>>>    - Explore the UTS_RELEASE users, and check if you can get rid of it.
>> Unfortunately I expect resistance for this. I also expect places like FW
>> loader it is necessary. And when this is used in sysfs, people will say
>> that it is part of the ABI now.
>>
>> How about I send the patch to update to use init_uts_ns and mention also
>> that it would be better to not use at all, if possible? I can cc you.
> 
> OK.
> 
> 
> As I mentioned in the previous reply, the replacement is safe
> for builtin code.
> 
> When you touch modular code, please pay a little more care,
> because UTS_RELEASE and init_utsname()->release
> may differ when CONFIG_MODVERSIONS=y.
> 

Are you saying that we may have a different release version kernel and 
module built with CONFIG_MODVERSIONS=y, and the module was using 
UTS_RELEASE for something? That something may be like setting some info 
in a sysfs file, like in this example:

static ssize_t target_core_item_version_show(struct config_item *item,
		char *page)
{
	return sprintf(page, "Target Engine Core ConfigFS Infrastructure %s"
		" on %s/%s on "UTS_RELEASE"\n", TARGET_CORE_VERSION,
		utsname()->sysname, utsname()->machine);
}

And the intention is to use the module codebase release version and not 
the kernel codebase release version. Hence utsname() is used for 
.sysname and .machine, but not .release .

Thanks,
John

Re: [PATCH RFC 0/4] Introduce uts_release

[John Garry]

On 08/02/2024 10:08, John Garry wrote:
> On 05/02/2024 23:10, Masahiro Yamada wrote:
>>>> I think what you can contribute are:
>>>>
>>>>    - Explore the UTS_RELEASE users, and check if you can get rid of it.
>>> Unfortunately I expect resistance for this. I also expect places like FW
>>> loader it is necessary. And when this is used in sysfs, people will say
>>> that it is part of the ABI now.
>>>
>>> How about I send the patch to update to use init_uts_ns and mention also
>>> that it would be better to not use at all, if possible? I can cc you.
>>
>> OK.
>>
>>
>> As I mentioned in the previous reply, the replacement is safe
>> for builtin code.
>>
>> When you touch modular code, please pay a little more care,
>> because UTS_RELEASE and init_utsname()->release
>> may differ when CONFIG_MODVERSIONS=y.
>>
> 
> Are you saying that we may have a different release version kernel and 
> module built with CONFIG_MODVERSIONS=y, and the module was using 
> UTS_RELEASE for something? That something may be like setting some info 
> in a sysfs file, like in this example:
> 
> static ssize_t target_core_item_version_show(struct config_item *item,
>          char *page)
> {
>      return sprintf(page, "Target Engine Core ConfigFS Infrastructure %s"
>          " on %s/%s on "UTS_RELEASE"\n", TARGET_CORE_VERSION,
>          utsname()->sysname, utsname()->machine);
> }
> 
> And the intention is to use the module codebase release version and not 
> the kernel codebase release version. Hence utsname() is used for 
> .sysname and .machine, but not .release .

Hi Masahiro,

Can you comment on whether I am right about CONFIG_MODVERSIONS, above?

Thanks,
John

Re: [PATCH RFC 0/4] Introduce uts_release

[Masahiro Yamada]

On Wed, Feb 21, 2024 at 6:01 PM John Garry <john.g.garry@oracle.com> wrote:
>
> On 08/02/2024 10:08, John Garry wrote:
> > On 05/02/2024 23:10, Masahiro Yamada wrote:
> >>>> I think what you can contribute are:
> >>>>
> >>>>    - Explore the UTS_RELEASE users, and check if you can get rid of it.
> >>> Unfortunately I expect resistance for this. I also expect places like FW
> >>> loader it is necessary. And when this is used in sysfs, people will say
> >>> that it is part of the ABI now.
> >>>
> >>> How about I send the patch to update to use init_uts_ns and mention also
> >>> that it would be better to not use at all, if possible? I can cc you.
> >>
> >> OK.
> >>
> >>
> >> As I mentioned in the previous reply, the replacement is safe
> >> for builtin code.
> >>
> >> When you touch modular code, please pay a little more care,
> >> because UTS_RELEASE and init_utsname()->release
> >> may differ when CONFIG_MODVERSIONS=y.
> >>
> >
> > Are you saying that we may have a different release version kernel and
> > module built with CONFIG_MODVERSIONS=y, and the module was using
> > UTS_RELEASE for something? That something may be like setting some info
> > in a sysfs file, like in this example:
> >
> > static ssize_t target_core_item_version_show(struct config_item *item,
> >          char *page)
> > {
> >      return sprintf(page, "Target Engine Core ConfigFS Infrastructure %s"
> >          " on %s/%s on "UTS_RELEASE"\n", TARGET_CORE_VERSION,
> >          utsname()->sysname, utsname()->machine);
> > }
> >
> > And the intention is to use the module codebase release version and not
> > the kernel codebase release version. Hence utsname() is used for
> > .sysname and .machine, but not .release .
>
> Hi Masahiro,
>
> Can you comment on whether I am right about CONFIG_MODVERSIONS, above?
>
> Thanks,
> John



Your understanding about CONFIG_MODVERSIONS is correct.




-- 
Best Regards
Masahiro Yamada