Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581721 > unrolled thread

[PATCH 1/2] firmware: add more flexible request_firmware_async function

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki <zajec5@gmail.com> - 2017-02-15 23:40 +0100
    [PATCH 2/2] brcmfmac: don't warn user if loading firmware file with NVRAM fails Rafał Miłecki <zajec5@gmail.com> - 2017-02-15 23:40 +0100
      Re: [PATCH 2/2] brcmfmac: don't warn user if loading firmware file  with NVRAM fails Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-16 02:20 +0100
        Re: [PATCH 2/2] brcmfmac: don't warn user if loading firmware file  with NVRAM fails Rafał Miłecki <rafal@milecki.pl> - 2017-02-16 08:50 +0100
    [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds Rafał Miłecki <zajec5@gmail.com> - 2017-02-16 08:30 +0100
      Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback  to platform one succeeds Arend Van Spriel <arend.vanspriel@broadcom.com> - 2017-02-16 09:40 +0100
        Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback  to platform one succeeds Arend Van Spriel <arend.vanspriel@broadcom.com> - 2017-02-16 10:20 +0100
          Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback  to platform one succeeds Rafał Miłecki <rafal@milecki.pl> - 2017-02-16 11:00 +0100
            Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback  to platform one succeeds Arend Van Spriel <arend.vanspriel@broadcom.com> - 2017-02-16 11:20 +0100
        Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback  to platform one succeeds Rafał Miłecki <rafal@milecki.pl> - 2017-02-16 11:30 +0100
    [PATCH V2 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki <zajec5@gmail.com> - 2017-02-16 08:30 +0100
      [PATCH V3 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki <zajec5@gmail.com> - 2017-02-21 10:50 +0100
        [PATCH V3 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds Rafał Miłecki <zajec5@gmail.com> - 2017-02-21 10:50 +0100
          Re: [PATCH V3 2/2] brcmfmac: don't warn user about NVRAM if fallback  to platform one succeeds Arend Van Spriel <arend.vanspriel@broadcom.com> - 2017-02-21 11:00 +0100
            Re: [PATCH V3 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds Kalle Valo <kvalo@codeaurora.org> - 2017-02-21 12:50 +0100
        Re: [PATCH V3 1/2] firmware: add more flexible  request_firmware_async function "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-21 19:10 +0100
        [PATCH V4 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds Rafał Miłecki <zajec5@gmail.com> - 2017-02-23 19:40 +0100
        [PATCH V4 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki <zajec5@gmail.com> - 2017-02-23 19:40 +0100
      Re: [PATCH V2 1/2] firmware: add more flexible  request_firmware_async function "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-21 19:10 +0100
    Re: [PATCH 1/2] firmware: add more flexible request_firmware_async  function "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-21 19:10 +0100

#1581721 — [PATCH 1/2] firmware: add more flexible request_firmware_async function

From: Rafał Miłecki <rafal@milecki.pl>

So far we got only one function for loading firmware asynchronously:
request_firmware_nowait. It didn't allow much customization of firmware
loading process - there is only one bool uevent argument. Moreover this
bool also controls user helper in an unclear way.

Resolve this problem by adding a one flexible function and making old
request_firmware_nowait a simple inline using new solution. This
implementation:
1) Modifies only single bits on existing code
2) Doesn't require adjusting / rewriting current drivers
3) Adds new function for drivers that need more control over loading a
   firmware. Thanks to using flags more features can be added later.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patch is based on top of
[PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
applied on top of Linux 4.10-rc8.

Ming/Luis/Greg: assuming this gets a positive review, could someone of you pick
this patchset? Second patch modifies brcmfmac, I'll try to get a proper Ack for
that one.
Unless you want this to go through wireless tree, then let me know please.
---
 drivers/base/firmware_class.c | 25 ++++++-------------------
 include/linux/firmware.h      | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d05be1732c8b..7b3f0a018dc3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -182,18 +182,6 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT	(1U << 0)
-#define FW_OPT_NOWAIT	(1U << 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_USERHELPER	(1U << 2)
-#else
-#define FW_OPT_USERHELPER	0
-#endif
-#define FW_OPT_NO_WARN	(1U << 3)
-#define FW_OPT_NOCACHE	(1U << 4)
-#define FW_OPT_FALLBACK	(1U << 5)
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -1356,7 +1344,7 @@ static void request_firmware_work_func(struct work_struct *work)
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device); /* taken in request_firmware_nowait() */
+	put_device(fw_work->device); /* taken in request_firmware_async() */
 
 	module_put(fw_work->module);
 	kfree_const(fw_work->name);
@@ -1364,7 +1352,7 @@ static void request_firmware_work_func(struct work_struct *work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_async - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  *	is non-zero else the firmware copy must be done manually.
@@ -1387,8 +1375,8 @@ static void request_firmware_work_func(struct work_struct *work)
  *		- can't sleep at all if @gfp is GFP_ATOMIC.
  **/
 int
-request_firmware_nowait(
-	struct module *module, bool uevent,
+request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -1407,8 +1395,7 @@ request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+	fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
@@ -1421,7 +1408,7 @@ request_firmware_nowait(
 	schedule_work(&fw_work->work);
 	return 0;
 }
-EXPORT_SYMBOL(request_firmware_nowait);
+EXPORT_SYMBOL(request_firmware_async);
 
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..1f2bf14aa441 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -8,6 +8,18 @@
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+/* firmware behavior options */
+#define FW_OPT_UEVENT	(1U << 0)
+#define FW_OPT_NOWAIT	(1U << 1)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_USERHELPER	(1U << 2)
+#else
+#define FW_OPT_USERHELPER	0
+#endif
+#define FW_OPT_NO_WARN	(1U << 3)
+#define FW_OPT_NOCACHE	(1U << 4)
+#define FW_OPT_FALLBACK	(1U << 5)
+
 struct firmware {
 	size_t size;
 	const u8 *data;
@@ -41,8 +53,8 @@ struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
-int request_firmware_nowait(
-	struct module *module, bool uevent,
+int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
@@ -58,8 +70,8 @@ static inline int request_firmware(const struct firmware **fw,
 {
 	return -EINVAL;
 }
-static inline int request_firmware_nowait(
-	struct module *module, bool uevent,
+static inline int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -82,6 +94,18 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 {
 	return -EINVAL;
 }
-
 #endif
+
+static inline 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))
+{
+	unsigned int opt_flags = FW_OPT_FALLBACK |
+		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+
+	return request_firmware_async(module, opt_flags, name, device, gfp,
+				      context, cont);
+}
+
 #endif
-- 
2.11.0

[toc] | [next] | [standalone]

#1581725 — [PATCH 2/2] brcmfmac: don't warn user if loading firmware file with NVRAM fails

From: Rafał Miłecki <rafal@milecki.pl>

This isn't critical as we use platform NVRAM as fallback and it's very
common case of all Broadcom home routers. Thanks for the new firmware
loading function we can achieve this by simply passing FW_OPT_NO_WARN.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
Kalle, Arend: this patch is strictly related to the bigger 1/2. Could you ack
this change as I expect this patchset to be picked by Ming, Luis or Greg.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index c7c1e9906500..26ec445a8d2d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -504,9 +504,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 		return;
 	}
 	fwctx->code = fw;
-	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
-				      fwctx->dev, GFP_KERNEL, fwctx,
-				      brcmf_fw_request_nvram_done);
+	ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
+				     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
+				     fwctx, brcmf_fw_request_nvram_done);
 
 	if (!ret)
 		return;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1582211 — Re: [PATCH 2/2] brcmfmac: don't warn user if loading firmware file with NVRAM fails

On Thu, Feb 16, 2017 at 12:29 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This isn't critical as we use platform NVRAM as fallback and it's very
> common case of all Broadcom home routers. Thanks for the new firmware
> loading function we can achieve this by simply passing FW_OPT_NO_WARN.
>

What kind of warning you are talking about?

On Intel Edison brcmfmac isn't backed up by NVRAM (there no such) and
the platform is not ACPI-based. The warning might be crucial for
debugging Wi-Fi issues on such platforms.

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> Kalle, Arend: this patch is strictly related to the bigger 1/2. Could you ack
> this change as I expect this patchset to be picked by Ming, Luis or Greg.
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index c7c1e9906500..26ec445a8d2d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -504,9 +504,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>                 return;
>         }
>         fwctx->code = fw;
> -       ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
> -                                     fwctx->dev, GFP_KERNEL, fwctx,
> -                                     brcmf_fw_request_nvram_done);
> +       ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
> +                                    fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
> +                                    fwctx, brcmf_fw_request_nvram_done);
>
>         if (!ret)
>                 return;
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1582369 — Re: [PATCH 2/2] brcmfmac: don't warn user if loading firmware file with NVRAM fails

On 2017-02-16 02:19, Andy Shevchenko wrote:
> On Thu, Feb 16, 2017 at 12:29 AM, Rafał Miłecki <zajec5@gmail.com> 
> wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> This isn't critical as we use platform NVRAM as fallback and it's very
>> common case of all Broadcom home routers. Thanks for the new firmware
>> loading function we can achieve this by simply passing FW_OPT_NO_WARN.
>> 
> 
> What kind of warning you are talking about?
> 
> On Intel Edison brcmfmac isn't backed up by NVRAM (there no such) and
> the platform is not ACPI-based. The warning might be crucial for
> debugging Wi-Fi issues on such platforms.

Sorry, I should have tried better with that commit message. V2 in a 
second.

[toc] | [prev] | [next] | [standalone]

#1582365 — [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

From: Rafał Miłecki <rafal@milecki.pl>

Failing to load NVRAM file isn't critical if we manage to get platform
one in the fallback path. It means warnings like:
[   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
are unnecessary & disturbing for people with platform NVRAM. This is
very common case for Broadcom home routers.

So instead of printing warning immediately with the firmware subsystem
let's first try our fallback code. If that fails as well, then it's a
right moment to print an error.

This should reduce amount of false reports from users seeing this
warning while having wireless working perfectly fine.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
    messages to the firmware.c.

Kalle, Arend: this patch is strictly related to the bigger 1/2. Could you ack
this change as I expect this patchset to be picked by Ming, Luis or Greg?
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c  | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index c7c1e9906500..510a76d99eee 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 		raw_nvram = false;
 	} else {
 		data = bcm47xx_nvram_get_contents(&data_len);
-		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
-			goto fail;
+		if (!data) {
+			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
+			if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
+				brcmf_err("Loading NVRAM from %s and using platform one both failed\n",
+					  fwctx->nvram_name);
+				goto fail;
+			}
+		}
 		raw_nvram = true;
 	}
 
@@ -504,9 +510,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 		return;
 	}
 	fwctx->code = fw;
