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

Commit Message

Dave Airlie July 4, 2023, 2:50 a.m. UTC

From: Dave Airlie <[e-mail]>

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 <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Dave Airlie <[e-mail]>
---
 include/linux/module.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Luis Chamberlain July 7, 2023, 6:41 p.m. UTC | #1

On Tue, Jul 04, 2023 at 12:50:50PM +1000, Dave Airlie wrote:
> From: Dave Airlie <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Dave Airlie <[e-mail]>

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

  Luis

Lucas De Marchi July 17, 2023, 7:41 p.m. UTC | #2

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Signed-off-by: Dave Airlie <[e-mail]>
>
>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

David Airlie July 18, 2023, 12:52 a.m. UTC | #3

On Tue, Jul 18, 2023 at 5:41 AM Lucas De Marchi
<[e-mail]> 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 <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Signed-off-by: Dave Airlie <[e-mail]>
> >
> >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.

Lucas De Marchi July 18, 2023, 12:28 p.m. UTC | #4

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
><[e-mail]> 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 <[e-mail]>
>> >>
>> >> 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 <[e-mail]>
>> >> Cc: [e-mail]
>> >> Cc: [e-mail]
>> >> Signed-off-by: Dave Airlie <[e-mail]>
>> >
>> >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.
>

Patch

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.