LinuxLists.cc - [PATCH 0/8] MODULE_LICENSE removals, first tranche

2023-02-10 16:58:02

Subject: [PATCH 0/8] MODULE_LICENSE removals, first tranche

This series, based on current modules-next, is part of a treewide cleanup
suggested by Luis Chamberlain, to remove the LICENSE_MODULE usage from
files/objects that are not tristate. Due to recent changes to kbuild, these
uses are now problematic. See the commit logs for more details.

This is a small initial tranche to see if the general approach is valid:
larger tranches can follow if desired and if these patches seem OK.
(In total, there are 123 patches.)

The series at a whole can be found here:
https://github.com/nickalcock/linux module-license

The kallsyms series is at https://github.com/nickalcock/linux kallsyms

The tristate checker used to put this series together (not for upstreaming yet,
and not necessary for any of this to work) is at
https://github.com/nickalcock/linux tristate-checker

Cc: Luis Chamberlain <[email protected]>
Cc: [email protected]
Cc: [email protected]

Nick Alcock (8):
kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in
non-modules
kbuild, PCI: mobiveil: comment out MODULE_LICENSE in non-modules
kbuild, ARM: tegra: comment out MODULE_LICENSE in non-modules
kbuild, PCI: endpoint: comment out MODULE_LICENSE in non-modules
kbuild, PCI: hip: comment out MODULE_LICENSE in non-modules
kbuild, shpchp: comment out MODULE_LICENSE in non-modules
kbuild, PCI: dwc: histb: comment out MODULE_LICENSE in non-modules
kbuild, PCI: microchip: comment out MODULE_LICENSE in non-modules

drivers/pci/controller/dwc/pcie-histb.c | 2 +-
drivers/pci/controller/mobiveil/pcie-mobiveil-plat.c | 2 +-
drivers/pci/controller/pci-tegra.c | 2 +-
drivers/pci/controller/pci-versatile.c | 2 +-
drivers/pci/controller/pcie-hisi-error.c | 2 +-
drivers/pci/controller/pcie-microchip-host.c | 2 +-
drivers/pci/endpoint/pci-ep-cfs.c | 2 +-
drivers/pci/endpoint/pci-epc-core.c | 2 +-
drivers/pci/endpoint/pci-epc-mem.c | 2 +-
drivers/pci/endpoint/pci-epf-core.c | 2 +-
drivers/pci/hotplug/shpchp_core.c | 2 +-
11 files changed, 11 insertions(+), 11 deletions(-)

--
2.39.1.268.g9de2f9a303

2023-02-10 17:06:02

by Nick Alcock

[permalink] [raw]

Subject: [PATCH 1/8] kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in non-modules

2023-02-10 17:14:58

by Nick Alcock

[permalink] [raw]

Subject: [PATCH 2/8] kbuild, PCI: mobiveil: comment out MODULE_LICENSE in non-modules

2023-02-10 17:33:13

by Nick Alcock

[permalink] [raw]

Subject: [PATCH 3/8] kbuild, ARM: tegra: comment out MODULE_LICENSE in non-modules

2023-02-10 17:36:33

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 1/8] kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in non-modules

On Fri, Feb 10, 2023 at 11:05 AM Nick Alcock <[email protected]> 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.

How is there an issue when any given module could be built-in instead?

The general trend is to make all PCI host drivers modules, the primary
reason this one, IIRC, is not a module is because it is missing
remove() hook to de-init the PCI bus. Not too hard to add, but I
wanted to do a common devm hook to do that instead of an explicit
.remove() hook in each driver. I suppose we could just ignore that and
allow building as a module. Unloading is optional anyways.

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

That's a comment for the series more than just this patch.

> (the license declaration is left in as documentation).
>
> Signed-off-by: Nick Alcock <[email protected]>
> Suggested-by: Luis Chamberlain <[email protected]>
> Cc: Luis Chamberlain <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Rob Herring <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/pci/controller/pci-versatile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

2023-02-10 17:41:44

by Nick Alcock

[permalink] [raw]

Subject: [PATCH 4/8] kbuild, PCI: endpoint: comment out MODULE_LICENSE in non-modules

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
(the license declaration is left in as documentation).

Signed-off-by: Nick Alcock <[email protected]>
Suggested-by: Luis Chamberlain <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/pci/endpoint/pci-ep-cfs.c | 2 +-
drivers/pci/endpoint/pci-epc-core.c | 2 +-
drivers/pci/endpoint/pci-epc-mem.c | 2 +-
drivers/pci/endpoint/pci-epf-core.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index d4850bdd837f..52cbe1a5d677 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -728,4 +728,4 @@ module_exit(pci_ep_cfs_exit);

MODULE_DESCRIPTION("PCI EP CONFIGFS");
MODULE_AUTHOR("Kishon Vijay Abraham I <[email protected]>");
-MODULE_LICENSE("GPL v2");
+/* MODULE_LICENSE("GPL v2"); */
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 2542196e8c3d..52498ac868ab 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -861,4 +861,4 @@ module_exit(pci_epc_exit);

