| Message ID | 20230210164749.368998-9-nick.alcock@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | None | expand |
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/20230111125323.1911373-4-daire.mcnamara@microchip.com/ > (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 <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? 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 <daire.mcnamara@microchip.com>"); > -- > 2.39.1.268.g9de2f9a303 >
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!)
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@microchip.com/ > > 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@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. 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!)
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
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.)
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?
[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/Y5AgMuMu75gne6Ka@bombadil.infradead.org/> 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).
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/Y5AgMuMu75gne6Ka@bombadil.infradead.org/> > > 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)
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/Y5AgMuMu75gne6Ka@bombadil.infradead.org/> > > 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)
Leon Romanovsky <leon@kernel.org> 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
On Mon, Feb 13, 2023 at 10:30:44AM -0700, Jonathan Corbet wrote: > Leon Romanovsky <leon@kernel.org> 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
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.
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
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 <daire.mcnamara@microchip.com>");
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 <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 --- drivers/pci/controller/pcie-microchip-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)