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

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

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This adds a tag that will go into the module info, only one firmware from
the group given needs to be available for this driver to work. This allows
dracut to avoid adding in firmware that aren't needed.

This just brackets a module list in the modinfo, the modules in the list
will get entries in reversed order so the last module in the list is the
preferred one.

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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index 4435ad9439ab..f02448ed5e2b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -289,6 +289,8 @@ extern typeof(name) __mod_##type##__##name##_device_table		\
  * files require multiple MODULE_FIRMWARE() specifiers */
 #define MODULE_FIRMWARE(_firmware) MODULE_INFO(firmware, _firmware)
 
+#define MODULE_FIRMWARE_GROUP_ONLY_ONE(_grpname) MODULE_INFO(firmware_group_only_one, _grpname)
+
 #define MODULE_IMPORT_NS(ns)	MODULE_INFO(import_ns, __stringify(ns))
 
 struct notifier_block;
-- 
2.39.2

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

[Lucas De Marchi]

On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
>From: Dave Airlie <airlied@redhat.com>
>
>This adds a tag that will go into the module info, only one firmware from
>the group given needs to be available for this driver to work. This allows
>dracut to avoid adding in firmware that aren't needed.
>
>This just brackets a module list in the modinfo, the modules in the list
>will get entries in reversed order so the last module in the list is the
>preferred one.
>
>The corresponding dracut code it at:
>https://github.com/dracutdevs/dracut/pull/2309

it would be good to have the example usage in the commit message here so
it can be easily checked as reference for other drivers.

I don't think we ever had any ordering in modinfo being relevant for
other things. Considering the use case and that we could also use a
similar thing for i915 / xe modules wrt to the major version,
couldn't we do something like below?

	MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
	MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
	MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");

so the group is created by startswith() rather than by the order the
modinfo appears in the elf section. In i915 we'd have:

MODULE_FIRMWARE_GROUP("i915/tgl_guc")

There is still an order the kernel would probably like: latest version.
But then it's an order only among things with the same key.

Lucas De Marchi

>
>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 | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 4435ad9439ab..f02448ed5e2b 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -289,6 +289,8 @@ extern typeof(name) __mod_##type##__##name##_device_table		\
>  * files require multiple MODULE_FIRMWARE() specifiers */
> #define MODULE_FIRMWARE(_firmware) MODULE_INFO(firmware, _firmware)
>
>+#define MODULE_FIRMWARE_GROUP_ONLY_ONE(_grpname) MODULE_INFO(firmware_group_only_one, _grpname)
>+
> #define MODULE_IMPORT_NS(ns)	MODULE_INFO(import_ns, __stringify(ns))
>
> struct notifier_block;
>-- 
>2.39.2
>

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

[Lucas De Marchi]

On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
>From: Dave Airlie <airlied@redhat.com>
>
>This adds a tag that will go into the module info, only one firmware from
>the group given needs to be available for this driver to work. This allows
>dracut to avoid adding in firmware that aren't needed.
>
>This just brackets a module list in the modinfo, the modules in the list
>will get entries in reversed order so the last module in the list is the
>preferred one.
>
>The corresponding dracut code it at:
>https://github.com/dracutdevs/dracut/pull/2309

it would be good to have the example usage in the commit message here so
it can be easily checked as reference for other drivers.

I don't think we ever had any ordering in modinfo being relevant for
other things. Considering the use case and that we could also use a
similar thing for i915 / xe modules wrt to the major version,
couldn't we do something like below?

	MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
	MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
	MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");

so the group is created by startswith() rather than by the order the
modinfo appears in the elf section. In i915 we'd have:

MODULE_FIRMWARE_GROUP("i915/tgl_guc")

There is still an order the kernel would probably like: latest version.
But then it's an order only among things with the same key.

Lucas De Marchi

>
>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 | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 4435ad9439ab..f02448ed5e2b 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -289,6 +289,8 @@ extern typeof(name) __mod_##type##__##name##_device_table		\
>  * files require multiple MODULE_FIRMWARE() specifiers */
> #define MODULE_FIRMWARE(_firmware) MODULE_INFO(firmware, _firmware)
>
>+#define MODULE_FIRMWARE_GROUP_ONLY_ONE(_grpname) MODULE_INFO(firmware_group_only_one, _grpname)
>+
> #define MODULE_IMPORT_NS(ns)	MODULE_INFO(import_ns, __stringify(ns))
>
> struct notifier_block;
>-- 
>2.39.2
>

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

[Dave Airlie]

On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
> >From: Dave Airlie <airlied@redhat.com>
> >
> >This adds a tag that will go into the module info, only one firmware from
> >the group given needs to be available for this driver to work. This allows
> >dracut to avoid adding in firmware that aren't needed.
> >
> >This just brackets a module list in the modinfo, the modules in the list
> >will get entries in reversed order so the last module in the list is the
> >preferred one.
> >
> >The corresponding dracut code it at:
> >https://github.com/dracutdevs/dracut/pull/2309
>
> it would be good to have the example usage in the commit message here so
> it can be easily checked as reference for other drivers.

Good point.

>
> I don't think we ever had any ordering in modinfo being relevant for
> other things. Considering the use case and that we could also use a
> similar thing for i915 / xe modules wrt to the major version,
> couldn't we do something like below?
>
>         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
>
> so the group is created by startswith() rather than by the order the
> modinfo appears in the elf section. In i915 we'd have:

The way userspace parses these is reverse order, and it doesn't see
the GROUP until after the FIRMWARE, so this can't work, as it already
will have included all the ones below, hence why I bracketed top and
bottom with a group.

>
> MODULE_FIRMWARE_GROUP("i915/tgl_guc")
>
> There is still an order the kernel would probably like: latest version.
> But then it's an order only among things with the same key.

Dave.

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

[Dave Airlie]

On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
> >From: Dave Airlie <airlied@redhat.com>
> >
> >This adds a tag that will go into the module info, only one firmware from
> >the group given needs to be available for this driver to work. This allows
> >dracut to avoid adding in firmware that aren't needed.
> >
> >This just brackets a module list in the modinfo, the modules in the list
> >will get entries in reversed order so the last module in the list is the
> >preferred one.
> >
> >The corresponding dracut code it at:
> >https://github.com/dracutdevs/dracut/pull/2309
>
> it would be good to have the example usage in the commit message here so
> it can be easily checked as reference for other drivers.

Good point.

>
> I don't think we ever had any ordering in modinfo being relevant for
> other things. Considering the use case and that we could also use a
> similar thing for i915 / xe modules wrt to the major version,
> couldn't we do something like below?
>
>         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
>
> so the group is created by startswith() rather than by the order the
> modinfo appears in the elf section. In i915 we'd have:

The way userspace parses these is reverse order, and it doesn't see
the GROUP until after the FIRMWARE, so this can't work, as it already
will have included all the ones below, hence why I bracketed top and
bottom with a group.

>
> MODULE_FIRMWARE_GROUP("i915/tgl_guc")
>
> There is still an order the kernel would probably like: latest version.
> But then it's an order only among things with the same key.

Dave.

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

[Lucas De Marchi]

