Re: [PATCH v3 05/20] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further

[Luis R. Rodriguez]

On Wed, Mar 14, 2018 at 11:53 AM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
>> ---
>>  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