| Message ID | 20180310141501.2214-6-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Sat, Mar 10, 2018 at 06:14:46AM -0800, Luis R. Rodriguez wrote: > All CONFIG_FW_LOADER_USER_HELPER_FALLBACK really is, is just a bool, > initailized at build time. Define it as such. This simplifies the > logic even further, removing now all explicit #ifdefs around the code. > > Acked-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > drivers/base/firmware_loader.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c > index 7dd36ace6152..59dba794ce1a 100644 > --- a/drivers/base/firmware_loader.c > +++ b/drivers/base/firmware_loader.c > @@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) > > #ifdef CONFIG_FW_LOADER_USER_HELPER > > +/** > + * struct firmware_fallback_config - firmware fallback configuratioon settings > + * > + * Helps describe and fine tune the fallback mechanism. > + * > + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used > + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. > + */ > +struct firmware_fallback_config { > + bool force_sysfs_fallback; > +}; This is C, why are you messing around with a structure and "getters and setters" for a set of simple values? > +static const struct firmware_fallback_config fw_fallback_config = { > + .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), > +}; static bool force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK); Then just read/write the foolish thing, don't make this more complex than is needed please. There is a "getter and setters are evil" rant somewhere out there online, if you really need me to dig up the old tired arguments :) thanks, greg k-h > + > static inline bool fw_sysfs_done(struct fw_priv *fw_priv) > { > return __fw_state_check(fw_priv, FW_STATUS_DONE); > @@ -1151,19 +1167,14 @@ static int fw_load_from_user_helper(struct firmware *firmware, > return ret; > } > > -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK > -static bool fw_force_sysfs_fallback(unsigned int opt_flags) > -{ > - return true; > -} > -#else > static bool fw_force_sysfs_fallback(unsigned int opt_flags) > { > + if (fw_fallback_config.force_sysfs_fallback) Ok, you directly access it here, but your later patches get a lot more "complex" it seems... thanks, greg k-h
On Wed, Mar 14, 2018 at 11:53 AM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Sat, Mar 10, 2018 at 06:14:46AM -0800, Luis R. Rodriguez wrote: >> All CONFIG_FW_LOADER_USER_HELPER_FALLBACK really is, is just a bool, >> initailized at build time. Define it as such. This simplifies the >> logic even further, removing now all explicit #ifdefs around the code. >> >> Acked-by: Kees Cook <keescook@chromium.org> >> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> >> --- >> drivers/base/firmware_loader.c | 25 ++++++++++++++++++------- >> 1 file changed, 18 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c >> index 7dd36ace6152..59dba794ce1a 100644 >> --- a/drivers/base/firmware_loader.c >> +++ b/drivers/base/firmware_loader.c >> @@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) >> >> #ifdef CONFIG_FW_LOADER_USER_HELPER >> >> +/** >> + * struct firmware_fallback_config - firmware fallback configuratioon settings >> + * >> + * Helps describe and fine tune the fallback mechanism. >> + * >> + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used >> + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. >> + */ >> +struct firmware_fallback_config { >> + bool force_sysfs_fallback; >> +}; > > This is C, why are you messing around with a structure and "getters and > setters" for a set of simple values? > >> +static const struct firmware_fallback_config fw_fallback_config = { >> + .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), >> +}; > > static bool force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK); For this case yes, but I later expand on this structure for all fallback configuration items. So we have to start it somewhere. > Then just read/write the foolish thing, don't make this more complex > than is needed please. Please read the final results and let me know what you think. > There is a "getter and setters are evil" rant somewhere out there > online, if you really need me to dig up the old tired arguments :) I don't use them for sport, I use them given the fallback stuff *is* not something the direct core firmware code should use openly, it will really depend on if the fallback interface was enabled. As such we provide wrapper for access into them. Luis
diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c index 7dd36ace6152..59dba794ce1a 100644 --- a/drivers/base/firmware_loader.c +++ b/drivers/base/firmware_loader.c @@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) #ifdef CONFIG_FW_LOADER_USER_HELPER +/** + * struct firmware_fallback_config - firmware fallback configuratioon settings + * + * Helps describe and fine tune the fallback mechanism. + * + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. + */ +struct firmware_fallback_config { + bool force_sysfs_fallback; +}; + +static const struct firmware_fallback_config fw_fallback_config = { + .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), +}; + static inline bool fw_sysfs_done(struct fw_priv *fw_priv) { return __fw_state_check(fw_priv, FW_STATUS_DONE); @@ -1151,19 +1167,14 @@ static int fw_load_from_user_helper(struct firmware *firmware, return ret; } -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK -static bool fw_force_sysfs_fallback(unsigned int opt_flags) -{ - return true; -} -#else static bool fw_force_sysfs_fallback(unsigned int opt_flags) { + if (fw_fallback_config.force_sysfs_fallback) + return true; if (!(opt_flags & FW_OPT_USERHELPER)) return false; return true; } -#endif static bool fw_run_sysfs_fallback(unsigned int opt_flags) {