Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752654Ab3GHJEY (ORCPT ); Mon, 8 Jul 2013 05:04:24 -0400 Received: from cantor2.suse.de ([195.135.220.15]:49839 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751470Ab3GHJEQ (ORCPT ); Mon, 8 Jul 2013 05:04:16 -0400 Date: Mon, 08 Jul 2013 11:05:20 +0200 Message-ID: From: Takashi Iwai To: Greg KH Cc: Dave Jones , Ming Lei , Linux Kernel Mailing List Subject: Re: dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly In-Reply-To: <20130706223003.GA10697@kroah.com> References: <20130702192749.A2E7E660D9B@gitolite.kernel.org> <20130706221401.GA3652@redhat.com> <20130706223003.GA10697@kroah.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9490 Lines: 269 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 > > > AuthorDate: Wed May 22 18:28:37 2013 +0200 > > > Committer: Greg Kroah-Hartman > > > 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 > > > Signed-off-by: Greg Kroah-Hartman > > > --- > > > 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, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/