From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754028AbdBURLE (ORCPT ); Tue, 21 Feb 2017 12:11:04 -0500 Received: from mx2.suse.de ([195.135.220.15]:56361 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753989AbdBURK4 (ORCPT ); Tue, 21 Feb 2017 12:10:56 -0500 Date: Tue, 21 Feb 2017 18:10:53 +0100 From: "Luis R. Rodriguez" To: =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= Cc: Ming Lei , "Luis R . Rodriguez" , Linux Kernel Mailing List , Greg KH , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= Subject: Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK Message-ID: <20170221171053.GH31264@wotan.suse.de> References: <20170215150513.3703-1-zajec5@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170215150513.3703-1-zajec5@gmail.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 15, 2017 at 04:05:13PM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki > > 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 > --- > 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