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

[Conor Dooley]

On Fri, Feb 10, 2023 at 07:26:38PM +0000, Nick Alcock wrote:
> On 10 Feb 2023, Conor Dooley said:
> > 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!

> > 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@xxxxxxxxxxxxx/
> 
> 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.

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

> >> Signed-off-by: Nick Alcock <nick.alcock@xxxxxxxxxx>
> >> Suggested-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> >> Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> >> Cc: linux-modules@xxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> Cc: linux-pci@xxxxxxxxxxxxxxx
> >> Cc: linux-riscv@xxxxxxxxxxxxxxxxxxx
> >
> > 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@xxxxxxxxxxxxx> (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!)

Attachment: signature.asc
Description: PGP signature