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

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

[Nick Alcock]

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: Rob Herring <robh@kernel.org>
Cc: linux-pci@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/pci/controller/pci-versatile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-versatile.c b/drivers/pci/controller/pci-versatile.c
index 7991d334e0f1..afa9d4694501 100644
--- a/drivers/pci/controller/pci-versatile.c
+++ b/drivers/pci/controller/pci-versatile.c
@@ -169,4 +169,4 @@ static struct platform_driver versatile_pci_driver = {
 module_platform_driver(versatile_pci_driver);
 
 MODULE_DESCRIPTION("Versatile PCI driver");
-MODULE_LICENSE("GPL v2");
+/* MODULE_LICENSE("GPL v2"); */
-- 
2.39.1.268.g9de2f9a303


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Fri, Feb 10, 2023 at 11:05 AM Nick Alcock <nick.alcock@oracle.com> 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 <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: Rob Herring <robh@kernel.org>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/pci/controller/pci-versatile.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nick Alcock]

On 10 Feb 2023, Rob Herring uttered the following:

> On Fri, Feb 10, 2023 at 11:05 AM Nick Alcock <nick.alcock@oracle.com> 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)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

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 linux-pci@vger.kernel.org.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nick Alcock]

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 linux-pci@vger.kernel.org.
>
>   - 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)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nick Alcock]

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)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel