To avoid duplication between host programs, move the crc32 code to a
shared header file.
Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Acked-by: Neal Gompa <neal@gompa.dev>
---
scripts/genksyms/genksyms.c | 77 +-----------------------------
scripts/include/crc32.h | 93 +++++++++++++++++++++++++++++++++++++
2 files changed, 94 insertions(+), 76 deletions(-)
create mode 100644 scripts/include/crc32.h
diff --git a/scripts/genksyms/genksyms.c b/scripts/genksyms/genksyms.c
index f3901c55df23..2885bbcb9eec 100644
--- a/scripts/genksyms/genksyms.c
+++ b/scripts/genksyms/genksyms.c
@@ -18,6 +18,7 @@
#include <stdarg.h>
#include <getopt.h>
+#include <crc32.h>
#include "genksyms.h"
/*----------------------------------------------------------------------*/
@@ -58,82 +59,6 @@ static struct string_list *mk_node(const char *string);
static void print_location(void);
static void print_type_name(enum symbol_type type, const char *name);
-/*----------------------------------------------------------------------*/
-
-static const unsigned int crctab32[] = {
- 0x00000000U, 0x77073096U, 0xee0e612cU, 0x990951baU, 0x076dc419U,
- 0x706af48fU, 0xe963a535U, 0x9e6495a3U, 0x0edb8832U, 0x79dcb8a4U,
- 0xe0d5e91eU, 0x97d2d988U, 0x09b64c2bU, 0x7eb17cbdU, 0xe7b82d07U,
- 0x90bf1d91U, 0x1db71064U, 0x6ab020f2U, 0xf3b97148U, 0x84be41deU,
- 0x1adad47dU, 0x6ddde4ebU, 0xf4d4b551U, 0x83d385c7U, 0x136c9856U,
- 0x646ba8c0U, 0xfd62f97aU, 0x8a65c9ecU, 0x14015c4fU, 0x63066cd9U,
- 0xfa0f3d63U, 0x8d080df5U, 0x3b6e20c8U, 0x4c69105eU, 0xd56041e4U,
- 0xa2677172U, 0x3c03e4d1U, 0x4b04d447U, 0xd20d85fdU, 0xa50ab56bU,
- 0x35b5a8faU, 0x42b2986cU, 0xdbbbc9d6U, 0xacbcf940U, 0x32d86ce3U,
- 0x45df5c75U, 0xdcd60dcfU, 0xabd13d59U, 0x26d930acU, 0x51de003aU,
- 0xc8d75180U, 0xbfd06116U, 0x21b4f4b5U, 0x56b3c423U, 0xcfba9599U,
- 0xb8bda50fU, 0x2802b89eU, 0x5f058808U, 0xc60cd9b2U, 0xb10be924U,
- 0x2f6f7c87U, 0x58684c11U, 0xc1611dabU, 0xb6662d3dU, 0x76dc4190U,
- 0x01db7106U, 0x98d220bcU, 0xefd5102aU, 0x71b18589U, 0x06b6b51fU,
- 0x9fbfe4a5U, 0xe8b8d433U, 0x7807c9a2U, 0x0f00f934U, 0x9609a88eU,
- 0xe10e9818U, 0x7f6a0dbbU, 0x086d3d2dU, 0x91646c97U, 0xe6635c01U,
- 0x6b6b51f4U, 0x1c6c6162U, 0x856530d8U, 0xf262004eU, 0x6c0695edU,
- 0x1b01a57bU, 0x8208f4c1U, 0xf50fc457U, 0x65b0d9c6U, 0x12b7e950U,
- 0x8bbeb8eaU, 0xfcb9887cU, 0x62dd1ddfU, 0x15da2d49U, 0x8cd37cf3U,
- 0xfbd44c65U, 0x4db26158U, 0x3ab551ceU, 0xa3bc0074U, 0xd4bb30e2U,
- 0x4adfa541U, 0x3dd895d7U, 0xa4d1c46dU, 0xd3d6f4fbU, 0x4369e96aU,
- 0x346ed9fcU, 0xad678846U, 0xda60b8d0U, 0x44042d73U, 0x33031de5U,
- 0xaa0a4c5fU, 0xdd0d7cc9U, 0x5005713cU, 0x270241aaU, 0xbe0b1010U,
- 0xc90c2086U, 0x5768b525U, 0x206f85b3U, 0xb966d409U, 0xce61e49fU,
- 0x5edef90eU, 0x29d9c998U, 0xb0d09822U, 0xc7d7a8b4U, 0x59b33d17U,
- 0x2eb40d81U, 0xb7bd5c3bU, 0xc0ba6cadU, 0xedb88320U, 0x9abfb3b6U,
- 0x03b6e20cU, 0x74b1d29aU, 0xead54739U, 0x9dd277afU, 0x04db2615U,
- 0x73dc1683U, 0xe3630b12U, 0x94643b84U, 0x0d6d6a3eU, 0x7a6a5aa8U,
- 0xe40ecf0bU, 0x9309ff9dU, 0x0a00ae27U, 0x7d079eb1U, 0xf00f9344U,
- 0x8708a3d2U, 0x1e01f268U, 0x6906c2feU, 0xf762575dU, 0x806567cbU,
- 0x196c3671U, 0x6e6b06e7U, 0xfed41b76U, 0x89d32be0U, 0x10da7a5aU,
- 0x67dd4accU, 0xf9b9df6fU, 0x8ebeeff9U, 0x17b7be43U, 0x60b08ed5U,
- 0xd6d6a3e8U, 0xa1d1937eU, 0x38d8c2c4U, 0x4fdff252U, 0xd1bb67f1U,
- 0xa6bc5767U, 0x3fb506ddU, 0x48b2364bU, 0xd80d2bdaU, 0xaf0a1b4cU,
- 0x36034af6U, 0x41047a60U, 0xdf60efc3U, 0xa867df55U, 0x316e8eefU,
- 0x4669be79U, 0xcb61b38cU, 0xbc66831aU, 0x256fd2a0U, 0x5268e236U,
- 0xcc0c7795U, 0xbb0b4703U, 0x220216b9U, 0x5505262fU, 0xc5ba3bbeU,
- 0xb2bd0b28U, 0x2bb45a92U, 0x5cb36a04U, 0xc2d7ffa7U, 0xb5d0cf31U,
- 0x2cd99e8bU, 0x5bdeae1dU, 0x9b64c2b0U, 0xec63f226U, 0x756aa39cU,
- 0x026d930aU, 0x9c0906a9U, 0xeb0e363fU, 0x72076785U, 0x05005713U,
- 0x95bf4a82U, 0xe2b87a14U, 0x7bb12baeU, 0x0cb61b38U, 0x92d28e9bU,
- 0xe5d5be0dU, 0x7cdcefb7U, 0x0bdbdf21U, 0x86d3d2d4U, 0xf1d4e242U,
- 0x68ddb3f8U, 0x1fda836eU, 0x81be16cdU, 0xf6b9265bU, 0x6fb077e1U,
- 0x18b74777U, 0x88085ae6U, 0xff0f6a70U, 0x66063bcaU, 0x11010b5cU,
- 0x8f659effU, 0xf862ae69U, 0x616bffd3U, 0x166ccf45U, 0xa00ae278U,
- 0xd70dd2eeU, 0x4e048354U, 0x3903b3c2U, 0xa7672661U, 0xd06016f7U,
- 0x4969474dU, 0x3e6e77dbU, 0xaed16a4aU, 0xd9d65adcU, 0x40df0b66U,
- 0x37d83bf0U, 0xa9bcae53U, 0xdebb9ec5U, 0x47b2cf7fU, 0x30b5ffe9U,
- 0xbdbdf21cU, 0xcabac28aU, 0x53b39330U, 0x24b4a3a6U, 0xbad03605U,
- 0xcdd70693U, 0x54de5729U, 0x23d967bfU, 0xb3667a2eU, 0xc4614ab8U,
- 0x5d681b02U, 0x2a6f2b94U, 0xb40bbe37U, 0xc30c8ea1U, 0x5a05df1bU,
- 0x2d02ef8dU
-};
-
-static unsigned long partial_crc32_one(unsigned char c, unsigned long crc)
-{
- return crctab32[(crc ^ c) & 0xff] ^ (crc >> 8);
-}
-
-static unsigned long partial_crc32(const char *s, unsigned long crc)
-{
- while (*s)
- crc = partial_crc32_one(*s++, crc);
- return crc;
-}
-
-static unsigned long crc32(const char *s)
-{
- return partial_crc32(s, 0xffffffff) ^ 0xffffffff;
-}
-
-/*----------------------------------------------------------------------*/
-
static enum symbol_type map_to_ns(enum symbol_type t)
{
switch (t) {
diff --git a/scripts/include/crc32.h b/scripts/include/crc32.h
new file mode 100644
index 000000000000..06eedd273717
--- /dev/null
+++ b/scripts/include/crc32.h
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * CRC32 implementation.
+ *
+ * Moved from scripts/genksyms/genksyms.c, which has the following
+ * notice:
+ *
+ * Generate kernel symbol version hashes.
+ * Copyright 1996, 1997 Linux International.
+ *
+ * New implementation contributed by Richard Henderson <rth@tamu.edu>
+ * Based on original work by Bjorn Ekwall <bj0rn@blox.se>
+ *
+ * This file was part of the Linux modutils 2.4.22: moved back into the
+ * kernel sources by Rusty Russell/Kai Germaschewski.
+ */
+
+#ifndef __CRC32_H
+#define __CRC32_H
+
+static const unsigned int crctab32[] = {
+ 0x00000000U, 0x77073096U, 0xee0e612cU, 0x990951baU, 0x076dc419U,
+ 0x706af48fU, 0xe963a535U, 0x9e6495a3U, 0x0edb8832U, 0x79dcb8a4U,
+ 0xe0d5e91eU, 0x97d2d988U, 0x09b64c2bU, 0x7eb17cbdU, 0xe7b82d07U,
+ 0x90bf1d91U, 0x1db71064U, 0x6ab020f2U, 0xf3b97148U, 0x84be41deU,
+ 0x1adad47dU, 0x6ddde4ebU, 0xf4d4b551U, 0x83d385c7U, 0x136c9856U,
+ 0x646ba8c0U, 0xfd62f97aU, 0x8a65c9ecU, 0x14015c4fU, 0x63066cd9U,
+ 0xfa0f3d63U, 0x8d080df5U, 0x3b6e20c8U, 0x4c69105eU, 0xd56041e4U,
+ 0xa2677172U, 0x3c03e4d1U, 0x4b04d447U, 0xd20d85fdU, 0xa50ab56bU,
+ 0x35b5a8faU, 0x42b2986cU, 0xdbbbc9d6U, 0xacbcf940U, 0x32d86ce3U,
+ 0x45df5c75U, 0xdcd60dcfU, 0xabd13d59U, 0x26d930acU, 0x51de003aU,
+ 0xc8d75180U, 0xbfd06116U, 0x21b4f4b5U, 0x56b3c423U, 0xcfba9599U,
+ 0xb8bda50fU, 0x2802b89eU, 0x5f058808U, 0xc60cd9b2U, 0xb10be924U,
+ 0x2f6f7c87U, 0x58684c11U, 0xc1611dabU, 0xb6662d3dU, 0x76dc4190U,
+ 0x01db7106U, 0x98d220bcU, 0xefd5102aU, 0x71b18589U, 0x06b6b51fU,
+ 0x9fbfe4a5U, 0xe8b8d433U, 0x7807c9a2U, 0x0f00f934U, 0x9609a88eU,
+ 0xe10e9818U, 0x7f6a0dbbU, 0x086d3d2dU, 0x91646c97U, 0xe6635c01U,
+ 0x6b6b51f4U, 0x1c6c6162U, 0x856530d8U, 0xf262004eU, 0x6c0695edU,
+ 0x1b01a57bU, 0x8208f4c1U, 0xf50fc457U, 0x65b0d9c6U, 0x12b7e950U,
+ 0x8bbeb8eaU, 0xfcb9887cU, 0x62dd1ddfU, 0x15da2d49U, 0x8cd37cf3U,
+ 0xfbd44c65U, 0x4db26158U, 0x3ab551ceU, 0xa3bc0074U, 0xd4bb30e2U,
+ 0x4adfa541U, 0x3dd895d7U, 0xa4d1c46dU, 0xd3d6f4fbU, 0x4369e96aU,
+ 0x346ed9fcU, 0xad678846U, 0xda60b8d0U, 0x44042d73U, 0x33031de5U,
+ 0xaa0a4c5fU, 0xdd0d7cc9U, 0x5005713cU, 0x270241aaU, 0xbe0b1010U,
+ 0xc90c2086U, 0x5768b525U, 0x206f85b3U, 0xb966d409U, 0xce61e49fU,
+ 0x5edef90eU, 0x29d9c998U, 0xb0d09822U, 0xc7d7a8b4U, 0x59b33d17U,
+ 0x2eb40d81U, 0xb7bd5c3bU, 0xc0ba6cadU, 0xedb88320U, 0x9abfb3b6U,
+ 0x03b6e20cU, 0x74b1d29aU, 0xead54739U, 0x9dd277afU, 0x04db2615U,
+ 0x73dc1683U, 0xe3630b12U, 0x94643b84U, 0x0d6d6a3eU, 0x7a6a5aa8U,
+ 0xe40ecf0bU, 0x9309ff9dU, 0x0a00ae27U, 0x7d079eb1U, 0xf00f9344U,
+ 0x8708a3d2U, 0x1e01f268U, 0x6906c2feU, 0xf762575dU, 0x806567cbU,
+ 0x196c3671U, 0x6e6b06e7U, 0xfed41b76U, 0x89d32be0U, 0x10da7a5aU,
+ 0x67dd4accU, 0xf9b9df6fU, 0x8ebeeff9U, 0x17b7be43U, 0x60b08ed5U,
+ 0xd6d6a3e8U, 0xa1d1937eU, 0x38d8c2c4U, 0x4fdff252U, 0xd1bb67f1U,
+ 0xa6bc5767U, 0x3fb506ddU, 0x48b2364bU, 0xd80d2bdaU, 0xaf0a1b4cU,
+ 0x36034af6U, 0x41047a60U, 0xdf60efc3U, 0xa867df55U, 0x316e8eefU,
+ 0x4669be79U, 0xcb61b38cU, 0xbc66831aU, 0x256fd2a0U, 0x5268e236U,
+ 0xcc0c7795U, 0xbb0b4703U, 0x220216b9U, 0x5505262fU, 0xc5ba3bbeU,
+ 0xb2bd0b28U, 0x2bb45a92U, 0x5cb36a04U, 0xc2d7ffa7U, 0xb5d0cf31U,
+ 0x2cd99e8bU, 0x5bdeae1dU, 0x9b64c2b0U, 0xec63f226U, 0x756aa39cU,
+ 0x026d930aU, 0x9c0906a9U, 0xeb0e363fU, 0x72076785U, 0x05005713U,
+ 0x95bf4a82U, 0xe2b87a14U, 0x7bb12baeU, 0x0cb61b38U, 0x92d28e9bU,
+ 0xe5d5be0dU, 0x7cdcefb7U, 0x0bdbdf21U, 0x86d3d2d4U, 0xf1d4e242U,
+ 0x68ddb3f8U, 0x1fda836eU, 0x81be16cdU, 0xf6b9265bU, 0x6fb077e1U,
+ 0x18b74777U, 0x88085ae6U, 0xff0f6a70U, 0x66063bcaU, 0x11010b5cU,
+ 0x8f659effU, 0xf862ae69U, 0x616bffd3U, 0x166ccf45U, 0xa00ae278U,
+ 0xd70dd2eeU, 0x4e048354U, 0x3903b3c2U, 0xa7672661U, 0xd06016f7U,
+ 0x4969474dU, 0x3e6e77dbU, 0xaed16a4aU, 0xd9d65adcU, 0x40df0b66U,
+ 0x37d83bf0U, 0xa9bcae53U, 0xdebb9ec5U, 0x47b2cf7fU, 0x30b5ffe9U,
+ 0xbdbdf21cU, 0xcabac28aU, 0x53b39330U, 0x24b4a3a6U, 0xbad03605U,
+ 0xcdd70693U, 0x54de5729U, 0x23d967bfU, 0xb3667a2eU, 0xc4614ab8U,
+ 0x5d681b02U, 0x2a6f2b94U, 0xb40bbe37U, 0xc30c8ea1U, 0x5a05df1bU,
+ 0x2d02ef8dU
+};
+
+static inline unsigned long partial_crc32_one(unsigned char c, unsigned long crc)
+{
+ return crctab32[(crc ^ c) & 0xff] ^ (crc >> 8);
+}
+
+static inline unsigned long partial_crc32(const char *s, unsigned long crc)
+{
+ while (*s)
+ crc = partial_crc32_one(*s++, crc);
+ return crc;
+}
+
+static inline unsigned long crc32(const char *s)
+{
+ return partial_crc32(s, 0xffffffff) ^ 0xffffffff;
+}
+
+#endif /* __CRC32_H */
--
2.47.0.163.g1226f6d8fa-googOn Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> To avoid duplication between host programs, move the crc32 code to a
> shared header file.
Only the motivation to use this long table is to keep compatibility
between genksyms and gendwarfksyms.
I do not think this should be exposed to other programs.
If you avoid the code duplication, you can do
// scripts/gendwarfksyms/crc.c
#include "../genksyms/crc.c"
>
> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> Acked-by: Neal Gompa <neal@gompa.dev>
Does this Ack add any value?
Acked-by is meaningful only when it is given by someone who
maintains the relevant area or has established a reputation.
$ git grep "Neal Gompa"
$ git shortlog -n -s | grep "Neal Gompa"
2 Neal Gompa
His Ack feels more like "I like it" rather than a qualified endorsement.
--
Best Regards
Masahiro Yamada
Hi, On Mon, Nov 11, 2024 at 8:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > To avoid duplication between host programs, move the crc32 code to a > > shared header file. > > > Only the motivation to use this long table is to keep compatibility > between genksyms and gendwarfksyms. > I do not think this should be exposed to other programs. > > > If you avoid the code duplication, you can do > > // scripts/gendwarfksyms/crc.c > #include "../genksyms/crc.c" Sure, that sounds reasonable. I'll change this in the next version. > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Acked-by: Neal Gompa <neal@gompa.dev> > > Does this Ack add any value? > > Acked-by is meaningful only when it is given by someone who > maintains the relevant area or has established a reputation. > > $ git grep "Neal Gompa" > $ git shortlog -n -s | grep "Neal Gompa" > 2 Neal Gompa > > His Ack feels more like "I like it" rather than a qualified endorsement. Like Neal explained, an Ack from a potential user of this feature seemed relevant, but if you don't think it's meaningful, I can certainly drop it. Sami
On Thu, Nov 14, 2024 at 2:54 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > Hi, > > On Mon, Nov 11, 2024 at 8:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > To avoid duplication between host programs, move the crc32 code to a > > > shared header file. > > > > > > Only the motivation to use this long table is to keep compatibility > > between genksyms and gendwarfksyms. > > I do not think this should be exposed to other programs. > > > > > > If you avoid the code duplication, you can do > > > > // scripts/gendwarfksyms/crc.c > > #include "../genksyms/crc.c" > > Sure, that sounds reasonable. I'll change this in the next version. BTW, is it necessary to share the same crc function between genksyms and gendwarfksyms? If CONFIG_GENKSYMS and CONFIG_GENDWARFKSYMS were able to produce the same CRC, it would be a good motivation to share the same function. However, as far as I tested, gendwarfksyms generates different CRC values. When a distro migrates to CONFIG_GENDWARFKSYMS, the new kernel cannot load old modules built with CONFIG_GENKSYMS. So, there is no need to share the old code. Another solution might be to use crc32() provided by zlib, for example. It requires another external library, but this already depends on libdw and libelf. > > > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > Acked-by: Neal Gompa <neal@gompa.dev> > > > > Does this Ack add any value? > > > > Acked-by is meaningful only when it is given by someone who > > maintains the relevant area or has established a reputation. > > > > $ git grep "Neal Gompa" > > $ git shortlog -n -s | grep "Neal Gompa" > > 2 Neal Gompa > > > > His Ack feels more like "I like it" rather than a qualified endorsement. > > Like Neal explained, an Ack from a potential user of this feature > seemed relevant, but if you don't think it's meaningful, I can > certainly drop it. Tested-by is more suitable if he wants to leave something. -- Best Regards Masahiro Yamada
Hi, On Sat, Nov 16, 2024 at 9:09 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Nov 14, 2024 at 2:54 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Hi, > > > > On Mon, Nov 11, 2024 at 8:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > To avoid duplication between host programs, move the crc32 code to a > > > > shared header file. > > > > > > > > > Only the motivation to use this long table is to keep compatibility > > > between genksyms and gendwarfksyms. > > > I do not think this should be exposed to other programs. > > > > > > > > > If you avoid the code duplication, you can do > > > > > > // scripts/gendwarfksyms/crc.c > > > #include "../genksyms/crc.c" > > > > Sure, that sounds reasonable. I'll change this in the next version. > > > BTW, is it necessary to share the same crc function > between genksyms and gendwarfksyms? > > If CONFIG_GENKSYMS and CONFIG_GENDWARFKSYMS > were able to produce the same CRC, it would be a good motivation > to share the same function. > However, as far as I tested, gendwarfksyms generates different CRC values. > > When a distro migrates to CONFIG_GENDWARFKSYMS, > the new kernel cannot load old modules built with CONFIG_GENKSYMS. No, it's not necessary to use the exact same function, this was just to avoid adding more external dependencies. > So, there is no need to share the old code. > Another solution might be to use crc32() provided by zlib, for example. > It requires another external library, but this already depends on > libdw and libelf. Makes sense. I'll switch to the zlib implementation in v6. > > > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > > Acked-by: Neal Gompa <neal@gompa.dev> > > > > > > Does this Ack add any value? > > > > > > Acked-by is meaningful only when it is given by someone who > > > maintains the relevant area or has established a reputation. > > > > > > $ git grep "Neal Gompa" > > > $ git shortlog -n -s | grep "Neal Gompa" > > > 2 Neal Gompa > > > > > > His Ack feels more like "I like it" rather than a qualified endorsement. > > > > Like Neal explained, an Ack from a potential user of this feature > > seemed relevant, but if you don't think it's meaningful, I can > > certainly drop it. > > Tested-by is more suitable if he wants to leave something. Ack. Neal, I'll drop the acks from v6, but if you end up testing that series, please feel free to add your Tested-by. Sami
On Mon, Nov 18, 2024 at 09:58:09PM +0000, Sami Tolvanen wrote: > Hi, > > On Sat, Nov 16, 2024 at 9:09 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Thu, Nov 14, 2024 at 2:54 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > Hi, > > > > > > On Mon, Nov 11, 2024 at 8:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > > > To avoid duplication between host programs, move the crc32 code to a > > > > > shared header file. > > > > > > > > > > > > Only the motivation to use this long table is to keep compatibility > > > > between genksyms and gendwarfksyms. > > > > I do not think this should be exposed to other programs. > > > > > > > > > > > > If you avoid the code duplication, you can do > > > > > > > > // scripts/gendwarfksyms/crc.c > > > > #include "../genksyms/crc.c" > > > > > > Sure, that sounds reasonable. I'll change this in the next version. > > > > > > BTW, is it necessary to share the same crc function > > between genksyms and gendwarfksyms? > > > > If CONFIG_GENKSYMS and CONFIG_GENDWARFKSYMS > > were able to produce the same CRC, it would be a good motivation > > to share the same function. > > However, as far as I tested, gendwarfksyms generates different CRC values. crc32() is operating on different data, right? CONFIG_GENDWARFKSYMS computes a crc of the DWARF data, whereas CONFIG_GENKSYMS computes a crc of a magic string from ... the source code, right? Hence the crcs will never match? > > When a distro migrates to CONFIG_GENDWARFKSYMS, > > the new kernel cannot load old modules built with CONFIG_GENKSYMS. > > No, it's not necessary to use the exact same function, this was just > to avoid adding more external dependencies. > > > So, there is no need to share the old code. > > Another solution might be to use crc32() provided by zlib, for example. > > It requires another external library, but this already depends on > > libdw and libelf. > > Makes sense. I'll switch to the zlib implementation in v6. > > > > > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > > > Acked-by: Neal Gompa <neal@gompa.dev> > > > > > > > > Does this Ack add any value? > > > > > > > > Acked-by is meaningful only when it is given by someone who > > > > maintains the relevant area or has established a reputation. > > > > > > > > $ git grep "Neal Gompa" > > > > $ git shortlog -n -s | grep "Neal Gompa" > > > > 2 Neal Gompa > > > > > > > > His Ack feels more like "I like it" rather than a qualified endorsement. > > > > > > Like Neal explained, an Ack from a potential user of this feature > > > seemed relevant, but if you don't think it's meaningful, I can > > > certainly drop it. > > > > Tested-by is more suitable if he wants to leave something. > > Ack. Neal, I'll drop the acks from v6, but if you end up testing that > series, please feel free to add your Tested-by. Just my 2 cents, but it seems rude to me to *remove* an Ack from an existing patchset on the grounds that person doesn't appear often in the kernel log. "We won't hire you for this entry level job because you don't have experience" etc. Also, wouldn't Neal be one of the people shepherding this change into distro kernels? He seems to show up somewhat frequently in the Fedora and SUSE ecosystems. Is the problem here that you all think "Acked-by" isn't appropriate from someone who isn't a subsystem maintainer, but the kernel doesn't seem to have a tag for "downstream consumer of this change says they're willing to put their name on the line for this"? Maybe should we go full Parks and Rec: Not-Against-You-On-This-One: Neal Gompa <neal@gompa.dev> --D
Hi Darrick, On Tue, Nov 19, 2024 at 12:48 PM Darrick J. Wong <djwong@kernel.org> wrote: > > On Mon, Nov 18, 2024 at 09:58:09PM +0000, Sami Tolvanen wrote: > > Hi, > > > > On Sat, Nov 16, 2024 at 9:09 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Thu, Nov 14, 2024 at 2:54 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > Hi, > > > > > > > > On Mon, Nov 11, 2024 at 8:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > > > > > To avoid duplication between host programs, move the crc32 code to a > > > > > > shared header file. > > > > > > > > > > > > > > > Only the motivation to use this long table is to keep compatibility > > > > > between genksyms and gendwarfksyms. > > > > > I do not think this should be exposed to other programs. > > > > > > > > > > > > > > > If you avoid the code duplication, you can do > > > > > > > > > > // scripts/gendwarfksyms/crc.c > > > > > #include "../genksyms/crc.c" > > > > > > > > Sure, that sounds reasonable. I'll change this in the next version. > > > > > > > > > BTW, is it necessary to share the same crc function > > > between genksyms and gendwarfksyms? > > > > > > If CONFIG_GENKSYMS and CONFIG_GENDWARFKSYMS > > > were able to produce the same CRC, it would be a good motivation > > > to share the same function. > > > However, as far as I tested, gendwarfksyms generates different CRC values. > > crc32() is operating on different data, right? CONFIG_GENDWARFKSYMS > computes a crc of the DWARF data, whereas CONFIG_GENKSYMS computes a crc > of a magic string from ... the source code, right? Hence the crcs will > never match? Correct, they will never match. > > > > > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > > > > Acked-by: Neal Gompa <neal@gompa.dev> > > > > > > > > > > Does this Ack add any value? > > > > > > > > > > Acked-by is meaningful only when it is given by someone who > > > > > maintains the relevant area or has established a reputation. > > > > > > > > > > $ git grep "Neal Gompa" > > > > > $ git shortlog -n -s | grep "Neal Gompa" > > > > > 2 Neal Gompa > > > > > > > > > > His Ack feels more like "I like it" rather than a qualified endorsement. > > > > > > > > Like Neal explained, an Ack from a potential user of this feature > > > > seemed relevant, but if you don't think it's meaningful, I can > > > > certainly drop it. > > > > > > Tested-by is more suitable if he wants to leave something. > > > > Ack. Neal, I'll drop the acks from v6, but if you end up testing that > > series, please feel free to add your Tested-by. > > Just my 2 cents, but it seems rude to me to *remove* an Ack from an > existing patchset on the grounds that person doesn't appear often in the > kernel log. "We won't hire you for this entry level job because you > don't have experience" etc. > > Also, wouldn't Neal be one of the people shepherding this change into > distro kernels? He seems to show up somewhat frequently in the Fedora > and SUSE ecosystems. > > Is the problem here that you all think "Acked-by" isn't appropriate from > someone who isn't a subsystem maintainer, but the kernel doesn't seem to > have a tag for "downstream consumer of this change says they're willing > to put their name on the line for this"? I certainly appreciate Neal's input, but I don't have a strong opinion about which tag is appropriate. The documentation seems to suggest that Acked-by is _often_ used by maintainers and focuses on that use case, but doesn't explicitly rule out other folks acking patches either: https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by Perhaps Greg, or someone else with more experience with the nuances of acking, can clarify the policy in this situation? Sami
On Mon, Nov 11, 2024 at 11:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Acked-by: Neal Gompa <neal@gompa.dev> > > Does this Ack add any value? > > Acked-by is meaningful only when it is given by someone who > maintains the relevant area or has established a reputation. > > $ git grep "Neal Gompa" > $ git shortlog -n -s | grep "Neal Gompa" > 2 Neal Gompa > > His Ack feels more like "I like it" rather than a qualified endorsement. > I am going to be one of the primary consumers of this code as the Fedora Asahi kernel maintainer and have been driving Rust enablement in the Fedora Linux kernel (and eventually the RHEL kernel). I have tested and looked over the patches from that lens. While I might not be well-known here, I am known by the Rust for Linux folks, as well as the filesystem folks (since those are the main places I tend to frequent). I am also a member of the Asahi Linux team. I've been engaging with the Linux kernel community directly in some fashion for nearly a decade. Unfortunately, most of that isn't in the form of commits directly to the Linux kernel, though. -- 真実はいつも一つ！/ Always, there's only one truth!
On Wed, Nov 13, 2024 at 09:04:51AM -0500, Neal Gompa wrote: > On Mon, Nov 11, 2024 at 11:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > Acked-by: Neal Gompa <neal@gompa.dev> > > > > Does this Ack add any value? > > > > Acked-by is meaningful only when it is given by someone who > > maintains the relevant area or has established a reputation. > > > > $ git grep "Neal Gompa" > > $ git shortlog -n -s | grep "Neal Gompa" > > 2 Neal Gompa > > > > His Ack feels more like "I like it" rather than a qualified endorsement. > > > > I have tested and looked over the patches from that lens. The tests you did, what exaclty was tested? If it was to just ensure no regressions, then that information is useful too, and with that you can just provide: Tested-by. But actual details of what you test are useful. We now also have automation of tests for modules, and expanding test coverage on that is always welcomed too [0] [1] [2], so far we have 0 Rust coverage. [0] https://github.com/linux-kdevops/kdevops [1] https://github.com/linux-kdevops/kdevops-ci [2] https://github.com/linux-kdevops/linux-modules-kpd/actions Luis
On Wed, Nov 13, 2024 at 2:35 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Wed, Nov 13, 2024 at 09:04:51AM -0500, Neal Gompa wrote: > > On Mon, Nov 11, 2024 at 11:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > Suggested-by: Petr Pavlu <petr.pavlu@suse.com> > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > > Acked-by: Neal Gompa <neal@gompa.dev> > > > > > > Does this Ack add any value? > > > > > > Acked-by is meaningful only when it is given by someone who > > > maintains the relevant area or has established a reputation. > > > > > > $ git grep "Neal Gompa" > > > $ git shortlog -n -s | grep "Neal Gompa" > > > 2 Neal Gompa > > > > > > His Ack feels more like "I like it" rather than a qualified endorsement. > > > > > > > I have tested and looked over the patches from that lens. > > The tests you did, what exaclty was tested? > > If it was to just ensure no regressions, then that information is > useful too, and with that you can just provide: Tested-by. > > But actual details of what you test are useful. We now also have > automation of tests for modules, and expanding test coverage on that is > always welcomed too [0] [1] [2], so far we have 0 Rust coverage. > I tested that I could turn on MODVERSIONS with the relevant patch series, and get symbol expressions out of it. I didn't go very far into it, because I didn't want to invest in reworking the kernel symbol dependency generator for RPM packaged kernels to leverage this until after it is finally merged. But it worked as described, even if none of our kernel packaging infrastructure is adapted for it yet. To be honest, I considered sending both Acked-by and Tested-by, but I figured Acked-by would be sufficient given that I have also looked over the code as well and thought it was reasonable. Strictly speaking, we don't really use MODVERSIONS in Fedora, it's a CentOS/RHEL thing. But, pushing this forward helps make Rust enablement for Fedora easier because the Fedora kernel is managed through the ARK project, which is mostly oriented around the needs of the RHEL kernel team. And CentOS Hyperscale (which I am also part of and maintain a kernel for) does have MODVERSIONS on and I would like to be able to start enabling Rust stuff there (particularly Asahi code, as that's currently the main interesting thing in Rust to use). -- 真実はいつも一つ！/ Always, there's only one truth!
© 2016 - 2024 Red Hat, Inc.