On Mon, Apr 24, 2023 at 03:44:18PM +1000, Dave Airlie wrote:
>On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
>> >From: Dave Airlie <airlied@redhat.com>
>> >
>> >This adds a tag that will go into the module info, only one firmware from
>> >the group given needs to be available for this driver to work. This allows
>> >dracut to avoid adding in firmware that aren't needed.
>> >
>> >This just brackets a module list in the modinfo, the modules in the list
>> >will get entries in reversed order so the last module in the list is the
>> >preferred one.
>> >
>> >The corresponding dracut code it at:
>> >https://github.com/dracutdevs/dracut/pull/2309
>>
>> it would be good to have the example usage in the commit message here so
>> it can be easily checked as reference for other drivers.
>
>Good point.
>
>>
>> I don't think we ever had any ordering in modinfo being relevant for
>> other things. Considering the use case and that we could also use a
>> similar thing for i915 / xe modules wrt to the major version,
>> couldn't we do something like below?
>>
>>         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
>>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
>>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
>>
>> so the group is created by startswith() rather than by the order the
>> modinfo appears in the elf section. In i915 we'd have:
>
>The way userspace parses these is reverse order, and it doesn't see

the main issue I have with it is that it relies on a order that is
implicit rather than intended. The order comes from how the .modinfo ELF
section is assembled together... so the fact that your call to
kmod_module_get_info() returns a list with the keys in the reverse order
of the MODULE_FIRMWARE() definitions, is basically because the compiler
toolchain did it did that way.

It's worse when those sections come from different compilation units as
the order then is not predictable and can easily break with changes to
the build infra if the files are linked in different order.

I think the grouping thing here would only be supported with firmware
defined on the same compilation unit, but it's something to keep in mind
and document.

>the GROUP until after the FIRMWARE, so this can't work, as it already
>will have included all the ones below, hence why I bracketed top and
>bottom with a group.

well... that is something that can be adapted easily by using a 2 pass
approach, filtering out the list based on the groups.

I agree that yours is simpler though.  If we can rely on the
order produced by the compiler and we document the expectations of
MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
simpler approach.

Luis, any thoughts here?

thanks
Lucas De Marchi

>
>>
>> MODULE_FIRMWARE_GROUP("i915/tgl_guc")
>>
>> There is still an order the kernel would probably like: latest version.
>> But then it's an order only among things with the same key.
>
>Dave.

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

[Lucas De Marchi]

On Mon, Apr 24, 2023 at 03:44:18PM +1000, Dave Airlie wrote:
>On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
>> >From: Dave Airlie <airlied@redhat.com>
>> >
>> >This adds a tag that will go into the module info, only one firmware from
>> >the group given needs to be available for this driver to work. This allows
>> >dracut to avoid adding in firmware that aren't needed.
>> >
>> >This just brackets a module list in the modinfo, the modules in the list
>> >will get entries in reversed order so the last module in the list is the
>> >preferred one.
>> >
>> >The corresponding dracut code it at:
>> >https://github.com/dracutdevs/dracut/pull/2309
>>
>> it would be good to have the example usage in the commit message here so
>> it can be easily checked as reference for other drivers.
>
>Good point.
>
>>
>> I don't think we ever had any ordering in modinfo being relevant for
>> other things. Considering the use case and that we could also use a
>> similar thing for i915 / xe modules wrt to the major version,
>> couldn't we do something like below?
>>
>>         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
>>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
>>         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
>>
>> so the group is created by startswith() rather than by the order the
>> modinfo appears in the elf section. In i915 we'd have:
>
>The way userspace parses these is reverse order, and it doesn't see

the main issue I have with it is that it relies on a order that is
implicit rather than intended. The order comes from how the .modinfo ELF
section is assembled together... so the fact that your call to
kmod_module_get_info() returns a list with the keys in the reverse order
of the MODULE_FIRMWARE() definitions, is basically because the compiler
toolchain did it did that way.

It's worse when those sections come from different compilation units as
the order then is not predictable and can easily break with changes to
the build infra if the files are linked in different order.

I think the grouping thing here would only be supported with firmware
defined on the same compilation unit, but it's something to keep in mind
and document.

>the GROUP until after the FIRMWARE, so this can't work, as it already
>will have included all the ones below, hence why I bracketed top and
>bottom with a group.

well... that is something that can be adapted easily by using a 2 pass
approach, filtering out the list based on the groups.

I agree that yours is simpler though.  If we can rely on the
order produced by the compiler and we document the expectations of
MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
simpler approach.

Luis, any thoughts here?

thanks
Lucas De Marchi

>
>>
>> MODULE_FIRMWARE_GROUP("i915/tgl_guc")
>>
>> There is still an order the kernel would probably like: latest version.
>> But then it's an order only among things with the same key.
>
>Dave.

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

[Luis Chamberlain]

On Mon, Apr 24, 2023 at 10:01:13AM -0700, Lucas De Marchi wrote:
> On Mon, Apr 24, 2023 at 03:44:18PM +1000, Dave Airlie wrote:
> > On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > > 
> > > On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
> > > >From: Dave Airlie <airlied@redhat.com>
> > > >
> > > >This adds a tag that will go into the module info, only one firmware from
> > > >the group given needs to be available for this driver to work. This allows
> > > >dracut to avoid adding in firmware that aren't needed.
> > > >
> > > >This just brackets a module list in the modinfo, the modules in the list
> > > >will get entries in reversed order so the last module in the list is the
> > > >preferred one.
> > > >
> > > >The corresponding dracut code it at:
> > > >https://github.com/dracutdevs/dracut/pull/2309
> > > 
> > > it would be good to have the example usage in the commit message here so
> > > it can be easily checked as reference for other drivers.
> > 
> > Good point.
> > 
> > > 
> > > I don't think we ever had any ordering in modinfo being relevant for
> > > other things. Considering the use case and that we could also use a
> > > similar thing for i915 / xe modules wrt to the major version,
> > > couldn't we do something like below?
> > > 
> > >         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
> > > 
> > > so the group is created by startswith() rather than by the order the
> > > modinfo appears in the elf section. In i915 we'd have:
> > 
> > The way userspace parses these is reverse order, and it doesn't see
> 
> the main issue I have with it is that it relies on a order that is
> implicit rather than intended. The order comes from how the .modinfo ELF
> section is assembled together... so the fact that your call to
> kmod_module_get_info() returns a list with the keys in the reverse order
> of the MODULE_FIRMWARE() definitions, is basically because the compiler
> toolchain did it did that way.
> 
> It's worse when those sections come from different compilation units as
> the order then is not predictable and can easily break with changes to
> the build infra if the files are linked in different order.
> 
> I think the grouping thing here would only be supported with firmware
> defined on the same compilation unit, but it's something to keep in mind
> and document.

I had provided a simple API to help with explicit linker order years ago:

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8

Other than that you have to rely on the order in the Makefile or way
in which they are declared.

> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > will have included all the ones below, hence why I bracketed top and
> > bottom with a group.
> 
> well... that is something that can be adapted easily by using a 2 pass
> approach, filtering out the list based on the groups.
> 
> I agree that yours is simpler though.  If we can rely on the
> order produced by the compiler and we document the expectations of
> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> simpler approach.
> 
> Luis, any thoughts here?

