modules: firmware groups attempt two

modules: firmware groups attempt two

[Dave Airlie]

This is a second attempt at firmware grouping so we can have dracut
only pull one firmware into the initramfs instead of a bunch of legacy ones.

v1 got derailed when it was pointed out that ordering isn't consistent in
modinfo with C to sections, so that plan no longer was feasible.

This was the simplest next best option, it keeps the group tags, and
adds a new list of acceptable firmwares. New dracut will ignore legacy
firmware in favour of the list, old dracut will just read the legacy info
lines.

This is ugly because it requires duplication, but I can't think of a nice
way beyond horrible unscaleable C preprocessor tricks to make it nicer.

Dave.

[PATCH 1/2] docs: module: start adding some docs for MODULE_ macros.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

In order to add a new macro, Luis suggested converting some docs
for the new ones.

This tries to keep exisiting module_init, module_exit where they are,
and adds the new docs to the module section.

Cc: linux-doc@vger.kernel.org
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: linux-modules@vger.kernel.org
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 Documentation/core-api/kernel-api.rst |  3 ++
 Documentation/driver-api/basics.rst   |  2 +-
 include/linux/module.h                | 76 ++++++++++++++++++---------
 3 files changed, 54 insertions(+), 27 deletions(-)

diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index 9b3f3e5f5a95..5079e48dd58c 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -226,6 +226,9 @@ Kernel module auto-loading
 .. kernel-doc:: kernel/module/kmod.c
    :export:
 
+.. kernel-doc:: include/linux/module.h
+   :no-identifiers: module_init module_exit klp_modinfo
+
 Module debugging
 ----------------
 
diff --git a/Documentation/driver-api/basics.rst b/Documentation/driver-api/basics.rst
index 4b4d8e28d3be..fea42d6cad80 100644
--- a/Documentation/driver-api/basics.rst
+++ b/Documentation/driver-api/basics.rst
@@ -5,7 +5,7 @@ Driver Entry and Exit points
 ----------------------------
 
 .. kernel-doc:: include/linux/module.h
-   :internal:
+   :identifiers: module_init module_exit
 
 Driver device table
 -------------------
diff --git a/include/linux/module.h b/include/linux/module.h
index 9e56763dff81..b255db33b40f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -183,23 +183,27 @@ extern void cleanup_module(void);
 #define MODULE_FILE	MODULE_INFO(file, KBUILD_MODFILE);
 #endif
 
-/*
+/**
+ * MODULE_LICENSE - module license
+ * @_license: license covering this module.
+ *
  * The following license idents are currently accepted as indicating free
  * software modules
  *
- *	"GPL"				[GNU Public License v2]
- *	"GPL v2"			[GNU Public License v2]
- *	"GPL and additional rights"	[GNU Public License v2 rights and more]
- *	"Dual BSD/GPL"			[GNU Public License v2
- *					 or BSD license choice]
- *	"Dual MIT/GPL"			[GNU Public License v2
- *					 or MIT license choice]
- *	"Dual MPL/GPL"			[GNU Public License v2
- *					 or Mozilla license choice]
+ *    "GPL"                        [GNU Public License v2]
  *
- * The following other idents are available
+ *    "GPL v2"                     [GNU Public License v2]
  *
- *	"Proprietary"			[Non free products]
+ *    "GPL and additional rights"  [GNU Public License v2 rights and more]
+ *
+ *    "Dual BSD/GPL"               [GNU Public License v2 or BSD license choice]
+ *
+ *    "Dual MIT/GPL"               [GNU Public License v2 or MIT license choice]
+ *
+ *    "Dual MPL/GPL"               [GNU Public License v2 or Mozilla license choice]
+ *
+ * The following other idents are available
+ *    "Proprietary"                [Non free products]
  *
  * Both "GPL v2" and "GPL" (the latter also in dual licensed strings) are
  * merely stating that the module is licensed under the GPL v2, but are not
@@ -222,20 +226,26 @@ extern void cleanup_module(void);
  * is a GPL combined work.
  *
  * This exists for several reasons
- * 1.	So modinfo can show license info for users wanting to vet their setup
- *	is free
+ *
+ * 1.	So modinfo can show license info for users wanting to vet their setup is free
+ *
  * 2.	So the community can ignore bug reports including proprietary modules
+ *
  * 3.	So vendors can do likewise based on their own policies
  */
 #define MODULE_LICENSE(_license) MODULE_FILE MODULE_INFO(license, _license)
 