-	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
-				      fwctx->dev, GFP_KERNEL, fwctx,
-				      brcmf_fw_request_nvram_done);
+	ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
+				     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
+				     fwctx, brcmf_fw_request_nvram_done);
 
 	if (!ret)
 		return;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1582383 — Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

On 16-2-2017 8:26, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Failing to load NVRAM file isn't critical if we manage to get platform
> one in the fallback path. It means warnings like:
> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
> are unnecessary & disturbing for people with platform NVRAM. This is
> very common case for Broadcom home routers.
> 
> So instead of printing warning immediately with the firmware subsystem
> let's first try our fallback code. If that fails as well, then it's a
> right moment to print an error.
> 
> This should reduce amount of false reports from users seeing this
> warning while having wireless working perfectly fine.

There are of course people with issues who take this warning as a straw
to clutch.

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
>     messages to the firmware.c.
> 
> Kalle, Arend: this patch is strictly related to the bigger 1/2. Could you ack
> this change as I expect this patchset to be picked by Ming, Luis or Greg?
> ---
>  .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c  | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index c7c1e9906500..510a76d99eee 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>  		raw_nvram = false;
>  	} else {
>  		data = bcm47xx_nvram_get_contents(&data_len);
> -		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> -			goto fail;
> +		if (!data) {
> +			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
> +			if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
> +				brcmf_err("Loading NVRAM from %s and using platform one both failed\n",
> +					  fwctx->nvram_name);
> +				goto fail;
> +			}
> +		}
>  		raw_nvram = true;
>  	}
>  
> @@ -504,9 +510,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>  		return;
>  	}
>  	fwctx->code = fw;
> -	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
> -				      fwctx->dev, GFP_KERNEL, fwctx,
> -				      brcmf_fw_request_nvram_done);
> +	ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
> +				     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
> +				     fwctx, brcmf_fw_request_nvram_done);

