From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
On Tue, Feb 21, 2017 at 10:47:53AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
Arend Van Spriel <[email protected]> writes:
> On 21-2-2017 10:47, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <[email protected]>
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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 <[email protected]>
Acked-by: Kalle Valo <[email protected]>
Feel free to this via some another tree.
--=20
Kalle Valo
On Wed, Feb 15, 2017 at 11:29:47PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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
From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
On 2017-02-16 02:19, Andy Shevchenko wrote:
> On Thu, Feb 16, 2017 at 12:29 AM, Rafał Miłecki <[email protected]>
> wrote:
>> From: Rafał Miłecki <[email protected]>
>>
>> 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.
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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.
On Thu, Feb 16, 2017 at 12:29 AM, Rafa=C5=82 Mi=C5=82ecki <[email protected]=
> wrote:
> From: Rafa=C5=82 Mi=C5=82ecki <[email protected]>
>
> 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=C5=82 Mi=C5=82ecki <[email protected]>
> ---
> 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 f=
irmware *fw, void *ctx)
> return;
> }
> fwctx->code =3D fw;
> - ret =3D request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_n=
ame,
> - fwctx->dev, GFP_KERNEL, fwctx,
> - brcmf_fw_request_nvram_done);
> + ret =3D request_firmware_async(THIS_MODULE, FW_OPT_NO_WARN,
> + fwctx->nvram_name, fwctx->dev, GFP_K=
ERNEL,
> + fwctx, brcmf_fw_request_nvram_done);
>
> if (!ret)
> return;
> --
> 2.11.0
>
--=20
With Best Regards,
Andy Shevchenko
From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
On 21-2-2017 10:47, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> 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);
From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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
From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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
On 2017-02-16 09:38, Arend Van Spriel wrote:
> On 16-2-2017 8:26, Rafał Miłecki wrote:
>> From: Rafał Miłecki <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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.
On Thu, Feb 16, 2017 at 08:26:35AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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
On 16-2-2017 8:26, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Fri, Mar 17, 2017 at 06:29:36PM +0100, Luis R. Rodriguez wrote:
> On Thu, Mar 16, 2017 at 11:03:52PM +0900, Greg KH wrote:
> > On Thu, Mar 16, 2017 at 02:55:09PM +0100, Rafał Miłecki wrote:
> > > Luis: would you ack this patch now I followed your guidance?
> >
> > It's up to Luis now :)
>
> I'm reviewing now!
This review has taken longer than expected given I was also reviewing
how to mesh in the new API I had been working on longer than this
patch series, and also address the long standing issues with the
UMH I have cringed over. Last week and this weekend I did most of
the hard work to feel happy with a resolution, and will now be folding
this functionality into my series.
So -- please have a bit of patience, and I will be spinning out a new
series which incorporates the functionality you have requested, just
meshed more in line with the other requirements we have had for a
while.
Luis
On Thu, Mar 16, 2017 at 02:55:09PM +0100, Rafał Miłecki wrote:
> On 02/23/2017 07:30 PM, Rafał Miłecki wrote:
> > From: Rafał Miłecki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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?
>
> Hi Ming,
>
> It seems you changed e-mail address somewhere between V3 and V4 of this
> patchset. Could you review/comment/ack this, please?
Ming isn't the firmware maintainer anymore :(
> Luis: would you ack this patch now I followed your guidance?
It's up to Luis now :)
greg k-h
On 02/23/2017 07:30 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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?
Hi Ming,
It seems you changed e-mail address somewhere between V3 and V4 of this
patchset. Could you review/comment/ack this, please?
Luis: would you ack this patch now I followed your guidance?
> ---
> 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
>
On Thu, Mar 16, 2017 at 10:57:00AM +0100, Rafał Miłecki wrote:
> On 23 February 2017 at 19:30, Rafał Miłecki <[email protected]> wrote:
> > From: Rafał Miłecki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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?
>
> Ping. I hope it's relatively simple and non-intrusive change with a
> proper design now.
>
> Is there some who could pick this small patchset?
It would be nice if the firmware maintainer could review it, I can't do
anything with this until then...
thanks,
greg k-h
On 23 February 2017 at 19:30, Rafa=C5=82 Mi=C5=82ecki <[email protected]> wr=
ote:
> From: Rafa=C5=82 Mi=C5=82ecki <[email protected]>
>
> 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=C5=82 Mi=C5=82ecki <[email protected]>
> ---
> 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 yo=
u pick
> this patchset?
Ping. I hope it's relatively simple and non-intrusive change with a
proper design now.
Is there some who could pick this small patchset?
On Thu, Mar 16, 2017 at 11:03:52PM +0900, Greg KH wrote:
> On Thu, Mar 16, 2017 at 02:55:09PM +0100, Rafał Miłecki wrote:
> > Luis: would you ack this patch now I followed your guidance?
>
> It's up to Luis now :)
I'm reviewing now!
Luis