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

From: Nick Alcock <nick.alcock@oracle.com>
To: Conor Dooley <conor@kernel.org>
Cc: mcgrof@kernel.org, linux-modules@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH 8/8] kbuild, PCI: microchip: comment out MODULE_LICENSE in non-modules
Date: Fri, 10 Feb 2023 19:26:38 +0000	[thread overview]
Message-ID: <871qmx1fv5.fsf@esperi.org.uk> (raw)
In-Reply-To: <Y+aMh5e9bWaTphiZ@spud> (Conor Dooley's message of "Fri, 10 Feb 2023 18:27:19 +0000")

On 10 Feb 2023, Conor Dooley said:

> Hey Nick,
>
> 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.

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.

> On Fri, Feb 10, 2023 at 04:47:49PM +0000, Nick Alcock wrote:
>> 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/

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

>> (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.)

>> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
>> 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 <conor.dooley@microchip.com> (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.

Sorry about that.

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