You changed the behaviour, because of your change in patch 1/2:

-	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+	fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;

So: (FW_OPT_NOWAIT | FW_OPT_UEVENT) vs (FW_OPT_NOWAIT | FW_OPT_NO_WARN)

Regards,
Arend

[toc] | [prev] | [next] | [standalone]

#1582404 — Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

On 16-2-2017 10:04, Rafał Miłecki wrote:
> On 2017-02-16 09:38, Arend Van Spriel wrote:
>> On 16-2-2017 8:26, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Failing to load NVRAM file isn't critical if we manage to get platform
>>> one in the fallback path. It means warnings like:
>>> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for
>>> brcm/brcmfmac43602-pcie.txt failed with error -2
>>> are unnecessary & disturbing for people with platform NVRAM. This is
>>> very common case for Broadcom home routers.
>>>
>>> So instead of printing warning immediately with the firmware subsystem
>>> let's first try our fallback code. If that fails as well, then it's a
>>> right moment to print an error.
>>>
>>> This should reduce amount of false reports from users seeing this
>>> warning while having wireless working perfectly fine.
>>
>> There are of course people with issues who take this warning as a straw
>> to clutch.
>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>> V2: Update commit message as it wasn't clear enough (thanks Andy) &
>>> add extra
>>>     messages to the firmware.c.
>>>
>>> Kalle, Arend: this patch is strictly related to the bigger 1/2. Could
>>> you ack
>>> this change as I expect this patchset to be picked by Ming, Luis or
>>> Greg?
>>> ---
>>>  .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c  | 16
>>> +++++++++++-----
>>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git
>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> index c7c1e9906500..510a76d99eee 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const
>>> struct firmware *fw, void *ctx)
>>>          raw_nvram = false;
>>>      } else {
>>>          data = bcm47xx_nvram_get_contents(&data_len);
>>> -        if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>> -            goto fail;
>>> +        if (!data) {
>>> +            brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
>>> +            if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
>>> +                brcmf_err("Loading NVRAM from %s and using platform
>>> one both failed\n",
>>> +                      fwctx->nvram_name);
>>> +                goto fail;
>>> +            }
>>> +        }
>>>          raw_nvram = true;
>>>      }
>>>
>>> @@ -504,9 +510,9 @@ static void brcmf_fw_request_code_done(const
>>> struct firmware *fw, void *ctx)
>>>          return;
>>>      }
>>>      fwctx->code = fw;
>>> -    ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
>>> -                      fwctx->dev, GFP_KERNEL, fwctx,
>>> -                      brcmf_fw_request_nvram_done);
>>> +    ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
>>> +                     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
>>> +                     fwctx, brcmf_fw_request_nvram_done);
>>
>> You changed the behaviour, because of your change in patch 1/2:
>>
>> -    fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
>> -        (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
>> +    fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
>>
>> So: (FW_OPT_NOWAIT | FW_OPT_UEVENT) vs (FW_OPT_NOWAIT | FW_OPT_NO_WARN)
> 
> Sorry, I didn't realize brcmfmac needs FW_OPT_UEVENT. I'll re-add it in
> V3, just
> let me wait to see if there will be more comments.

To be honest whether or not FW_OPT_UEVENT is needed should not be
something a driver needs to concern about. It is really a system
configuration issue if you ask me. So the only thing we could do is to
have it just in case.

Regards,
Arend

[toc] | [prev] | [next] | [standalone]

#1582422 — Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

On 2017-02-16 10:18, Arend Van Spriel wrote:
> On 16-2-2017 10:04, Rafał Miłecki wrote:
>> On 2017-02-16 09:38, Arend Van Spriel wrote:
>>> On 16-2-2017 8:26, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>> 
>>>> Failing to load NVRAM file isn't critical if we manage to get 
>>>> platform
>>>> one in the fallback path. It means warnings like:
>>>> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for
>>>> brcm/brcmfmac43602-pcie.txt failed with error -2
>>>> are unnecessary & disturbing for people with platform NVRAM. This is
>>>> very common case for Broadcom home routers.
>>>> 
>>>> So instead of printing warning immediately with the firmware 
>>>> subsystem
>>>> let's first try our fallback code. If that fails as well, then it's 
>>>> a
>>>> right moment to print an error.
>>>> 
>>>> This should reduce amount of false reports from users seeing this
>>>> warning while having wireless working perfectly fine.
>>> 
>>> There are of course people with issues who take this warning as a 
>>> straw
>>> to clutch.
>>> 
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> ---
>>>> V2: Update commit message as it wasn't clear enough (thanks Andy) &
>>>> add extra
>>>>     messages to the firmware.c.
>>>> 
>>>> Kalle, Arend: this patch is strictly related to the bigger 1/2. 
>>>> Could
>>>> you ack
>>>> this change as I expect this patchset to be picked by Ming, Luis or
>>>> Greg?
>>>> ---
>>>>  .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c  | 16
>>>> +++++++++++-----
>>>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git
>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>> index c7c1e9906500..510a76d99eee 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const
>>>> struct firmware *fw, void *ctx)
>>>>          raw_nvram = false;
>>>>      } else {
>>>>          data = bcm47xx_nvram_get_contents(&data_len);
>>>> -        if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>>> -            goto fail;
>>>> +        if (!data) {
>>>> +            brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
>>>> +            if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
>>>> +                brcmf_err("Loading NVRAM from %s and using platform
>>>> one both failed\n",
>>>> +                      fwctx->nvram_name);
>>>> +                goto fail;
>>>> +            }
>>>> +        }
>>>>          raw_nvram = true;
>>>>      }
>>>> 
>>>> @@ -504,9 +510,9 @@ static void brcmf_fw_request_code_done(const
>>>> struct firmware *fw, void *ctx)
>>>>          return;
>>>>      }
>>>>      fwctx->code = fw;
>>>> -    ret = request_firmware_nowait(THIS_MODULE, true, 
>>>> fwctx->nvram_name,
>>>> -                      fwctx->dev, GFP_KERNEL, fwctx,
>>>> -                      brcmf_fw_request_nvram_done);
>>>> +    ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
>>>> +                     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
>>>> +                     fwctx, brcmf_fw_request_nvram_done);
>>> 
>>> You changed the behaviour, because of your change in patch 1/2:
>>> 
>>> -    fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
>>> -        (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
>>> +    fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
>>> 
>>> So: (FW_OPT_NOWAIT | FW_OPT_UEVENT) vs (FW_OPT_NOWAIT | 
>>> FW_OPT_NO_WARN)
>> 
>> Sorry, I didn't realize brcmfmac needs FW_OPT_UEVENT. I'll re-add it 
>> in
>> V3, just
>> let me wait to see if there will be more comments.
> 
> To be honest whether or not FW_OPT_UEVENT is needed should not be
> something a driver needs to concern about. It is really a system
> configuration issue if you ask me. So the only thing we could do is to
> have it just in case.

Drivers always got a choice (see bool uevent) so I didn't want to change 
it.

[toc] | [prev] | [next] | [standalone]

#1582433 — Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

On 16-2-2017 10:32, Rafał Miłecki wrote:
> On 2017-02-16 10:18, Arend Van Spriel wrote:
>> On 16-2-2017 10:04, Rafał Miłecki wrote:
>>> On 2017-02-16 09:38, Arend Van Spriel wrote:
>>>> On 16-2-2017 8:26, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> Failing to load NVRAM file isn't critical if we manage to get platform
>>>>> one in the fallback path. It means warnings like:
>>>>> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for
>>>>> brcm/brcmfmac43602-pcie.txt failed with error -2
>>>>> are unnecessary & disturbing for people with platform NVRAM. This is
>>>>> very common case for Broadcom home routers.
>>>>>
>>>>> So instead of printing warning immediately with the firmware subsystem
>>>>> let's first try our fallback code. If that fails as well, then it's a
>>>>> right moment to print an error.
>>>>>
>>>>> This should reduce amount of false reports from users seeing this
>>>>> warning while having wireless working perfectly fine.
>>>>
>>>> There are of course people with issues who take this warning as a straw
>>>> to clutch.
>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>> ---
>>>>> V2: Update commit message as it wasn't clear enough (thanks Andy) &
>>>>> add extra
>>>>>     messages to the firmware.c.
>>>>>
>>>>> Kalle, Arend: this patch is strictly related to the bigger 1/2. Could
>>>>> you ack
>>>>> this change as I expect this patchset to be picked by Ming, Luis or
>>>>> Greg?
>>>>> ---
>>>>>  .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c  | 16
>>>>> +++++++++++-----
>>>>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>>> index c7c1e9906500..510a76d99eee 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>>>> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const
>>>>> struct firmware *fw, void *ctx)
>>>>>          raw_nvram = false;
>>>>>      } else {
>>>>>          data = bcm47xx_nvram_get_contents(&data_len);
>>>>> -        if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>>>> -            goto fail;
>>>>> +        if (!data) {
>>>>> +            brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
>>>>> +            if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
>>>>> +                brcmf_err("Loading NVRAM from %s and using platform
>>>>> one both failed\n",
>>>>> +                      fwctx->nvram_name);
>>>>> +                goto fail;
>>>>> +            }
>>>>> +        }
>>>>>          raw_nvram = true;
>>>>>      }
>>>>>
>>>>> @@ -504,9 +510,9 @@ static void brcmf_fw_request_code_done(const
>>>>> struct firmware *fw, void *ctx)
>>>>>          return;
>>>>>      }
>>>>>      fwctx->code = fw;
>>>>> -    ret = request_firmware_nowait(THIS_MODULE, true,
>>>>> fwctx->nvram_name,
>>>>> -                      fwctx->dev, GFP_KERNEL, fwctx,
>>>>> -                      brcmf_fw_request_nvram_done);
>>>>> +    ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
>>>>> +                     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
>>>>> +                     fwctx, brcmf_fw_request_nvram_done);
>>>>
>>>> You changed the behaviour, because of your change in patch 1/2:
>>>>
>>>> -    fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
>>>> -        (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
>>>> +    fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
>>>>
>>>> So: (FW_OPT_NOWAIT | FW_OPT_UEVENT) vs (FW_OPT_NOWAIT | FW_OPT_NO_WARN)
>>>
>>> Sorry, I didn't realize brcmfmac needs FW_OPT_UEVENT. I'll re-add it in
>>> V3, just
>>> let me wait to see if there will be more comments.
>>
>> To be honest whether or not FW_OPT_UEVENT is needed should not be
>> something a driver needs to concern about. It is really a system
>> configuration issue if you ask me. So the only thing we could do is to
>> have it just in case.
> 
> Drivers always got a choice (see bool uevent) so I didn't want to change
> it.

Sure, I know. I just wanted to vent an opinion for the firmware_class
maintainers.

Regards,
Arend

[toc] | [prev] | [next] | [standalone]

#1582440 — Re: [PATCH V2 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

On 2017-02-16 09:38, Arend Van Spriel wrote:
> On 16-2-2017 8:26, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> Failing to load NVRAM file isn't critical if we manage to get platform
>> one in the fallback path. It means warnings like:
>> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for 
>> brcm/brcmfmac43602-pcie.txt failed with error -2
>> are unnecessary & disturbing for people with platform NVRAM. This is
>> very common case for Broadcom home routers.
>> 
>> So instead of printing warning immediately with the firmware subsystem
>> let's first try our fallback code. If that fails as well, then it's a
>> right moment to print an error.
>> 
>> This should reduce amount of false reports from users seeing this
>> warning while having wireless working perfectly fine.
> 
> There are of course people with issues who take this warning as a straw
> to clutch.
> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> V2: Update commit message as it wasn't clear enough (thanks Andy) & 
>> add extra
>>     messages to the firmware.c.
>> 
>> Kalle, Arend: this patch is strictly related to the bigger 1/2. Could 
>> you ack
>> this change as I expect this patchset to be picked by Ming, Luis or 
>> Greg?
>> ---
>>  .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c  | 16 
>> +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>> 
>> diff --git 
>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> index c7c1e9906500..510a76d99eee 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const 
>> struct firmware *fw, void *ctx)
>>  		raw_nvram = false;
>>  	} else {
>>  		data = bcm47xx_nvram_get_contents(&data_len);
>> -		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>> -			goto fail;
>> +		if (!data) {
>> +			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
>> +			if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
>> +				brcmf_err("Loading NVRAM from %s and using platform one both 
>> failed\n",
>> +					  fwctx->nvram_name);
>> +				goto fail;
>> +			}
>> +		}
>>  		raw_nvram = true;
>>  	}
>> 
>> @@ -504,9 +510,9 @@ static void brcmf_fw_request_code_done(const 
>> struct firmware *fw, void *ctx)
>>  		return;
>>  	}
>>  	fwctx->code = fw;
>> -	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
>> -				      fwctx->dev, GFP_KERNEL, fwctx,
>> -				      brcmf_fw_request_nvram_done);
>> +	ret = request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
>> +				     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
>> +				     fwctx, brcmf_fw_request_nvram_done);
> 
> You changed the behaviour, because of your change in patch 1/2:
> 
> -	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
> -		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> +	fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
> 
> So: (FW_OPT_NOWAIT | FW_OPT_UEVENT) vs (FW_OPT_NOWAIT | FW_OPT_NO_WARN)

