Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1581385 > unrolled thread
| Started by | Rafał Miłecki <zajec5@gmail.com> |
|---|---|
| First post | 2017-02-15 16:10 +0100 |
| Last post | 2017-02-23 14:10 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK Rafał Miłecki <zajec5@gmail.com> - 2017-02-15 16:10 +0100
Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-21 18:20 +0100
Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK Rafał Miłecki <zajec5@gmail.com> - 2017-02-23 14:10 +0100
| From | Rafał Miłecki <zajec5@gmail.com> |
|---|---|
| Date | 2017-02-15 16:10 +0100 |
| Subject | [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK |
| Message-ID | <tb9ns-1Ak-13@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-02-21 18:20 +0100 |
| Subject | Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK |
| Message-ID | <tdmgy-5z4-29@gated-at.bofh.it> |
| In reply to | #1581385 |
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
[toc] | [prev] | [next] | [standalone]
| From | Rafał Miłecki <zajec5@gmail.com> |
|---|---|
| Date | 2017-02-23 14:10 +0100 |
| Subject | Re: [PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK |
| Message-ID | <te1jI-1Fq-7@gated-at.bofh.it> |
| In reply to | #1585542 |
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?
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web