[RFC 0/1] Loading optional firmware

[RFC 0/1] Loading optional firmware

[Andres Rodriguez]

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.

As part of this series, there is a small patch to enable
request_firmware_optional(), which has the same logging behaviour as
request_firmware_direct(), but will maintain the usermodehelper fallback.

Let me know if you find this change suitable, or if you would prefer a
different strategy to tackle this issue.

Andres Rodriguez (1):
  firmware: add a function to load optional firmware

 drivers/base/firmware_class.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

-- 
2.14.1

[PATCH 1/1] firmware: add a function to load optional firmware

[Andres Rodriguez]

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.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/base/firmware_class.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7dd36ace6152..a5a4dfb90e2f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1181,7 +1181,7 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name,
 	if (!fw_run_sysfs_fallback(opt_flags))
 		return ret;
 
-	dev_warn(device, "Falling back to user helper\n");
+	dev_warn(device, "Falling back to user helper for %s\n", name);
 	return fw_load_from_user_helper(fw, name, device, opt_flags);
 }
 #else /* CONFIG_FW_LOADER_USER_HELPER */
@@ -1351,6 +1351,31 @@ 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)
+{
+	int ret;
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, NULL, 0,
+				FW_OPT_UEVENT | FW_OPT_FALLBACK |
+				FW_OPT_NO_WARN);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL(request_firmware_optional);
+
 /**
  * request_firmware_direct: - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
-- 
2.14.1

[PATCH] firmware: add a function to load optional firmware v2

[Andres Rodriguez]

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 <andresx7@gmail.com>
---

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(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7dd36ace6152..4e1eddea241b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1181,7 +1181,7 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name,
 	if (!fw_run_sysfs_fallback(opt_flags))
 		return ret;
 
-	dev_warn(device, "Falling back to user helper\n");
+	dev_warn(device, "Falling back to user helper for %s\n", name);
 	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)
+{
+	int ret;
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+		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);
+
 /**
  * request_firmware_direct: - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d4508080348d..6f386eeb8efc 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -46,6 +46,8 @@ int request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
+int request_firmware_optional(const struct firmware **fw, const char *name,
+			      struct device *device);
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
-- 
2.14.1

Re: [RFC 0/1] Loading optional firmware

[Luis R. Rodriguez]

On Fri, Mar 9, 2018 at 2:12 PM, Andres Rodriguez <andresx7@gmail.com> wrote:
> 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.

Yeah, this is a common enough reported type of feature request that I
think it makes sense to add now. I'll reply to your patch with a bit
more details.

 Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

First, thanks for your patch!

On Fri, Mar 9, 2018 at 3:09 PM, Andres Rodriguez <andresx7@gmail.com> wrote:
> 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.

You should explain on the commit log how at least there is one driver
which needs full silence. Also FYI I think there are others who have
asked for the same before, on the 802.11 world. Kalle, Arend, do you
guys recall if it was a synchronous or async use case?

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

This looks good for a commit message so far modulo the above.

> will still attempt to query the usermodehelper

The "usermodehelper" is a loose term which I've been trying to fight
in the firmware API. Please refer to this as the fallback mechanism.
The usermode helper actually is kernel/umh.c and in so far as the
firmware API is concerned the kernel/umc.c is used only for the uevent
for the default fallback case. In the custom fallback case there is no
kernel/umc.c API use, and I'm now wondering if use of the UMH lock
doesn't make sense there and we should remove it.

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

Can you do me a favor? The above flags are not well documented at all.
I have a big sized cleanup for v4.17 on my , can you base your patch
on top of my tree, modify these flags to become enums, and in the
process kdocify them as part of your work? Refer to
include/uapi/linux/nl80211.h for a good example of how to properly
kdocify enums.

Please base your patch on top of my tree and branch
20180307-firmware-dev-for-v4.17 [0] and resubmit with the above and
below feedback into consideration. 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.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180307-firmware-dev-for-v4.17

> v2: add header prototype, use updated opt_flags
>
> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
> ---
>
> 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(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 7dd36ace6152..4e1eddea241b 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1181,7 +1181,7 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name,
>         if (!fw_run_sysfs_fallback(opt_flags))
>                 return ret;
>
> -       dev_warn(device, "Falling back to user helper\n");
> +       dev_warn(device, "Falling back to user helper for %s\n", name);

This seems like it should be a separate patch, and it should be
justified, also please modify the language given what I said above
about kernel/umc.c API use. In so far as your actual change is
concerned for this print, *why* do we need another print with the
firmware name on it? The commit log should explain that very well in a
separate patch.

>         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)
> +{
> +       int ret;
> +
> +       /* Need to pin this module until return */
> +       __module_get(THIS_MODULE);
> +               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().

  Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