Sorry, I didn't realize brcmfmac needs FW_OPT_UEVENT. I'll re-add it in 
V3, just
let me wait to see if there will be more comments.

[toc] | [prev] | [next] | [standalone]

#1582366 — [PATCH V2 1/2] firmware: add more flexible request_firmware_async function

From: Rafał Miłecki <rafal@milecki.pl>

So far we got only one function for loading firmware asynchronously:
request_firmware_nowait. It didn't allow much customization of firmware
loading process - there is only one bool uevent argument. Moreover this
bool also controls user helper in an unclear way.

Resolve this problem by adding a one flexible function and making old
request_firmware_nowait a simple inline using new solution. This
implementation:
1) Modifies only single bits on existing code
2) Doesn't require adjusting / rewriting current drivers
3) Adds new function for drivers that need more control over loading a
   firmware. Thanks to using flags more features can be added later.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patch is based on top of
[PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
applied on top of Linux 4.10-rc8.

Ming/Luis/Greg: assuming this gets a positive review, could someone of you pick
this patchset? Second patch modifies brcmfmac, I'll try to get a proper Ack for
that one.
Unless you want this to go through wireless tree, then let me know please.
---
 drivers/base/firmware_class.c | 25 ++++++-------------------
 include/linux/firmware.h      | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d05be1732c8b..7b3f0a018dc3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -182,18 +182,6 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT	(1U << 0)
-#define FW_OPT_NOWAIT	(1U << 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_USERHELPER	(1U << 2)
-#else
-#define FW_OPT_USERHELPER	0
-#endif
-#define FW_OPT_NO_WARN	(1U << 3)
-#define FW_OPT_NOCACHE	(1U << 4)
-#define FW_OPT_FALLBACK	(1U << 5)
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -1356,7 +1344,7 @@ static void request_firmware_work_func(struct work_struct *work)
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device); /* taken in request_firmware_nowait() */
+	put_device(fw_work->device); /* taken in request_firmware_async() */
 
 	module_put(fw_work->module);
 	kfree_const(fw_work->name);
