[PATCH v2 0/3] Add request_firmware_direct() for microcode loader

[PATCH v2 0/3] Add request_firmware_direct() for microcode loader

[Takashi Iwai]

Hi,

this is a revised patch series to introduce request_firmware_direct()
helper for avoiding the lengthy udev issue on microcode loader.
The original problem was stated in Prarit's post:
   https://lkml.org/lkml/2013/10/28/221

In short, microcode loader probes non-existing firmware files (which
are cases with every new chip), and each probe takes 60 seconds,
resulting in too long time until completed.

This solution is simply avoiding the udev fallback in
request_firmware() explicitly for drivers like microcode.

 [PATCH v2 1/3] firmware: Introduce request_firmware_direct()
 [PATCH v2 2/3] microcode: Use request_firmware_direct()
 [PATCH v2 3/3] firmware: Avoid bogus fallback warning

Of course, this doesn't mean to throw away further optimizations like
Prarit's patch.  It can be implemented in parallel with this.


v1->v2: Rebased on linux-next, add a fix for a bogus warning message


thanks,

Takashi

[PATCH v2 1/3] firmware: Introduce request_firmware_direct()

[Takashi Iwai]

When CONFIG_FW_LOADER_USER_HELPER is set, request_firmware() falls
back to the usermode helper for loading via udev when the direct
loading fails.  But the recent udev takes way too long timeout (60
seconds) for non-existing firmware.  This is unacceptable for the
drivers like microcode loader where they load firmwares optionally,
i.e. it's no error even if no requested file exists.

This patch provides a new helper function, request_firmware_direct().
It behaves as same as request_firmware() except for that it doesn't
fall back to usermode helper but returns an error immediately if the
f/w can't be loaded directly in kernel.

Without CONFIG_FW_LOADER_USER_HELPER=y, request_firmware_direct() is
just an alias of request_firmware(), due to obvious reason.

