On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
> Gitweb: http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
> Commit: d05c39ea67f5786a549ac9d672d2951992b658c6
> Parent: a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
> Author: Takashi Iwai <[email protected]>
> AuthorDate: Wed May 22 18:28:37 2013 +0200
> Committer: Greg Kroah-Hartman <[email protected]>
> CommitDate: Mon Jun 3 13:57:29 2013 -0700
>
> dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
>
> The usermode helper is mandatory for this driver.
>
> Signed-off-by: Takashi Iwai <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> drivers/firmware/Kconfig | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 9387630..0747872 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -64,6 +64,7 @@ config DELL_RBU
> tristate "BIOS update support for DELL systems via sysfs"
> depends on X86
> select FW_LOADER
> + select FW_LOADER_USER_HELPER
> help
> Say m if you want to have the option of updating the BIOS for your
> DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
This is pretty crappy. Now every distro kernel has to have that enabled,
which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
when this is enabled).
Dave
On Sat, Jul 06, 2013 at 06:14:01PM -0400, Dave Jones wrote:
> On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
> > Gitweb: http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
> > Commit: d05c39ea67f5786a549ac9d672d2951992b658c6
> > Parent: a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
> > Author: Takashi Iwai <[email protected]>
> > AuthorDate: Wed May 22 18:28:37 2013 +0200
> > Committer: Greg Kroah-Hartman <[email protected]>
> > CommitDate: Mon Jun 3 13:57:29 2013 -0700
> >
> > dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
> >
> > The usermode helper is mandatory for this driver.
> >
> > Signed-off-by: Takashi Iwai <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > drivers/firmware/Kconfig | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 9387630..0747872 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -64,6 +64,7 @@ config DELL_RBU
> > tristate "BIOS update support for DELL systems via sysfs"
> > depends on X86
> > select FW_LOADER
> > + select FW_LOADER_USER_HELPER
> > help
> > Say m if you want to have the option of updating the BIOS for your
> > DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
>
>
> This is pretty crappy. Now every distro kernel has to have that enabled,
> which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
> when this is enabled).
I'll let you and Takashi battle this one out, for some reason I thought
he added it _because_ a distro kernel needed it...
Takashi?
At Sat, 6 Jul 2013 15:30:03 -0700,
Greg KH wrote:
>
> On Sat, Jul 06, 2013 at 06:14:01PM -0400, Dave Jones wrote:
> > On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
> > > Gitweb: http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
> > > Commit: d05c39ea67f5786a549ac9d672d2951992b658c6
> > > Parent: a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
> > > Author: Takashi Iwai <[email protected]>
> > > AuthorDate: Wed May 22 18:28:37 2013 +0200
> > > Committer: Greg Kroah-Hartman <[email protected]>
> > > CommitDate: Mon Jun 3 13:57:29 2013 -0700
> > >
> > > dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
> > >
> > > The usermode helper is mandatory for this driver.
> > >
> > > Signed-off-by: Takashi Iwai <[email protected]>
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > > ---
> > > drivers/firmware/Kconfig | 1 +
> > > 1 files changed, 1 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > > index 9387630..0747872 100644
> > > --- a/drivers/firmware/Kconfig
> > > +++ b/drivers/firmware/Kconfig
> > > @@ -64,6 +64,7 @@ config DELL_RBU
> > > tristate "BIOS update support for DELL systems via sysfs"
> > > depends on X86
> > > select FW_LOADER
> > > + select FW_LOADER_USER_HELPER
> > > help
> > > Say m if you want to have the option of updating the BIOS for your
> > > DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
> >
> >
> > This is pretty crappy. Now every distro kernel has to have that enabled,
> > which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
> > when this is enabled).
>
> I'll let you and Takashi battle this one out, for some reason I thought
> he added it _because_ a distro kernel needed it...
The reason is that dell_rbu driver requires it. Without the kconfig
option, this driver won't work at all. So, it's a right fix for
dell_rbu.
AFAIK, the consensus in the kernel side is that this too long fw
loading time is basically a regression of user-space (udev or
whatever). There is no change in the kernel behavior. The problem
must exist even with the older kernels.
But, looking at the development, we can't expect that udev will be
fixed soon, and this breakage persists already way too long. Maybe a
better solution is to kill the fallback to udev for normal f/w loading
(i.e. for distro kernels).
The patch below is an untested quick hack. It adds a new Kconfig and
a new function request_firmware_via_user_helper(). Distro kernels may
set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
stall for non-existing firmware file access -- as distributions know
that the firmware files should be placed in the right path.
Thoughts?
Takashi
---
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 5daa259..bfbfa75 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -144,9 +144,13 @@ config EXTRA_FIRMWARE_DIR
some other directory containing the firmware files.
config FW_LOADER_USER_HELPER
+ bool
+ depends on FW_LOADER
+
+config FW_LOADER_USER_HELPER_FALLBACK
bool "Fallback user-helper invocation for firmware loading"
depends on FW_LOADER
- default y
+ select FW_LOADER_USER_HELPER
help
This option enables / disables the invocation of user-helper
(e.g. udev) for loading firmware files as a fallback after the
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index a439602..8d24576 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1054,7 +1054,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
/* called from request_firmware() and request_firmware_work_func() */
static int
_request_firmware(const struct firmware **firmware_p, const char *name,
- struct device *device, bool uevent, bool nowait)
+ struct device *device, bool uevent, bool nowait,
+ bool user_helper_only)
{
struct firmware *fw;
long timeout;
@@ -1086,9 +1087,17 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
}
}
- if (!fw_get_filesystem_firmware(device, fw->priv))
+ if (user_helper_only)
ret = fw_load_from_user_helper(fw, name, device,
uevent, nowait, timeout);
+ else if (!fw_get_filesystem_firmware(device, fw->priv)) {
+#ifdef FW_LOADER_USER_HELPER_FALLBACK
+ ret = fw_load_from_user_helper(fw, name, device,
+ uevent, nowait, timeout);
+#else
+ ret = -ENOENT;
+#endif
+ }
/* don't cache firmware handled without uevent */
if (!ret)
@@ -1134,7 +1143,7 @@ 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, true, false);
+ ret = _request_firmware(firmware_p, name, device, true, false, false);
module_put(THIS_MODULE);
return ret;
}
@@ -1163,6 +1172,7 @@ struct firmware_work {
void *context;
void (*cont)(const struct firmware *fw, void *context);
bool uevent;
+ bool user_helper;
};
static void request_firmware_work_func(struct work_struct *work)
@@ -1173,7 +1183,7 @@ static void request_firmware_work_func(struct work_struct *work)
fw_work = container_of(work, struct firmware_work, work);
_request_firmware(&fw, fw_work->name, fw_work->device,
- fw_work->uevent, true);
+ fw_work->uevent, true, fw_work->user_helper);
fw_work->cont(fw, fw_work->context);
put_device(fw_work->device); /* taken in request_firmware_nowait() */
@@ -1181,6 +1191,37 @@ static void request_firmware_work_func(struct work_struct *work)
kfree(fw_work);
}
+static int
+__request_firmware_nowait(
+ struct module *module, bool uevent, bool user_helper,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ struct firmware_work *fw_work;
+
+ fw_work = kzalloc(sizeof (struct firmware_work), gfp);
+ if (!fw_work)
+ return -ENOMEM;
+
+ fw_work->module = module;
+ fw_work->name = name;
+ fw_work->device = device;
+ fw_work->context = context;
+ fw_work->cont = cont;
+ fw_work->uevent = uevent;
+ fw_work->user_helper = user_helper;
+
+ if (!try_module_get(module)) {
+ kfree(fw_work);
+ return -EFAULT;
+ }
+
+ get_device(fw_work->device);
+ INIT_WORK(&fw_work->work, request_firmware_work_func);
+ schedule_work(&fw_work->work);
+ return 0;
+}
+
/**
* request_firmware_nowait - asynchronous version of request_firmware
* @module: module requesting the firmware
@@ -1210,31 +1251,24 @@ request_firmware_nowait(
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context))
{
- struct firmware_work *fw_work;
-
- fw_work = kzalloc(sizeof (struct firmware_work), gfp);
- if (!fw_work)
- return -ENOMEM;
-
- fw_work->module = module;
- fw_work->name = name;
- fw_work->device = device;
- fw_work->context = context;
- fw_work->cont = cont;
- fw_work->uevent = uevent;
-
- if (!try_module_get(module)) {
- kfree(fw_work);
- return -EFAULT;
- }
-
- get_device(fw_work->device);
- INIT_WORK(&fw_work->work, request_firmware_work_func);
- schedule_work(&fw_work->work);
- return 0;
+ return __request_firmware_nowait(module, uevent, false, name, device,
+ gfp, context, cont);
}
EXPORT_SYMBOL(request_firmware_nowait);
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+int
+request_firmware_via_user_helper(
+ struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ return __request_firmware_nowait(module, uevent, true, name, device,
+ gfp, context, cont);
+}
+EXPORT_SYMBOL_GPL(request_firmware_via_user_helper);
+#endif
+
#ifdef CONFIG_PM_SLEEP
static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1..e511a96 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -619,7 +619,7 @@ static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
*/
if (!rbu_data.entry_created) {
spin_unlock(&rbu_data.lock);
- req_firm_rc = request_firmware_nowait(THIS_MODULE,
+ req_firm_rc = request_firmware_via_user_helper(THIS_MODULE,
FW_ACTION_NOHOTPLUG, "dell_rbu",
&rbu_device->dev, GFP_KERNEL, &context,
callbackfn_rbu);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index e154c10..d507122 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -46,6 +46,13 @@ int request_firmware_nowait(
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context));
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+int request_firmware_via_user_helper(
+ struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context));
+#endif
+
void release_firmware(const struct firmware *fw);
#else
static inline int request_firmware(const struct firmware **fw,
On Mon, Jul 08, 2013 at 11:05:20AM +0200, Takashi Iwai wrote:
> The reason is that dell_rbu driver requires it. Without the kconfig
> option, this driver won't work at all. So, it's a right fix for
> dell_rbu.
>
> AFAIK, the consensus in the kernel side is that this too long fw
> loading time is basically a regression of user-space (udev or
> whatever). There is no change in the kernel behavior. The problem
> must exist even with the older kernels.
>
> But, looking at the development, we can't expect that udev will be
> fixed soon, and this breakage persists already way too long. Maybe a
> better solution is to kill the fallback to udev for normal f/w loading
> (i.e. for distro kernels).
I thought udev was already fixed for this issue, so why would a "modern"
distro need to worry about this?
> The patch below is an untested quick hack. It adds a new Kconfig and
> a new function request_firmware_via_user_helper(). Distro kernels may
> set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
> stall for non-existing firmware file access -- as distributions know
> that the firmware files should be placed in the right path.
>
> Thoughts?
There's no way we can just fix up the driver instead of doing this in
the firmware core?
thanks,
greg k-h
At Mon, 8 Jul 2013 09:49:33 -0700,
Greg KH wrote:
>
> On Mon, Jul 08, 2013 at 11:05:20AM +0200, Takashi Iwai wrote:
> > The reason is that dell_rbu driver requires it. Without the kconfig
> > option, this driver won't work at all. So, it's a right fix for
> > dell_rbu.
> >
> > AFAIK, the consensus in the kernel side is that this too long fw
> > loading time is basically a regression of user-space (udev or
> > whatever). There is no change in the kernel behavior. The problem
> > must exist even with the older kernels.
> >
> > But, looking at the development, we can't expect that udev will be
> > fixed soon, and this breakage persists already way too long. Maybe a
> > better solution is to kill the fallback to udev for normal f/w loading
> > (i.e. for distro kernels).
>
> I thought udev was already fixed for this issue, so why would a "modern"
> distro need to worry about this?
Hm, I expected Dave is using the udev "modern" enough. If it's about
an old udev behavior, we don't have to care so much...
> > The patch below is an untested quick hack. It adds a new Kconfig and
> > a new function request_firmware_via_user_helper(). Distro kernels may
> > set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
> > stall for non-existing firmware file access -- as distributions know
> > that the firmware files should be placed in the right path.
> >
> > Thoughts?
>
> There's no way we can just fix up the driver instead of doing this in
> the firmware core?
The problem is that this non-udev-hotplug firmware loading mechanism
was introduced exactly for this driver. Thus, we can't change the
driver (thus ABI) without breaking user-space.
Though, there are only two users of this interface. So, once when the
user-space side is fixed, we can easily fix the kernel side, too,
e.g. rewrite these drivers without abusing firmware interface.
Takashi
On Mon, Jul 8, 2013 at 5:05 PM, Takashi Iwai <[email protected]> wrote:
> At Sat, 6 Jul 2013 15:30:03 -0700,
> Greg KH wrote:
>>
>> On Sat, Jul 06, 2013 at 06:14:01PM -0400, Dave Jones wrote:
>> > On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
>> > > Gitweb: http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
>> > > Commit: d05c39ea67f5786a549ac9d672d2951992b658c6
>> > > Parent: a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
>> > > Author: Takashi Iwai <[email protected]>
>> > > AuthorDate: Wed May 22 18:28:37 2013 +0200
>> > > Committer: Greg Kroah-Hartman <[email protected]>
>> > > CommitDate: Mon Jun 3 13:57:29 2013 -0700
>> > >
>> > > dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
>> > >
>> > > The usermode helper is mandatory for this driver.
>> > >
>> > > Signed-off-by: Takashi Iwai <[email protected]>
>> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
>> > > ---
>> > > drivers/firmware/Kconfig | 1 +
>> > > 1 files changed, 1 insertions(+), 0 deletions(-)
>> > >
>> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> > > index 9387630..0747872 100644
>> > > --- a/drivers/firmware/Kconfig
>> > > +++ b/drivers/firmware/Kconfig
>> > > @@ -64,6 +64,7 @@ config DELL_RBU
>> > > tristate "BIOS update support for DELL systems via sysfs"
>> > > depends on X86
>> > > select FW_LOADER
>> > > + select FW_LOADER_USER_HELPER
>> > > help
>> > > Say m if you want to have the option of updating the BIOS for your
>> > > DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
>> >
>> >
>> > This is pretty crappy. Now every distro kernel has to have that enabled,
>> > which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
>> > when this is enabled).
>>
>> I'll let you and Takashi battle this one out, for some reason I thought
>> he added it _because_ a distro kernel needed it...
>
> The reason is that dell_rbu driver requires it. Without the kconfig
> option, this driver won't work at all. So, it's a right fix for
> dell_rbu.
>
> AFAIK, the consensus in the kernel side is that this too long fw
> loading time is basically a regression of user-space (udev or
> whatever). There is no change in the kernel behavior. The problem
> must exist even with the older kernels.
>
> But, looking at the development, we can't expect that udev will be
> fixed soon, and this breakage persists already way too long. Maybe a
> better solution is to kill the fallback to udev for normal f/w loading
> (i.e. for distro kernels).
>
> The patch below is an untested quick hack. It adds a new Kconfig and
> a new function request_firmware_via_user_helper(). Distro kernels may
> set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
I understand your proposed approach is basically equal to unset DELL_RBU,
don't I?
Actually if DELL_RBU is set, FW_LOADER_USER_HELPER is still set, and
it won't avoid the fallback to loading from userspace.
> stall for non-existing firmware file access -- as distributions know
> that the firmware files should be placed in the right path.
>
> Thoughts?
I suggest not to introduce new config options in firmware loader any more,
and the current options are too many already and it is very easy to trigger
build problem, and introduce extra complexity to users at the same time.
About Dave's problem, I think distribution may not trigger it since
cpu microcode
should exist, so I am wondering if Dave can disable DELL_RBU to work around
the problem in his system? Or fake a one byte microcode file to fool the driver?
Also looks the problem should be handled inside x86 microcode driver because
the usage is unique in x86 microcode driver. Other drivers either need one
firmware or stop to work without a firmware, but this driver can work
well no matter
the firmware loading is satisfied or not.
Or could we force dell rbu to use uevent based loading now?
thanks
--
Ming Lei
At Tue, 9 Jul 2013 11:15:20 +0800,
Ming Lei wrote:
>
> On Mon, Jul 8, 2013 at 5:05 PM, Takashi Iwai <[email protected]> wrote:
> > At Sat, 6 Jul 2013 15:30:03 -0700,
> > Greg KH wrote:
> >>
> >> On Sat, Jul 06, 2013 at 06:14:01PM -0400, Dave Jones wrote:
> >> > On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
> >> > > Gitweb: http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
> >> > > Commit: d05c39ea67f5786a549ac9d672d2951992b658c6
> >> > > Parent: a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
> >> > > Author: Takashi Iwai <[email protected]>
> >> > > AuthorDate: Wed May 22 18:28:37 2013 +0200
> >> > > Committer: Greg Kroah-Hartman <[email protected]>
> >> > > CommitDate: Mon Jun 3 13:57:29 2013 -0700
> >> > >
> >> > > dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
> >> > >
> >> > > The usermode helper is mandatory for this driver.
> >> > >
> >> > > Signed-off-by: Takashi Iwai <[email protected]>
> >> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >> > > ---
> >> > > drivers/firmware/Kconfig | 1 +
> >> > > 1 files changed, 1 insertions(+), 0 deletions(-)
> >> > >
> >> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> >> > > index 9387630..0747872 100644
> >> > > --- a/drivers/firmware/Kconfig
> >> > > +++ b/drivers/firmware/Kconfig
> >> > > @@ -64,6 +64,7 @@ config DELL_RBU
> >> > > tristate "BIOS update support for DELL systems via sysfs"
> >> > > depends on X86
> >> > > select FW_LOADER
> >> > > + select FW_LOADER_USER_HELPER
> >> > > help
> >> > > Say m if you want to have the option of updating the BIOS for your
> >> > > DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
> >> >
> >> >
> >> > This is pretty crappy. Now every distro kernel has to have that enabled,
> >> > which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
> >> > when this is enabled).
> >>
> >> I'll let you and Takashi battle this one out, for some reason I thought
> >> he added it _because_ a distro kernel needed it...
> >
> > The reason is that dell_rbu driver requires it. Without the kconfig
> > option, this driver won't work at all. So, it's a right fix for
> > dell_rbu.
> >
> > AFAIK, the consensus in the kernel side is that this too long fw
> > loading time is basically a regression of user-space (udev or
> > whatever). There is no change in the kernel behavior. The problem
> > must exist even with the older kernels.
> >
> > But, looking at the development, we can't expect that udev will be
> > fixed soon, and this breakage persists already way too long. Maybe a
> > better solution is to kill the fallback to udev for normal f/w loading
> > (i.e. for distro kernels).
> >
> > The patch below is an untested quick hack. It adds a new Kconfig and
> > a new function request_firmware_via_user_helper(). Distro kernels may
> > set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
>
> I understand your proposed approach is basically equal to unset DELL_RBU,
> don't I?
>
> Actually if DELL_RBU is set, FW_LOADER_USER_HELPER is still set, and
> it won't avoid the fallback to loading from userspace.
No, it changes the behavior. The fallback is now checked explicitly
via #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK (this is the only
place where this kconfig is referred to). The patch isn't great and
can be rewritten better, but the idea is to split the fallback
behavior (for normal drivers) and the firmware loading that mandates
the user-mode helper (for dell_rbu).
> > stall for non-existing firmware file access -- as distributions know
> > that the firmware files should be placed in the right path.
> >
> > Thoughts?
>
> I suggest not to introduce new config options in firmware loader any more,
> and the current options are too many already and it is very easy to trigger
> build problem, and introduce extra complexity to users at the same time.
Yeah, I hate adding a new kconfig, too. But, in this case, I couldn't
have other idea for solving in a reasonable amount of change.
> About Dave's problem, I think distribution may not trigger it since
> cpu microcode
> should exist,
It doesn't have to exist -- imagine a brand new CPU that is shipped
without errata (I guess it's the case Dave hit).
> so I am wondering if Dave can disable DELL_RBU to work around
> the problem in his system? Or fake a one byte microcode file to fool the driver?
Well, the kernel cannot know whether the microcode f/w exists or not
beforehand. It needs to probe. And the probing itself stalls for 60
seconds...
> Also looks the problem should be handled inside x86 microcode driver because
> the usage is unique in x86 microcode driver. Other drivers either need one
> firmware or stop to work without a firmware, but this driver can work
> well no matter
> the firmware loading is satisfied or not.
Not really specific to microcode driver. Other drivers work without
the inquired firmware file. For example, iwlwifi has a fallback
mechanism to fetch the old version of firmware. If each probe of
non-existing firmware file stalls for 60 seconds, it may take really
long.
> Or could we force dell rbu to use uevent based loading now?
.... only if we break dell_rbu user-space behavior.
So, our options are:
A. Ignore, with a hope that udev is/will be fixed.
B. Break dell_rbu, rewrite dell_rbu driver code and user-space at the
same time
C. Split the user-space fallback and the mandatory user-space loading
(as my patch does)
Takashi
On Tue, Jul 9, 2013 at 1:33 PM, Takashi Iwai <[email protected]> wrote:
> At Tue, 9 Jul 2013 11:15:20 +0800,
> Ming Lei wrote:
>>
>> On Mon, Jul 8, 2013 at 5:05 PM, Takashi Iwai <[email protected]> wrote:
>> > At Sat, 6 Jul 2013 15:30:03 -0700,
>> > Greg KH wrote:
>> >>
>> >> On Sat, Jul 06, 2013 at 06:14:01PM -0400, Dave Jones wrote:
>> >> > On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
>> >> > > Gitweb: http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
>> >> > > Commit: d05c39ea67f5786a549ac9d672d2951992b658c6
>> >> > > Parent: a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
>> >> > > Author: Takashi Iwai <[email protected]>
>> >> > > AuthorDate: Wed May 22 18:28:37 2013 +0200
>> >> > > Committer: Greg Kroah-Hartman <[email protected]>
>> >> > > CommitDate: Mon Jun 3 13:57:29 2013 -0700
>> >> > >
>> >> > > dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
>> >> > >
>> >> > > The usermode helper is mandatory for this driver.
>> >> > >
>> >> > > Signed-off-by: Takashi Iwai <[email protected]>
>> >> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
>> >> > > ---
>> >> > > drivers/firmware/Kconfig | 1 +
>> >> > > 1 files changed, 1 insertions(+), 0 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> >> > > index 9387630..0747872 100644
>> >> > > --- a/drivers/firmware/Kconfig
>> >> > > +++ b/drivers/firmware/Kconfig
>> >> > > @@ -64,6 +64,7 @@ config DELL_RBU
>> >> > > tristate "BIOS update support for DELL systems via sysfs"
>> >> > > depends on X86
>> >> > > select FW_LOADER
>> >> > > + select FW_LOADER_USER_HELPER
>> >> > > help
>> >> > > Say m if you want to have the option of updating the BIOS for your
>> >> > > DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
>> >> >
>> >> >
>> >> > This is pretty crappy. Now every distro kernel has to have that enabled,
>> >> > which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
>> >> > when this is enabled).
>> >>
>> >> I'll let you and Takashi battle this one out, for some reason I thought
>> >> he added it _because_ a distro kernel needed it...
>> >
>> > The reason is that dell_rbu driver requires it. Without the kconfig
>> > option, this driver won't work at all. So, it's a right fix for
>> > dell_rbu.
>> >
>> > AFAIK, the consensus in the kernel side is that this too long fw
>> > loading time is basically a regression of user-space (udev or
>> > whatever). There is no change in the kernel behavior. The problem
>> > must exist even with the older kernels.
>> >
>> > But, looking at the development, we can't expect that udev will be
>> > fixed soon, and this breakage persists already way too long. Maybe a
>> > better solution is to kill the fallback to udev for normal f/w loading
>> > (i.e. for distro kernels).
>> >
>> > The patch below is an untested quick hack. It adds a new Kconfig and
>> > a new function request_firmware_via_user_helper(). Distro kernels may
>> > set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
>>
>> I understand your proposed approach is basically equal to unset DELL_RBU,
>> don't I?
>>
>> Actually if DELL_RBU is set, FW_LOADER_USER_HELPER is still set, and
>> it won't avoid the fallback to loading from userspace.
>
> No, it changes the behavior. The fallback is now checked explicitly
> via #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK (this is the only
> place where this kconfig is referred to). The patch isn't great and
> can be rewritten better, but the idea is to split the fallback
> behavior (for normal drivers) and the firmware loading that mandates
> the user-mode helper (for dell_rbu).
Right, sorry for missing that.
>
>
>> > stall for non-existing firmware file access -- as distributions know
>> > that the firmware files should be placed in the right path.
>> >
>> > Thoughts?
>>
>> I suggest not to introduce new config options in firmware loader any more,
>> and the current options are too many already and it is very easy to trigger
>> build problem, and introduce extra complexity to users at the same time.
>
> Yeah, I hate adding a new kconfig, too. But, in this case, I couldn't
> have other idea for solving in a reasonable amount of change.
One approach I thought of is to introduce request_firmware_user()
which should be used only in FW_ACTION_NOHOTPLUG situation,
but allows CONFIG_FW_LOADER_USER_HELPER disabled.
Actually from view of distribution, dell rbu has to be compiled in, then
no code size can be saved any more, which means looks the introduced
option in this patch isn't necessary.
>
>> About Dave's problem, I think distribution may not trigger it since
>> cpu microcode
>> should exist,
>
> It doesn't have to exist -- imagine a brand new CPU that is shipped
> without errata (I guess it's the case Dave hit).
Yes, I mean other drivers(or most of drivers) may have not the situation
to be afraid of.
>
>> so I am wondering if Dave can disable DELL_RBU to work around
>> the problem in his system? Or fake a one byte microcode file to fool the driver?
>
> Well, the kernel cannot know whether the microcode f/w exists or not
> beforehand. It needs to probe. And the probing itself stalls for 60
> seconds...
The driver can use request_firmware_no_wait() to work around it too...
>
>> Also looks the problem should be handled inside x86 microcode driver because
>> the usage is unique in x86 microcode driver. Other drivers either need one
>> firmware or stop to work without a firmware, but this driver can work
>> well no matter
>> the firmware loading is satisfied or not.
>
> Not really specific to microcode driver. Other drivers work without
> the inquired firmware file. For example, iwlwifi has a fallback
> mechanism to fetch the old version of firmware. If each probe of
> non-existing firmware file stalls for 60 seconds, it may take really
> long.
I am wondering why people didn't complain the loading in iwlwifi, :-)
>
>> Or could we force dell rbu to use uevent based loading now?
>
> .... only if we break dell_rbu user-space behavior.
>
> So, our options are:
>
> A. Ignore, with a hope that udev is/will be fixed.
>
> B. Break dell_rbu, rewrite dell_rbu driver code and user-space at the
> same time
>
> C. Split the user-space fallback and the mandatory user-space loading
> (as my patch does)
Thanks,
--
Ming Lei
At Tue, 9 Jul 2013 16:43:30 +0800,
Ming Lei wrote:
>
> On Tue, Jul 9, 2013 at 1:33 PM, Takashi Iwai <[email protected]> wrote:
> > At Tue, 9 Jul 2013 11:15:20 +0800,
> > Ming Lei wrote:
> >>
> >> On Mon, Jul 8, 2013 at 5:05 PM, Takashi Iwai <[email protected]> wrote:
> >> > At Sat, 6 Jul 2013 15:30:03 -0700,
> >> > Greg KH wrote:
> >> >>
> >> >> On Sat, Jul 06, 2013 at 06:14:01PM -0400, Dave Jones wrote:
> >> >> > On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
> >> >> > > Gitweb: http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
> >> >> > > Commit: d05c39ea67f5786a549ac9d672d2951992b658c6
> >> >> > > Parent: a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
> >> >> > > Author: Takashi Iwai <[email protected]>
> >> >> > > AuthorDate: Wed May 22 18:28:37 2013 +0200
> >> >> > > Committer: Greg Kroah-Hartman <[email protected]>
> >> >> > > CommitDate: Mon Jun 3 13:57:29 2013 -0700
> >> >> > >
> >> >> > > dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
> >> >> > >
> >> >> > > The usermode helper is mandatory for this driver.
> >> >> > >
> >> >> > > Signed-off-by: Takashi Iwai <[email protected]>
> >> >> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >> >> > > ---
> >> >> > > drivers/firmware/Kconfig | 1 +
> >> >> > > 1 files changed, 1 insertions(+), 0 deletions(-)
> >> >> > >
> >> >> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> >> >> > > index 9387630..0747872 100644
> >> >> > > --- a/drivers/firmware/Kconfig
> >> >> > > +++ b/drivers/firmware/Kconfig
> >> >> > > @@ -64,6 +64,7 @@ config DELL_RBU
> >> >> > > tristate "BIOS update support for DELL systems via sysfs"
> >> >> > > depends on X86
> >> >> > > select FW_LOADER
> >> >> > > + select FW_LOADER_USER_HELPER
> >> >> > > help
> >> >> > > Say m if you want to have the option of updating the BIOS for your
> >> >> > > DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
> >> >> >
> >> >> >
> >> >> > This is pretty crappy. Now every distro kernel has to have that enabled,
> >> >> > which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
> >> >> > when this is enabled).
> >> >>
> >> >> I'll let you and Takashi battle this one out, for some reason I thought
> >> >> he added it _because_ a distro kernel needed it...
> >> >
> >> > The reason is that dell_rbu driver requires it. Without the kconfig
> >> > option, this driver won't work at all. So, it's a right fix for
> >> > dell_rbu.
> >> >
> >> > AFAIK, the consensus in the kernel side is that this too long fw
> >> > loading time is basically a regression of user-space (udev or
> >> > whatever). There is no change in the kernel behavior. The problem
> >> > must exist even with the older kernels.
> >> >
> >> > But, looking at the development, we can't expect that udev will be
> >> > fixed soon, and this breakage persists already way too long. Maybe a
> >> > better solution is to kill the fallback to udev for normal f/w loading
> >> > (i.e. for distro kernels).
> >> >
> >> > The patch below is an untested quick hack. It adds a new Kconfig and
> >> > a new function request_firmware_via_user_helper(). Distro kernels may
> >> > set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
> >>
> >> I understand your proposed approach is basically equal to unset DELL_RBU,
> >> don't I?
> >>
> >> Actually if DELL_RBU is set, FW_LOADER_USER_HELPER is still set, and
> >> it won't avoid the fallback to loading from userspace.
> >
> > No, it changes the behavior. The fallback is now checked explicitly
> > via #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK (this is the only
> > place where this kconfig is referred to). The patch isn't great and
> > can be rewritten better, but the idea is to split the fallback
> > behavior (for normal drivers) and the firmware loading that mandates
> > the user-mode helper (for dell_rbu).
>
> Right, sorry for missing that.
>
> >
> >
> >> > stall for non-existing firmware file access -- as distributions know
> >> > that the firmware files should be placed in the right path.
> >> >
> >> > Thoughts?
> >>
> >> I suggest not to introduce new config options in firmware loader any more,
> >> and the current options are too many already and it is very easy to trigger
> >> build problem, and introduce extra complexity to users at the same time.
> >
> > Yeah, I hate adding a new kconfig, too. But, in this case, I couldn't
> > have other idea for solving in a reasonable amount of change.
>
> One approach I thought of is to introduce request_firmware_user()
> which should be used only in FW_ACTION_NOHOTPLUG situation,
> but allows CONFIG_FW_LOADER_USER_HELPER disabled.
>
> Actually from view of distribution, dell rbu has to be compiled in, then
> no code size can be saved any more, which means looks the introduced
> option in this patch isn't necessary.
Well, but it means that there is no way not to compile user-helper
mode code in firmware_class.c. In other words, in which condition,
request_firmware_user() will be compiled?
> >> About Dave's problem, I think distribution may not trigger it since
> >> cpu microcode
> >> should exist,
> >
> > It doesn't have to exist -- imagine a brand new CPU that is shipped
> > without errata (I guess it's the case Dave hit).
>
> Yes, I mean other drivers(or most of drivers) may have not the situation
> to be afraid of.
>
> >
> >> so I am wondering if Dave can disable DELL_RBU to work around
> >> the problem in his system? Or fake a one byte microcode file to fool the driver?
> >
> > Well, the kernel cannot know whether the microcode f/w exists or not
> > beforehand. It needs to probe. And the probing itself stalls for 60
> > seconds...
>
> The driver can use request_firmware_no_wait() to work around it too...
Yes, but it sucks :)
> >> Also looks the problem should be handled inside x86 microcode driver because
> >> the usage is unique in x86 microcode driver. Other drivers either need one
> >> firmware or stop to work without a firmware, but this driver can work
> >> well no matter
> >> the firmware loading is satisfied or not.
> >
> > Not really specific to microcode driver. Other drivers work without
> > the inquired firmware file. For example, iwlwifi has a fallback
> > mechanism to fetch the old version of firmware. If each probe of
> > non-existing firmware file stalls for 60 seconds, it may take really
> > long.
>
> I am wondering why people didn't complain the loading in iwlwifi, :-)
Maybe just because people update kernel-firmware together with the
kernel itself. Or, it happens with a certain udev like Fedora.
I haven't seen 60 seconds delay on openSUSE, for example.
Takashi
> >
> >> Or could we force dell rbu to use uevent based loading now?
> >
> > .... only if we break dell_rbu user-space behavior.
> >
> > So, our options are:
> >
> > A. Ignore, with a hope that udev is/will be fixed.
> >
> > B. Break dell_rbu, rewrite dell_rbu driver code and user-space at the
> > same time
> >
> > C. Split the user-space fallback and the mandatory user-space loading
> > (as my patch does)
>
>
>
> Thanks,
> --
> Ming Lei
>
On Tue, Jul 9, 2013 at 9:32 PM, Takashi Iwai <[email protected]> wrote:
> At Tue, 9 Jul 2013 16:43:30 +0800,
>> Actually from view of distribution, dell rbu has to be compiled in, then
>> no code size can be saved any more, which means looks the introduced
>> option in this patch isn't necessary.
>
> Well, but it means that there is no way not to compile user-helper
> mode code in firmware_class.c. In other words, in which condition,
> request_firmware_user() will be compiled?
It can be always compiled in. Basically dell rbu is always enabled in
distributions, which means the user helper code has to be compiled in.
Thanks,
--
Ming Lei
At Tue, 9 Jul 2013 22:39:36 +0800,
Ming Lei wrote:
>
> On Tue, Jul 9, 2013 at 9:32 PM, Takashi Iwai <[email protected]> wrote:
> > At Tue, 9 Jul 2013 16:43:30 +0800,
> >> Actually from view of distribution, dell rbu has to be compiled in, then
> >> no code size can be saved any more, which means looks the introduced
> >> option in this patch isn't necessary.
> >
> > Well, but it means that there is no way not to compile user-helper
> > mode code in firmware_class.c. In other words, in which condition,
> > request_firmware_user() will be compiled?
>
> It can be always compiled in. Basically dell rbu is always enabled in
> distributions, which means the user helper code has to be compiled in.
But how non-distro kernel disables this being built-in...?
Or do you suggest to enable the user-helper codes always built, no
matter whether it's used or not?
Takashi
On Tue, Jul 9, 2013 at 10:45 PM, Takashi Iwai <[email protected]> wrote:
> At Tue, 9 Jul 2013 22:39:36 +0800,
> Ming Lei wrote:
>>
>> On Tue, Jul 9, 2013 at 9:32 PM, Takashi Iwai <[email protected]> wrote:
>> > At Tue, 9 Jul 2013 16:43:30 +0800,
>> >> Actually from view of distribution, dell rbu has to be compiled in, then
>> >> no code size can be saved any more, which means looks the introduced
>> >> option in this patch isn't necessary.
>> >
>> > Well, but it means that there is no way not to compile user-helper
>> > mode code in firmware_class.c. In other words, in which condition,
>> > request_firmware_user() will be compiled?
>>
>> It can be always compiled in. Basically dell rbu is always enabled in
>> distributions, which means the user helper code has to be compiled in.
>
> But how non-distro kernel disables this being built-in...?
> Or do you suggest to enable the user-helper codes always built, no
> matter whether it's used or not?
The only cost of built-in user helper code is ~6Kbytes code on my armv7
box.
If the 6KB memory does mater for this non-distro kernel, we can introduce
more options.
Thanks,
--
Ming Lei
At Tue, 9 Jul 2013 23:06:05 +0800,
Ming Lei wrote:
>
> On Tue, Jul 9, 2013 at 10:45 PM, Takashi Iwai <[email protected]> wrote:
> > At Tue, 9 Jul 2013 22:39:36 +0800,
> > Ming Lei wrote:
> >>
> >> On Tue, Jul 9, 2013 at 9:32 PM, Takashi Iwai <[email protected]> wrote:
> >> > At Tue, 9 Jul 2013 16:43:30 +0800,
> >> >> Actually from view of distribution, dell rbu has to be compiled in, then
> >> >> no code size can be saved any more, which means looks the introduced
> >> >> option in this patch isn't necessary.
> >> >
> >> > Well, but it means that there is no way not to compile user-helper
> >> > mode code in firmware_class.c. In other words, in which condition,
> >> > request_firmware_user() will be compiled?
> >>
> >> It can be always compiled in. Basically dell rbu is always enabled in
> >> distributions, which means the user helper code has to be compiled in.
> >
> > But how non-distro kernel disables this being built-in...?
> > Or do you suggest to enable the user-helper codes always built, no
> > matter whether it's used or not?
>
> The only cost of built-in user helper code is ~6Kbytes code on my armv7
> box.
>
> If the 6KB memory does mater for this non-distro kernel, we can introduce
> more options.
Well, if it doesn't matter, we don't need CONFIG_FW_LOADER_USER_HELPER
either. The goal of this option is to kill the user helper code, not
intended as a workaround for 60 seconds stall.
Takashi
On Tue, Jul 9, 2013 at 11:13 PM, Takashi Iwai <[email protected]> wrote:
>>
>> The only cost of built-in user helper code is ~6Kbytes code on my armv7
>> box.
>>
>> If the 6KB memory does mater for this non-distro kernel, we can introduce
>> more options.
>
> Well, if it doesn't matter, we don't need CONFIG_FW_LOADER_USER_HELPER
> either. The goal of this option is to kill the user helper code, not
> intended as a workaround for 60 seconds stall.
Actually, the Kconfig help is "Fallback user-helper invocation for
firmware loading",
so looks the description in help doc isn't consistent with what you wanted, :-)
Thanks,
--
Ming Lei
At Tue, 9 Jul 2013 23:22:33 +0800,
Ming Lei wrote:
>
> On Tue, Jul 9, 2013 at 11:13 PM, Takashi Iwai <[email protected]> wrote:
> >>
> >> The only cost of built-in user helper code is ~6Kbytes code on my armv7
> >> box.
> >>
> >> If the 6KB memory does mater for this non-distro kernel, we can introduce
> >> more options.
> >
> > Well, if it doesn't matter, we don't need CONFIG_FW_LOADER_USER_HELPER
> > either. The goal of this option is to kill the user helper code, not
> > intended as a workaround for 60 seconds stall.
>
> Actually, the Kconfig help is "Fallback user-helper invocation for
> firmware loading",
> so looks the description in help doc isn't consistent with what you wanted, :-)
There is always a secret plan for world conquer :)
Takashi