* [PATCH] firmware loader: inform direct failure when udev loader is disabled
@ 2014-06-30 23:30 Luis R. Rodriguez
2014-07-01 3:54 ` Ming Lei
2014-07-01 7:23 ` Tom Gundersen
0 siblings, 2 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2014-06-30 23:30 UTC (permalink / raw)
To: ming.lei, gregkh
Cc: linux-kernel, Luis R. Rodriguez, Tom Gundersen, Abhay Salunke,
Stefan Roese, Arnd Bergmann, Kay Sievers, Takashi Iwai
From: "Luis R. Rodriguez" <mcgrof@suse.com>
Now that the udev firmware loader is optional request_firmware()
will not provide any information on the kernel ring buffer if
direct firmware loading failed and udev firmware loading is disabled.
If no information is needed request_firmware_direct() should be used
for optional firmware, at which point drivers can take on the onus
over informing of any failures, if udev firmware loading is disabled
though we should at the very least provide some sort of information
as when the udev loader was enabled by default back in the days.
With this change with a simple firmware load test module [0]:
Example output without FW_LOADER_USER_HELPER_FALLBACK
platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
Example without FW_LOADER_USER_HELPER_FALLBACK
platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
platform fake-dev.0: Falling back to user helper
Without this change without FW_LOADER_USER_HELPER_FALLBACK we get no output
logged upon failure.
[0] https://github.com/mcgrof/fake-firmware-test.git
Cc: Tom Gundersen <teg@jklm.no>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Abhay Salunke <Abhay_Salunke@dell.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Kay Sievers <kay@vrfy.org>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
drivers/base/firmware_class.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 46ea5f4..23274d8 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -101,8 +101,10 @@ static inline long firmware_loading_timeout(void)
#define FW_OPT_NOWAIT (1U << 1)
#ifdef CONFIG_FW_LOADER_USER_HELPER
#define FW_OPT_USERHELPER (1U << 2)
+#define FW_OPT_FAIL_WARN 0
#else
#define FW_OPT_USERHELPER 0
+#define FW_OPT_FAIL_WARN (1U << 3)
#endif
#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
#define FW_OPT_FALLBACK FW_OPT_USERHELPER
@@ -1116,10 +1118,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
ret = fw_get_filesystem_firmware(device, fw->priv);
if (ret) {
- if (opt_flags & FW_OPT_USERHELPER) {
+ if (opt_flags & (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER))
dev_warn(device,
- "Direct firmware load failed with error %d\n",
- ret);
+ "Direct firmware load for %s failed with error %d\n",
+ name, ret);
+ if (opt_flags & FW_OPT_USERHELPER) {
dev_warn(device, "Falling back to user helper\n");
ret = fw_load_from_user_helper(fw, name, device,
opt_flags, timeout);
@@ -1170,7 +1173,8 @@ request_firmware(const struct firmware **firmware_p, const char *name,
/* Need to pin this module until return */
__module_get(THIS_MODULE);
ret = _request_firmware(firmware_p, name, device,
- FW_OPT_UEVENT | FW_OPT_FALLBACK);
+ FW_OPT_UEVENT | FW_OPT_FALLBACK |
+ FW_OPT_FAIL_WARN);
module_put(THIS_MODULE);
return ret;
}
--
2.0.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware loader: inform direct failure when udev loader is disabled
2014-06-30 23:30 [PATCH] firmware loader: inform direct failure when udev loader is disabled Luis R. Rodriguez
@ 2014-07-01 3:54 ` Ming Lei
2014-07-01 9:22 ` Takashi Iwai
2014-07-01 7:23 ` Tom Gundersen
1 sibling, 1 reply; 7+ messages in thread
From: Ming Lei @ 2014-07-01 3:54 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Greg Kroah-Hartman, Linux Kernel Mailing List, Luis R. Rodriguez,
Tom Gundersen, Abhay Salunke, Stefan Roese, Arnd Bergmann,
Kay Sievers, Takashi Iwai
On Tue, Jul 1, 2014 at 7:30 AM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> Now that the udev firmware loader is optional request_firmware()
> will not provide any information on the kernel ring buffer if
> direct firmware loading failed and udev firmware loading is disabled.
> If no information is needed request_firmware_direct() should be used
> for optional firmware, at which point drivers can take on the onus
> over informing of any failures, if udev firmware loading is disabled
> though we should at the very least provide some sort of information
> as when the udev loader was enabled by default back in the days.
>
> With this change with a simple firmware load test module [0]:
>
> Example output without FW_LOADER_USER_HELPER_FALLBACK
>
> platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
>
> Example without FW_LOADER_USER_HELPER_FALLBACK
>
> platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> platform fake-dev.0: Falling back to user helper
>
> Without this change without FW_LOADER_USER_HELPER_FALLBACK we get no output
> logged upon failure.
>
> [0] https://github.com/mcgrof/fake-firmware-test.git
>
> Cc: Tom Gundersen <teg@jklm.no>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kay Sievers <kay@vrfy.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>
> drivers/base/firmware_class.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 46ea5f4..23274d8 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -101,8 +101,10 @@ static inline long firmware_loading_timeout(void)
> #define FW_OPT_NOWAIT (1U << 1)
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> #define FW_OPT_USERHELPER (1U << 2)
> +#define FW_OPT_FAIL_WARN 0
> #else
> #define FW_OPT_USERHELPER 0
> +#define FW_OPT_FAIL_WARN (1U << 3)
> #endif
> #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> #define FW_OPT_FALLBACK FW_OPT_USERHELPER
> @@ -1116,10 +1118,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>
> ret = fw_get_filesystem_firmware(device, fw->priv);
> if (ret) {
> - if (opt_flags & FW_OPT_USERHELPER) {
> + if (opt_flags & (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER))
> dev_warn(device,
> - "Direct firmware load failed with error %d\n",
> - ret);
> + "Direct firmware load for %s failed with error %d\n",
> + name, ret);
Maybe the warning can be always printed out since
(FW_OPT_FAIL_WARN | FW_OPT_USERHELPER) should be
always true.
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware loader: inform direct failure when udev loader is disabled
2014-06-30 23:30 [PATCH] firmware loader: inform direct failure when udev loader is disabled Luis R. Rodriguez
2014-07-01 3:54 ` Ming Lei
@ 2014-07-01 7:23 ` Tom Gundersen
1 sibling, 0 replies; 7+ messages in thread
From: Tom Gundersen @ 2014-07-01 7:23 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Ming Lei, Greg KH, LKML, Luis R. Rodriguez, Abhay Salunke,
Stefan Roese, Arnd Bergmann, Kay Sievers, Takashi Iwai
On Tue, Jul 1, 2014 at 1:30 AM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> Now that the udev firmware loader is optional request_firmware()
> will not provide any information on the kernel ring buffer if
> direct firmware loading failed and udev firmware loading is disabled.
> If no information is needed request_firmware_direct() should be used
> for optional firmware, at which point drivers can take on the onus
> over informing of any failures, if udev firmware loading is disabled
> though we should at the very least provide some sort of information
> as when the udev loader was enabled by default back in the days.
>
> With this change with a simple firmware load test module [0]:
>
> Example output without FW_LOADER_USER_HELPER_FALLBACK
>
> platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
>
> Example without FW_LOADER_USER_HELPER_FALLBACK
This ^^ should be "Example output with [...]" ?
Otherwise looks good, so:
Reviewed-by: Tom Gundersen <teg@jklm.no>
> platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> platform fake-dev.0: Falling back to user helper
>
> Without this change without FW_LOADER_USER_HELPER_FALLBACK we get no output
> logged upon failure.
>
> [0] https://github.com/mcgrof/fake-firmware-test.git
>
> Cc: Tom Gundersen <teg@jklm.no>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kay Sievers <kay@vrfy.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>
> drivers/base/firmware_class.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 46ea5f4..23274d8 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -101,8 +101,10 @@ static inline long firmware_loading_timeout(void)
> #define FW_OPT_NOWAIT (1U << 1)
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> #define FW_OPT_USERHELPER (1U << 2)
> +#define FW_OPT_FAIL_WARN 0
> #else
> #define FW_OPT_USERHELPER 0
> +#define FW_OPT_FAIL_WARN (1U << 3)
> #endif
> #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> #define FW_OPT_FALLBACK FW_OPT_USERHELPER
> @@ -1116,10 +1118,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>
> ret = fw_get_filesystem_firmware(device, fw->priv);
> if (ret) {
> - if (opt_flags & FW_OPT_USERHELPER) {
> + if (opt_flags & (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER))
> dev_warn(device,
> - "Direct firmware load failed with error %d\n",
> - ret);
> + "Direct firmware load for %s failed with error %d\n",
> + name, ret);
> + if (opt_flags & FW_OPT_USERHELPER) {
> dev_warn(device, "Falling back to user helper\n");
> ret = fw_load_from_user_helper(fw, name, device,
> opt_flags, timeout);
> @@ -1170,7 +1173,8 @@ request_firmware(const struct firmware **firmware_p, const char *name,
> /* Need to pin this module until return */
> __module_get(THIS_MODULE);
> ret = _request_firmware(firmware_p, name, device,
> - FW_OPT_UEVENT | FW_OPT_FALLBACK);
> + FW_OPT_UEVENT | FW_OPT_FALLBACK |
> + FW_OPT_FAIL_WARN);
> module_put(THIS_MODULE);
> return ret;
> }
> --
> 2.0.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware loader: inform direct failure when udev loader is disabled
2014-07-01 3:54 ` Ming Lei
@ 2014-07-01 9:22 ` Takashi Iwai
2014-07-02 1:01 ` Luis R. Rodriguez
0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2014-07-01 9:22 UTC (permalink / raw)
To: Ming Lei
Cc: Luis R. Rodriguez, Greg Kroah-Hartman, Linux Kernel Mailing List,
Luis R. Rodriguez, Tom Gundersen, Abhay Salunke, Stefan Roese,
Arnd Bergmann, Kay Sievers
At Tue, 1 Jul 2014 11:54:24 +0800,
Ming Lei wrote:
>
> On Tue, Jul 1, 2014 at 7:30 AM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > Now that the udev firmware loader is optional request_firmware()
> > will not provide any information on the kernel ring buffer if
> > direct firmware loading failed and udev firmware loading is disabled.
> > If no information is needed request_firmware_direct() should be used
> > for optional firmware, at which point drivers can take on the onus
> > over informing of any failures, if udev firmware loading is disabled
> > though we should at the very least provide some sort of information
> > as when the udev loader was enabled by default back in the days.
> >
> > With this change with a simple firmware load test module [0]:
> >
> > Example output without FW_LOADER_USER_HELPER_FALLBACK
> >
> > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> >
> > Example without FW_LOADER_USER_HELPER_FALLBACK
> >
> > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> > platform fake-dev.0: Falling back to user helper
> >
> > Without this change without FW_LOADER_USER_HELPER_FALLBACK we get no output
> > logged upon failure.
> >
> > [0] https://github.com/mcgrof/fake-firmware-test.git
> >
> > Cc: Tom Gundersen <teg@jklm.no>
> > Cc: Ming Lei <ming.lei@canonical.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Kay Sievers <kay@vrfy.org>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >
> > drivers/base/firmware_class.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 46ea5f4..23274d8 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -101,8 +101,10 @@ static inline long firmware_loading_timeout(void)
> > #define FW_OPT_NOWAIT (1U << 1)
> > #ifdef CONFIG_FW_LOADER_USER_HELPER
> > #define FW_OPT_USERHELPER (1U << 2)
> > +#define FW_OPT_FAIL_WARN 0
> > #else
> > #define FW_OPT_USERHELPER 0
> > +#define FW_OPT_FAIL_WARN (1U << 3)
> > #endif
> > #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> > #define FW_OPT_FALLBACK FW_OPT_USERHELPER
> > @@ -1116,10 +1118,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >
> > ret = fw_get_filesystem_firmware(device, fw->priv);
> > if (ret) {
> > - if (opt_flags & FW_OPT_USERHELPER) {
> > + if (opt_flags & (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER))
> > dev_warn(device,
> > - "Direct firmware load failed with error %d\n",
> > - ret);
> > + "Direct firmware load for %s failed with error %d\n",
> > + name, ret);
>
> Maybe the warning can be always printed out since
> (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER) should be
> always true.
Yes, it'd be simpler. Let's reduce lines! :)
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware loader: inform direct failure when udev loader is disabled
2014-07-01 9:22 ` Takashi Iwai
@ 2014-07-02 1:01 ` Luis R. Rodriguez
2014-07-02 1:51 ` Ming Lei
0 siblings, 1 reply; 7+ messages in thread
From: Luis R. Rodriguez @ 2014-07-02 1:01 UTC (permalink / raw)
To: Takashi Iwai
Cc: Ming Lei, Luis R. Rodriguez, Greg Kroah-Hartman,
Linux Kernel Mailing List, Tom Gundersen, Abhay Salunke,
Stefan Roese, Arnd Bergmann, Kay Sievers
On Tue, Jul 01, 2014 at 11:22:07AM +0200, Takashi Iwai wrote:
> At Tue, 1 Jul 2014 11:54:24 +0800,
> Ming Lei wrote:
> >
> > On Tue, Jul 1, 2014 at 7:30 AM, Luis R. Rodriguez
> > <mcgrof@do-not-panic.com> wrote:
> > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > >
> > > Now that the udev firmware loader is optional request_firmware()
> > > will not provide any information on the kernel ring buffer if
> > > direct firmware loading failed and udev firmware loading is disabled.
> > > If no information is needed request_firmware_direct() should be used
> > > for optional firmware, at which point drivers can take on the onus
> > > over informing of any failures, if udev firmware loading is disabled
> > > though we should at the very least provide some sort of information
> > > as when the udev loader was enabled by default back in the days.
> > >
> > > With this change with a simple firmware load test module [0]:
> > >
> > > Example output without FW_LOADER_USER_HELPER_FALLBACK
> > >
> > > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> > >
> > > Example without FW_LOADER_USER_HELPER_FALLBACK
> > >
> > > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> > > platform fake-dev.0: Falling back to user helper
> > >
> > > Without this change without FW_LOADER_USER_HELPER_FALLBACK we get no output
> > > logged upon failure.
> > >
> > > [0] https://github.com/mcgrof/fake-firmware-test.git
> > >
> > > Cc: Tom Gundersen <teg@jklm.no>
> > > Cc: Ming Lei <ming.lei@canonical.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > > Cc: Stefan Roese <sr@denx.de>
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Kay Sievers <kay@vrfy.org>
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > > ---
> > >
> > > drivers/base/firmware_class.c | 12 ++++++++----
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 46ea5f4..23274d8 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -101,8 +101,10 @@ static inline long firmware_loading_timeout(void)
> > > #define FW_OPT_NOWAIT (1U << 1)
> > > #ifdef CONFIG_FW_LOADER_USER_HELPER
> > > #define FW_OPT_USERHELPER (1U << 2)
> > > +#define FW_OPT_FAIL_WARN 0
> > > #else
> > > #define FW_OPT_USERHELPER 0
> > > +#define FW_OPT_FAIL_WARN (1U << 3)
> > > #endif
> > > #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> > > #define FW_OPT_FALLBACK FW_OPT_USERHELPER
> > > @@ -1116,10 +1118,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> > >
> > > ret = fw_get_filesystem_firmware(device, fw->priv);
> > > if (ret) {
> > > - if (opt_flags & FW_OPT_USERHELPER) {
> > > + if (opt_flags & (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER))
> > > dev_warn(device,
> > > - "Direct firmware load failed with error %d\n",
> > > - ret);
> > > + "Direct firmware load for %s failed with error %d\n",
> > > + name, ret);
> >
> > Maybe the warning can be always printed out since
> > (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER) should be
> > always true.
>
> Yes, it'd be simpler. Let's reduce lines! :)
Actually we don't want to warn when request_firmware_direct() is used right? That's really what
I meant to upkeep while adding a warning when _direct() is not used. So how about
this as an ammendment:
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 23274d8..4f6adf3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1180,7 +1180,6 @@ request_firmware(const struct firmware **firmware_p, const char *name,
}
EXPORT_SYMBOL(request_firmware);
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
/**
* request_firmware: - load firmware directly without usermode helper
* @firmware_p: pointer to firmware image
@@ -1202,7 +1201,6 @@ int request_firmware_direct(const struct firmware **firmware_p,
return ret;
}
EXPORT_SYMBOL_GPL(request_firmware_direct);
-#endif
/**
* release_firmware: - release the resource associated with a firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 67e5b80..5c41c5e 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -45,6 +45,8 @@ int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context));
+int request_firmware_direct(const struct firmware **fw, const char *name,
+ struct device *device);
void release_firmware(const struct firmware *fw);
#else
@@ -66,13 +68,12 @@ static inline void release_firmware(const struct firmware *fw)
{
}
-#endif
+static inline int request_firmware_direct(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+ return -EINVAL;
+}
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-int request_firmware_direct(const struct firmware **fw, const char *name,
- struct device *device);
-#else
-#define request_firmware_direct request_firmware
#endif
-
#endif
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware loader: inform direct failure when udev loader is disabled
2014-07-02 1:01 ` Luis R. Rodriguez
@ 2014-07-02 1:51 ` Ming Lei
2014-07-02 2:34 ` Luis R. Rodriguez
0 siblings, 1 reply; 7+ messages in thread
From: Ming Lei @ 2014-07-02 1:51 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Takashi Iwai, Luis R. Rodriguez, Greg Kroah-Hartman,
Linux Kernel Mailing List, Tom Gundersen, Abhay Salunke,
Stefan Roese, Arnd Bergmann, Kay Sievers
On Wed, Jul 2, 2014 at 9:01 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Jul 01, 2014 at 11:22:07AM +0200, Takashi Iwai wrote:
>> At Tue, 1 Jul 2014 11:54:24 +0800,
>> Ming Lei wrote:
>> >
>> > On Tue, Jul 1, 2014 at 7:30 AM, Luis R. Rodriguez
>> > <mcgrof@do-not-panic.com> wrote:
>> > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> > >
>> > > Now that the udev firmware loader is optional request_firmware()
>> > > will not provide any information on the kernel ring buffer if
>> > > direct firmware loading failed and udev firmware loading is disabled.
>> > > If no information is needed request_firmware_direct() should be used
>> > > for optional firmware, at which point drivers can take on the onus
>> > > over informing of any failures, if udev firmware loading is disabled
>> > > though we should at the very least provide some sort of information
>> > > as when the udev loader was enabled by default back in the days.
>> > >
>> > > With this change with a simple firmware load test module [0]:
>> > >
>> > > Example output without FW_LOADER_USER_HELPER_FALLBACK
>> > >
>> > > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
>> > >
>> > > Example without FW_LOADER_USER_HELPER_FALLBACK
>> > >
>> > > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
>> > > platform fake-dev.0: Falling back to user helper
>> > >
>> > > Without this change without FW_LOADER_USER_HELPER_FALLBACK we get no output
>> > > logged upon failure.
>> > >
>> > > [0] https://github.com/mcgrof/fake-firmware-test.git
>> > >
>> > > Cc: Tom Gundersen <teg@jklm.no>
>> > > Cc: Ming Lei <ming.lei@canonical.com>
>> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
>> > > Cc: Stefan Roese <sr@denx.de>
>> > > Cc: Arnd Bergmann <arnd@arndb.de>
>> > > Cc: Kay Sievers <kay@vrfy.org>
>> > > Cc: Takashi Iwai <tiwai@suse.de>
>> > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
>> > > ---
>> > >
>> > > drivers/base/firmware_class.c | 12 ++++++++----
>> > > 1 file changed, 8 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> > > index 46ea5f4..23274d8 100644
>> > > --- a/drivers/base/firmware_class.c
>> > > +++ b/drivers/base/firmware_class.c
>> > > @@ -101,8 +101,10 @@ static inline long firmware_loading_timeout(void)
>> > > #define FW_OPT_NOWAIT (1U << 1)
>> > > #ifdef CONFIG_FW_LOADER_USER_HELPER
>> > > #define FW_OPT_USERHELPER (1U << 2)
>> > > +#define FW_OPT_FAIL_WARN 0
>> > > #else
>> > > #define FW_OPT_USERHELPER 0
>> > > +#define FW_OPT_FAIL_WARN (1U << 3)
>> > > #endif
>> > > #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
>> > > #define FW_OPT_FALLBACK FW_OPT_USERHELPER
>> > > @@ -1116,10 +1118,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>> > >
>> > > ret = fw_get_filesystem_firmware(device, fw->priv);
>> > > if (ret) {
>> > > - if (opt_flags & FW_OPT_USERHELPER) {
>> > > + if (opt_flags & (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER))
>> > > dev_warn(device,
>> > > - "Direct firmware load failed with error %d\n",
>> > > - ret);
>> > > + "Direct firmware load for %s failed with error %d\n",
>> > > + name, ret);
>> >
>> > Maybe the warning can be always printed out since
>> > (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER) should be
>> > always true.
>>
>> Yes, it'd be simpler. Let's reduce lines! :)
>
> Actually we don't want to warn when request_firmware_direct() is used right? That's really what
Yes, that is for the CPU microcode update in which it is common to
fail in direct loading, and x86 guys hate the warning.
> I meant to upkeep while adding a warning when _direct() is not used. So how about
> this as an ammendment:
Yes, the idea is right, and it is good to make request_firmware_direct()
not depend on CONFIG_FW_LOADER_USER_HELPER, and with
one FW_OPT_DIRECT_ONLY flag together it should work.
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 23274d8..4f6adf3 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1180,7 +1180,6 @@ request_firmware(const struct firmware **firmware_p, const char *name,
> }
> EXPORT_SYMBOL(request_firmware);
>
> -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> /**
> * request_firmware: - load firmware directly without usermode helper
> * @firmware_p: pointer to firmware image
> @@ -1202,7 +1201,6 @@ int request_firmware_direct(const struct firmware **firmware_p,
> return ret;
> }
> EXPORT_SYMBOL_GPL(request_firmware_direct);
> -#endif
>
> /**
> * release_firmware: - release the resource associated with a firmware image
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 67e5b80..5c41c5e 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -45,6 +45,8 @@ int request_firmware_nowait(
> struct module *module, bool uevent,
> const char *name, struct device *device, gfp_t gfp, void *context,
> void (*cont)(const struct firmware *fw, void *context));
> +int request_firmware_direct(const struct firmware **fw, const char *name,
> + struct device *device);
>
> void release_firmware(const struct firmware *fw);
> #else
> @@ -66,13 +68,12 @@ static inline void release_firmware(const struct firmware *fw)
> {
> }
>
> -#endif
> +static inline int request_firmware_direct(const struct firmware **fw,
> + const char *name,
> + struct device *device)
> +{
> + return -EINVAL;
> +}
You define two request_firmware_direct?
>
> -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> -int request_firmware_direct(const struct firmware **fw, const char *name,
> - struct device *device);
> -#else
> -#define request_firmware_direct request_firmware
> #endif
> -
> #endif
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware loader: inform direct failure when udev loader is disabled
2014-07-02 1:51 ` Ming Lei
@ 2014-07-02 2:34 ` Luis R. Rodriguez
0 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2014-07-02 2:34 UTC (permalink / raw)
To: Ming Lei
Cc: Takashi Iwai, Luis R. Rodriguez, Greg Kroah-Hartman,
Linux Kernel Mailing List, Tom Gundersen, Abhay Salunke,
Stefan Roese, Arnd Bergmann, Kay Sievers
On Wed, Jul 02, 2014 at 09:51:36AM +0800, Ming Lei wrote:
> On Wed, Jul 2, 2014 at 9:01 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Tue, Jul 01, 2014 at 11:22:07AM +0200, Takashi Iwai wrote:
> >> At Tue, 1 Jul 2014 11:54:24 +0800,
> >> Ming Lei wrote:
> >> >
> >> > On Tue, Jul 1, 2014 at 7:30 AM, Luis R. Rodriguez
> >> > <mcgrof@do-not-panic.com> wrote:
> >> > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> > >
> >> > > Now that the udev firmware loader is optional request_firmware()
> >> > > will not provide any information on the kernel ring buffer if
> >> > > direct firmware loading failed and udev firmware loading is disabled.
> >> > > If no information is needed request_firmware_direct() should be used
> >> > > for optional firmware, at which point drivers can take on the onus
> >> > > over informing of any failures, if udev firmware loading is disabled
> >> > > though we should at the very least provide some sort of information
> >> > > as when the udev loader was enabled by default back in the days.
> >> > >
> >> > > With this change with a simple firmware load test module [0]:
> >> > >
> >> > > Example output without FW_LOADER_USER_HELPER_FALLBACK
> >> > >
> >> > > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> >> > >
> >> > > Example without FW_LOADER_USER_HELPER_FALLBACK
> >> > >
> >> > > platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> >> > > platform fake-dev.0: Falling back to user helper
> >> > >
> >> > > Without this change without FW_LOADER_USER_HELPER_FALLBACK we get no output
> >> > > logged upon failure.
> >> > >
> >> > > [0] https://github.com/mcgrof/fake-firmware-test.git
> >> > >
> >> > > Cc: Tom Gundersen <teg@jklm.no>
> >> > > Cc: Ming Lei <ming.lei@canonical.com>
> >> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> >> > > Cc: Stefan Roese <sr@denx.de>
> >> > > Cc: Arnd Bergmann <arnd@arndb.de>
> >> > > Cc: Kay Sievers <kay@vrfy.org>
> >> > > Cc: Takashi Iwai <tiwai@suse.de>
> >> > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> >> > > ---
> >> > >
> >> > > drivers/base/firmware_class.c | 12 ++++++++----
> >> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> >> > > index 46ea5f4..23274d8 100644
> >> > > --- a/drivers/base/firmware_class.c
> >> > > +++ b/drivers/base/firmware_class.c
> >> > > @@ -101,8 +101,10 @@ static inline long firmware_loading_timeout(void)
> >> > > #define FW_OPT_NOWAIT (1U << 1)
> >> > > #ifdef CONFIG_FW_LOADER_USER_HELPER
> >> > > #define FW_OPT_USERHELPER (1U << 2)
> >> > > +#define FW_OPT_FAIL_WARN 0
> >> > > #else
> >> > > #define FW_OPT_USERHELPER 0
> >> > > +#define FW_OPT_FAIL_WARN (1U << 3)
> >> > > #endif
> >> > > #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> >> > > #define FW_OPT_FALLBACK FW_OPT_USERHELPER
> >> > > @@ -1116,10 +1118,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >> > >
> >> > > ret = fw_get_filesystem_firmware(device, fw->priv);
> >> > > if (ret) {
> >> > > - if (opt_flags & FW_OPT_USERHELPER) {
> >> > > + if (opt_flags & (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER))
> >> > > dev_warn(device,
> >> > > - "Direct firmware load failed with error %d\n",
> >> > > - ret);
> >> > > + "Direct firmware load for %s failed with error %d\n",
> >> > > + name, ret);
> >> >
> >> > Maybe the warning can be always printed out since
> >> > (FW_OPT_FAIL_WARN | FW_OPT_USERHELPER) should be
> >> > always true.
> >>
> >> Yes, it'd be simpler. Let's reduce lines! :)
> >
> > Actually we don't want to warn when request_firmware_direct() is used right? That's really what
>
> Yes, that is for the CPU microcode update in which it is common to
> fail in direct loading, and x86 guys hate the warning.
I've extended use of request_firmware_direct() to drivers that also load
non-firmware but optional config files, EEPROM override, etc.
> > I meant to upkeep while adding a warning when _direct() is not used. So how about
> > this as an ammendment:
>
> Yes, the idea is right, and it is good to make request_firmware_direct()
> not depend on CONFIG_FW_LOADER_USER_HELPER, and with
> one FW_OPT_DIRECT_ONLY flag together it should work.
OK.
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 23274d8..4f6adf3 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -66,13 +68,12 @@ static inline void release_firmware(const struct firmware *fw)
> > {
> > }
> >
> > -#endif
> > +static inline int request_firmware_direct(const struct firmware **fw,
> > + const char *name,
> > + struct device *device)
> > +{
> > + return -EINVAL;
> > +}
>
> You define two request_firmware_direct?
This is the negative of
#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
We have two also for request_firmware().
Will send out a v2.
Luis
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-07-02 2:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-30 23:30 [PATCH] firmware loader: inform direct failure when udev loader is disabled Luis R. Rodriguez
2014-07-01 3:54 ` Ming Lei
2014-07-01 9:22 ` Takashi Iwai
2014-07-02 1:01 ` Luis R. Rodriguez
2014-07-02 1:51 ` Ming Lei
2014-07-02 2:34 ` Luis R. Rodriguez
2014-07-01 7:23 ` Tom Gundersen
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.