MODULE_DESCRIPTION("PCI EPC Library");
MODULE_AUTHOR("Kishon Vijay Abraham I <[email protected]>");
-MODULE_LICENSE("GPL v2");
+/* MODULE_LICENSE("GPL v2"); */
diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
index a97b56a6d2db..a3280f699f96 100644
--- a/drivers/pci/endpoint/pci-epc-mem.c
+++ b/drivers/pci/endpoint/pci-epc-mem.c
@@ -260,4 +260,4 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_free_addr);

MODULE_DESCRIPTION("PCI EPC Address Space Management");
MODULE_AUTHOR("Kishon Vijay Abraham I <[email protected]>");
-MODULE_LICENSE("GPL v2");
+/* MODULE_LICENSE("GPL v2"); */
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 9ed556936f48..002310a3b55f 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -568,4 +568,4 @@ module_exit(pci_epf_exit);

MODULE_DESCRIPTION("PCI EPF Library");
MODULE_AUTHOR("Kishon Vijay Abraham I <[email protected]>");
-MODULE_LICENSE("GPL v2");
+/* MODULE_LICENSE("GPL v2"); */
--
2.39.1.268.g9de2f9a303

2023-02-10 17:51:06

by Nick Alcock

[permalink] [raw]

Subject: [PATCH 5/8] kbuild, PCI: hip: comment out MODULE_LICENSE in non-modules

2023-02-10 17:59:41

by Nick Alcock

[permalink] [raw]

Subject: [PATCH 6/8] kbuild, shpchp: comment out MODULE_LICENSE in non-modules

2023-02-10 18:08:44

by Nick Alcock

[permalink] [raw]

Subject: [PATCH 7/8] kbuild, PCI: dwc: histb: comment out MODULE_LICENSE in non-modules

2023-02-10 18:17:52

by Nick Alcock

[permalink] [raw]

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

2023-02-10 18:27:30

by Conor Dooley

[permalink] [raw]

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

Hey Nick,

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

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/[email protected]/

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

> Signed-off-by: Nick Alcock <[email protected]>
> Suggested-by: Luis Chamberlain <[email protected]>
> Cc: Luis Chamberlain <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]

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

Cheers,
Conor.

> ---
> drivers/pci/controller/pcie-microchip-host.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 0ebf7015e9af..08c0b778ce67 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -1135,6 +1135,6 @@ static struct platform_driver mc_pcie_driver = {
> };
>
> builtin_platform_driver(mc_pcie_driver);
> -MODULE_LICENSE("GPL");
> +/* MODULE_LICENSE("GPL"); */
> MODULE_DESCRIPTION("Microchip PCIe host controller driver");
> MODULE_AUTHOR("Daire McNamara <[email protected]>");
> --
> 2.39.1.268.g9de2f9a303
>

Attachments:

(No filename) (2.04 kB)
signature.asc (228.00 B)
Download all attachments

2023-02-10 18:53:05

by Nick Alcock

[permalink] [raw]

Subject: Re: [PATCH 1/8] kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in non-modules

On 10 Feb 2023, Rob Herring uttered the following:

> On Fri, Feb 10, 2023 at 11:05 AM Nick Alcock <[email protected]> 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.
>
> How is there an issue when any given module could be built-in instead?

"modprobe module-that-might-be-built-in" is always meant to succeed if
the module is built-in; but "modprobe thing-that-can't-be-a-module-at-all"
is meant to fail. e.g. on a system in which ext4 is built in, I see

loom:~# modprobe ext4
loom:~# lsmod | grep ext4

(with either reporting any answer, and the modprobe returning exitcode 0).

But trying to modprobe something that cannot be a module says, e.g.
(sorry for old kernel, just happens to be what I can lay my hands on
easily right now):

loom:~# modprobe slab
modprobe: FATAL: Module slab not found in directory /lib/modules/5.16.19-00037-ge8dfda4e77fb-dirty
[exitcode nonzero]

This is what is expected, even though slab is built in. It's not a
module, it cannot be a module, so trying to modprobe it should fail.

But right now we have things like this:

silk:~# modprobe zswap
[nothing, exitcode 0]

zswap cannot be built as a module, so this output is wrong (and
inconsistent with the slab attempt above). (Sure, this isn't exactly a
disastrous consequence, but I have other things I'm going to contribute
after this series that depend on this being got right consistently.)

> The general trend is to make all PCI host drivers modules, the primary
> reason this one, IIRC, is not a module is because it is missing
> remove() hook to de-init the PCI bus. Not too hard to add, but I
> wanted to do a common devm hook to do that instead of an explicit
> .remove() hook in each driver. I suppose we could just ignore that and
> allow building as a module. Unloading is optional anyways.

That's perfectly acceptable for me -- I'm not saying that these things
should not be modular, only that *as long as* they are not modular, they
should not have a MODULE_LICENSE. Making it possible to build them as
modules again is fine!

--
NULL && (void)

2023-02-10 19:27:03

by Nick Alcock