Tested-by: Prarit Bhargava <prarit@redhat.com>
Acked-by: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 36 +++++++++++++++++++++++++++++++-----
 include/linux/firmware.h      |  7 +++++++
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index eb8fb94ae2c5..7f48a6ffb0df 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1061,7 +1061,7 @@ 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 fallback)
 {
 	struct firmware *fw;
 	long timeout;
@@ -1097,9 +1097,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret) {
 		dev_warn(device, "Direct firmware load failed with error %d\n",
 			 ret);
-		dev_warn(device, "Falling back to user helper\n");
-		ret = fw_load_from_user_helper(fw, name, device,
+		if (fallback) {
+			dev_warn(device, "Falling back to user helper\n");
+			ret = fw_load_from_user_helper(fw, name, device,
 					       uevent, nowait, timeout);
+		}
 	}
 
 	/* don't cache firmware handled without uevent */
@@ -1146,12 +1148,36 @@ 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, true);
 	module_put(THIS_MODULE);
 	return ret;
 }
 EXPORT_SYMBOL(request_firmware);
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+/**
+ * request_firmware: - load firmware directly without usermode helper
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but this doesn't
+ * fall back to usermode helper even if the firmware couldn't be loaded
+ * directly from fs.  Hence it's useful for loading optional firmwares, which
+ * aren't always present, without extra long timeouts of udev.
+ **/
+int request_firmware_direct(const struct firmware **firmware_p,
+			    const char *name, struct device *device)
+{
+	int ret;
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, true, false, false);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_direct);
+#endif
+
 /**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
@@ -1185,7 +1211,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, true);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index e154c1005cd1..59529330efd6 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -68,4 +68,11 @@ static inline void release_firmware(const struct firmware *fw)
 
 #endif
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+int request_firmware_direct(const struct firmware **fw, const char *name,
+			    struct device *device);
+#else
+#define request_firmware_direct	request_firmware
+#endif
+
 #endif
-- 
1.8.4.2

[PATCH v2 2/3] microcode: Use request_firmware_direct()

[Takashi Iwai]

Use the new helper, request_firmware_direct(), for avoiding the
lengthy timeout of non-existing firmware loads.  Especially the Intel
microcode driver suffers from this problem because each CPU triggers
the f/w loading, thus it ends up taking (literally) hours with many
cores.

Tested-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 arch/x86/kernel/microcode_amd.c   | 2 +-
 arch/x86/kernel/microcode_intel.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index af99f71aeb7f..539a01a91210 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -430,7 +430,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	if (c->x86 >= 0x15)
 		snprintf(fw_name, sizeof(fw_name), "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
 
-	if (request_firmware(&fw, (const char *)fw_name, device)) {
+	if (request_firmware_direct(&fw, (const char *)fw_name, device)) {
 		pr_err("failed to load file %s\n", fw_name);
 		goto out;
 	}
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 5fb2cebf556b..a276fa75d9b5 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -278,7 +278,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 	sprintf(name, "intel-ucode/%02x-%02x-%02x",
 		c->x86, c->x86_model, c->x86_mask);
 
-	if (request_firmware(&firmware, name, device)) {
+	if (request_firmware_direct(&firmware, name, device)) {
 		pr_debug("data file %s load failed\n", name);
 		return UCODE_NFOUND;
 	}
-- 
1.8.4.2

[PATCH v2 3/3] firmware: Avoid bogus fallback warning

[Takashi Iwai]

The commit [3e358ac2bb5b: firmware: Be a bit more verbose about direct
firmware loading failure] introduced a new warning message about
falling back to user helper, but this isn't true when
CONFIG_FW_LOADER_USER_HELPER isn't set.

For avoiding the confusion, add a proper ifdef.  And now we can remove
the dummy fw_load_from_user_helper(), too, since it's no longer called
with CONFIG_FW_LOADER_USER_HELPER=n.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7f48a6ffb0df..bb03c71bd94d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -940,14 +940,6 @@ static void kill_requests_without_uevent(void)
 #endif
 
 #else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int
-fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, bool uevent, bool nowait,
-			 long timeout)
-{
-	return -ENOENT;
-}
-
 /* No abort during direct loading */
 #define is_fw_load_aborted(buf) false
 
@@ -1097,11 +1089,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret) {
 		dev_warn(device, "Direct firmware load failed with error %d\n",
 			 ret);
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 		if (fallback) {
 			dev_warn(device, "Falling back to user helper\n");
 			ret = fw_load_from_user_helper(fw, name, device,
 					       uevent, nowait, timeout);
 		}
+#endif
 	}
 
 	/* don't cache firmware handled without uevent */
-- 
1.8.4.2

Re: [PATCH v2 1/3] firmware: Introduce request_firmware_direct()

[Borislav Petkov]

On Mon, Nov 11, 2013 at 04:21:16PM +0100, Takashi Iwai wrote:
> When CONFIG_FW_LOADER_USER_HELPER is set, request_firmware() falls
> back to the usermode helper for loading via udev when the direct
> loading fails.  But the recent udev takes way too long timeout (60
> seconds) for non-existing firmware.  This is unacceptable for the
> drivers like microcode loader where they load firmwares optionally,
> i.e. it's no error even if no requested file exists.
> 
> This patch provides a new helper function, request_firmware_direct().
> It behaves as same as request_firmware() except for that it doesn't
> fall back to usermode helper but returns an error immediately if the
> f/w can't be loaded directly in kernel.
> 
> Without CONFIG_FW_LOADER_USER_HELPER=y, request_firmware_direct() is
> just an alias of request_firmware(), due to obvious reason.
> 
> Tested-by: Prarit Bhargava <prarit@redhat.com>
> Acked-by: Ming Lei <ming.lei@canonical.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/firmware_class.c | 36 +++++++++++++++++++++++++++++++-----
>  include/linux/firmware.h      |  7 +++++++
>  2 files changed, 38 insertions(+), 5 deletions(-)

