[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 at 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 at 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 at kernel.org>
>> >> Cc: linux-modules at vger.kernel.org
>> >> Cc: dri-devel at lists.freedesktop.org
>> >> Signed-off-by: Dave Airlie <airlied at 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.
>