* [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
@ 2017-02-15 15:05 Rafał Miłecki
2017-02-21 17:10 ` Luis R. Rodriguez
0 siblings, 1 reply; 3+ messages in thread
From: Rafał Miłecki @ 2017-02-15 15:05 UTC (permalink / raw)
To: Ming Lei, Luis R . Rodriguez, Linux Kernel Mailing List, Greg KH
Cc: Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
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@milecki.pl>
---
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);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
2017-02-15 15:05 [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK Rafał Miłecki
@ 2017-02-21 17:10 ` Luis R. Rodriguez
2017-02-23 13:00 ` Rafał Miłecki
0 siblings, 1 reply; 3+ messages in thread
From: Luis R. Rodriguez @ 2017-02-21 17:10 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Ming Lei, Luis R . Rodriguez, Linux Kernel Mailing List, Greg KH,
Rafał Miłecki
On Wed, Feb 15, 2017 at 04:05:13PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> 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@milecki.pl>
> ---
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
2017-02-21 17:10 ` Luis R. Rodriguez
@ 2017-02-23 13:00 ` Rafał Miłecki
0 siblings, 0 replies; 3+ messages in thread
From: Rafał Miłecki @ 2017-02-23 13:00 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Ming Lei, Linux Kernel Mailing List, Greg KH, Rafał Miłecki
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@milecki.pl>
>>
>> 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@milecki.pl>
>> ---
>> 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.
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?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-02-23 13:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15 15:05 [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK Rafał Miłecki
2017-02-21 17:10 ` Luis R. Rodriguez
2017-02-23 13:00 ` Rafał Miłecki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.