I like it, the 60 seconds thing has been a senseless PITA for no good
reason. I have always wondered what might change in 60 seconds wrt to us
being able to load the firmware...

> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index eb8fb94ae2c5..7f48a6ffb0df 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1061,7 +1061,7 @@ 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 fallback)

Just a nitpick: three boolean args in a row starts to slowly look like a
function from the windoze API. Can we do:

_request_firmware(..., unsigned long flags)

instead and have nice bit flags for that?

That could be a nice cleanup ontop though. Other than that:

Acked-by: Borislav Petkov <bp@suse.de>

for all three.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v2 1/3] firmware: Introduce request_firmware_direct()

[Takashi Iwai]

At Mon, 11 Nov 2013 16:34:26 +0100,
Borislav Petkov wrote:
> 
> On Mon, Nov 11, 2013 at 04:21:16PM +0100, Takashi Iwai wrote:
> > When CONFIG_FW_LOADER_USER_HELPER is set, request_firmware() falls
> > back to the usermode helper for loading via udev when the direct
> > loading fails.  But the recent udev takes way too long timeout (60
> > seconds) for non-existing firmware.  This is unacceptable for the
> > drivers like microcode loader where they load firmwares optionally,
> > i.e. it's no error even if no requested file exists.
> > 
> > This patch provides a new helper function, request_firmware_direct().
> > It behaves as same as request_firmware() except for that it doesn't
> > fall back to usermode helper but returns an error immediately if the
> > f/w can't be loaded directly in kernel.
> > 
> > Without CONFIG_FW_LOADER_USER_HELPER=y, request_firmware_direct() is
> > just an alias of request_firmware(), due to obvious reason.
> > 
> > Tested-by: Prarit Bhargava <prarit@redhat.com>
> > Acked-by: Ming Lei <ming.lei@canonical.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/firmware_class.c | 36 +++++++++++++++++++++++++++++++-----
> >  include/linux/firmware.h      |  7 +++++++
> >  2 files changed, 38 insertions(+), 5 deletions(-)
> 
> I like it, the 60 seconds thing has been a senseless PITA for no good
> reason. I have always wondered what might change in 60 seconds wrt to us
> being able to load the firmware...
> 
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index eb8fb94ae2c5..7f48a6ffb0df 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -1061,7 +1061,7 @@ 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 fallback)
> 
> Just a nitpick: three boolean args in a row starts to slowly look like a
> function from the windoze API. Can we do:
> 
> _request_firmware(..., unsigned long flags)
> 
> instead and have nice bit flags for that?

Sounds like a good idea.  How about the patch below?
(I used unsigned int since there shouldn't be so many different
 behaviors.)


thanks,

Takashi

===

From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] firmware: Use bit flags instead of boolean combos

More than two boolean arguments to a function are rather confusing and
error-prone for callers.  Let's make the behavior bit flags instead of
triple combos.

A nice suggestion by Borislav Petkov.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 49 ++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index bb03c71bd94d..a4eaa7b82882 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -96,6 +96,11 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
 }
 
+/* firmware behavior options */
+#define FW_OPT_UEVENT	(1U << 0)
+#define FW_OPT_NOWAIT	(1U << 1)
+#define FW_OPT_FALLBACK	(1U << 2)
+
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -820,7 +825,7 @@ static void firmware_class_timeout_work(struct work_struct *work)
 
 static struct firmware_priv *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
-		   struct device *device, bool uevent, bool nowait)
+		   struct device *device, unsigned int opt_flags)
 {
 	struct firmware_priv *fw_priv;
 	struct device *f_dev;
@@ -832,7 +837,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 		goto exit;
 	}
 
-	fw_priv->nowait = nowait;
+	fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
 	fw_priv->fw = firmware;
 	INIT_DELAYED_WORK(&fw_priv->timeout_work,
 		firmware_class_timeout_work);
@@ -848,8 +853,8 @@ exit:
 }
 
 /* load a firmware via user helper */
