Loading optional firmware

Currently the firmware loader only exposes one silent path for querying optional firmware, and that is request_firmware_direct(). This function also disables the usermodehelper fallback which might not always be the desired behaviour.

This patch introduces request_firmware_optional(), which will not produce error/warning messages if the firmware file is not found, but

will still attempt to query the usermodehelper

for the optional
firmware. Effectively, FW_OPT_UEVENT | FW_OPT_FALLBACK | FW_OPT_NO_WARN.

v2: add header prototype, use updated opt_flags

Signed-off-by Andres Rodriguez <[email protected]>

---

Sorry, I messed up the v1 patch and sent the wrong one from before I rebased.

 drivers/base/firmware_class.c | 26 +++++++++++++++++++++++++-
 include/linux/firmware.h      |  2 ++
 2 files changed, 27 insertions(+), 1 deletion(-)

drivers/base/firmware_class.c +0 -0

index 7dd36ace6152..4e1eddea241b 100644

drivers/base/firmware_class.c +13 -0

        return fw_load_from_user_helper(fw, name, device, opt_flags);
 }
 #else /* CONFIG_FW_LOADER_USER_HELPER */

@@ -1351,6 +1351,30 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 }
 EXPORT_SYMBOL(request_firmware);
 
+/**
+ * request_firmware_optional: - request for an optional fw module
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ **/
+int
+request_firmware_optional(const struct firmware **firmware_p, const char *name,
+                         struct device *device)
+{

You also I take it have users in
mind? I'd like to see at least one user of the API or this fixing a reported issue. Ie, if users have reported this as issues incorrectly, referring to those incorrect posts as issues and how this created confusion would help.

Quoted 5 lines · Luis R. Rodriguez

You also I take it have users in
mind? I'd like to see at least one user of the API or this fixing a reported issue. Ie, if users have reported this as issues incorrectly, referring to those incorrect posts as issues and how this created confusion would help.

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks the other drivers which I think they wanted changed as well.

The old up on
that front was that the firmware API was in a huge state of flux and debate about *how* we'd evolve the API, either through a data driven API or functional driven API, ie whether or not we'd add a flexible one API call with a set of options, or keep extending functionality with new exported symbols per use case. The later is how we'd keep evolving the API as such the way you are doing it is fine. Ie, if there is a use case for an optional firmware also for the async case a new API call will have to be made. As stupid as this sounds.

Also please take a look at lib/test_firmware.c -- I don't think it makes sense to add a new test case for this API call, so at least worth documenting why somewhere if you find a suitable place for that.

Quoted 1 line

Also - I forgot to ask you to extend the

Documentation/driver-api/firmware/ documentation accordingly. Please do that.

Luis

Quoted 3 lines

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks the other drivers which I think they wanted changed as well.

Arend, Kalle, would love to hear your feedback.

Quoted 5 lines · Andres Rodriguez

Quoted 3 lines

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks the other drivers which I think they wanted changed as well.

Arend, Kalle, would love to hear your feedback.

I am not sure if it was ath10k, but Kalle will surely know. The other driver firing a whole batch of firmware requests is iwlwifi. These basically try to get latest firmware version and if not there try an older one.

The brcmfmac driver I maintain is slightly different. It downloads two distinct pieces of firmware of which one is optional for certain configurations. Currently, my driver does two asynchronous requests for it, but I consider changing it and only make the first request asynchronous and the second request synchronous. You can look at the current code in drivers/net/wireless/broadcom/brcmfmac/firmware.c. However, I did quite some restructuring last week. Anyway, I probably will end up using the "optional" api where appropriate.

Quoted 10 lines · Arend van Spriel

Quoted 5 lines · Andres Rodriguez

Quoted 3 lines

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks the other drivers which I think they wanted changed as well.

Arend, Kalle, would love to hear your feedback.

I am not sure if it was ath10k, but Kalle will surely know. The other driver firing a whole batch of firmware requests is iwlwifi. These basically try to get latest firmware version and if not there try an older one.

Ah I recall now. At least for iwlwifi its that it requests firmware with a range of api files, and we don't need information about files in the middle not found, given all we need to know if is if at lest one file was found or not.

I have future code to also enable use of a range request which would replace the recursive nature of iwlwifi's firmware API request, so it simplifies it considerably.

Once we get this flag to be silent in, this can be used later. Ie, the new API I'd add would replace the complex api revision thing for an internal set.

Quoted 4 lines · Luis R. Rodriguez

Quoted 3 lines · Arend van Spriel

Quoted 2 lines · Andres Rodriguez

Quoted 2 lines

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks

the

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

other drivers which I think they wanted changed as well.

Arend, Kalle, would love to hear your feedback.

I am not sure if it was ath10k, but Kalle will surely know. The other

driver

Quoted 1 line

Quoted 1 line

firing a whole batch of firmware requests is iwlwifi. These basically

try to

Quoted 3 lines

Quoted 1 line

get latest firmware version and if not there try an older one.

Ah I recall now. At least for iwlwifi its that it requests firmware with

a

Quoted 1 line

range of api files, and we don't need information about files in the

middle

Quoted 4 lines

not found, given all we need to know if is if at lest one file was found or not.

I have future code to also enable use of a range request which would

replace

Quoted 1 line

the recursive nature of iwlwifi's firmware API request, so it simplifies

it

Quoted 3 lines

considerably.

Once we get this flag to be silent in, this can be used later. Ie, the

new

Quoted 1 line

API I'd add would replace the complex api revision thing for an internal

set.

TBH I doubt we would use this kind of "range" request in ath10k, the current code works just fine only if we can get rid of the annoying warning from request_firmware(). Unless if it's significantly faster or something.

--
Kalle Valo

Quoted 17 lines · Luis R. Rodriguez

Quoted 10 lines · Arend van Spriel

Quoted 5 lines · Andres Rodriguez

Quoted 3 lines

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks the other drivers which I think they wanted changed as well.

Arend, Kalle, would love to hear your feedback.

I am not sure if it was ath10k, but Kalle will surely know. The other driver firing a whole batch of firmware requests is iwlwifi. These basically try to get latest firmware version and if not there try an older one.

Ah I recall now. At least for iwlwifi its that it requests firmware with a range of api files, and we don't need information about files in the middle not found, given all we need to know if is if at lest one file was found or not.

Quoted 6 lines

I have future code to also enable use of a range request which would replace the recursive nature of iwlwifi's firmware API request, so it simplifies it considerably.

Once we get this flag to be silent in, this can be used later. Ie, the new API I'd add would replace the complex api revision thing for an internal set.

TBH I doubt we would use this kind of "range" request in ath10k,

the
current code works just fine only if we can get rid of the annoying warning from request_firmware(). Unless if it's significantly faster or something.

Quoted 19 lines · Kalle Valo

Quoted 17 lines · Luis R. Rodriguez

Quoted 10 lines · Arend van Spriel

Quoted 5 lines · Andres Rodriguez

Quoted 3 lines

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks the other drivers which I think they wanted changed as well.

Arend, Kalle, would love to hear your feedback.

I am not sure if it was ath10k, but Kalle will surely know. The other driver firing a whole batch of firmware requests is iwlwifi. These basically try to get latest firmware version and if not there try an older one.

Ah I recall now. At least for iwlwifi its that it requests firmware with a range of api files, and we don't need information about files in the middle not found, given all we need to know if is if at lest one file was found or not.

Quoted 8 lines

Quoted 6 lines

I have future code to also enable use of a range request which would replace the recursive nature of iwlwifi's firmware API request, so it simplifies it considerably.

Once we get this flag to be silent in, this can be used later. Ie, the new API I'd add would replace the complex api revision thing for an internal set.

TBH I doubt we would use this kind of "range" request in ath10k,

Well it doesn't have the form to use a range either so it wouldn't make sense.

Quoted 4 lines

the
current code works just fine only if we can get rid of the annoying warning from request_firmware(). Unless if it's significantly faster or something.

Thanks, I see ath10k uses the sync request_firmware() call, so indeed it would be a trivial conversion.

Andres can you roll that in for your patch series?

Luis

Quoted 5 lines · Andres Rodriguez

Quoted 3 lines

Your patch series then should also have the driver callers who you want to modify to use this new API. Collect from the 802.11 folks the other drivers which I think they wanted changed as well.

Arend, Kalle, would love to hear your feedback.

I am not sure if it was ath10k, but Kalle will surely know. The other driver firing a whole batch of firmware requests is iwlwifi. These basically try to get latest firmware version and if not there try an older one.

Quoted 17 lines

drivers/base/firmware_class.c +17 -0

+/**
+ * request_firmware_optional: - request for an optional fw module
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ **/
+int
+request_firmware_optional(const struct firmware **firmware_p, const char *name,
+                         struct device *device)
+{
+       int ret;
+
+       /* Need to pin this module until return */
+       __module_get(THIS_MODULE);

drivers/base/firmware_class.c request_firmware(const struct firmware **firmware_p, const char *name, · line 1376

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+               ret = _request_firmware(firmware_p, name, device, NULL, 0,
+                               FW_OPT_UEVENT | FW_OPT_NO_WARN );
+       module_put(THIS_MODULE);
+       return ret;
+}
+EXPORT_SYMBOL(request_firmware_optional);

New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().

Quoted 17 lines · Luis R. Rodriguez

Quoted 17 lines

drivers/base/firmware_class.c +17 -0

+/**
+ * request_firmware_optional: - request for an optional fw module
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ **/
+int
+request_firmware_optional(const struct firmware **firmware_p, const char *name,
+                         struct device *device)
+{
+       int ret;
+
+       /* Need to pin this module until return */
+       __module_get(THIS_MODULE);

Quoted 8 lines

drivers/base/firmware_class.c request_firmware(const struct firmware **firmware_p, const char *name, · line 1376

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+               ret = _request_firmware(firmware_p, name, device, NULL, 0,
+                               FW_OPT_UEVENT | FW_OPT_NO_WARN );
+       module_put(THIS_MODULE);
+       return ret;
+}
+EXPORT_SYMBOL(request_firmware_optional);

New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().

To me the word optional feels weird to me. For example, in ath10k I suspect we would be only calling request_firmware_optional() with all firmware and not request_firmware() at all.

How about request_firmware_nowarn()? That would even match the documentation above.

Quoted 9 lines · Kalle Valo

Quoted 7 lines · Luis R. Rodriguez

Quoted 7 lines

+/**
+ * request_firmware_optional: - request for an optional fw module
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(),

except

Quoted 4 lines

Quoted 4 lines

Review

Quoted 4 lines

+ * it doesn't produce warning messages when the file is not found.
+ **/

(quoted diff) +2 -0

+int
+request_firmware_optional(const struct firmware **firmware_p, const

char *name,

Quoted 7 lines

Quoted 7 lines

drivers/base/firmware_class.c request_firmware(const struct firmware **firmware_p, const char *name,

Quoted 7 lines

drivers/base/firmware_class.c +7 -0

+                         struct device *device)
+{
+       int ret;
+
+       /* Need to pin this module until return */
+       __module_get(THIS_MODULE);
+               ret = _request_firmware(firmware_p, name, device,

NULL, 0,

Quoted 7 lines

Quoted 7 lines

drivers/base/firmware_class.c request_firmware(const struct firmware **firmware_p, const char *name, · line 1376

Quoted 5 lines

drivers/base/firmware_class.c +5 -0

+                               FW_OPT_UEVENT | FW_OPT_NO_WARN );
+       module_put(THIS_MODULE);
+       return ret;
+}
+EXPORT_SYMBOL(request_firmware_optional);

New exported symbols for the firmware API should be

EXPORT_SYMBOL_GPL().

Quoted 7 lines

To me the word optional feels weird to me. For example, in ath10k I suspect we would be only calling request_firmware_optional() with all firmware and not request_firmware() at all.

How about request_firmware_nowarn()? That would even match the documentation above.

_nowarn() works with me. Do you at least want the return value to give an error value if no file was found? This way the driver can decide when to issue an error if it wants to.

Luis

Quoted 32 lines · Kalle Valo

Quoted 17 lines · Luis R. Rodriguez

Quoted 17 lines

drivers/base/firmware_class.c +17 -0

+/**
+ * request_firmware_optional: - request for an optional fw module
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ **/
+int
+request_firmware_optional(const struct firmware **firmware_p, const char *name,
+                         struct device *device)
+{
+       int ret;
+
+       /* Need to pin this module until return */
+       __module_get(THIS_MODULE);

Quoted 8 lines

drivers/base/firmware_class.c request_firmware(const struct firmware **firmware_p, const char *name, · line 1376

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+               ret = _request_firmware(firmware_p, name, device, NULL, 0,
+                               FW_OPT_UEVENT | FW_OPT_NO_WARN );
+       module_put(THIS_MODULE);
+       return ret;
+}
+EXPORT_SYMBOL(request_firmware_optional);

New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().

To me the word optional feels weird to me. For example, in ath10k I suspect we would be only calling request_firmware_optional() with all firmware and not request_firmware() at all.

Quoted 2 lines

How about request_firmware_nowarn()? That would even match the documentation above.

_nowarn() works with me. Do you at least want the return value to give an error value if no file was found? This way the driver can decide when to issue an error if it wants to.

Quoted 36 lines · Luis R. Rodriguez

Quoted 34 lines · Kalle Valo

Quoted 17 lines · Luis R. Rodriguez

Quoted 17 lines

drivers/base/firmware_class.c +17 -0

+/**
+ * request_firmware_optional: - request for an optional fw module
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ **/
+int
+request_firmware_optional(const struct firmware **firmware_p, const char *name,
+                         struct device *device)
+{
+       int ret;
+
+       /* Need to pin this module until return */
+       __module_get(THIS_MODULE);

Quoted 8 lines

drivers/base/firmware_class.c request_firmware(const struct firmware **firmware_p, const char *name, · line 1376

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+               ret = _request_firmware(firmware_p, name, device, NULL, 0,
+                               FW_OPT_UEVENT | FW_OPT_NO_WARN );
+       module_put(THIS_MODULE);
+       return ret;
+}
+EXPORT_SYMBOL(request_firmware_optional);

New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().

To me the word optional feels weird to me. For example, in ath10k I suspect we would be only calling request_firmware_optional() with all firmware and not request_firmware() at all.

How about request_firmware_nowarn()? That would even match the documentation above.

Quoted 3 lines

_nowarn() works with me. Do you at least want the return value to give an error value if no file was found? This way the driver can decide when to issue an error if it wants to.

Yes, it would be very good to return the error value to ath10k. That way we can give a proper error message to the user if we can't find a suitable firmware image.

Quoted 42 lines · Kalle Valo

Quoted 39 lines · Luis R. Rodriguez

Quoted 28 lines · Kalle Valo

Quoted 17 lines · Luis R. Rodriguez

Quoted 17 lines

drivers/base/firmware_class.c +17 -0

+/**
+ * request_firmware_optional: - request for an optional fw module
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ **/
+int
+request_firmware_optional(const struct firmware **firmware_p, const char *name,
+                         struct device *device)
+{
+       int ret;
+
+       /* Need to pin this module until return */
+       __module_get(THIS_MODULE);

Quoted 8 lines

drivers/base/firmware_class.c request_firmware(const struct firmware **firmware_p, const char *name, · line 1376

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+               ret = _request_firmware(firmware_p, name, device, NULL, 0,
+                               FW_OPT_UEVENT | FW_OPT_NO_WARN );
+       module_put(THIS_MODULE);
+       return ret;
+}
+EXPORT_SYMBOL(request_firmware_optional);

New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().

Quoted 6 lines

To me the word optional feels weird to me. For example, in ath10k I suspect we would be only calling request_firmware_optional() with all firmware and not request_firmware() at all.

How about request_firmware_nowarn()? That would even match the documentation above.

_nowarn() works with me. Do you at least want the return value to give an error value if no file was found? This way the driver can decide when to issue an error if it wants to.

Quoted 3 lines

Yes, it would be very good to return the error value to ath10k. That way we can give a proper error message to the user if we can't find a suitable firmware image.

I fully agree with the _nowarn() and returning an error. However, the firmware_p parameter (btw. do we really want the _p postfix?)

is an output parameter which will be null in case of an error so do you really need a specific error code for the proper error message.

Currently the firmware loader only exposes one silent path for querying optional firmware, and that is request_firmware_direct(). This function also disables the usermodehelper fallback which might not always be the desired behaviour.

This patch introduces request_firmware_optional(), which will not produce error/warning messages if the firmware file is not found, but will still attempt to query the usermodehelper for the optional firmware. Effectively, FW_OPT_UEVENT | FW_OPT_FALLBACK | FW_OPT_NO_WARN.

v2: add header prototype, use updated opt_flags

Signed-off-by Andres Rodriguez <[email protected]>

---

Sorry, I messed up the v1 patch and sent the wrong one from before I rebased.

  drivers/base/firmware_class.c | 26 +++++++++++++++++++++++++-
  include/linux/firmware.h      |  2 ++
  2 files changed, 27 insertions(+), 1 deletion(-)

drivers/base/firmware_class.c +0 -0

index 7dd36ace6152..4e1eddea241b 100644

drivers/base/firmware_class.c +2 -0

  	return fw_load_from_user_helper(fw, name, device, opt_flags);
  }
  #else /* CONFIG_FW_LOADER_USER_HELPER */

@@ -1351,6 +1351,30 @@ request_firmware(const struct firmware **firmware_p, const char *name,
  }
  EXPORT_SYMBOL(request_firmware);
 
+/**
+ * request_firmware_optional: - request for an optional fw module

Hi Everyone,

Wanted to inquire your opinions about the following matter.

We are experiencing some end user confusion regarding the following messages being printed to dmesg:

[    0.571324] amdgpu 0000:01:00.0: Direct firmware load for amdgpu/polaris10_pfp_2.bin failed with error -2
[    0.571338] amdgpu 0000:01:00.0: Direct firmware load for amdgpu/polaris10_me_2.bin failed with error -2
[    0.571348] amdgpu 0000:01:00.0: Direct firmware load for amdgpu/polaris10_ce_2.bin failed with error -2
[    0.571366] amdgpu 0000:01:00.0: Direct firmware load for amdgpu/polaris10_mec_2.bin failed with error -2
[    0.571404] amdgpu 0000:01:00.0: Direct firmware load for amdgpu/polaris10_mec2_2.bin failed with error -2

These firmware blobs are optional. If they aren't available, the graphics card can still function normally. But having these messages causes the user to think their current problems are related to missing firmware.

It would be great if we could have a mechanism that enabled us to load a firmware blob without any dmesg spew in case of file not found errors.Currently request_firmware_direct() implements this functionality, but as a drawback it forfeits the usermodehelper fallback path.