-/*
- * Author(s), use "Name <email>" or just "Name", for multiple
- * authors use multiple MODULE_AUTHOR() statements/lines.
+/**
+ * MODULE_AUTHOR - Module author
+ * @_author: Author(s), use "Name <email>" or just "Name", for multiple
+ *           authors use multiple MODULE_AUTHOR() statements/lines.
  */
 #define MODULE_AUTHOR(_author) MODULE_INFO(author, _author)
 
-/* What your module does. */
+/**
+ * MODULE_DESCRIPTION - Module description
+ * @_description: What your module does.
+ */
 #define MODULE_DESCRIPTION(_description) MODULE_INFO(description, _description)
 
 #ifdef MODULE
@@ -247,19 +257,23 @@ extern typeof(name) __mod_##type##__##name##_device_table		\
 #define MODULE_DEVICE_TABLE(type, name)
 #endif
 
-/* Version of form [<epoch>:]<version>[-<extra-version>].
+/**
+ * MODULE_VERSION: version of module
+ * @_version: version in the form below
+ *
+ * Version of form [<epoch>:]<version>[-<extra-version>].
  * Or for CVS/RCS ID version, everything but the number is stripped.
  * <epoch>: A (small) unsigned integer which allows you to start versions
  * anew. If not mentioned, it's zero.  eg. "2:1.0" is after
  * "1:2.0".
-
+ *
  * <version>: The <version> may contain only alphanumerics and the
- * character `.'.  Ordered by numeric sort for numeric parts,
+ * character '.'.  Ordered by numeric sort for numeric parts,
  * ascii sort for ascii parts (as per RPM or DEB algorithm).
-
+ *
  * <extraversion>: Like <version>, but inserted for local
  * customizations, eg "rh3" or "rusty1".
-
+ *
  * Using this automatically adds a checksum of the .c files and the
  * local headers in "srcversion".
  */
@@ -285,11 +299,21 @@ extern typeof(name) __mod_##type##__##name##_device_table		\
 		}
 #endif
 
-/* Optional firmware file (or files) needed by the module
- * format is simply firmware file name.  Multiple firmware
- * files require multiple MODULE_FIRMWARE() specifiers */
+/**
+ * MODULE_FIRMWARE - Optional firmware files needed by the module
+ * @_firmware: firmware file name
+ *
+ * Multiple firmware files require multiple MODULE_FIRMWARE() specifiers.
+ */
 #define MODULE_FIRMWARE(_firmware) MODULE_INFO(firmware, _firmware)
 
+/**
+ * MODULE_IMPORT_NS - Set the symbol namespace for the module.
+ * @ns: symbol namespace to import the module into.
+ *
+ * This adds a modinfo tag 'import_ns' to the module. This is observed
+ * by userspace at module loading time.
+ */
 #define MODULE_IMPORT_NS(ns)	MODULE_INFO(import_ns, __stringify(ns))
 
 struct notifier_block;
-- 
2.41.0

[PATCH 2/2] modules/firmware: add a new option to denote a firmware group to choose one.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This adds two tags that will go into the module info.

The first denotes a group of firmwares, when that tag is present all
MODULE_FIRMWARE lines between the tags will be ignored by new versions of
dracut.

The second makes an explicitly ordered group of firmwares to search for
inside a group setting. New dracut will pick the first available firmware
from this to put in the initramfs.

Old dracut will just ignore these tags and fallback to installing all
the firmwares.

The corresponding dracut code it at:
https://github.com/dracutdevs/dracut/pull/2309

Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: linux-modules@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 include/linux/module.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index b255db33b40f..b7bef5814034 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -307,6 +307,40 @@ extern typeof(name) __mod_##type##__##name##_device_table		\
  */
 #define MODULE_FIRMWARE(_firmware) MODULE_INFO(firmware, _firmware)
 