-static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
-				  long timeout)
+static int _request_firmware_load(struct firmware_priv *fw_priv,
+				  unsigned int opt_flags, long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
@@ -885,7 +890,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 		goto err_del_bin_attr;
 	}
 
-	if (uevent) {
+	if (opt_flags & FW_OPT_UEVENT) {
 		buf->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
 		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
@@ -911,16 +916,16 @@ err_put_dev:
 
 static int fw_load_from_user_helper(struct firmware *firmware,
 				    const char *name, struct device *device,
-				    bool uevent, bool nowait, long timeout)
+				    unsigned int opt_flags, long timeout)
 {
 	struct firmware_priv *fw_priv;
 
-	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
+	fw_priv = fw_create_instance(firmware, name, device, opt_flags);
 	if (IS_ERR(fw_priv))
 		return PTR_ERR(fw_priv);
 
 	fw_priv->buf = firmware->priv;
-	return _request_firmware_load(fw_priv, uevent, timeout);
+	return _request_firmware_load(fw_priv, opt_flags, timeout);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1015,7 +1020,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 }
 
 static int assign_firmware_buf(struct firmware *fw, struct device *device,
-				bool skip_cache)
+			       unsigned int opt_flags)
 {
 	struct firmware_buf *buf = fw->priv;
 
@@ -1032,7 +1037,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	 * device may has been deleted already, but the problem
 	 * should be fixed in devres or driver core.
 	 */
-	if (device && !skip_cache)
+	/* don't cache firmware handled without uevent */
+	if (device && (opt_flags & FW_OPT_UEVENT))
 		fw_add_devm_name(device, buf->fw_id);
 
 	/*
@@ -1053,7 +1059,7 @@ 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, bool fallback)
+		  struct device *device, unsigned int opt_flags)
 {
 	struct firmware *fw;
 	long timeout;
@@ -1068,7 +1074,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 
 	ret = 0;
 	timeout = firmware_loading_timeout();
-	if (nowait) {
+	if (opt_flags & FW_OPT_NOWAIT) {
 		timeout = usermodehelper_read_lock_wait(timeout);
 		if (!timeout) {
 			dev_dbg(device, "firmware: %s loading timed out\n",
@@ -1090,17 +1096,16 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		dev_warn(device, "Direct firmware load failed with error %d\n",
 			 ret);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-		if (fallback) {
+		if (opt_flags & FW_OPT_FALLBACK) {
 			dev_warn(device, "Falling back to user helper\n");
 			ret = fw_load_from_user_helper(fw, name, device,
-					       uevent, nowait, timeout);
+						       opt_flags, timeout);
 		}
 #endif
 	}
 
-	/* don't cache firmware handled without uevent */
 	if (!ret)
-		ret = assign_firmware_buf(fw, device, !uevent);
+		ret = assign_firmware_buf(fw, device, opt_flags);
 
 	usermodehelper_read_unlock();
 
@@ -1142,7 +1147,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, true, false, true);
+	ret = _request_firmware(firmware_p, name, device,
+				FW_OPT_UEVENT | FW_OPT_FALLBACK);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1165,7 +1171,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
 {
 	int ret;
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, true, false, false);
+	ret = _request_firmware(firmware_p, name, device, FW_OPT_UEVENT);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1194,7 +1200,7 @@ struct firmware_work {
 	struct device *device;
 	void *context;
 	void (*cont)(const struct firmware *fw, void *context);
-	bool uevent;
+	unsigned int opt_flags;
 };
 
 static void request_firmware_work_func(struct work_struct *work)
@@ -1205,7 +1211,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, true);
+			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
@@ -1253,7 +1259,8 @@ request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->uevent = uevent;
+	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
+		(uevent ? FW_OPT_UEVENT : 0);
 
 	if (!try_module_get(module)) {
 		kfree(fw_work);
-- 
1.8.4.2

Re: [PATCH v2 1/3] firmware: Introduce request_firmware_direct()

[Borislav Petkov]

On Mon, Nov 11, 2013 at 06:30:08PM +0100, Takashi Iwai wrote:
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] firmware: Use bit flags instead of boolean combos
> 
> More than two boolean arguments to a function are rather confusing and
> error-prone for callers.  Let's make the behavior bit flags instead of
> triple combos.
> 
> A nice suggestion by Borislav Petkov.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/firmware_class.c | 49 ++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)

