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

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

[Takashi Iwai]

Hi,

this is a 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 1/2] firmware: Introduce request_firmware_direct()
 [PATCH 2/2] microcode: Use request_firmware_direct()

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


thanks,

Takashi

[PATCH 1/2] 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>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 32 ++++++++++++++++++++++++++++----
 include/linux/firmware.h      |  7 +++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 10a4467c63f1..76f2d8a6c544 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1054,7 +1054,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;
@@ -1086,7 +1086,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		}
 	}
 
-	if (!fw_get_filesystem_firmware(device, fw->priv))
+	if (!fw_get_filesystem_firmware(device, fw->priv) && fallback)
 		ret = fw_load_from_user_helper(fw, name, device,
 					       uevent, nowait, timeout);
 
@@ -1134,12 +1134,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
@@ -1173,7 +1197,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.1

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

[Ming Lei]

Hi Takashi,

On Tue, Nov 5, 2013 at 7:57 PM, Takashi Iwai <tiwai@suse.de> 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>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

The patch is fine, except it should be against -next, so you can feel
free to add my Acked-by in your next submission.

Thanks,
--
Ming Lei

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

[Takashi Iwai]

At Mon, 11 Nov 2013 15:44:38 +0800,
Ming Lei wrote:
> 
> Hi Takashi,
> 
> On Tue, Nov 5, 2013 at 7:57 PM, Takashi Iwai <tiwai@suse.de> 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>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> The patch is fine, except it should be against -next, so you can feel
> free to add my Acked-by in your next submission.

OK, I'm going to resubmit the patch series based on next.

BTW, looking at linux-next code, I found another issue in the relevant 
place; the fallback warning is shown no matter whether
CONFIG_FIRMWAMRE_USERMODE_HELPER is set or not.  I'll put the fix into
the next series, too.


thanks,

Takashi

[PATCH 2/2] 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.1