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

[Luis R. Rodriguez]

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
> configuration).
> 
> With this patch FW_OPT_USERHELPER gets its own bit and is explicitly
> checked in the _request_firmware which hopefully makes code easier to
> understand.
> 
> 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)
>  #else
>  #define FW_OPT_USERHELPER	0
>  #endif
> -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> -#define FW_OPT_FALLBACK		FW_OPT_USERHELPER
> -#else
> -#define FW_OPT_FALLBACK		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 */
> @@ -1210,8 +1206,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  			dev_warn(device,
>  				 "Direct firmware load for %s failed with error %d\n",
>  				 name, ret);
> -		if (opt_flags & FW_OPT_USERHELPER) {
> +		if (IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK) &&
> +		    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.

  Luis