Yeah, looks good, thanks!

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v2 1/3] firmware: Introduce request_firmware_direct()

[Prarit Bhargava]

On 11/11/2013 12:30 PM, Takashi Iwai wrote:
> At Mon, 11 Nov 2013 16:34:26 +0100,
> Borislav Petkov wrote:
>>
>> On Mon, Nov 11, 2013 at 04:21:16PM +0100, Takashi Iwai wrote:
>>> When CONFIG_FW_LOADER_USER_HELPER is set, request_firmware() falls
>>> back to the usermode helper for loading via udev when the direct
>>> loading fails.  But the recent udev takes way too long timeout (60
>>> seconds) for non-existing firmware.  This is unacceptable for the
>>> drivers like microcode loader where they load firmwares optionally,
>>> i.e. it's no error even if no requested file exists.
>>>
>>> This patch provides a new helper function, request_firmware_direct().
>>> It behaves as same as request_firmware() except for that it doesn't
>>> fall back to usermode helper but returns an error immediately if the
>>> f/w can't be loaded directly in kernel.
>>>
>>> Without CONFIG_FW_LOADER_USER_HELPER=y, request_firmware_direct() is
>>> just an alias of request_firmware(), due to obvious reason.
>>>
>>> Tested-by: Prarit Bhargava <prarit@redhat.com>
>>> Acked-by: Ming Lei <ming.lei@canonical.com>
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>> ---
>>>  drivers/base/firmware_class.c | 36 +++++++++++++++++++++++++++++++-----
>>>  include/linux/firmware.h      |  7 +++++++
>>>  2 files changed, 38 insertions(+), 5 deletions(-)
>>
>> I like it, the 60 seconds thing has been a senseless PITA for no good
>> reason. I have always wondered what might change in 60 seconds wrt to us
>> being able to load the firmware...
>>
>>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>>> index eb8fb94ae2c5..7f48a6ffb0df 100644
>>> --- a/drivers/base/firmware_class.c
>>> +++ b/drivers/base/firmware_class.c
>>> @@ -1061,7 +1061,7 @@ 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 fallback)
>>
>> Just a nitpick: three boolean args in a row starts to slowly look like a
>> function from the windoze API. Can we do:
>>
>> _request_firmware(..., unsigned long flags)
>>
>> instead and have nice bit flags for that?
> 
> Sounds like a good idea.  How about the patch below?
> (I used unsigned int since there shouldn't be so many different
>  behaviors.)
> 
> 
> thanks,
> 
> Takashi
> 
> ===
> 
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] firmware: Use bit flags instead of boolean combos
> 
> More than two boolean arguments to a function are rather confusing and
> error-prone for callers.  Let's make the behavior bit flags instead of
> triple combos.
> 
> A nice suggestion by Borislav Petkov.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Sure -- looks good.

Acked-by: Prarit Bhargava <prarit@redhat.com>

P.

Re: [PATCH v2 3/3] firmware: Avoid bogus fallback warning

[Ming Lei]