@@ -1364,7 +1352,7 @@ static void request_firmware_work_func(struct work_struct *work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_async - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  *	is non-zero else the firmware copy must be done manually.
@@ -1387,8 +1375,8 @@ static void request_firmware_work_func(struct work_struct *work)
  *		- can't sleep at all if @gfp is GFP_ATOMIC.
  **/
 int
-request_firmware_nowait(
-	struct module *module, bool uevent,
+request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -1407,8 +1395,7 @@ request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+	fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
@@ -1421,7 +1408,7 @@ request_firmware_nowait(
 	schedule_work(&fw_work->work);
 	return 0;
 }
-EXPORT_SYMBOL(request_firmware_nowait);
+EXPORT_SYMBOL(request_firmware_async);
 
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..1f2bf14aa441 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -8,6 +8,18 @@
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+/* firmware behavior options */
+#define FW_OPT_UEVENT	(1U << 0)
+#define FW_OPT_NOWAIT	(1U << 1)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_USERHELPER	(1U << 2)
+#else
+#define FW_OPT_USERHELPER	0
+#endif
+#define FW_OPT_NO_WARN	(1U << 3)
+#define FW_OPT_NOCACHE	(1U << 4)
+#define FW_OPT_FALLBACK	(1U << 5)
+
 struct firmware {
 	size_t size;
 	const u8 *data;
@@ -41,8 +53,8 @@ struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
-int request_firmware_nowait(
-	struct module *module, bool uevent,
+int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
@@ -58,8 +70,8 @@ static inline int request_firmware(const struct firmware **fw,
 {
 	return -EINVAL;
 }
-static inline int request_firmware_nowait(
-	struct module *module, bool uevent,
+static inline int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -82,6 +94,18 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 {
 	return -EINVAL;
 }
-
 #endif
+
+static inline 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))
+{
+	unsigned int opt_flags = FW_OPT_FALLBACK |
+		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+
+	return request_firmware_async(module, opt_flags, name, device, gfp,
+				      context, cont);
+}
+
 #endif
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1585167 — [PATCH V3 1/2] firmware: add more flexible request_firmware_async function

From: Rafał Miłecki <rafal@milecki.pl>

So far we got only one function for loading firmware asynchronously:
request_firmware_nowait. It didn't allow much customization of firmware
loading process - there is only one bool uevent argument. Moreover this
bool also controls user helper in an unclear way.

Resolve this problem by adding a one flexible function and making old
request_firmware_nowait a simple inline using new solution. This
implementation:
1) Modifies only single bits on existing code
2) Doesn't require adjusting / rewriting current drivers
3) Adds new function for drivers that need more control over loading a
   firmware. Thanks to using flags more features can be added later.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patch is based on top of
[PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
applied on top of Linux 4.10-rc8.

Ming/Luis/Greg: assuming this gets a positive review, could someone of you pick
this patchset? Second patch modifies brcmfmac, I'll try to get a proper Ack for
that one.
Unless you want this to go through wireless tree, then let me know please.
---
 drivers/base/firmware_class.c | 25 ++++++-------------------
 include/linux/firmware.h      | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d05be1732c8b..7b3f0a018dc3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -182,18 +182,6 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT	(1U << 0)
-#define FW_OPT_NOWAIT	(1U << 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_USERHELPER	(1U << 2)
-#else
-#define FW_OPT_USERHELPER	0
-#endif
-#define FW_OPT_NO_WARN	(1U << 3)
-#define FW_OPT_NOCACHE	(1U << 4)
-#define FW_OPT_FALLBACK	(1U << 5)
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -1356,7 +1344,7 @@ static void request_firmware_work_func(struct work_struct *work)
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device); /* taken in request_firmware_nowait() */
+	put_device(fw_work->device); /* taken in request_firmware_async() */
 
 	module_put(fw_work->module);
 	kfree_const(fw_work->name);
@@ -1364,7 +1352,7 @@ static void request_firmware_work_func(struct work_struct *work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_async - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  *	is non-zero else the firmware copy must be done manually.
@@ -1387,8 +1375,8 @@ static void request_firmware_work_func(struct work_struct *work)
  *		- can't sleep at all if @gfp is GFP_ATOMIC.
  **/
 int
-request_firmware_nowait(
-	struct module *module, bool uevent,
+request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -1407,8 +1395,7 @@ request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+	fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
@@ -1421,7 +1408,7 @@ request_firmware_nowait(
 	schedule_work(&fw_work->work);
 	return 0;
 }
-EXPORT_SYMBOL(request_firmware_nowait);
+EXPORT_SYMBOL(request_firmware_async);
 
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..1f2bf14aa441 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -8,6 +8,18 @@
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+/* firmware behavior options */
+#define FW_OPT_UEVENT	(1U << 0)
+#define FW_OPT_NOWAIT	(1U << 1)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_USERHELPER	(1U << 2)
+#else
+#define FW_OPT_USERHELPER	0
+#endif
+#define FW_OPT_NO_WARN	(1U << 3)
+#define FW_OPT_NOCACHE	(1U << 4)
+#define FW_OPT_FALLBACK	(1U << 5)
+
 struct firmware {
 	size_t size;
 	const u8 *data;
@@ -41,8 +53,8 @@ struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
-int request_firmware_nowait(
-	struct module *module, bool uevent,
+int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
@@ -58,8 +70,8 @@ static inline int request_firmware(const struct firmware **fw,
 {
 	return -EINVAL;
 }
-static inline int request_firmware_nowait(
-	struct module *module, bool uevent,
+static inline int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -82,6 +94,18 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 {
 	return -EINVAL;
 }
-
 #endif
+
+static inline 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))
+{
+	unsigned int opt_flags = FW_OPT_FALLBACK |
+		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+
+	return request_firmware_async(module, opt_flags, name, device, gfp,
+				      context, cont);
+}
+
 #endif
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1585172 — [PATCH V3 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

From: Rafał Miłecki <rafal@milecki.pl>

Failing to load NVRAM file isn't critical if we manage to get platform
one in the fallback path. It means warnings like:
[   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
are unnecessary & disturbing for people with platform NVRAM. This is
very common case for Broadcom home routers.

So instead of printing warning immediately with the firmware subsystem
let's first try our fallback code. If that fails as well, then it's a
right moment to print an error.

This should reduce amount of false reports from users seeing this
warning while having wireless working perfectly fine.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
    messages to the firmware.c.
V3: Set FW_OPT_UEVENT to don't change behavior

Kalle, Arend: this patch is strictly related to the bigger 1/2. Could you ack
this change as I expect this patchset to be picked by Ming, Luis or Greg?
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index c7c1e9906500..6dbcceff2529 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 		raw_nvram = false;
 	} else {
 		data = bcm47xx_nvram_get_contents(&data_len);
-		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
-			goto fail;
+		if (!data) {
+			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
+			if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
+				brcmf_err("Loading NVRAM from %s and using platform one both failed\n",
+					  fwctx->nvram_name);
+				goto fail;
+			}
+		}
 		raw_nvram = true;
 	}
 
@@ -504,9 +510,10 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 		return;
 	}
 	fwctx->code = fw;
-	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
-				      fwctx->dev, GFP_KERNEL, fwctx,
-				      brcmf_fw_request_nvram_done);
+	ret = request_firmware_async(THIS_MODULE,
+				     FW_OPT_UEVENT | FW_OPT_NO_WARN,
+				     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
+				     fwctx, brcmf_fw_request_nvram_done);
 
 	if (!ret)
 		return;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1585178 — Re: [PATCH V3 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

On 21-2-2017 10:47, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Failing to load NVRAM file isn't critical if we manage to get platform
> one in the fallback path. It means warnings like:
> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
> are unnecessary & disturbing for people with platform NVRAM. This is
> very common case for Broadcom home routers.
> 
> So instead of printing warning immediately with the firmware subsystem
> let's first try our fallback code. If that fails as well, then it's a
> right moment to print an error.
> 
> This should reduce amount of false reports from users seeing this
> warning while having wireless working perfectly fine.

I think FW_OPT_NO_WARN does not cover all warnings in firmware_class
although I did not check. Anyway...

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
>     messages to the firmware.c.
> V3: Set FW_OPT_UEVENT to don't change behavior
> 
> Kalle, Arend: this patch is strictly related to the bigger 1/2. Could you ack
> this change as I expect this patchset to be picked by Ming, Luis or Greg?
> ---
>  .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index c7c1e9906500..6dbcceff2529 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>  		raw_nvram = false;
>  	} else {
>  		data = bcm47xx_nvram_get_contents(&data_len);
> -		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> -			goto fail;
> +		if (!data) {
> +			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
> +			if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
> +				brcmf_err("Loading NVRAM from %s and using platform one both failed\n",
> +					  fwctx->nvram_name);
> +				goto fail;
> +			}
> +		}
>  		raw_nvram = true;
>  	}
>  
> @@ -504,9 +510,10 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>  		return;
>  	}
>  	fwctx->code = fw;
> -	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
> -				      fwctx->dev, GFP_KERNEL, fwctx,
> -				      brcmf_fw_request_nvram_done);
> +	ret = request_firmware_async(THIS_MODULE,
> +				     FW_OPT_UEVENT | FW_OPT_NO_WARN,
> +				     fwctx->nvram_name, fwctx->dev, GFP_KERNEL,
> +				     fwctx, brcmf_fw_request_nvram_done);