I see the Dracut code indicates that the order says now that you should
put the preferred firmware last, and that seems to match most coding
conventions, ie, new firmwares likely get added last, so it's a nice
coincidence. Will this always work? I don't know. But if you like to
hedge, then this seems fine so long as I'm folks follow up to fix issues
later. I think it should and the simplicity is preferred, worth a shot
I think.

But the examples on both sides are pretty terrible. I'd just like to ask
all this gets extended in proper kdoc form and we are able to get users
and developers to read this under "Module support" in:

https://docs.kernel.org/core-api/kernel-api.html

So go to town with a new section for:

Documentation/core-api/kernel-api.rst

  Luis

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

[Luis Chamberlain]

On Mon, Apr 24, 2023 at 10:01:13AM -0700, Lucas De Marchi wrote:
> On Mon, Apr 24, 2023 at 03:44:18PM +1000, Dave Airlie wrote:
> > On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > > 
> > > On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
> > > >From: Dave Airlie <airlied@redhat.com>
> > > >
> > > >This adds a tag that will go into the module info, only one firmware from
> > > >the group given needs to be available for this driver to work. This allows
> > > >dracut to avoid adding in firmware that aren't needed.
> > > >
> > > >This just brackets a module list in the modinfo, the modules in the list
> > > >will get entries in reversed order so the last module in the list is the
> > > >preferred one.
> > > >
> > > >The corresponding dracut code it at:
> > > >https://github.com/dracutdevs/dracut/pull/2309
> > > 
> > > it would be good to have the example usage in the commit message here so
> > > it can be easily checked as reference for other drivers.
> > 
> > Good point.
> > 
> > > 
> > > I don't think we ever had any ordering in modinfo being relevant for
> > > other things. Considering the use case and that we could also use a
> > > similar thing for i915 / xe modules wrt to the major version,
> > > couldn't we do something like below?
> > > 
> > >         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
> > > 
> > > so the group is created by startswith() rather than by the order the
> > > modinfo appears in the elf section. In i915 we'd have:
> > 
> > The way userspace parses these is reverse order, and it doesn't see
> 
> the main issue I have with it is that it relies on a order that is
> implicit rather than intended. The order comes from how the .modinfo ELF
> section is assembled together... so the fact that your call to
> kmod_module_get_info() returns a list with the keys in the reverse order
> of the MODULE_FIRMWARE() definitions, is basically because the compiler
> toolchain did it did that way.
> 
> It's worse when those sections come from different compilation units as
> the order then is not predictable and can easily break with changes to
> the build infra if the files are linked in different order.
> 
> I think the grouping thing here would only be supported with firmware
> defined on the same compilation unit, but it's something to keep in mind
> and document.

I had provided a simple API to help with explicit linker order years ago:

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8

Other than that you have to rely on the order in the Makefile or way
in which they are declared.

> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > will have included all the ones below, hence why I bracketed top and
> > bottom with a group.
> 
> well... that is something that can be adapted easily by using a 2 pass
> approach, filtering out the list based on the groups.
> 
> I agree that yours is simpler though.  If we can rely on the
> order produced by the compiler and we document the expectations of
> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> simpler approach.
> 
> Luis, any thoughts here?

I see the Dracut code indicates that the order says now that you should
put the preferred firmware last, and that seems to match most coding
conventions, ie, new firmwares likely get added last, so it's a nice
coincidence. Will this always work? I don't know. But if you like to
hedge, then this seems fine so long as I'm folks follow up to fix issues
later. I think it should and the simplicity is preferred, worth a shot
I think.

But the examples on both sides are pretty terrible. I'd just like to ask
all this gets extended in proper kdoc form and we are able to get users
and developers to read this under "Module support" in:

https://docs.kernel.org/core-api/kernel-api.html

So go to town with a new section for:

Documentation/core-api/kernel-api.rst

  Luis

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

[Lucas De Marchi]

On Mon, Apr 24, 2023 at 03:56:53PM -0700, Luis Chamberlain wrote:
>On Mon, Apr 24, 2023 at 10:01:13AM -0700, Lucas De Marchi wrote:
>> On Mon, Apr 24, 2023 at 03:44:18PM +1000, Dave Airlie wrote:
>> > On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> > >
>> > > On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
>> > > >From: Dave Airlie <airlied@redhat.com>
>> > > >
>> > > >This adds a tag that will go into the module info, only one firmware from
>> > > >the group given needs to be available for this driver to work. This allows
>> > > >dracut to avoid adding in firmware that aren't needed.
>> > > >
>> > > >This just brackets a module list in the modinfo, the modules in the list
>> > > >will get entries in reversed order so the last module in the list is the
>> > > >preferred one.
>> > > >
>> > > >The corresponding dracut code it at:
>> > > >https://github.com/dracutdevs/dracut/pull/2309
>> > >
>> > > it would be good to have the example usage in the commit message here so
>> > > it can be easily checked as reference for other drivers.
>> >
>> > Good point.
>> >
>> > >
>> > > I don't think we ever had any ordering in modinfo being relevant for
>> > > other things. Considering the use case and that we could also use a
>> > > similar thing for i915 / xe modules wrt to the major version,
>> > > couldn't we do something like below?
>> > >
>> > >         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
>> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
>> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
>> > >
>> > > so the group is created by startswith() rather than by the order the
>> > > modinfo appears in the elf section. In i915 we'd have:
>> >
>> > The way userspace parses these is reverse order, and it doesn't see
>>
>> the main issue I have with it is that it relies on a order that is
>> implicit rather than intended. The order comes from how the .modinfo ELF
>> section is assembled together... so the fact that your call to
>> kmod_module_get_info() returns a list with the keys in the reverse order
>> of the MODULE_FIRMWARE() definitions, is basically because the compiler
>> toolchain did it did that way.
>>
>> It's worse when those sections come from different compilation units as
>> the order then is not predictable and can easily break with changes to
>> the build infra if the files are linked in different order.
>>
>> I think the grouping thing here would only be supported with firmware
>> defined on the same compilation unit, but it's something to keep in mind
>> and document.
>
>I had provided a simple API to help with explicit linker order years ago:
>
>https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8
>
>Other than that you have to rely on the order in the Makefile or way
>in which they are declared.

but that doesn't change the order inside a single section, .modinfo in
this case. Does it?

>
>> > the GROUP until after the FIRMWARE, so this can't work, as it already
>> > will have included all the ones below, hence why I bracketed top and
>> > bottom with a group.
>>
>> well... that is something that can be adapted easily by using a 2 pass
>> approach, filtering out the list based on the groups.
>>
>> I agree that yours is simpler though.  If we can rely on the
>> order produced by the compiler and we document the expectations of
>> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
>> simpler approach.
>>
>> Luis, any thoughts here?
>
>I see the Dracut code indicates that the order says now that you should
>put the preferred firmware last, and that seems to match most coding
>conventions, ie, new firmwares likely get added last, so it's a nice

not all. i915 for example keeps it newest first so when attempting
multiple firmware versions it starts from the preferred version.  It
will be harder to convert since it also uses a x-macro to make sure the
MODULE_FIRMWARE() and the the platform mapping are actually using the same
firmware.

>coincidence. Will this always work? I don't know. But if you like to