[permalink] [raw]

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

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/[email protected]/

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 <[email protected]>
>> Suggested-by: Luis Chamberlain <[email protected]>
>> Cc: Luis Chamberlain <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>
> 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 <[email protected]> (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!)

2023-02-10 20:10:53

by Conor Dooley

[permalink] [raw]

Subject: 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:
> > 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/[email protected]/
>
> 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 <[email protected]>
> >> Suggested-by: Luis Chamberlain <[email protected]>
> >> Cc: Luis Chamberlain <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >
> > 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 <[email protected]> (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!)

Attachments:

(No filename) (4.34 kB)
signature.asc (228.00 B)
Download all attachments

2023-02-12 18:37:29

by Leon Romanovsky

[permalink] [raw]

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

On Fri, Feb 10, 2023 at 08:10:43PM +0000, Conor Dooley wrote:
> 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.

<...>

> > 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!

It looks odd to me too. Please add SPDX tag in modules which don't have
it already, instead of commenting code.

Thanks

2023-02-12 19:53:20

by Nick Alcock

[permalink] [raw]

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

On 12 Feb 2023, Leon Romanovsky uttered the following:

> On Fri, Feb 10, 2023 at 08:10:43PM +0000, Conor Dooley wrote:
>> 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.
>
> <...>
>
>> > 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!
>
> It looks odd to me too. Please add SPDX tag in modules which don't have
> it already, instead of commenting code.

OK, I now have two votes for removal-and-SPDX (you and Luis) and nobody
suggesting that keeping it in but commented out is actually a good idea:
I'll respin with removals instead, and add SPDX to anything so adjusted
that doesn't already have it (if anything).

(I'll stick both the removal and addition in the same commit, so there
is no point at which such files have no declared license at all.)

--
NULL && (void)

2023-02-13 15:54:29

by Nick Alcock

[permalink] [raw]

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

On 12 Feb 2023, Leon Romanovsky told this:

> On Fri, Feb 10, 2023 at 08:10:43PM +0000, Conor Dooley wrote:
>> 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.
>
> <...>
>
>> > 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!
>
> It looks odd to me too. Please add SPDX tag in modules which don't have
> it already, instead of commenting code.

Alas... nearly all of them *do* have it already, and in most cases it is
different. Usually not *very* different, but different. In most cases it
is more specific, e.g. drivers/soc/fujitsu/a64fx-diag.c, where we have
MODULE_LICENSE("GPL") but SPDX says it's GPL-2.0-only, but then there
are things like lib/packing.c, which throughout its history in the tree
has combined // SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
and MODULE_LICENSE("GPL v2"); which are just not the same thing.

I commented the MODULE_LICENSEs out specifically because I wanted to
avoid getting into hundreds of simultaneous license flamewars while
trying to get *a different thing entirely* into the kernel (kallmodsyms,
which depends on modules.builtin.objs being correct).

I still don't want to get into hundreds of simultaneous license
flamewars, so I think I'll leave things commented out and let
individual maintainers decide whether they want to reconcile
contradictory info or not.

And if I'm not doing that, I feel I shouldn't really be adding SPDX
headers to files that lack them, given that I demonstrably cannot use
MODULE_LICENSE to tell me what the license is meant to be. But if we
can't rely on MODULE_LICENSE to specify the license, and it seems like
we can't, I'd say that it is truly redundant in those files that have
SPDXs, and should probably emit a series that removes MODULE_LICENSE
when files have SPDXes, and comments them out otherwise.

Does that sound reasonable to everyone?

--
NULL && (void)

2023-02-13 16:13:27

by Nick Alcock

[permalink] [raw]

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

