Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK

From: RafaÅ MiÅecki
Date: Thu Feb 23 2017 - 08:00:50 EST

On 02/21/2017 06:10 PM, Luis R. Rodriguez wrote:
On Wed, Feb 15, 2017 at 04:05:13PM +0100, RafaÅ MiÅecki wrote:
From: RafaÅ MiÅecki <rafal@xxxxxxxxxx>

I found handling of FW_OPT_FALLBACK a bit complex. It was defined using
another option and their values were dependent on kernel config.

It was also non-trivial to follow the code. Some callers were using
FW_OPT_FALLBACK which was confusing since the _request_firmware function
was always checking for FW_OPT_USERHELPER (the same bit in a relevant

With this patch FW_OPT_USERHELPER gets its own bit and is explicitly
checked in the _request_firmware which hopefully makes code easier to

Signed-off-by: RafaÅ MiÅecki <rafal@xxxxxxxxxx>
V2: s/config_enabled/IS_ENABLED/ to compile since c0a0aba8e47 ("kconfig.h: remove config_enabled() macro")

RESEND: I was suggested to resend this patch (thanks Greg!)

Ming/Luis/Greg: could someone accept this patch, please? I hope this trivial
cleanup isn't too big deal.
drivers/base/firmware_class.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index ac350c518e0c..d05be1732c8b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -190,13 +190,9 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
-#define FW_OPT_FALLBACK 0
#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 */
@@ -1210,8 +1206,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
"Direct firmware load for %s failed with error %d\n",
name, ret);
- if (opt_flags & FW_OPT_USERHELPER) {
+ opt_flags & FW_OPT_FALLBACK) {
dev_warn(device, "Falling back to user helper\n");
+ opt_flags |= FW_OPT_USERHELPER;
+ }
+ if (opt_flags & FW_OPT_USERHELPER) {
ret = fw_load_from_user_helper(fw, name, device,
opt_flags, timeout);

I've given this some thought and while at first glance it seems like an
improvement but it deviates from the traditional way we fold out features in
Linux. Instead let's just wrap properly the entire functionality into wrappers
which will no-op for when the fallback mechanism is disabled. That dev_warn()
for example can just be moved to the call fw_load_from_user_helper() and when
we disable the feature it will be no-op. Ultimately I'd like to shove the
entire fallback mechanism to its own file.

I don't understand this part: "dev_warn() for example can just be moved to the
call fw_load_from_user_helper()".

Did you mean moving dev_warn into fw_load_from_user_helper function? Should I
move (opt_flags & FW_OPT_USERHELPER) condition there as well?