short answer: it depends if your compiler is gcc *and* -O2 is used
Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
baked in:

	$ cat /tmp/a.c
	static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
	static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
	$ gcc -c -o /tmp/a.o /tmp/a.c
	$ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
	$ strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar

However that doesn't match when building modules. In kmod:

diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
index 503e4d8..6dd5771 100644
--- a/testsuite/module-playground/mod-simple.c
+++ b/testsuite/module-playground/mod-simple.c
@@ -30,3 +30,9 @@ module_exit(test_module_exit);
  
  MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
  MODULE_LICENSE("GPL");
+
+
+static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
+static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";

	$ make ....
	$ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
	$ strings /tmp/modinfo_cpp
	modinfo_cpp_bar
	modinfo_cpp_foo

It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
inverted in testsuite/module-playground/mod-simple.o

After checking the options passed to gcc, here is the "culprit": -O2

	$ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar
	$ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_bar
	modinfo_manual_foo

It seems anything but -O0 inverts the section.

	$ gcc --version 
	gcc (GCC) 12.2.1 20230201

It doesn't match the behavior described in its man page though. Manually
specifying all the options that -O1 turns on doesn't invert it.

	$ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
		-fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
		-fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
		-fif-conversion2 -finline-functions-called-once -fipa-modref \
		-fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
		-fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
		-fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
		-fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
		-ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
		-ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
		-ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
		&& objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	cc1: warning: this target machine does not have delayed branches
	modinfo_manual_foo
	modinfo_manual_bar

Why? I'm not sure.

Finally, clang doesn't invert it for any optimization value.

	$ clang -O -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar
	$ clang -O1 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar
	$ clang -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar

>hedge, then this seems fine so long as I'm folks follow up to fix issues
>later. I think it should and the simplicity is preferred, worth a shot
>I think.

Based on the above and my previous reply, I think we should have
something more explicit about the order rather than relying on the
toolchain behavior.

Lucas De Marchi

>
>But the examples on both sides are pretty terrible. I'd just like to ask
>all this gets extended in proper kdoc form and we are able to get users
>and developers to read this under "Module support" in:
>
>https://docs.kernel.org/core-api/kernel-api.html
>
>So go to town with a new section for:
>
>Documentation/core-api/kernel-api.rst
>
>  Luis

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

[Lucas De Marchi]

On Mon, Apr 24, 2023 at 03:56:53PM -0700, Luis Chamberlain wrote:
>On Mon, Apr 24, 2023 at 10:01:13AM -0700, Lucas De Marchi wrote:
>> On Mon, Apr 24, 2023 at 03:44:18PM +1000, Dave Airlie wrote:
>> > On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> > >
>> > > On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
>> > > >From: Dave Airlie <airlied@redhat.com>
>> > > >
>> > > >This adds a tag that will go into the module info, only one firmware from
>> > > >the group given needs to be available for this driver to work. This allows
>> > > >dracut to avoid adding in firmware that aren't needed.
>> > > >
>> > > >This just brackets a module list in the modinfo, the modules in the list
>> > > >will get entries in reversed order so the last module in the list is the
>> > > >preferred one.
>> > > >
>> > > >The corresponding dracut code it at:
>> > > >https://github.com/dracutdevs/dracut/pull/2309
>> > >
>> > > it would be good to have the example usage in the commit message here so
>> > > it can be easily checked as reference for other drivers.
>> >
>> > Good point.
>> >
>> > >
>> > > I don't think we ever had any ordering in modinfo being relevant for
>> > > other things. Considering the use case and that we could also use a
>> > > similar thing for i915 / xe modules wrt to the major version,
>> > > couldn't we do something like below?
>> > >
>> > >         MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
>> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
>> > >         MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
>> > >
>> > > so the group is created by startswith() rather than by the order the
>> > > modinfo appears in the elf section. In i915 we'd have:
>> >
>> > The way userspace parses these is reverse order, and it doesn't see
>>
>> the main issue I have with it is that it relies on a order that is
>> implicit rather than intended. The order comes from how the .modinfo ELF
>> section is assembled together... so the fact that your call to
>> kmod_module_get_info() returns a list with the keys in the reverse order
>> of the MODULE_FIRMWARE() definitions, is basically because the compiler
>> toolchain did it did that way.
>>
>> It's worse when those sections come from different compilation units as
>> the order then is not predictable and can easily break with changes to
>> the build infra if the files are linked in different order.
>>
>> I think the grouping thing here would only be supported with firmware
>> defined on the same compilation unit, but it's something to keep in mind
>> and document.
>
>I had provided a simple API to help with explicit linker order years ago:
>
>https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8
>
>Other than that you have to rely on the order in the Makefile or way
>in which they are declared.

but that doesn't change the order inside a single section, .modinfo in
this case. Does it?

>
>> > the GROUP until after the FIRMWARE, so this can't work, as it already
>> > will have included all the ones below, hence why I bracketed top and
>> > bottom with a group.
>>
>> well... that is something that can be adapted easily by using a 2 pass
>> approach, filtering out the list based on the groups.
>>
>> I agree that yours is simpler though.  If we can rely on the
>> order produced by the compiler and we document the expectations of
>> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
>> simpler approach.
>>
>> Luis, any thoughts here?
>
>I see the Dracut code indicates that the order says now that you should
>put the preferred firmware last, and that seems to match most coding
>conventions, ie, new firmwares likely get added last, so it's a nice

not all. i915 for example keeps it newest first so when attempting
multiple firmware versions it starts from the preferred version.  It
will be harder to convert since it also uses a x-macro to make sure the
MODULE_FIRMWARE() and the the platform mapping are actually using the same
firmware.

>coincidence. Will this always work? I don't know. But if you like to

short answer: it depends if your compiler is gcc *and* -O2 is used
Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
baked in:

	$ cat /tmp/a.c
	static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
	static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
	$ gcc -c -o /tmp/a.o /tmp/a.c
	$ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
	$ strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar

However that doesn't match when building modules. In kmod:

diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
index 503e4d8..6dd5771 100644
--- a/testsuite/module-playground/mod-simple.c
+++ b/testsuite/module-playground/mod-simple.c
@@ -30,3 +30,9 @@ module_exit(test_module_exit);
  
  MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
  MODULE_LICENSE("GPL");
+
+
+static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
+static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";

	$ make ....
	$ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
	$ strings /tmp/modinfo_cpp
	modinfo_cpp_bar
	modinfo_cpp_foo

It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
inverted in testsuite/module-playground/mod-simple.o

After checking the options passed to gcc, here is the "culprit": -O2

	$ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar
	$ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_bar
	modinfo_manual_foo

It seems anything but -O0 inverts the section.

	$ gcc --version 
	gcc (GCC) 12.2.1 20230201

It doesn't match the behavior described in its man page though. Manually
specifying all the options that -O1 turns on doesn't invert it.

	$ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
		-fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
		-fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
		-fif-conversion2 -finline-functions-called-once -fipa-modref \
		-fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
		-fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
		-fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
		-fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
		-ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
		-ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
		-ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
		&& objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	cc1: warning: this target machine does not have delayed branches
	modinfo_manual_foo
	modinfo_manual_bar

Why? I'm not sure.