On Sat, Mar 10, 2018 at 6:35 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> 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.

Also - I forgot to ask you to extend the
Documentation/driver-api/firmware/ documentation accordingly. Please
do that.

  Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Andres Rodriguez]

Hi Luis,

Thanks for all your feedback, greatly appreciated.

On 2018-03-10 09:40 AM, Luis R. Rodriguez wrote:
> On Sat, Mar 10, 2018 at 6:35 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> 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.

The current user I have in mind is amdgpu. I've got some local patches 
for changing it to use request_firmware_optional() for the optional 
firmware files. I will include them in the v3 of this series.

I've also queried some devs from the other DRM drivers in case this 
might be useful to them. So far I've gotten a reply from the nouveau 
devs who are also interested.

> 
> 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.

> 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.
>

Seems like I got lucky with my timing for this request :)


> 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.
> > Also - I forgot to ask you to extend the
> Documentation/driver-api/firmware/ documentation accordingly. Please
> do that.
> 

Will do, for these and the feedback in the previous Email.

-Andres

>    Luis
> 

Re: [PATCH] firmware: add a function to load optional firmware v2

[Arend van Spriel]

On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
>> 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.

Regards,
Arend

Re: [PATCH] firmware: add a function to load optional firmware v2

[Arend van Spriel]

On 3/10/2018 12:09 AM, Andres Rodriguez wrote:
> 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 <andresx7@gmail.com>
> ---
>
> 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(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 7dd36ace6152..4e1eddea241b 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1181,7 +1181,7 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name,
>   	if (!fw_run_sysfs_fallback(opt_flags))
>   		return ret;
>
> -	dev_warn(device, "Falling back to user helper\n");
> +	dev_warn(device, "Falling back to user helper for %s\n", name);

Helpful, but not really related to this change.

>   	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)

So it still returns an error code. If it were truly optional I kinda 
expected a void return type. This is more request_firmware_silent(). 
Anyway, I guess Luis would call this bike-shedding ;-)

Regards,
Arend

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

On Mon, Mar 12, 2018 at 12:10:47AM +0100, Arend van Spriel wrote:
> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
> > > 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.

> 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.

Thanks for the details.

  Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Kalle Valo]

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:

>> +/**
>> + * 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);
>> +               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.

-- 
Kalle Valo

Re: [PATCH] firmware: add a function to load optional firmware v2

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
>>> 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.

Oh yeah, ath10k definitely needs this! It tries different firmware API
versions from latest to older (firmware-6.bin, firmware-5.bin,
firmware-4.bin and so on) to find a compatible firmware and the error
messages from request_firmware() are constantly confusing the users, I
think the latest query about these errors from last week on IRC. So
having request_firmware_nowarn() (or similar) would help users a lot.

We tried to workaround this by using request_firmware_direct() (which
oddly doesn't print anything) but that caused issues with OpenWRT/LEDE:

https://git.kernel.org/linus/c0cc00f250e1

And iwlwifi has a similar problem, adding Luca to the loop.

-- 
Kalle Valo

Re: [PATCH] firmware: add a function to load optional firmware v2

[Kalle Valo]

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:

> On Mon, Mar 12, 2018 at 12:10:47AM +0100, Arend van Spriel wrote:
>> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
>> > > 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.

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

Re: [PATCH] firmware: add a function to load optional firmware v2

[Andres Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

Hi All,

I've been a little busy at the hospital with a kidney stone, sorry for no
updates :(

Just wanted to also chime in to say that I'm also okay with using a
different name. Preference for nowarn over quiet since I don't think we'll
provide a guarantee about other error messages being printed.

I probably won't have time for a respin of the patch until at least next
week.

Regards,
Andres



On Mar 13, 2018 9:39 AM, "Kalle Valo" <kvalo@codeaurora.org> wrote:

> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>
> > On Mon, Mar 12, 2018 at 12:10:47AM +0100, Arend van Spriel wrote:
> >> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
> >> > > 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.
>
> 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
>

[-- Attachment #2: Type: text/html, Size: 2875 bytes --]

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

On Tue, Mar 13, 2018 at 03:39:23PM +0200, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> 
> > On Mon, Mar 12, 2018 at 12:10:47AM +0100, Arend van Spriel wrote:
> >> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
> >> > > 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.
> 
> 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.

> 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

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

On Tue, Mar 13, 2018 at 03:16:34PM +0200, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> 
> >> +/**
> >> + * 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);
> >> +               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.

_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

Re: [PATCH] firmware: add a function to load optional firmware v2

[Andres Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

Hi Luis,

On our side we would definitely want a return value to trigger a fallback
mechanism.

Also sorry for the multiple HTML emails which are not hitting the list.
Only have my phone.

Regards,.
Andres

On Mar 13, 2018 12:40 PM, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:

> On Tue, Mar 13, 2018 at 03:16:34PM +0200, Kalle Valo wrote:
> > "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> >
> > >> +/**
> > >> + * 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);
> > >> +               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.
>
> _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
>

[-- Attachment #2: Type: text/html, Size: 2908 bytes --]

Re: [PATCH] firmware: add a function to load optional firmware v2

[Kalle Valo]

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:

> On Tue, Mar 13, 2018 at 03:16:34PM +0200, Kalle Valo wrote:
>> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>> 
>> >> +/**
>> >> + * 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);
>> >> +               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.
>
> _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.

-- 
Kalle Valo

Re: [PATCH] firmware: add a function to load optional firmware v2

[Arend van Spriel]

On 3/13/2018 5:46 PM, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>
>> On Tue, Mar 13, 2018 at 03:16:34PM +0200, Kalle Valo wrote:
>>> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>>>
>>>>> +/**
>>>>> + * 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);
>>>>> +               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.
>>
>> _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.

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.

Regards,
Arend

Re: [PATCH] firmware: add a function to load optional firmware v2

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 3/13/2018 5:46 PM, Kalle Valo wrote:
>> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>>
>>> On Tue, Mar 13, 2018 at 03:16:34PM +0200, Kalle Valo wrote:
>>>> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>>>>
>>>>>> +/**
>>>>>> + * 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);
>>>>>> +               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.
>>>
>>> _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.
>
> I fully agree with the _nowarn() and returning an error. However, the
> firmware_p parameter (btw. do we really want the _p postfix?)

Oh yeah, that _p is ugly. Please get rid of it, hungarian notation is
awful.

> 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.

Sometimes the error code helps with debugging. But let's ask it this
way: why would we NOT return an error code? What would we gain from
that? I don't see any advantage from dropping the error code, on the
contrary better to be consistent with request_firmware() to avoid any
confusion.

-- 
Kalle Valo

Re: [PATCH] firmware: add a function to load optional firmware v2

[Andres Rodriguez]

On 2018-03-13 12:38 PM, Luis R. Rodriguez wrote:
> On Tue, Mar 13, 2018 at 03:39:23PM +0200, Kalle Valo wrote:
>> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>>
>>> On Mon, Mar 12, 2018 at 12:10:47AM +0100, Arend van Spriel wrote:
>>>> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
>>>>>> 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.
>>
>> 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.
> 
>> 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?
> 

Can do, although it will take a while. Kidney stone woes and other life 
things are keeping me busy for the following weeks.

Sorry for the delay.

Kind Regards,
Andres

>    Luis
>