[toc] | [prev] | [next] | [standalone]

#1585242 — Re: [PATCH V3 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

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

> On 21-2-2017 10:47, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> Failing to load NVRAM file isn't critical if we manage to get platform
>> one in the fallback path. It means warnings like:
>> [ 10.801506] brcmfmac 0000:01:00.0: Direct firmware load for
>> brcm/brcmfmac43602-pcie.txt failed with error -2
>> are unnecessary & disturbing for people with platform NVRAM. This is
>> very common case for Broadcom home routers.
>> 
>> So instead of printing warning immediately with the firmware subsystem
>> let's first try our fallback code. If that fails as well, then it's a
>> right moment to print an error.
>> 
>> This should reduce amount of false reports from users seeing this
>> warning while having wireless working perfectly fine.
>
> I think FW_OPT_NO_WARN does not cover all warnings in firmware_class
> although I did not check. Anyway...
>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Acked-by: Kalle Valo <kvalo@codeaurora.org>

Feel free to this via some another tree.

-- 
Kalle Valo

[toc] | [prev] | [next] | [standalone]

#1585580 — Re: [PATCH V3 1/2] firmware: add more flexible request_firmware_async function

On Tue, Feb 21, 2017 at 10:47:53AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> So far we got only one function for loading firmware asynchronously:
> request_firmware_nowait. It didn't allow much customization of firmware
> loading process - there is only one bool uevent argument. Moreover this
> bool also controls user helper in an unclear way.
> 
> Resolve this problem by adding a one flexible function and making old
> request_firmware_nowait a simple inline using new solution. This
> implementation:
> 1) Modifies only single bits on existing code
> 2) Doesn't require adjusting / rewriting current drivers
> 3) Adds new function for drivers that need more control over loading a
>    firmware. Thanks to using flags more features can be added later.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This patch is based on top of
> [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
> applied on top of Linux 4.10-rc8.
> 
> Ming/Luis/Greg: assuming this gets a positive review, could someone of you pick
> this patchset? Second patch modifies brcmfmac, I'll try to get a proper Ack for
> that one.
> Unless you want this to go through wireless tree, then let me know please.

As noted in the v1 and v2 series:

This is not as flexible as we need. I'll fold this functionality in to the
driver data series as it provides more flexibility and enables more testing to
be done in userspace.

  Luis

[toc] | [prev] | [next] | [standalone]

#1587076 — [PATCH V4 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds

From: Rafał Miłecki <rafal@milecki.pl>

Failing to load NVRAM file isn't critical if we manage to get platform
one in the fallback path. It means warnings like:
[   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
are unnecessary & disturbing for people with platform NVRAM. This is
very common case for Broadcom home routers.

So instead of printing warning immediately with the firmware subsystem
let's first try our fallback code. If that fails as well, then it's a
right moment to print an error.

This should reduce amount of false reports from users seeing this
warning while having wireless working perfectly fine.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
    messages to the firmware.c.
V3: Set FW_OPT_UEVENT to don't change behavior
V4: Switch to the new request_firmware_async syntax
---
 .../wireless/broadcom/brcm80211/brcmfmac/firmware.c    | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index c7c1e9906500..da7da1dc059d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 		raw_nvram = false;
 	} else {
 		data = bcm47xx_nvram_get_contents(&data_len);
-		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
-			goto fail;
+		if (!data) {
+			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
+			if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
+				brcmf_err("Loading NVRAM from %s and using platform one both failed\n",
+					  fwctx->nvram_name);
+				goto fail;
+			}
+		}
 		raw_nvram = true;
 	}
 