Finally, clang doesn't invert it for any optimization value.

	$ clang -O -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar
	$ clang -O1 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar
	$ clang -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
	modinfo_manual_foo
	modinfo_manual_bar

>hedge, then this seems fine so long as I'm folks follow up to fix issues
>later. I think it should and the simplicity is preferred, worth a shot
>I think.

Based on the above and my previous reply, I think we should have
something more explicit about the order rather than relying on the
toolchain behavior.

Lucas De Marchi

>
>But the examples on both sides are pretty terrible. I'd just like to ask
>all this gets extended in proper kdoc form and we are able to get users
>and developers to read this under "Module support" in:
>
>https://docs.kernel.org/core-api/kernel-api.html
>
>So go to town with a new section for:
>
>Documentation/core-api/kernel-api.rst
>
>  Luis

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

[Luis Chamberlain]

On Tue, May 02, 2023 at 11:11:58AM -0700, Lucas De Marchi wrote:
> Based on the above and my previous reply, I think we should have
> something more explicit about the order rather than relying on the
> toolchain behavior.

You can open code ELF sections and provide SORT() but you can also use
helpers for all this. Long ago I provided low level ELF helpers to
provide the ability to easily sort through data / code using
linker-tables [0], to help with ELF section with explicit ordering,
perhaps this could be leveraged?

I *think* for instance, one could do, using the built-in firmware
conversion as a slightly relateed example [1], provide a firmware helper for
drivers which uses something like DECLARE_FIRMWARE_TABLE(acme_gpu_fw),
then that is declared as the ELF table for acme_gpu_fw, the firmware API
could then get the hint to use that table for iterating over with
linktable_for_each(fw, acme_gpu_fw). One would not be using the linker
table for the actual firmware but instead for the firmware odering.


The firmware loader could be extended with something like

#define DECLARE_FIRMWARE_TABLE(fw_table)  DECLARE_LINKTABLE_RO(struct fw_foo, fw_table)

struct fw_foo {
	const char *opt_subfamily;
};

#define FW_NAME_ORDERED(__level, __family, __sub_family)	\
	static LINKTABLE_INIT_DATA(fw_foo, __level) 		\
	__fw_ordered_##__family = {		     		\
	opt_subfamily = sub_family,				\
};

Then firmware could would use 

FW_NAME_ORDERED(01, acme_gpu_fw, coyote);

And helpers can use it to look for the firmware an firmare API call.

As to why linker-tables never got upstream? It promised / documented
too much, we need to just make the API conversion smooth and target
that. The ordering is a secondary win. The fact that we can simplify
init levels etc, is more futuristic and should only be documented once
we get there.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/commit/?h=20170620-linker-tables-v8&id=162698d2f1a2406c6a7a4d39f13113ca789fd2ec

  Luis

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

[Luis Chamberlain]

On Tue, May 02, 2023 at 11:11:58AM -0700, Lucas De Marchi wrote:
> Based on the above and my previous reply, I think we should have
> something more explicit about the order rather than relying on the
> toolchain behavior.

You can open code ELF sections and provide SORT() but you can also use
helpers for all this. Long ago I provided low level ELF helpers to
provide the ability to easily sort through data / code using
linker-tables [0], to help with ELF section with explicit ordering,
perhaps this could be leveraged?

I *think* for instance, one could do, using the built-in firmware
conversion as a slightly relateed example [1], provide a firmware helper for
drivers which uses something like DECLARE_FIRMWARE_TABLE(acme_gpu_fw),
then that is declared as the ELF table for acme_gpu_fw, the firmware API
could then get the hint to use that table for iterating over with
linktable_for_each(fw, acme_gpu_fw). One would not be using the linker
table for the actual firmware but instead for the firmware odering.


The firmware loader could be extended with something like

#define DECLARE_FIRMWARE_TABLE(fw_table)  DECLARE_LINKTABLE_RO(struct fw_foo, fw_table)

struct fw_foo {
	const char *opt_subfamily;
};

#define FW_NAME_ORDERED(__level, __family, __sub_family)	\
	static LINKTABLE_INIT_DATA(fw_foo, __level) 		\
	__fw_ordered_##__family = {		     		\
	opt_subfamily = sub_family,				\
};

Then firmware could would use 

FW_NAME_ORDERED(01, acme_gpu_fw, coyote);

And helpers can use it to look for the firmware an firmare API call.

As to why linker-tables never got upstream? It promised / documented
too much, we need to just make the API conversion smooth and target
that. The ordering is a secondary win. The fact that we can simplify
init levels etc, is more futuristic and should only be documented once
we get there.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/commit/?h=20170620-linker-tables-v8&id=162698d2f1a2406c6a7a4d39f13113ca789fd2ec

  Luis

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

[Dave Airlie]

>
> >
> >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> >> > will have included all the ones below, hence why I bracketed top and
> >> > bottom with a group.
> >>
> >> well... that is something that can be adapted easily by using a 2 pass
> >> approach, filtering out the list based on the groups.
> >>
> >> I agree that yours is simpler though.  If we can rely on the
> >> order produced by the compiler and we document the expectations of
> >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> >> simpler approach.
> >>
> >> Luis, any thoughts here?
> >
> >I see the Dracut code indicates that the order says now that you should
> >put the preferred firmware last, and that seems to match most coding
> >conventions, ie, new firmwares likely get added last, so it's a nice
>
> not all. i915 for example keeps it newest first so when attempting
> multiple firmware versions it starts from the preferred version.  It
> will be harder to convert since it also uses a x-macro to make sure the
> MODULE_FIRMWARE() and the the platform mapping are actually using the same
> firmware.
>
> >coincidence. Will this always work? I don't know. But if you like to
>
> short answer: it depends if your compiler is gcc *and* -O2 is used
> Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> baked in:
>
>         $ cat /tmp/a.c
>         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
>         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
>         $ gcc -c -o /tmp/a.o /tmp/a.c
>         $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
>         $ strings /tmp/modinfo_manual
>         modinfo_manual_foo
>         modinfo_manual_bar
>
> However that doesn't match when building modules. In kmod:
>
> diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
> index 503e4d8..6dd5771 100644
> --- a/testsuite/module-playground/mod-simple.c
> +++ b/testsuite/module-playground/mod-simple.c
> @@ -30,3 +30,9 @@ module_exit(test_module_exit);
>
>   MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
>   MODULE_LICENSE("GPL");
> +
> +
> +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
>
>         $ make ....
>         $ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
>         $ strings /tmp/modinfo_cpp
>         modinfo_cpp_bar
>         modinfo_cpp_foo
>
> It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> inverted in testsuite/module-playground/mod-simple.o
>
> After checking the options passed to gcc, here is the "culprit": -O2
>
>         $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         modinfo_manual_foo
>         modinfo_manual_bar
>         $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         modinfo_manual_bar
>         modinfo_manual_foo
>
> It seems anything but -O0 inverts the section.
>
>         $ gcc --version
>         gcc (GCC) 12.2.1 20230201
>
> It doesn't match the behavior described in its man page though. Manually
> specifying all the options that -O1 turns on doesn't invert it.
>
>         $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
>                 -fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
>                 -fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
>                 -fif-conversion2 -finline-functions-called-once -fipa-modref \
>                 -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
>                 -fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
>                 -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
>                 -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
>                 -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
>                 -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
>                 -ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
>                 && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         cc1: warning: this target machine does not have delayed branches
>         modinfo_manual_foo
>         modinfo_manual_bar
>