+/**
+ * MODULE_FIRMWARE_GROUP_ONLY_ONE - Create a need only one firmware group
+ * @_grpname: group name
+ *
+ * This creates a group boundary of which the driver only needs one firmware installed.
+ * This is to allow dracut to limit the number of firmwares in the initramfs.
+ * This just creates a new entry in the modinfo section, there should be one
+ * of these entries bracketing the group of MODULE_INFO lines.
+ *
+ * Old dracut will ignore this, and just read MODULE_FIRMWARE.
+ * New dracut will ignore MODULE_FIRMWARE lines between group boundaries,
+ * and will only parse the new group list.
+ * It will pick the first found firmware from the group list.
+ *
+ * ``MODULE_FIRMWARE_GROUP_ONLY_ONE("mygroup")``
+ *
+ * ``MODULE_FIRMWARE_GROUP_LIST("firmwarev2,firmwarev1")``
+ *
+ * ``MODULE_FIRMWARE("firmwarev1")``
+ *
+ * ``MODULE_FIRMWARE("firmwarev2")``
+ *
+ * ``MODULE_FIRMWARE_GROUP_ONLY_ONE("mygroup")``
+ */
+#define MODULE_FIRMWARE_GROUP_ONLY_ONE(_grpname) MODULE_INFO(firmware_group_only_one, _grpname)
+
+/**
+ * MODULE_FIRMWARE_GROUP_LIST - Create a need one firmware list
+ * @_fwnames: firmware names in the group.
+ *
+ * See MODULE_FIRMWARE_GROUP_ONLY_ONE.
+ */
+#define MODULE_FIRMWARE_GROUP_LIST(_fwnames) MODULE_INFO(firmware_group_list, _fwnames)
+
 /**
  * MODULE_IMPORT_NS - Set the symbol namespace for the module.
  * @ns: symbol namespace to import the module into.
-- 
2.41.0

Re: [PATCH 2/2] modules/firmware: add a new option to denote a firmware group to choose one.

[Luis Chamberlain]

On Tue, Jul 04, 2023 at 12:50:50PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This adds two tags that will go into the module info.
> 
> The first denotes a group of firmwares, when that tag is present all
> MODULE_FIRMWARE lines between the tags will be ignored by new versions of
> dracut.
> 
> The second makes an explicitly ordered group of firmwares to search for
> inside a group setting. New dracut will pick the first available firmware
> from this to put in the initramfs.
> 
> Old dracut will just ignore these tags and fallback to installing all
> the firmwares.
> 
> The corresponding dracut code it at:
> https://github.com/dracutdevs/dracut/pull/2309
> 
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: linux-modules@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Lucas, did this end up working for you as well?

  Luis

Re: [PATCH 2/2] modules/firmware: add a new option to denote a firmware group to choose one.

[Lucas De Marchi]

On Fri, Jul 07, 2023 at 11:41:48AM -0700, Luis Chamberlain wrote:
>On Tue, Jul 04, 2023 at 12:50:50PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> This adds two tags that will go into the module info.
>>
>> The first denotes a group of firmwares, when that tag is present all
>> MODULE_FIRMWARE lines between the tags will be ignored by new versions of
>> dracut.
>>
>> The second makes an explicitly ordered group of firmwares to search for
>> inside a group setting. New dracut will pick the first available firmware
>> from this to put in the initramfs.
>>
>> Old dracut will just ignore these tags and fallback to installing all
>> the firmwares.
>>
>> The corresponding dracut code it at:
>> https://github.com/dracutdevs/dracut/pull/2309
>>
>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> Cc: linux-modules@vger.kernel.org
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>
>Lucas, did this end up working for you as well?

I didn't try this yet, no. My opinion is similar to the first version of
this series:  this is the first case in which we are making the order of
2 different keys to be relevant and it doesn't look so pretty. It may
also be hard to adapt some of the drivers like i915 to intertwine the 2
modinfo keys.

However, the alternative I provided also has some flaws, so I said I'd
be ok continuing in this direction. Humn... how about merging part of my
suggestion to mitigate the duplication we have now?

	- Document that when using a fw group, it's expected userspace
	  will consider higher versions within a group to have higher
	  prio and add only one of them. Thinking on a distro packaging,
	  it could be extended to packaging fewer blobs too.

If we agree on "firmware files within a group are version-sorted", then
we don't need the extra MODULE_FIRMWARE_GROUP_LIST, do we?

Nit: referencing dracut implementation IMO is ok, but on kernel-doc
parts we should prefer something more generic since dracut is far from
the only initrd generator nowadays.

thanks
Lucas De Marchi

>
>  Luis

Re: [PATCH 2/2] modules/firmware: add a new option to denote a firmware group to choose one.

[David Airlie]

On Tue, Jul 18, 2023 at 5:41 AM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> On Fri, Jul 07, 2023 at 11:41:48AM -0700, Luis Chamberlain wrote:
> >On Tue, Jul 04, 2023 at 12:50:50PM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <airlied@redhat.com>
> >>
> >> This adds two tags that will go into the module info.
> >>
> >> The first denotes a group of firmwares, when that tag is present all
> >> MODULE_FIRMWARE lines between the tags will be ignored by new versions of
> >> dracut.
> >>
> >> The second makes an explicitly ordered group of firmwares to search for
> >> inside a group setting. New dracut will pick the first available firmware
> >> from this to put in the initramfs.
> >>
> >> Old dracut will just ignore these tags and fallback to installing all
> >> the firmwares.
> >>
> >> The corresponding dracut code it at:
> >> https://github.com/dracutdevs/dracut/pull/2309
> >>
> >> Cc: Luis Chamberlain <mcgrof@kernel.org>
> >> Cc: linux-modules@vger.kernel.org
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: Dave Airlie <airlied@redhat.com>
> >
> >Lucas, did this end up working for you as well?
>
> I didn't try this yet, no. My opinion is similar to the first version of
> this series:  this is the first case in which we are making the order of
> 2 different keys to be relevant and it doesn't look so pretty. It may
> also be hard to adapt some of the drivers like i915 to intertwine the 2
> modinfo keys.

This doesn't have as much reliance on order, it just relies on the
group tags not being reordered outside the modinfos between them.

>
> However, the alternative I provided also has some flaws, so I said I'd
> be ok continuing in this direction. Humn... how about merging part of my
> suggestion to mitigate the duplication we have now?
>
>         - Document that when using a fw group, it's expected userspace
>           will consider higher versions within a group to have higher
>           prio and add only one of them. Thinking on a distro packaging,
>           it could be extended to packaging fewer blobs too.
>
> If we agree on "firmware files within a group are version-sorted", then
> we don't need the extra MODULE_FIRMWARE_GROUP_LIST, do we?

But what is the versioning to be used, I have to be very careful to
have this be backwards compat, and not suddenly stop pulling in some
versions of a fw because they happen to have used a version scheme
that this tramples.

If you are saying, we need to define a firmware versioning rule, then
that's a big task and would involve changing a bunch of things in the
fw and drivers.

i915:
adlp_dmc_ver2_14.bin.xz
dg2_guc_70.1.2.bin.xz
mtl_guc_70.bin.xz

amdgpu:
polaris11_mec.bin.xz
polaris11_mec2.bin.xz
polaris11_mec_2.bin.xz
polaris11_mec2_2.bin.xz

I don't see what is a version field I can sort on reliably here. Now
maybe I could introduce a new field

Do you think we should just add explicit ordering to each module group?,

so we create a

MODULE_FIRMWARE_GROUP_VERSIONED("nvidia/ga106/gsp/gsp-5258902.bin", 1);
MODULE_FIRMWARE_GROUP_VERSIONED("nvidia/ga106/gsp/gsp-5303002.bin", 2);

and I just output group brackets + that? and fix dracut to deal with it?

Dave.

Re: [PATCH 2/2] modules/firmware: add a new option to denote a firmware group to choose one.

[Lucas De Marchi]

On Tue, Jul 18, 2023 at 10:52:47AM +1000, David Airlie wrote:
>On Tue, Jul 18, 2023 at 5:41 AM Lucas De Marchi
><lucas.demarchi@intel.com> wrote:
>>
>> On Fri, Jul 07, 2023 at 11:41:48AM -0700, Luis Chamberlain wrote:
>> >On Tue, Jul 04, 2023 at 12:50:50PM +1000, Dave Airlie wrote:
>> >> From: Dave Airlie <airlied@redhat.com>
>> >>
>> >> This adds two tags that will go into the module info.
>> >>
>> >> The first denotes a group of firmwares, when that tag is present all
>> >> MODULE_FIRMWARE lines between the tags will be ignored by new versions of
>> >> dracut.
>> >>
>> >> The second makes an explicitly ordered group of firmwares to search for
>> >> inside a group setting. New dracut will pick the first available firmware
>> >> from this to put in the initramfs.
>> >>
>> >> Old dracut will just ignore these tags and fallback to installing all
>> >> the firmwares.
>> >>
>> >> The corresponding dracut code it at:
>> >> https://github.com/dracutdevs/dracut/pull/2309
>> >>
>> >> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> >> Cc: linux-modules@vger.kernel.org
>> >> Cc: dri-devel@lists.freedesktop.org
>> >> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> >
>> >Lucas, did this end up working for you as well?
>>
>> I didn't try this yet, no. My opinion is similar to the first version of
>> this series:  this is the first case in which we are making the order of
>> 2 different keys to be relevant and it doesn't look so pretty. It may
>> also be hard to adapt some of the drivers like i915 to intertwine the 2
>> modinfo keys.
>
>This doesn't have as much reliance on order, it just relies on the
>group tags not being reordered outside the modinfos between them.
>
>>
>> However, the alternative I provided also has some flaws, so I said I'd
>> be ok continuing in this direction. Humn... how about merging part of my
>> suggestion to mitigate the duplication we have now?
>>
>>         - Document that when using a fw group, it's expected userspace
>>           will consider higher versions within a group to have higher
>>           prio and add only one of them. Thinking on a distro packaging,
>>           it could be extended to packaging fewer blobs too.
>>
>> If we agree on "firmware files within a group are version-sorted", then
>> we don't need the extra MODULE_FIRMWARE_GROUP_LIST, do we?
>
>But what is the versioning to be used, I have to be very careful to
>have this be backwards compat, and not suddenly stop pulling in some
>versions of a fw because they happen to have used a version scheme
>that this tramples.
>
>If you are saying, we need to define a firmware versioning rule, then
>that's a big task and would involve changing a bunch of things in the
>fw and drivers.
>
>i915:
>adlp_dmc_ver2_14.bin.xz
>dg2_guc_70.1.2.bin.xz
>mtl_guc_70.bin.xz

these I know would never be part of a single group. 

>
>amdgpu:
>polaris11_mec.bin.xz
>polaris11_mec2.bin.xz
>polaris11_mec_2.bin.xz
>polaris11_mec2_2.bin.xz

not sure about these

>
>I don't see what is a version field I can sort on reliably here. Now
>maybe I could introduce a new field

What I meant was to pass the entire name through `sort -V`.
Looking at a few examples in i915 of what could be within a
group:

$ modinfo i915 | grep adlp_guc | awk '{print $2}' | sort -V | head -n 3
i915/adlp_guc_69.0.3.bin
i915/adlp_guc_70.bin
i915/adlp_guc_70.1.1.bin

Unfortunately the version sort would fail for this platform that was in
the transition of full-version -> major-only. We should really prefer
i915/adlp_guc_70.bin (that is actually 70.x.y) over
i915/adlp_guc_70.1.1.bin. So for i915 this would be only forward-looking
and we wouldn't be able to add the the groups for older platforms.

>
>Do you think we should just add explicit ordering to each module group?,
>
>so we create a
>
>MODULE_FIRMWARE_GROUP_VERSIONED("nvidia/ga106/gsp/gsp-5258902.bin", 1);
>MODULE_FIRMWARE_GROUP_VERSIONED("nvidia/ga106/gsp/gsp-5303002.bin", 2);

not sure. What would be the output of this define? Maybe we can use
__COUNTER__ so we don't need the param? If with this we can use modpost
to fixup the section order, then we are back to your previous solution,
which would be nicer than the duplication we have in this one.

Lucas De Marchi

>
>and I just output group brackets + that? and fix dracut to deal with it?
>
>Dave.
>