@@ -491,6 +497,9 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 {
 	struct brcmf_fw *fwctx = ctx;
+	struct firmware_opts fw_opts = {
+		.optional = true,
+	};
 	int ret;
 
 	brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
@@ -504,9 +513,8 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 		return;
 	}
 	fwctx->code = fw;
-	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
-				      fwctx->dev, GFP_KERNEL, fwctx,
-				      brcmf_fw_request_nvram_done);
+	ret = request_firmware_async(fwctx->nvram_name, &fw_opts, fwctx->dev,
+				     fwctx, brcmf_fw_request_nvram_done);
 
 	if (!ret)
 		return;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1587077 — [PATCH V4 1/2] firmware: add more flexible request_firmware_async function

From: Rafał Miłecki <rafal@milecki.pl>

So far we got only one function for loading firmware asynchronously:
request_firmware_nowait. It didn't allow much customization of firmware
loading process - there is only one bool uevent argument. Moreover this
bool also controls user helper in an unclear way.

Resolve this problem by adding one internally shared  function that
allows specifying any flags manually.

This implementation:
1) Allows keeping old request_firmware_nowait API unchanged
2) Doesn't require adjusting / rewriting current drivers
3) Minimizes risk of regressions
4) Adds new function for drivers that need more control over loading a
   firmware.

The new function takes options struct pointer as an argument to make
further improvements possible (without any big reworks).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V3: Don't expose all FW_OPT_* flags.
    As Luis noted we want a struct so add struct firmware_opts for real
    flexibility.
    Thank you Luis for your review!