Thanks Lucas,

-ftoplevel-reorder is the one that does it, now that does mean how
I've done it isn't going to be robust.

I will reconsider but in order to keep backwards compat, it might be
easier to add firmware groups as an explicit list, but also spit out
the individual names, but not sure how clean this will end up on
dracut side.

I'll take a look at the other options, but it does seem like reworking
dracut is going to be the harder end of this, esp if I still want to
keep compat with older ones.

Dave.

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

[Dave Airlie]

>
> >
> >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> >> > will have included all the ones below, hence why I bracketed top and
> >> > bottom with a group.
> >>
> >> well... that is something that can be adapted easily by using a 2 pass
> >> approach, filtering out the list based on the groups.
> >>
> >> I agree that yours is simpler though.  If we can rely on the
> >> order produced by the compiler and we document the expectations of
> >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> >> simpler approach.
> >>
> >> Luis, any thoughts here?
> >
> >I see the Dracut code indicates that the order says now that you should
> >put the preferred firmware last, and that seems to match most coding
> >conventions, ie, new firmwares likely get added last, so it's a nice
>
> not all. i915 for example keeps it newest first so when attempting
> multiple firmware versions it starts from the preferred version.  It
> will be harder to convert since it also uses a x-macro to make sure the
> MODULE_FIRMWARE() and the the platform mapping are actually using the same
> firmware.
>
> >coincidence. Will this always work? I don't know. But if you like to
>
> short answer: it depends if your compiler is gcc *and* -O2 is used
> Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> baked in:
>
>         $ cat /tmp/a.c
>         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
>         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
>         $ gcc -c -o /tmp/a.o /tmp/a.c
>         $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
>         $ strings /tmp/modinfo_manual
>         modinfo_manual_foo
>         modinfo_manual_bar
>
> However that doesn't match when building modules. In kmod:
>
> diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
> index 503e4d8..6dd5771 100644
> --- a/testsuite/module-playground/mod-simple.c
> +++ b/testsuite/module-playground/mod-simple.c
> @@ -30,3 +30,9 @@ module_exit(test_module_exit);
>
>   MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
>   MODULE_LICENSE("GPL");
> +
> +
> +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
>
>         $ make ....
>         $ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
>         $ strings /tmp/modinfo_cpp
>         modinfo_cpp_bar
>         modinfo_cpp_foo
>
> It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> inverted in testsuite/module-playground/mod-simple.o
>
> After checking the options passed to gcc, here is the "culprit": -O2
>
>         $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         modinfo_manual_foo
>         modinfo_manual_bar
>         $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         modinfo_manual_bar
>         modinfo_manual_foo
>
> It seems anything but -O0 inverts the section.
>
>         $ gcc --version
>         gcc (GCC) 12.2.1 20230201
>
> It doesn't match the behavior described in its man page though. Manually
> specifying all the options that -O1 turns on doesn't invert it.
>
>         $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
>                 -fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
>                 -fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
>                 -fif-conversion2 -finline-functions-called-once -fipa-modref \
>                 -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
>                 -fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
>                 -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
>                 -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
>                 -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
>                 -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
>                 -ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
>                 && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         cc1: warning: this target machine does not have delayed branches
>         modinfo_manual_foo
>         modinfo_manual_bar
>

Thanks Lucas,

-ftoplevel-reorder is the one that does it, now that does mean how
I've done it isn't going to be robust.

I will reconsider but in order to keep backwards compat, it might be
easier to add firmware groups as an explicit list, but also spit out
the individual names, but not sure how clean this will end up on
dracut side.

I'll take a look at the other options, but it does seem like reworking
dracut is going to be the harder end of this, esp if I still want to
keep compat with older ones.

Dave.

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

[Luis Chamberlain]

On Wed, May 03, 2023 at 01:19:31PM +1000, Dave Airlie wrote:
> >
> > >
> > >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > >> > will have included all the ones below, hence why I bracketed top and
> > >> > bottom with a group.
> > >>
> > >> well... that is something that can be adapted easily by using a 2 pass
> > >> approach, filtering out the list based on the groups.
> > >>
> > >> I agree that yours is simpler though.  If we can rely on the
> > >> order produced by the compiler and we document the expectations of
> > >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> > >> simpler approach.
> > >>
> > >> Luis, any thoughts here?
> > >
> > >I see the Dracut code indicates that the order says now that you should
> > >put the preferred firmware last, and that seems to match most coding
> > >conventions, ie, new firmwares likely get added last, so it's a nice
> >
> > not all. i915 for example keeps it newest first so when attempting
> > multiple firmware versions it starts from the preferred version.  It
> > will be harder to convert since it also uses a x-macro to make sure the
> > MODULE_FIRMWARE() and the the platform mapping are actually using the same
> > firmware.
> >
> > >coincidence. Will this always work? I don't know. But if you like to
> >
> > short answer: it depends if your compiler is gcc *and* -O2 is used
> > Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> > baked in:
> >
> >         $ cat /tmp/a.c
> >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
> >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
> >         $ gcc -c -o /tmp/a.o /tmp/a.c
> >         $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
> >         $ strings /tmp/modinfo_manual
> >         modinfo_manual_foo
> >         modinfo_manual_bar
> >
> > However that doesn't match when building modules. In kmod:
> >
> > diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
> > index 503e4d8..6dd5771 100644
> > --- a/testsuite/module-playground/mod-simple.c
> > +++ b/testsuite/module-playground/mod-simple.c
> > @@ -30,3 +30,9 @@ module_exit(test_module_exit);
> >
> >   MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
> >   MODULE_LICENSE("GPL");
> > +
> > +
> > +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> > +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
> >
> >         $ make ....
> >         $ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
> >         $ strings /tmp/modinfo_cpp
> >         modinfo_cpp_bar
> >         modinfo_cpp_foo
> >
> > It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> > inverted in testsuite/module-playground/mod-simple.o
> >
> > After checking the options passed to gcc, here is the "culprit": -O2
> >
> >         $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> >         modinfo_manual_foo
> >         modinfo_manual_bar
> >         $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> >         modinfo_manual_bar
> >         modinfo_manual_foo
> >
> > It seems anything but -O0 inverts the section.
> >
> >         $ gcc --version
> >         gcc (GCC) 12.2.1 20230201
> >
> > It doesn't match the behavior described in its man page though. Manually
> > specifying all the options that -O1 turns on doesn't invert it.
> >
> >         $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
> >                 -fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
> >                 -fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
> >                 -fif-conversion2 -finline-functions-called-once -fipa-modref \
> >                 -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
> >                 -fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
> >                 -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
> >                 -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
> >                 -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
> >                 -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
> >                 -ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
> >                 && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> >         cc1: warning: this target machine does not have delayed branches
> >         modinfo_manual_foo
> >         modinfo_manual_bar
> >
> 
> Thanks Lucas,
> 
> -ftoplevel-reorder is the one that does it, now that does mean how
> I've done it isn't going to be robust.
> 
> I will reconsider but in order to keep backwards compat, it might be
> easier to add firmware groups as an explicit list, but also spit out
> the individual names, but not sure how clean this will end up on
> dracut side.
> 
> I'll take a look at the other options, but it does seem like reworking
> dracut is going to be the harder end of this, esp if I still want to
> keep compat with older ones.