On Mon, Nov 11, 2013 at 11:21 PM, Takashi Iwai <tiwai@suse.de> wrote:
> The commit [3e358ac2bb5b: firmware: Be a bit more verbose about direct
> firmware loading failure] introduced a new warning message about
> falling back to user helper, but this isn't true when
> CONFIG_FW_LOADER_USER_HELPER isn't set.
>
> For avoiding the confusion, add a proper ifdef.  And now we can remove
> the dummy fw_load_from_user_helper(), too, since it's no longer called
> with CONFIG_FW_LOADER_USER_HELPER=n.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/firmware_class.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 7f48a6ffb0df..bb03c71bd94d 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -940,14 +940,6 @@ static void kill_requests_without_uevent(void)
>  #endif
>
>  #else /* CONFIG_FW_LOADER_USER_HELPER */
> -static inline int
> -fw_load_from_user_helper(struct firmware *firmware, const char *name,
> -                        struct device *device, bool uevent, bool nowait,
> -                        long timeout)
> -{
> -       return -ENOENT;
> -}
> -
>  /* No abort during direct loading */
>  #define is_fw_load_aborted(buf) false
>
> @@ -1097,11 +1089,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>         if (ret) {
>                 dev_warn(device, "Direct firmware load failed with error %d\n",
>                          ret);
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
>                 if (fallback) {
>                         dev_warn(device, "Falling back to user helper\n");

I think it is simpler to put above line at the entry of
fw_load_from_user_helper()
since we always do direct-loading first, and code should be cleaner.

Thanks,
--
Ming Lei

Re: [PATCH v2 1/3] firmware: Introduce request_firmware_direct()

[Ming Lei]

On Tue, Nov 12, 2013 at 1:30 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 11 Nov 2013 16:34:26 +0100,
>
> Sounds like a good idea.  How about the patch below?
> (I used unsigned int since there shouldn't be so many different
>  behaviors.)
>
>
> thanks,
>
> Takashi
>
> ===
>
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] firmware: Use bit flags instead of boolean combos
>
> More than two boolean arguments to a function are rather confusing and
> error-prone for callers.  Let's make the behavior bit flags instead of
> triple combos.
>
> A nice suggestion by Borislav Petkov.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Nice work,

              Acked-by: Ming Lei <ming.lei@canonical.com>

Thanks,
--
Ming Lei

> ---
>  drivers/base/firmware_class.c | 49 ++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index bb03c71bd94d..a4eaa7b82882 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -96,6 +96,11 @@ static inline long firmware_loading_timeout(void)
>         return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
>  }
>
> +/* firmware behavior options */
> +#define FW_OPT_UEVENT  (1U << 0)
> +#define FW_OPT_NOWAIT  (1U << 1)
> +#define FW_OPT_FALLBACK        (1U << 2)
> +
>  struct firmware_cache {
>         /* firmware_buf instance will be added into the below list */
>         spinlock_t lock;
> @@ -820,7 +825,7 @@ static void firmware_class_timeout_work(struct work_struct *work)
>
>  static struct firmware_priv *
>  fw_create_instance(struct firmware *firmware, const char *fw_name,
> -                  struct device *device, bool uevent, bool nowait)
> +                  struct device *device, unsigned int opt_flags)
>  {
>         struct firmware_priv *fw_priv;
>         struct device *f_dev;
> @@ -832,7 +837,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
>                 goto exit;
>         }
>
> -       fw_priv->nowait = nowait;
> +       fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
>         fw_priv->fw = firmware;
>         INIT_DELAYED_WORK(&fw_priv->timeout_work,
>                 firmware_class_timeout_work);
> @@ -848,8 +853,8 @@ exit:
>  }
>
>  /* load a firmware via user helper */
> -static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
> -                                 long timeout)
> +static int _request_firmware_load(struct firmware_priv *fw_priv,
> +                                 unsigned int opt_flags, long timeout)
>  {
>         int retval = 0;
>         struct device *f_dev = &fw_priv->dev;
> @@ -885,7 +890,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>                 goto err_del_bin_attr;
>         }
>
> -       if (uevent) {
> +       if (opt_flags & FW_OPT_UEVENT) {
>                 buf->need_uevent = true;
>                 dev_set_uevent_suppress(f_dev, false);
>                 dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
> @@ -911,16 +916,16 @@ err_put_dev:
>
>  static int fw_load_from_user_helper(struct firmware *firmware,
>                                     const char *name, struct device *device,
> -                                   bool uevent, bool nowait, long timeout)
> +                                   unsigned int opt_flags, long timeout)
>  {
>         struct firmware_priv *fw_priv;
>
> -       fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
> +       fw_priv = fw_create_instance(firmware, name, device, opt_flags);
>         if (IS_ERR(fw_priv))
>                 return PTR_ERR(fw_priv);
>
>         fw_priv->buf = firmware->priv;
> -       return _request_firmware_load(fw_priv, uevent, timeout);
> +       return _request_firmware_load(fw_priv, opt_flags, timeout);
>  }
>
>  #ifdef CONFIG_PM_SLEEP
> @@ -1015,7 +1020,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
>  }
>
>  static int assign_firmware_buf(struct firmware *fw, struct device *device,
> -                               bool skip_cache)
> +                              unsigned int opt_flags)
>  {
>         struct firmware_buf *buf = fw->priv;
>
> @@ -1032,7 +1037,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
>          * device may has been deleted already, but the problem
>          * should be fixed in devres or driver core.
>          */
> -       if (device && !skip_cache)
> +       /* don't cache firmware handled without uevent */
> +       if (device && (opt_flags & FW_OPT_UEVENT))
>                 fw_add_devm_name(device, buf->fw_id);
>
>         /*
> @@ -1053,7 +1059,7 @@ 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, bool fallback)
> +                 struct device *device, unsigned int opt_flags)
>  {
>         struct firmware *fw;
>         long timeout;
> @@ -1068,7 +1074,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>
>         ret = 0;
>         timeout = firmware_loading_timeout();
> -       if (nowait) {
> +       if (opt_flags & FW_OPT_NOWAIT) {
>                 timeout = usermodehelper_read_lock_wait(timeout);
>                 if (!timeout) {
>                         dev_dbg(device, "firmware: %s loading timed out\n",
> @@ -1090,17 +1096,16 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>                 dev_warn(device, "Direct firmware load failed with error %d\n",
>                          ret);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> -               if (fallback) {
> +               if (opt_flags & FW_OPT_FALLBACK) {
>                         dev_warn(device, "Falling back to user helper\n");
>                         ret = fw_load_from_user_helper(fw, name, device,
> -                                              uevent, nowait, timeout);
> +                                                      opt_flags, timeout);
>                 }
>  #endif
>         }
>
> -       /* don't cache firmware handled without uevent */
>         if (!ret)
> -               ret = assign_firmware_buf(fw, device, !uevent);
> +               ret = assign_firmware_buf(fw, device, opt_flags);
>
>         usermodehelper_read_unlock();
>
> @@ -1142,7 +1147,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, true, false, true);
> +       ret = _request_firmware(firmware_p, name, device,
> +                               FW_OPT_UEVENT | FW_OPT_FALLBACK);
>         module_put(THIS_MODULE);
>         return ret;
>  }
> @@ -1165,7 +1171,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
>  {
>         int ret;
>         __module_get(THIS_MODULE);
> -       ret = _request_firmware(firmware_p, name, device, true, false, false);
> +       ret = _request_firmware(firmware_p, name, device, FW_OPT_UEVENT);
>         module_put(THIS_MODULE);
>         return ret;
>  }
> @@ -1194,7 +1200,7 @@ struct firmware_work {
>         struct device *device;
>         void *context;
>         void (*cont)(const struct firmware *fw, void *context);
> -       bool uevent;
> +       unsigned int opt_flags;
>  };
>
>  static void request_firmware_work_func(struct work_struct *work)
> @@ -1205,7 +1211,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, true);
> +                         fw_work->opt_flags);
>         fw_work->cont(fw, fw_work->context);
>         put_device(fw_work->device); /* taken in request_firmware_nowait() */
>
> @@ -1253,7 +1259,8 @@ request_firmware_nowait(
>         fw_work->device = device;
>         fw_work->context = context;
>         fw_work->cont = cont;
> -       fw_work->uevent = uevent;
> +       fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
> +               (uevent ? FW_OPT_UEVENT : 0);
>
>         if (!try_module_get(module)) {
>                 kfree(fw_work);
> --
> 1.8.4.2
>

Re: [PATCH v2 3/3] firmware: Avoid bogus fallback warning

[Takashi Iwai]

At Tue, 12 Nov 2013 09:40:24 +0800,
Ming Lei wrote:
At Tue, 12 Nov 2013 09:40:24 +0800,
Ming Lei wrote:
> 
> On Mon, Nov 11, 2013 at 11:21 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > The commit [3e358ac2bb5b: firmware: Be a bit more verbose about direct
> > firmware loading failure] introduced a new warning message about
> > falling back to user helper, but this isn't true when
> > CONFIG_FW_LOADER_USER_HELPER isn't set.
> >
> > For avoiding the confusion, add a proper ifdef.  And now we can remove
> > the dummy fw_load_from_user_helper(), too, since it's no longer called
> > with CONFIG_FW_LOADER_USER_HELPER=n.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/firmware_class.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 7f48a6ffb0df..bb03c71bd94d 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -940,14 +940,6 @@ static void kill_requests_without_uevent(void)
> >  #endif
> >
> >  #else /* CONFIG_FW_LOADER_USER_HELPER */
> > -static inline int
> > -fw_load_from_user_helper(struct firmware *firmware, const char *name,
> > -                        struct device *device, bool uevent, bool nowait,
> > -                        long timeout)
> > -{
> > -       return -ENOENT;
> > -}
> > -
> >  /* No abort during direct loading */
> >  #define is_fw_load_aborted(buf) false
> >
> > @@ -1097,11 +1089,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >         if (ret) {
> >                 dev_warn(device, "Direct firmware load failed with error %d\n",
> >                          ret);
> > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> >                 if (fallback) {
> >                         dev_warn(device, "Falling back to user helper\n");
> 
> I think it is simpler to put above line at the entry of
> fw_load_from_user_helper()
> since we always do direct-loading first, and code should be cleaner.
> 
> Thanks,
> --
> Ming Lei
> 
> 
> On Mon, Nov 11, 2013 at 11:21 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > The commit [3e358ac2bb5b: firmware: Be a bit more verbose about direct
> > firmware loading failure] introduced a new warning message about
> > falling back to user helper, but this isn't true when
> > CONFIG_FW_LOADER_USER_HELPER isn't set.
> >
> > For avoiding the confusion, add a proper ifdef.  And now we can remove
> > the dummy fw_load_from_user_helper(), too, since it's no longer called
> > with CONFIG_FW_LOADER_USER_HELPER=n.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/firmware_class.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 7f48a6ffb0df..bb03c71bd94d 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -940,14 +940,6 @@ static void kill_requests_without_uevent(void)
> >  #endif
> >
> >  #else /* CONFIG_FW_LOADER_USER_HELPER */
> > -static inline int
> > -fw_load_from_user_helper(struct firmware *firmware, const char *name,
> > -                        struct device *device, bool uevent, bool nowait,
> > -                        long timeout)
> > -{
> > -       return -ENOENT;
> > -}
> > -
> >  /* No abort during direct loading */
> >  #define is_fw_load_aborted(buf) false
> >
> > @@ -1097,11 +1089,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >         if (ret) {
> >                 dev_warn(device, "Direct firmware load failed with error %d\n",
> >                          ret);
> > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> >                 if (fallback) {
> >                         dev_warn(device, "Falling back to user helper\n");
> 
> I think it is simpler to put above line at the entry of
> fw_load_from_user_helper()
> since we always do direct-loading first, and code should be cleaner.

OK, that makes sense.

While looking back at the code, I think the first warning ("Direct
firmware load failed" should be suppressed, too, when no fallback is
set.  For example, non-existing firmware is no error at all for
microcode driver, as a firmware is purely optional.  Showing a warning
at each failure would result in lots of bogus warnings and it'd
confuse users as if something critical happened.

So, the first dev_warn() is better in the "if (fallback)" block, IMO.

I'm going to prepare the v3 patch series together with bit flags
change.


thanks,

Takashi