Re: dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly

[Takashi Iwai]

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 <tiwai@xxxxxxx>
> >  > AuthorDate: Wed May 22 18:28:37 2013 +0200
> >  > Committer:  Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >  > 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 <tiwai@xxxxxxx>
> >  >     Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >  > ---
> >  >  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/