Hey Dave, just curious if there was going to be another follow up patch
for this or if it was already posted. I don't see it clearly so just
wanted to double check.

  Luis

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

[Luis Chamberlain]

On Wed, May 03, 2023 at 01:19:31PM +1000, Dave Airlie wrote:
> >
> > >
> > >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > >> > will have included all the ones below, hence why I bracketed top and
> > >> > bottom with a group.
> > >>
> > >> well... that is something that can be adapted easily by using a 2 pass
> > >> approach, filtering out the list based on the groups.
> > >>
> > >> I agree that yours is simpler though.  If we can rely on the
> > >> order produced by the compiler and we document the expectations of
> > >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> > >> simpler approach.
> > >>
> > >> Luis, any thoughts here?
> > >
> > >I see the Dracut code indicates that the order says now that you should
> > >put the preferred firmware last, and that seems to match most coding
> > >conventions, ie, new firmwares likely get added last, so it's a nice
> >
> > not all. i915 for example keeps it newest first so when attempting
> > multiple firmware versions it starts from the preferred version.  It
> > will be harder to convert since it also uses a x-macro to make sure the
> > MODULE_FIRMWARE() and the the platform mapping are actually using the same
> > firmware.
> >
> > >coincidence. Will this always work? I don't know. But if you like to
> >
> > short answer: it depends if your compiler is gcc *and* -O2 is used
> > Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> > baked in:
> >
> >         $ cat /tmp/a.c
> >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
> >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
> >         $ gcc -c -o /tmp/a.o /tmp/a.c
> >         $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
> >         $ strings /tmp/modinfo_manual
> >         modinfo_manual_foo
> >         modinfo_manual_bar
> >
> > However that doesn't match when building modules. In kmod:
> >
> > diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
> > index 503e4d8..6dd5771 100644
> > --- a/testsuite/module-playground/mod-simple.c
> > +++ b/testsuite/module-playground/mod-simple.c
> > @@ -30,3 +30,9 @@ module_exit(test_module_exit);
> >
> >   MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
> >   MODULE_LICENSE("GPL");
> > +
> > +
> > +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> > +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
> >
> >         $ make ....
> >         $ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
> >         $ strings /tmp/modinfo_cpp
> >         modinfo_cpp_bar
> >         modinfo_cpp_foo
> >
> > It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> > inverted in testsuite/module-playground/mod-simple.o
> >
> > After checking the options passed to gcc, here is the "culprit": -O2
> >
> >         $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> >         modinfo_manual_foo
> >         modinfo_manual_bar
> >         $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> >         modinfo_manual_bar
> >         modinfo_manual_foo
> >
> > It seems anything but -O0 inverts the section.
> >
> >         $ gcc --version
> >         gcc (GCC) 12.2.1 20230201
> >
> > It doesn't match the behavior described in its man page though. Manually
> > specifying all the options that -O1 turns on doesn't invert it.
> >
> >         $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
> >                 -fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
> >                 -fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
> >                 -fif-conversion2 -finline-functions-called-once -fipa-modref \
> >                 -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
> >                 -fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
> >                 -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
> >                 -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
> >                 -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
> >                 -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
> >                 -ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
> >                 && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> >         cc1: warning: this target machine does not have delayed branches
> >         modinfo_manual_foo
> >         modinfo_manual_bar
> >
> 
> Thanks Lucas,
> 
> -ftoplevel-reorder is the one that does it, now that does mean how
> I've done it isn't going to be robust.
> 
> I will reconsider but in order to keep backwards compat, it might be
> easier to add firmware groups as an explicit list, but also spit out
> the individual names, but not sure how clean this will end up on
> dracut side.
> 
> I'll take a look at the other options, but it does seem like reworking
> dracut is going to be the harder end of this, esp if I still want to
> keep compat with older ones.

Hey Dave, just curious if there was going to be another follow up patch
for this or if it was already posted. I don't see it clearly so just
wanted to double check.

  Luis

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

[David Airlie]

On Wed, May 24, 2023 at 3:26 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, May 03, 2023 at 01:19:31PM +1000, Dave Airlie wrote:
> > >
> > > >
> > > >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > > >> > will have included all the ones below, hence why I bracketed top and
> > > >> > bottom with a group.
> > > >>
> > > >> well... that is something that can be adapted easily by using a 2 pass
> > > >> approach, filtering out the list based on the groups.
> > > >>
> > > >> I agree that yours is simpler though.  If we can rely on the
> > > >> order produced by the compiler and we document the expectations of
> > > >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> > > >> simpler approach.
> > > >>
> > > >> Luis, any thoughts here?
> > > >
> > > >I see the Dracut code indicates that the order says now that you should
> > > >put the preferred firmware last, and that seems to match most coding
> > > >conventions, ie, new firmwares likely get added last, so it's a nice
> > >
> > > not all. i915 for example keeps it newest first so when attempting
> > > multiple firmware versions it starts from the preferred version.  It
> > > will be harder to convert since it also uses a x-macro to make sure the
> > > MODULE_FIRMWARE() and the the platform mapping are actually using the same
> > > firmware.
> > >
> > > >coincidence. Will this always work? I don't know. But if you like to
> > >
> > > short answer: it depends if your compiler is gcc *and* -O2 is used
> > > Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> > > baked in:
> > >
> > >         $ cat /tmp/a.c
> > >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
> > >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
> > >         $ gcc -c -o /tmp/a.o /tmp/a.c
> > >         $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
> > >         $ strings /tmp/modinfo_manual
> > >         modinfo_manual_foo
> > >         modinfo_manual_bar
> > >
> > > However that doesn't match when building modules. In kmod:
> > >
> > > diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
> > > index 503e4d8..6dd5771 100644
> > > --- a/testsuite/module-playground/mod-simple.c
> > > +++ b/testsuite/module-playground/mod-simple.c
> > > @@ -30,3 +30,9 @@ module_exit(test_module_exit);
> > >
> > >   MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
> > >   MODULE_LICENSE("GPL");
> > > +
> > > +
> > > +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> > > +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
> > >
> > >         $ make ....
> > >         $ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
> > >         $ strings /tmp/modinfo_cpp
> > >         modinfo_cpp_bar
> > >         modinfo_cpp_foo
> > >
> > > It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> > > inverted in testsuite/module-playground/mod-simple.o
> > >
> > > After checking the options passed to gcc, here is the "culprit": -O2
> > >
> > >         $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> > >         modinfo_manual_foo
> > >         modinfo_manual_bar
> > >         $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> > >         modinfo_manual_bar
> > >         modinfo_manual_foo
> > >
> > > It seems anything but -O0 inverts the section.
> > >
> > >         $ gcc --version
> > >         gcc (GCC) 12.2.1 20230201
> > >
> > > It doesn't match the behavior described in its man page though. Manually
> > > specifying all the options that -O1 turns on doesn't invert it.
> > >
> > >         $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
> > >                 -fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
> > >                 -fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
> > >                 -fif-conversion2 -finline-functions-called-once -fipa-modref \
> > >                 -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
> > >                 -fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
> > >                 -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
> > >                 -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
> > >                 -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
> > >                 -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
> > >                 -ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
> > >                 && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> > >         cc1: warning: this target machine does not have delayed branches
> > >         modinfo_manual_foo
> > >         modinfo_manual_bar
> > >
> >
> > Thanks Lucas,
> >
> > -ftoplevel-reorder is the one that does it, now that does mean how
> > I've done it isn't going to be robust.
> >
> > I will reconsider but in order to keep backwards compat, it might be
> > easier to add firmware groups as an explicit list, but also spit out
> > the individual names, but not sure how clean this will end up on
> > dracut side.
> >
> > I'll take a look at the other options, but it does seem like reworking
> > dracut is going to be the harder end of this, esp if I still want to
> > keep compat with older ones.
>
> Hey Dave, just curious if there was going to be another follow up patch
> for this or if it was already posted. I don't see it clearly so just
> wanted to double check.

