Re: [PATCH v5 01/19] scripts: move genksyms crc32 implementation to a common include

[Sami Tolvanen <samitolvanen@google.com>]

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