Ming/Luis/Greg: assuming this gets a positive review, could someone of you pick
this patchset?
---
 drivers/base/firmware_class.c | 43 ++++++++++++++++++++++++++++++++++---------
 include/linux/firmware.h      | 15 ++++++++++++++-
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d05be1732c8b..b67b294eb31a 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1356,7 +1356,7 @@ static void request_firmware_work_func(struct work_struct *work)
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device); /* taken in request_firmware_nowait() */
+	put_device(fw_work->device); /* taken in __request_firmware_nowait() */
 
 	module_put(fw_work->module);
 	kfree_const(fw_work->name);
@@ -1364,10 +1364,9 @@ static void request_firmware_work_func(struct work_struct *work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * __request_firmware_nowait - asynchronous version of request_firmware
  * @module: module requesting the firmware
- * @uevent: sends uevent to copy the firmware image if this flag
- *	is non-zero else the firmware copy must be done manually.
+ * @opt_flags: flags that control firmware loading process, see FW_OPT_*
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
  * @gfp: allocation flags
@@ -1386,9 +1385,9 @@ static void request_firmware_work_func(struct work_struct *work)
  *
  *		- can't sleep at all if @gfp is GFP_ATOMIC.
  **/
-int
-request_firmware_nowait(
-	struct module *module, bool uevent,
+static int
+__request_firmware_nowait(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -1407,8 +1406,7 @@ request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+	fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
@@ -1421,8 +1419,35 @@ request_firmware_nowait(
 	schedule_work(&fw_work->work);
 	return 0;
 }
+
+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))
+{
+	unsigned int opt_flags = FW_OPT_FALLBACK |
+		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+
+	return __request_firmware_nowait(module, opt_flags, name, device, gfp,
+					 context, cont);
+}
 EXPORT_SYMBOL(request_firmware_nowait);
 
+int __request_firmware_async(struct module *module, const char *name,
+			     struct firmware_opts *fw_opts, struct device *dev,
+			     void *context,
+			     void (*cont)(const struct firmware *fw, void *context))
+{
+	unsigned int opt_flags = FW_OPT_UEVENT;
+
+	if (fw_opts->optional)
+		opt_flags |= FW_OPT_NO_WARN;
+
+	return __request_firmware_nowait(module, opt_flags, name, dev,
+					 GFP_KERNEL, context, cont);
+}
+EXPORT_SYMBOL(__request_firmware_async);
+
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..a32b6e67462d 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -82,6 +82,19 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 {
 	return -EINVAL;
 }
-
 #endif
+
+struct firmware_opts {
+	bool optional;
+};
+
+int __request_firmware_async(struct module *module, const char *name,
+			     struct firmware_opts *fw_opts, struct device *dev,
+			     void *context,
+			     void (*cont)(const struct firmware *fw, void *context));
+
+#define request_firmware_async(name, fw_opts, dev, context, cont)	\
+	__request_firmware_async(THIS_MODULE, name, fw_opts, dev,	\
+				 context, cont)
+
 #endif
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1585584 — Re: [PATCH V2 1/2] firmware: add more flexible request_firmware_async function

On Thu, Feb 16, 2017 at 08:26:35AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> So far we got only one function for loading firmware asynchronously:
> request_firmware_nowait. It didn't allow much customization of firmware
> loading process - there is only one bool uevent argument. Moreover this
> bool also controls user helper in an unclear way.
> 
> Resolve this problem by adding a one flexible function and making old
> request_firmware_nowait a simple inline using new solution. This
> implementation:
> 1) Modifies only single bits on existing code
> 2) Doesn't require adjusting / rewriting current drivers
> 3) Adds new function for drivers that need more control over loading a
>    firmware. Thanks to using flags more features can be added later.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This patch is based on top of
> [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
> applied on top of Linux 4.10-rc8.
> 
> Ming/Luis/Greg: assuming this gets a positive review, could someone of you pick
> this patchset? Second patch modifies brcmfmac, I'll try to get a proper Ack for
> that one.
> Unless you want this to go through wireless tree, then let me know please.

As I noted in the v1 post, just exposing all the flags under the hood is not
enough to ensure sanity here. Additionally just flags won't cut it to make this
as flexible as we need. I've addressed this in the driver data series, so I'll
take this feature request and fold it in there.

  Luis

[toc] | [prev] | [next] | [standalone]

#1585583 — Re: [PATCH 1/2] firmware: add more flexible request_firmware_async function

On Wed, Feb 15, 2017 at 11:29:47PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> So far we got only one function for loading firmware asynchronously:
> request_firmware_nowait. It didn't allow much customization of firmware
> loading process - there is only one bool uevent argument. Moreover this
> bool also controls user helper in an unclear way.
> 
> Resolve this problem by adding a one flexible function 

You've just taken under-the-hood flags and exposed them without considering and
enabling easy testing of any possible conflicts here.  Take for instance the
FW_OPT_NOCACHE feature -- not using caching functionality has implications for
driver developers -- they *must* resolve their own suspend/resume mishaps.
Another example, when we get firmware signing having just flags won't cut it
for optional parameters, we want a struct with the ability to specify custom
signing requirements. Exposing just flags will not cut it for us long term and
we'd have to then either add new export symbol or modify this one.

This is not as flexible nor as well documented as I want for future
functionality. Nor is there any series of battery of tests of all possible
options to ensure we do not regress. I've addressed this in the driver data
series.

> and making old
> request_firmware_nowait a simple inline using new solution. 

This I had not addressed in the driver data series but I will fold similar
strategy onto it.

> This
> implementation:
> 1) Modifies only single bits on existing code
> 2) Doesn't require adjusting / rewriting current drivers
> 3) Adds new function for drivers that need more control over loading a
>    firmware. Thanks to using flags more features can be added later.

As I noted in the driver data series -- we're passed using the firmware API for
non-firmware specific stuff, and as recent history shows regressions are are
easy, specially with the fallback mechanism. A new API which just gives us
2 calls: sync/async calls and shares a common set of optional parameters is
what we need, we need proper testing to ensure we also don't regress should
new features be introduced.

What I'll do is I'll integrate the feature you are asking for here and fold
this into the driver data series as what we need now is actual users of new
functionality, not just a test driver.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web