I'm still considering the options here.

I could leave the kernel patch as-is and add explicit sorting in
dracut for anything in the groups, but then we have to name/version
the firmware in a certain way, another option might be to emit the
group bounds and two records, one old, one new per-fw file, then have
some sort of explicit versioning by the driver over what order to load
them.

Dave.

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

[David Airlie]

On Wed, May 24, 2023 at 3:26 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, May 03, 2023 at 01:19:31PM +1000, Dave Airlie wrote:
> > >
> > > >
> > > >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > > >> > will have included all the ones below, hence why I bracketed top and
> > > >> > bottom with a group.
> > > >>
> > > >> well... that is something that can be adapted easily by using a 2 pass
> > > >> approach, filtering out the list based on the groups.
> > > >>
> > > >> I agree that yours is simpler though.  If we can rely on the
> > > >> order produced by the compiler and we document the expectations of
> > > >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> > > >> simpler approach.
> > > >>
> > > >> Luis, any thoughts here?
> > > >
> > > >I see the Dracut code indicates that the order says now that you should
> > > >put the preferred firmware last, and that seems to match most coding
> > > >conventions, ie, new firmwares likely get added last, so it's a nice
> > >
> > > not all. i915 for example keeps it newest first so when attempting
> > > multiple firmware versions it starts from the preferred version.  It
> > > will be harder to convert since it also uses a x-macro to make sure the
> > > MODULE_FIRMWARE() and the the platform mapping are actually using the same
> > > firmware.
> > >
> > > >coincidence. Will this always work? I don't know. But if you like to
> > >
> > > short answer: it depends if your compiler is gcc *and* -O2 is used
> > > Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> > > baked in:
> > >
> > >         $ cat /tmp/a.c
> > >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
> > >         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
> > >         $ gcc -c -o /tmp/a.o /tmp/a.c
> > >         $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
> > >         $ strings /tmp/modinfo_manual
> > >         modinfo_manual_foo
> > >         modinfo_manual_bar
> > >
> > > However that doesn't match when building modules. In kmod:
> > >
> > > diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
> > > index 503e4d8..6dd5771 100644
> > > --- a/testsuite/module-playground/mod-simple.c
> > > +++ b/testsuite/module-playground/mod-simple.c
> > > @@ -30,3 +30,9 @@ module_exit(test_module_exit);
> > >
> > >   MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@intel.com>");
> > >   MODULE_LICENSE("GPL");
> > > +
> > > +
> > > +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> > > +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
> > >
> > >         $ make ....
> > >         $ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
> > >         $ strings /tmp/modinfo_cpp
> > >         modinfo_cpp_bar
> > >         modinfo_cpp_foo
> > >
> > > It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> > > inverted in testsuite/module-playground/mod-simple.o
> > >
> > > After checking the options passed to gcc, here is the "culprit": -O2
> > >
> > >         $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> > >         modinfo_manual_foo
> > >         modinfo_manual_bar
> > >         $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> > >         modinfo_manual_bar
> > >         modinfo_manual_foo
> > >
> > > It seems anything but -O0 inverts the section.
> > >
> > >         $ gcc --version
> > >         gcc (GCC) 12.2.1 20230201
> > >
> > > It doesn't match the behavior described in its man page though. Manually
> > > specifying all the options that -O1 turns on doesn't invert it.
> > >
> > >         $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
> > >                 -fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
> > >                 -fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
> > >                 -fif-conversion2 -finline-functions-called-once -fipa-modref \
> > >                 -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
> > >                 -fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
> > >                 -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
> > >                 -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
> > >                 -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
> > >                 -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
> > >                 -ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
> > >                 && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
> > >         cc1: warning: this target machine does not have delayed branches
> > >         modinfo_manual_foo
> > >         modinfo_manual_bar
> > >
> >
> > Thanks Lucas,
> >
> > -ftoplevel-reorder is the one that does it, now that does mean how
> > I've done it isn't going to be robust.
> >
> > I will reconsider but in order to keep backwards compat, it might be
> > easier to add firmware groups as an explicit list, but also spit out
> > the individual names, but not sure how clean this will end up on
> > dracut side.
> >
> > I'll take a look at the other options, but it does seem like reworking
> > dracut is going to be the harder end of this, esp if I still want to
> > keep compat with older ones.
>
> Hey Dave, just curious if there was going to be another follow up patch
> for this or if it was already posted. I don't see it clearly so just
> wanted to double check.

I'm still considering the options here.

I could leave the kernel patch as-is and add explicit sorting in
dracut for anything in the groups, but then we have to name/version
the firmware in a certain way, another option might be to emit the
group bounds and two records, one old, one new per-fw file, then have
some sort of explicit versioning by the driver over what order to load
them.

Dave.

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

[Luis Chamberlain]

On Wed, May 24, 2023 at 03:35:41PM +1000, David Airlie wrote:
> On Wed, May 24, 2023 at 3:26 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > Hey Dave, just curious if there was going to be another follow up patch
> > for this or if it was already posted. I don't see it clearly so just
> > wanted to double check.
> 
> I'm still considering the options here.
> 
> I could leave the kernel patch as-is and add explicit sorting in
> dracut for anything in the groups, but then we have to name/version
> the firmware in a certain way, another option might be to emit the
> group bounds and two records, one old, one new per-fw file, then have
> some sort of explicit versioning by the driver over what order to load
> them.

Great thanks, just wanted to make sure I didn't neglect any pending
patch.

  Luis

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

[Luis Chamberlain]

On Wed, May 24, 2023 at 03:35:41PM +1000, David Airlie wrote:
> On Wed, May 24, 2023 at 3:26 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > Hey Dave, just curious if there was going to be another follow up patch
> > for this or if it was already posted. I don't see it clearly so just
> > wanted to double check.
> 
> I'm still considering the options here.
> 
> I could leave the kernel patch as-is and add explicit sorting in
> dracut for anything in the groups, but then we have to name/version
> the firmware in a certain way, another option might be to emit the
> group bounds and two records, one old, one new per-fw file, then have
> some sort of explicit versioning by the driver over what order to load
> them.

Great thanks, just wanted to make sure I didn't neglect any pending
patch.

  Luis