[Modified resend: my MTA claimed not to send it but then sent it to some
recipients anyway, and then I was asked not to do some of the things
I'd offered after I sent it.]

On 12 Feb 2023, Leon Romanovsky told this:

> On Fri, Feb 10, 2023 at 08:10:43PM +0000, Conor Dooley wrote:
>> 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.
>
> <...>
>
>> > 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!
>
> It looks odd to me too. Please add SPDX tag in modules which don't have
> it already, instead of commenting code.

Alas... nearly all of them *do* have it already, and in most cases it is
different. Usually not *very* different, but different. In most cases it
is more specific, e.g. drivers/soc/fujitsu/a64fx-diag.c, where we have
MODULE_LICENSE("GPL") but SPDX says it's GPL-2.0-only, but then there
are things like lib/packing.c, which throughout its history in the tree
has combined // SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
and MODULE_LICENSE("GPL v2"); which are just not the same thing.

I commented the MODULE_LICENSEs out specifically because I wanted to
avoid getting into hundreds of simultaneous license flamewars while
trying to get *a different thing entirely* into the kernel (kallmodsyms,
which depends on modules.builtin.objs being correct).

I still don't want to get into hundreds of simultaneous license
flamewars or get my employer into legal hot water, so I think I'll leave
things commented out and let individual maintainers decide whether they
want to reconcile any contradictory info that may exist or not (and as
noted *most* of these are conflicting.)

This email is the closest thing I have to indicating what Luis would
prefer (and the only reason I'm doing this is because I need it before
Luis's modules.builtin.objs change can work):

<https://lore.kernel.org/linux-modules/[email protected]/>

Yes, Luis thinks we can just use SPDX, but given that they are usually
different, making such a change seems well beyond my pay grade. Even in
the PCI domain, we see (second column, MODULE_LICENSE: third: SPDX,
sorry about the line lengths).

drivers/pci/controller/dwc/pcie-histb.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/controller/mobiveil/pcie-mobiveil-plat.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/controller/pci-tegra.c: GPL // SPDX-License-Identifier: GPL-2.0+
drivers/pci/controller/pci-versatile.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/controller/pcie-hisi-error.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/controller/pcie-microchip-host.c: GPL // SPDX-License-Identifier: GPL-2.0
drivers/pci/endpoint/pci-ep-cfs.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/endpoint/pci-epc-core.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/endpoint/pci-epc-mem.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/endpoint/pci-epf-core.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
drivers/pci/hotplug/acpiphp_core.c: GPL // SPDX-License-Identifier: GPL-2.0+
drivers/pci/hotplug/shpchp_core.c: GPL // SPDX-License-Identifier: GPL-2.0+

Not much in the way of consistency here: GPL sometimes means 2.0+ but
sometimes it means 2.0. GPL v2 appears to consistently mean GPL-2.0, but
if you look at other affected modules you soon see inconsistency:

drivers/powercap/powercap_sys.c: GPL v2 // SPDX-License-Identifier: GPL-2.0-only
drivers/firmware/imx/imx-scu.c: GPL v2 // SPDX-License-Identifier: GPL-2.0+
arch/x86/crypto/blake2s-glue.c: GPL v2 // SPDX-License-Identifier: GPL-2.0 OR MIT
drivers/iommu/sun50i-iommu.c: Dual BSD/GPL // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

We even have

drivers/gpu/drm/drm_mipi_dsi.c: "GPL and additional rights" (header is
non-SPDX -- a BSD license header with advertising clause!)

So SPDX is usually more precise than the MODULE_LICENSE, but is it more
*accurate*? I have no idea, and I don't see how I could possibly know:
going by the presence of advertising clauses that obviously nobody is
obeying it doesn't seem like we can trust header comments to be any more
accurate than MODULE_LICENSE. Best to just leave both in (and comment it
out so it has no side-effects on the build any more, which is all I'm
after).

--
NULL && (void)

2023-02-13 16:51:33

by Conor Dooley

[permalink] [raw]

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

On Mon, Feb 13, 2023 at 04:13:00PM +0000, Nick Alcock wrote:
> [Modified resend: my MTA claimed not to send it but then sent it to some
> recipients anyway, and then I was asked not to do some of the things
> I'd offered after I sent it.]
>
> On 12 Feb 2023, Leon Romanovsky told this:
>
> > On Fri, Feb 10, 2023 at 08:10:43PM +0000, Conor Dooley wrote:
> >> 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.
> >
> > <...>
> >
> >> > 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!
> >
> > It looks odd to me too. Please add SPDX tag in modules which don't have
> > it already, instead of commenting code.
>
> Alas... nearly all of them *do* have it already, and in most cases it is
> different. Usually not *very* different, but different. In most cases it
> is more specific, e.g. drivers/soc/fujitsu/a64fx-diag.c, where we have
> MODULE_LICENSE("GPL") but SPDX says it's GPL-2.0-only, but then there
> are things like lib/packing.c, which throughout its history in the tree
> has combined // SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> and MODULE_LICENSE("GPL v2"); which are just not the same thing.
>
> I commented the MODULE_LICENSEs out specifically because I wanted to
> avoid getting into hundreds of simultaneous license flamewars while
> trying to get *a different thing entirely* into the kernel (kallmodsyms,
> which depends on modules.builtin.objs being correct).
>
> I still don't want to get into hundreds of simultaneous license
> flamewars or get my employer into legal hot water, so I think I'll leave
> things commented out and let individual maintainers decide whether they
> want to reconcile any contradictory info that may exist or not (and as
> noted *most* of these are conflicting.)
>
>
> This email is the closest thing I have to indicating what Luis would
> prefer (and the only reason I'm doing this is because I need it before
> Luis's modules.builtin.objs change can work):
>
> <https://lore.kernel.org/linux-modules/[email protected]/>
>
> Yes, Luis thinks we can just use SPDX, but given that they are usually
> different, making such a change seems well beyond my pay grade. Even in
> the PCI domain, we see (second column, MODULE_LICENSE: third: SPDX,
> sorry about the line lengths).
>
> drivers/pci/controller/dwc/pcie-histb.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/mobiveil/pcie-mobiveil-plat.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/pci-tegra.c: GPL // SPDX-License-Identifier: GPL-2.0+
> drivers/pci/controller/pci-versatile.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/pcie-hisi-error.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/pcie-microchip-host.c: GPL // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-ep-cfs.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-epc-core.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-epc-mem.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-epf-core.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/hotplug/acpiphp_core.c: GPL // SPDX-License-Identifier: GPL-2.0+
> drivers/pci/hotplug/shpchp_core.c: GPL // SPDX-License-Identifier: GPL-2.0+
>
> Not much in the way of consistency here: GPL sometimes means 2.0+ but
> sometimes it means 2.0. GPL v2 appears to consistently mean GPL-2.0, but
> if you look at other affected modules you soon see inconsistency:
>
> drivers/powercap/powercap_sys.c: GPL v2 // SPDX-License-Identifier: GPL-2.0-only
> drivers/firmware/imx/imx-scu.c: GPL v2 // SPDX-License-Identifier: GPL-2.0+
> arch/x86/crypto/blake2s-glue.c: GPL v2 // SPDX-License-Identifier: GPL-2.0 OR MIT
> drivers/iommu/sun50i-iommu.c: Dual BSD/GPL // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

See bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2"
bogosity") for more information on the contents of MODULE_LICENSE.

I don't really have a comment on the rest of this, other than thinking
that, for the microchip one, you should leave it as is & the driver be
changed to be module capable.

>
> We even have
>
> drivers/gpu/drm/drm_mipi_dsi.c: "GPL and additional rights" (header is
> non-SPDX -- a BSD license header with advertising clause!)
>
> So SPDX is usually more precise than the MODULE_LICENSE, but is it more
> *accurate*? I have no idea, and I don't see how I could possibly know:
> going by the presence of advertising clauses that obviously nobody is
> obeying it doesn't seem like we can trust header comments to be any more
> accurate than MODULE_LICENSE. Best to just leave both in (and comment it
> out so it has no side-effects on the build any more, which is all I'm
> after).
>
> --
> NULL && (void)

Attachments:

(No filename) (5.11 kB)
signature.asc (228.00 B)
Download all attachments

2023-02-13 17:06:29

by Leon Romanovsky

[permalink] [raw]

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

On Mon, Feb 13, 2023 at 04:13:00PM +0000, Nick Alcock wrote:
> [Modified resend: my MTA claimed not to send it but then sent it to some
> recipients anyway, and then I was asked not to do some of the things
> I'd offered after I sent it.]
>
> On 12 Feb 2023, Leon Romanovsky told this:
>
> > On Fri, Feb 10, 2023 at 08:10:43PM +0000, Conor Dooley wrote:
> >> 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.
> >
> > <...>
> >
> >> > 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!
> >
> > It looks odd to me too. Please add SPDX tag in modules which don't have
> > it already, instead of commenting code.
>
> Alas... nearly all of them *do* have it already, and in most cases it is
> different. Usually not *very* different, but different. In most cases it
> is more specific, e.g. drivers/soc/fujitsu/a64fx-diag.c, where we have
> MODULE_LICENSE("GPL") but SPDX says it's GPL-2.0-only, but then there
> are things like lib/packing.c, which throughout its history in the tree
> has combined // SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> and MODULE_LICENSE("GPL v2"); which are just not the same thing.
>
> I commented the MODULE_LICENSEs out specifically because I wanted to
> avoid getting into hundreds of simultaneous license flamewars while
> trying to get *a different thing entirely* into the kernel (kallmodsyms,
> which depends on modules.builtin.objs being correct).
>
> I still don't want to get into hundreds of simultaneous license
> flamewars or get my employer into legal hot water, so I think I'll leave
> things commented out and let individual maintainers decide whether they
> want to reconcile any contradictory info that may exist or not (and as
> noted *most* of these are conflicting.)
>
>
> This email is the closest thing I have to indicating what Luis would
> prefer (and the only reason I'm doing this is because I need it before
> Luis's modules.builtin.objs change can work):
>
> <https://lore.kernel.org/linux-modules/[email protected]/>
>
> Yes, Luis thinks we can just use SPDX, but given that they are usually
> different, making such a change seems well beyond my pay grade. Even in
> the PCI domain, we see (second column, MODULE_LICENSE: third: SPDX,
> sorry about the line lengths).
>
> drivers/pci/controller/dwc/pcie-histb.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/mobiveil/pcie-mobiveil-plat.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/pci-tegra.c: GPL // SPDX-License-Identifier: GPL-2.0+
> drivers/pci/controller/pci-versatile.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/pcie-hisi-error.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/controller/pcie-microchip-host.c: GPL // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-ep-cfs.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-epc-core.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-epc-mem.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/endpoint/pci-epf-core.c: GPL v2 // SPDX-License-Identifier: GPL-2.0
> drivers/pci/hotplug/acpiphp_core.c: GPL // SPDX-License-Identifier: GPL-2.0+
> drivers/pci/hotplug/shpchp_core.c: GPL // SPDX-License-Identifier: GPL-2.0+
>
> Not much in the way of consistency here: GPL sometimes means 2.0+ but
> sometimes it means 2.0. GPL v2 appears to consistently mean GPL-2.0, but
> if you look at other affected modules you soon see inconsistency:
>
> drivers/powercap/powercap_sys.c: GPL v2 // SPDX-License-Identifier: GPL-2.0-only
> drivers/firmware/imx/imx-scu.c: GPL v2 // SPDX-License-Identifier: GPL-2.0+
> arch/x86/crypto/blake2s-glue.c: GPL v2 // SPDX-License-Identifier: GPL-2.0 OR MIT
> drivers/iommu/sun50i-iommu.c: Dual BSD/GPL // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>
> We even have
>
> drivers/gpu/drm/drm_mipi_dsi.c: "GPL and additional rights" (header is
> non-SPDX -- a BSD license header with advertising clause!)
>
> So SPDX is usually more precise than the MODULE_LICENSE, but is it more
> *accurate*? I have no idea, and I don't see how I could possibly know:
> going by the presence of advertising clauses that obviously nobody is
> obeying it doesn't seem like we can trust header comments to be any more
> accurate than MODULE_LICENSE. Best to just leave both in (and comment it
> out so it has no side-effects on the build any more, which is all I'm
> after).

You are overcomplicating things.

First, GPL == GPL v2.
Second, SPDX is the right one. License in module is needed to limit
EXPORT_SYMBOL* exposure.
Third, we have git log and git blame to audit and revert any change.
There is no need in leaving (even as commented) dead code.

Thanks

>
> --
> NULL && (void)

2023-02-13 17:30:51

by Jonathan Corbet

[permalink] [raw]

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

Leon Romanovsky <[email protected]> writes:

> It looks odd to me too. Please add SPDX tag in modules which don't have
> it already, instead of commenting code.

So I'm just a bystander here and should probably be ignored, but ...

From what I can see, Nick is attempting one of those cross-tree cleanups
that's painful enough to do on its own. This request is asking him to
perform a different, unrelated, and potentially fraught cleanup that the
maintainers of the code in question have not yet managed to get around
to taking care of. This will impede an already prolonged process and,
IMO, unnecessarily so.

Wouldn't it be better to let this work proceed while making a note
of the files still needing SPDX tags?

I'll shut up now :)

Thanks,

jon

2023-02-13 19:24:13

by Leon Romanovsky

[permalink] [raw]

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

On Mon, Feb 13, 2023 at 10:30:44AM -0700, Jonathan Corbet wrote:
> Leon Romanovsky <[email protected]> writes:
>
> > It looks odd to me too. Please add SPDX tag in modules which don't have
> > it already, instead of commenting code.
>
> So I'm just a bystander here and should probably be ignored, but ...
>
> From what I can see, Nick is attempting one of those cross-tree cleanups
> that's painful enough to do on its own. This request is asking him to
> perform a different, unrelated, and potentially fraught cleanup that the
> maintainers of the code in question have not yet managed to get around
> to taking care of. This will impede an already prolonged process and,
> IMO, unnecessarily so.
>
> Wouldn't it be better to let this work proceed while making a note
> of the files still needing SPDX tags?

Please see a note from Nick, who said that these tags were already
in-place for most of the files. If it is hard for him, he can skip
adding new tags. However, the proposed solution is to comment code
and leave dead code is not a right solution.

Thanks

>
> I'll shut up now :)
>
> Thanks,
>
> jon

2023-02-13 22:57:32

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH 1/8] kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in non-modules

On Fri, Feb 10, 2023 at 04:47:42PM +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
> (the license declaration is left in as documentation).

Weird that all the patches are for drivers/pci/, but the cover letter
didn't go to [email protected].

- Please drop "kbuild," from the subject; I don't think it's really
relevant.

- Please follow the subject line convention for each file. They're
mostly there after dropping "kbuild", but do capitalize the
sentence that follows the prefix. The prefix should always be
"PCI/<driver-tag>: "

- Remove the MODULE_LICENSE instead of commenting it out.

- I think every file in drivers/pci that needs one already has SPDX.

- AFAICT, SPDX is the dispositive license and MODULE_LICENSE just
determines which interfaces are available to the module, so
dropping MODULE_LICENSE shouldn't be a problem as far as legal
issues.

2023-02-14 15:41:59

by Nick Alcock

[permalink] [raw]

Subject: Re: [PATCH 1/8] kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in non-modules

On 13 Feb 2023, Bjorn Helgaas spake thusly:

> On Fri, Feb 10, 2023 at 04:47:42PM +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
>> (the license declaration is left in as documentation).
>
> Weird that all the patches are for drivers/pci/, but the cover letter
> didn't go to [email protected].
>
> - Please drop "kbuild," from the subject; I don't think it's really
> relevant.

Every other treewide kbuild modification that I can see has a kbuild
prefix. Many of them have *nothing* else, just kbuild:, no per-subsystem
descriptor at all. If I remove the kbuild: prefix from all these
commits, will other people promptly complain? Luis?

> - Please follow the subject line convention for each file. They're
> mostly there after dropping "kbuild", but do capitalize the
> sentence that follows the prefix. The prefix should always be
> "PCI/<driver-tag>: "

These prefixes were automatically generated by a splitting script which
computes a prefix to apply to each commit's log by reusing the most
commonly used prefix which is present at least once in all affected
files in the subsystem (in this case, that's simple because there's just
one file).

(However, there appears to be a bug in the script here: "PCI:
generic,versatile" appears only once. I would expect "PCI: versatile",
which is used repeatedly in the history of that file. Will fix.)

I guess the convention you refer to is new, because there are a total of
zero uses of "PCI/versatile" in the entirety of git history. Are you
*sure* you want me to use that?

(More generally, I have no idea where these "driver tags" come from,
hence the weird ad-hoc approach I had to use to generate commit log
first-line prefixes when splitting this commit by subsystem. There seems
to be very little consistency here. They certainly don't come from
MAINTAINERS or from the files themselves, nor are they the filename, at
least not always. Could you give me some sort of procedure for
generating them, if picking popular prefixes from git history won't cut
it? It's essential that the process be automatable: I've had to resplit
this commit more than half a dozen times already and if I have to label
each commit by hand every time it will become a nightmare of human
error. If the rules for generating prefixes vary by subsystem this means
I'll have to fight through God knows how many annoyed maintainers to get
this incredibly trivial change in.)

> - AFAICT, SPDX is the dispositive license and MODULE_LICENSE just
> determines which interfaces are available to the module, so
> dropping MODULE_LICENSE shouldn't be a problem as far as legal
> issues.

Yeah, looks like. I'll just drop them, everyone seems to want that. If
someone dislikes my doing that they can always put a commented-out
MODULE_LICENSE back.

--
NULL && (void)

2023-02-14 17:21:37

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH 1/8] kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in non-modules

On Tue, Feb 14, 2023 at 03:41:32PM +0000, Nick Alcock wrote:
> On 13 Feb 2023, Bjorn Helgaas spake thusly:
> > On Fri, Feb 10, 2023 at 04:47:42PM +0000, Nick Alcock wrote:

> > - Please follow the subject line convention for each file. They're
> > mostly there after dropping "kbuild", but do capitalize the
> > sentence that follows the prefix. The prefix should always be
> > "PCI/<driver-tag>: "

I misspoke about "PCI/<driver-tag>". I use "PCI/<feature>" for things
like MSI, AER, ASPM, etc. "PCI: <driver-tag>" is the usual pattern
for things specific to a driver, and it sounds like this is what
you've already done:

$ git log --oneline drivers/pci/controller/pci-versatile.c
6086987bdeb5 PCI: versatile: Remove redundant variable retval
b64aa11eb2dd PCI: Set bridge map_irq and swizzle_irq to default functions
669cbc708122 PCI: Move DT resource setup into devm_pci_alloc_host_bridge()
d3bb94d06aae PCI: Drop unnecessary zeroing of bridge fields
6a589900d050 PCI: Set default bridge parent device
79cbde56f98b PCI: versatile: Drop flag PCI_ENABLE_PROC_DOMAINS
3cf0eead9fb8 PCI: controller: Convert to devm_platform_ioremap_resource()
a4b21b858b56 PCI: versatile: Use pci_host_probe() to register host
331f63457165 PCI: of: Add inbound resource parsing to helpers
2999dea8e94a PCI: versatile: Remove usage of PHYS_OFFSET
f9f4fdaa3509 PCI: versatile: Use pci_parse_request_of_pci_ranges()
0018b265adf7 PCI: versatile: Fix I/O space page leak

> ... If the rules for generating prefixes vary by subsystem this means
> I'll have to fight through God knows how many annoyed maintainers to get
> this incredibly trivial change in.)

I think your script generally does the right thing, and it's already
far more than most folks do, so thank you for that!

I normally do that sort of minimal fixup silently when applying
because there's no point in reposting when it's faster to make those
trivial edits myself. In this case, removing MODULE_LICENSE instead
of commenting it out is just a little more than I like to do when
keeping your signed-off-by.

Bjorn

2023-02-15 19:07:14

by Nick Alcock

[permalink] [raw]

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

On 13 Feb 2023, Leon Romanovsky said:

> On Mon, Feb 13, 2023 at 04:13:00PM +0000, Nick Alcock wrote:
>> So SPDX is usually more precise than the MODULE_LICENSE, but is it more
>> *accurate*? I have no idea, and I don't see how I could possibly know:
>> going by the presence of advertising clauses that obviously nobody is
>> obeying it doesn't seem like we can trust header comments to be any more
>> accurate than MODULE_LICENSE. Best to just leave both in (and comment it
>> out so it has no side-effects on the build any more, which is all I'm
>> after).
>
> You are overcomplicating things.
>
> First, GPL == GPL v2.
> Second, SPDX is the right one. License in module is needed to limit
> EXPORT_SYMBOL* exposure.
> Third, we have git log and git blame to audit and revert any change.
> There is no need in leaving (even as commented) dead code.

Agreed. I audited the lot anyway -- all those files I'm touching that
lack SPDXes (14 of them) have copyright headers at the top of the file
anyway, so there is *definitely* no legal implication from dropping
this. Moving to just dropping them in the next round.

--
NULL && (void)

2023-02-16 12:05:31

by Nick Alcock

[permalink] [raw]

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

On 13 Feb 2023, Leon Romanovsky told this:

> On Mon, Feb 13, 2023 at 10:30:44AM -0700, Jonathan Corbet wrote:
>> Wouldn't it be better to let this work proceed while making a note
>> of the files still needing SPDX tags?

Since I have this list anyway, I might as well emit it, even if I
believe the general consensus is now to not add SPDXes but leave that up
to individual maintainers (phew).

The files (otherwise touched in the full series) that don't have SPDX tags:

drivers/bus/arm-cci.c
drivers/bus/imx-weim.c
drivers/bus/simple-pm-bus.c
drivers/gpu/drm/drm_mipi_dsi.c
drivers/irqchip/irq-mvebu-pic.c
drivers/reset/reset-axs10x.c
drivers/reset/reset-hsdk.c
drivers/soc/sunxi/sunxi_sram.c
drivers/video/console/vgacon.c
drivers/video/fbdev/asiliantfb.c
drivers/video/fbdev/gbefb.c
drivers/video/fbdev/imsttfb.c
drivers/xen/xenbus/xenbus_probe.c
lib/glob.c

--
NULL && (void)

2023-02-16 13:35:02

by Nick Alcock

[permalink] [raw]

Subject: Re: [PATCH 1/8] kbuild, PCI: generic,versatile: comment out MODULE_LICENSE in non-modules

On 14 Feb 2023, Bjorn Helgaas uttered the following:

> On Tue, Feb 14, 2023 at 03:41:32PM +0000, Nick Alcock wrote:
>> On 13 Feb 2023, Bjorn Helgaas spake thusly:
>> > On Fri, Feb 10, 2023 at 04:47:42PM +0000, Nick Alcock wrote:
>
>> > - Please follow the subject line convention for each file. They're
>> > mostly there after dropping "kbuild", but do capitalize the
>> > sentence that follows the prefix. The prefix should always be
>> > "PCI/<driver-tag>: "
>
> I misspoke about "PCI/<driver-tag>". I use "PCI/<feature>" for things
> like MSI, AER, ASPM, etc. "PCI: <driver-tag>" is the usual pattern
> for things specific to a driver, and it sounds like this is what
> you've already done:
>
> $ git log --oneline drivers/pci/controller/pci-versatile.c
> 6086987bdeb5 PCI: versatile: Remove redundant variable retval
> b64aa11eb2dd PCI: Set bridge map_irq and swizzle_irq to default functions
> 669cbc708122 PCI: Move DT resource setup into devm_pci_alloc_host_bridge()
> d3bb94d06aae PCI: Drop unnecessary zeroing of bridge fields
> 6a589900d050 PCI: Set default bridge parent device
> 79cbde56f98b PCI: versatile: Drop flag PCI_ENABLE_PROC_DOMAINS
> 3cf0eead9fb8 PCI: controller: Convert to devm_platform_ioremap_resource()
> a4b21b858b56 PCI: versatile: Use pci_host_probe() to register host
> 331f63457165 PCI: of: Add inbound resource parsing to helpers
> 2999dea8e94a PCI: versatile: Remove usage of PHYS_OFFSET
> f9f4fdaa3509 PCI: versatile: Use pci_parse_request_of_pci_ranges()
> 0018b265adf7 PCI: versatile: Fix I/O space page leak

Oh good, that's what I was hoping.

After a bunch more bugfixing It's coming out as 'kbuild, PCI: versatile"
now. (This seems better than 'kbuild: PCI: versatile' because 'PCI:
versatile' isn't a subsystem of 'kbuild'.)

>> ... If the rules for generating prefixes vary by subsystem this means
>> I'll have to fight through God knows how many annoyed maintainers to get
>> this incredibly trivial change in.)
>
> I think your script generally does the right thing, and it's already
> far more than most folks do, so thank you for that!

Heh, I knew it wouldn't be time totally wasted: I've saved time already
on account of having had to redo the individual patches in some way
eight or nine times by now, and being able to make the changes in one go
and split it out into separate commits after that was definitely easier
than the alternative.

FYI: the splitting script's improved a bit. it's still an undocumented,
uncommented horror, but it now supports arbitrary regex-replacements of
"bad prefixes" in a file prefix-transforms.yaml, for this run containing

treewide: ''
fix spelling mistake: ''
task_get_unused_fd_flags: ''
x86/mm/dump_pagetables: 'mm'

I stuffed the splitting script in
https://github.com/nickalcock/linux.git mass-split (as
scripts/kernel-mass-split) just in case it's useful to someone else.
Definitely absolutely not for upstreaming!

(Next tranche coming soon, the previous set again -- improved as
suggested, removing MODULE_LICENSE instead of commenting out, etc -- and
then another, bigger tranche inflicted on a different subset of
maintainers.)

--
NULL && (void)