Re: [PATCH 8/8] kbuild, PCI: microchip: comment out MODULE_LICENSE in non-modules

On Fri, Feb 10, 2023 at 07:26:38PM +0000, Nick Alcock wrote:

On 10 Feb 2023, Conor Dooley said:

quoted

FYI $subject seems wrong, this is a PCI patch AFAICT.

I'm deriving the prefixes automatically because there are so many of
them, picking that prefix which is most commonly used across all files I
touch in a given subsystem and which is present in all of them. So
the PCI: microchip part seems right to me.

Ye, not disagreeing with that part.

kbuild is present in every patch in the series because this is a
kbuild-driven change (the thing it disturbs is part of the build system,
the construction of modules.builtin*). This seems to be common practice
for kbuild-related treewide changes.

Okay, I'll take your word for it. It just looked/looks odd to me!

quoted

On Fri, Feb 10, 2023 at 04:47:49PM +0000, Nick Alcock wrote:

quoted

Since commit 8b41fc4454e ("kbuild: create modules.builtin without
Makefile.modbuiltin or tristate.conf"), MODULE_LICENSE declarations
are used to identify modules. As a consequence, uses of the macro
in non-modules will cause modprobe to misidentify their containing
object file as a module when it is not (false positives), and modprobe
might succeed rather than failing with a suitable error message.

So comment out all uses of MODULE_LICENSE that are not in real modules

This patch should not been needed, there's an existing patch to make
this a module:
https://lore.kernel.org/linux-riscv/20230111125323.1911373-4-daire.mcnamara@microchip.com/ (local)

Excellent: if that's likely to go in I can take this one out.

Hopefully! It's the removable modules that are seemingly a no-go for
PCI.
I'll prod Daire on Monday about responding to the comments and perhaps
the start of the series could get picked up. I'm not too sure if that's
something that the PCI folk do though.

quoted

quoted

(the license declaration is left in as documentation).

I don't really get this one though, why leave it there as
"documentation" when the file has an SPDX entry anyway?

I was asked to. (The first version of this series just deleted it, but
people were unhappy about the outright deletion of what looked like
license info and the loss of MODULE_LICENSE while other MODULE_* was
retained.)

I saw Luis' name on the suggestion which is why I asked rather than
dismiss it offhand. I think that's kinda silly, but I guess license
stuff invites paranoia.
I'd been kinda keeping an eye on the series as I know I've got a
non-module clk driver that has MODULE_CRAP() in it & been debating
whether I should just go & delete it all.
Still undecided.

quoted

quoted

Signed-off-by: Nick Alcock <redacted>
Suggested-by: Luis Chamberlain <mcgrof@kernel.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: linux-modules@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: linux-riscv@lists.infradead.org

One for the future:
How about also CCing those listed in MAINTAINERS for the file you're
changing?

That... was supposed to happen. I'm invoking

scripts/get_maintainer.pl --email --m --n --l --subsystem -f $files

and then parsing the result for maintainer:, open list: and moderated
list: lines. I thought that would catch everything, but now I see
this is more complex.  You are there as an M: line, so I thought you'd
be reported as a maintainer:, but nooo:

Conor Dooley [off-list ref] (supporter:RISC-V MICROCHIP FPGA SUPPORT)

I didn't realise that 'supporter' was a thing and thought
get_maintainer.pl always reported maintainers as maintainer:. I'll
resplit the series with supporters Cc:ed just like maintainers are.
Looks like those are the only two I need to pick up.

Supporter is a weird one I suppose, without reading the header* it's a
little confusing. There was a thread recently with an attempt to
disambiguate in the submitting patches docs, and I /think/ one of what
they wanted to do was make get_maintainer return maintainer for both
maintainer and supporter to align better with the docs.

*Because who reads the intro/header explanation of the file, right? Just
run the script and/or jump straight to the relevant section

Sorry about that.

Dw about it, sorry if I came across a little brisk.

(This sort of systemic scripting bug is why I did a small tranche first.
So thank you very much!)