[PATCH 0/2] Improve firmware loading times on AMD and Intel

[PATCH 0/2] Improve firmware loading times on AMD and Intel

[Prarit Bhargava]

If no firmware is found on the system that matches the processor, the
microcode module can take hours to load.  For example on recent kernels
when loading the microcode module on an Intel system:

[  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
[  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
[  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
[  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
[  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
...
[ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
[ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
[ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
[ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
[ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
...
etc.

Since this takes 1 minute per cpu, the microcode module requires two hours to
load on a 120 cpu Intel box.

Similarly there is a 60 second "hang" when loading the AMD module, which
isn't as bad as the Intel situation, but it is a noticeable delay in the
system boot.

Using request_firmware_nowait() seems more appropriate here and then we
can avoid these delays, resulting in very quick load times for the
microcode.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: x86@kernel.org
Cc: herrmann.der.user@googlemail.com
Cc: ming.lei@canonical.com
Cc: tigran@aivazian.fsnet.co.uk

Prarit Bhargava (2):
  firmware, fix request_firmware_nowait() freeze with no uevent
  intel_microcode, Fix long microcode load time when firmware file is
    missing

 arch/x86/include/asm/microcode.h  |    7 ++++
 arch/x86/kernel/microcode_amd.c   |   79 +++++++++++++++++++++++++++----------
 arch/x86/kernel/microcode_core.c  |    7 ++++
 arch/x86/kernel/microcode_intel.c |   67 +++++++++++++++++++++++++------
 drivers/base/firmware_class.c     |    6 ++-
 5 files changed, 132 insertions(+), 34 deletions(-)

-- 
1.7.9.3

[PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Prarit Bhargava]

If request_firmware_nowait() is called with uevent == NULL, the firmware
completion is never marked complete resulting in a hang in the process.

If uevent is undefined, that means we're not waiting on anything and the
process should just clean up and complete.  While we're at it, add a
debug dev_dbg() to indicate that the FW has not been found.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: x86@kernel.org
Cc: herrmann.der.user@googlemail.com
Cc: ming.lei@canonical.com
Cc: tigran@aivazian.fsnet.co.uk
---
 drivers/base/firmware_class.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 10a4467..95778dc 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -335,7 +335,8 @@ static bool fw_get_filesystem_firmware(struct device *device,
 		set_bit(FW_STATUS_DONE, &buf->status);
 		complete_all(&buf->completion);
 		mutex_unlock(&fw_lock);
-	}
+	} else
+		dev_dbg(device, "firmware: %s not found\n", buf->fw_id);
 
 	return success;
 }
@@ -886,6 +887,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 			schedule_delayed_work(&fw_priv->timeout_work, timeout);
 
 		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
+	} else {
+		/* if there is no uevent then just cleanup */
+		schedule_delayed_work(&fw_priv->timeout_work, 0);
 	}
 
 	wait_for_completion(&buf->completion);
-- 
1.7.9.3

[PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing

[Prarit Bhargava]

If no firmware is found on the system that matches the processor, the
microcode module can take hours to load.  For example on recent kernels
when loading the microcode module on an Intel system:

[  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
[  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
[  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
[  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
[  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
...
[ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
[ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
[ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
[ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
[ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
...
etc.

Similarly there is a 60 second "hang" when loading the AMD module, which
isn't as bad as the Intel situation, but it is a noticeable delay in the
system boot and can be improved upon.

Using request_firmware_nowait() seems more appropriate here and then we
can avoid these delays, resulting in very quick load times for the
microcode.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: x86@kernel.org
Cc: herrmann.der.user@googlemail.com
Cc: ming.lei@canonical.com
Cc: tigran@aivazian.fsnet.co.uk
---
 arch/x86/include/asm/microcode.h  |    7 ++++
 arch/x86/kernel/microcode_amd.c   |   79 +++++++++++++++++++++++++++----------
 arch/x86/kernel/microcode_core.c  |    7 ++++
 arch/x86/kernel/microcode_intel.c |   67 +++++++++++++++++++++++++------
 4 files changed, 127 insertions(+), 33 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index f98bd66..461a66f 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -11,6 +11,12 @@ struct device;
 
 enum ucode_state { UCODE_ERROR, UCODE_OK, UCODE_NFOUND };
 
+/* data struct for request_firmware_nowait callback */
+struct microcode_request_data {
+	unsigned long cpu;
+	char name[36];
+};
+
 struct microcode_ops {
 	enum ucode_state (*request_microcode_user) (int cpu,
 				const void __user *buf, size_t size);
@@ -34,6 +40,7 @@ struct ucode_cpu_info {
 	struct cpu_signature	cpu_sig;
 	int			valid;
 	void			*mc;
+	struct completion	completion;
 };
 extern struct ucode_cpu_info ucode_cpu_info[];
 
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index af99f71..17a2a09 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -27,6 +27,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/cpu.h>
 
 #include <asm/microcode.h>
 #include <asm/processor.h>
@@ -399,6 +400,40 @@ enum ucode_state load_microcode_amd(u8 family, const u8 *data, size_t size)
 	return ret;
 }
 
+static void amd_apply_firmware(const struct firmware *fw, void *context)
+{
+	struct microcode_request_data *mrd =
+				       (struct microcode_request_data *)context;
+	int cpu = mrd->cpu;
+	int ret = UCODE_ERROR;
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+
+	if (!fw) {
+		pr_warn("firmware %s not found\n", mrd->name);
+		goto out;
+	}
+
+	if (!fw->data || !fw->size) {
+		pr_warn("firmware %s invalid\n", mrd->name);
+		goto out;
+	}
+
+	if (*(u32 *)fw->data != UCODE_MAGIC) {
+		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
+		goto out;
+	}
+
+	ret = load_microcode_amd(c->x86, fw->data, fw->size);
+	if (ret == UCODE_OK)
+		pr_info("firmware %s is ready for cpu %d\n", mrd->name, cpu);
+out:
+	complete_all(&uci->completion);
+	if (fw)
+		release_firmware(fw);
+	vfree(mrd);
+}
+
 /*
  * AMD microcode firmware naming convention, up to family 15h they are in
  * the legacy file:
@@ -418,36 +453,38 @@ enum ucode_state load_microcode_amd(u8 family, const u8 *data, size_t size)
 static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 					      bool refresh_fw)
 {
-	char fw_name[36] = "amd-ucode/microcode_amd.bin";
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	enum ucode_state ret = UCODE_NFOUND;
-	const struct firmware *fw;
+	struct device *cpu_device;
+	struct microcode_request_data *mrd;
 
-	/* reload ucode container only on the boot cpu */
-	if (!refresh_fw || c->cpu_index != boot_cpu_data.cpu_index)
+	if (!refresh_fw)
 		return UCODE_OK;
 
-	if (c->x86 >= 0x15)
-		snprintf(fw_name, sizeof(fw_name), "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
+	mrd = vmalloc(sizeof(mrd));
+	if (!mrd)
+		return UCODE_ERROR;
 
-	if (request_firmware(&fw, (const char *)fw_name, device)) {
-		pr_err("failed to load file %s\n", fw_name);
-		goto out;
+	cpu_device = get_cpu_device(cpu);
+	if (!cpu_device) {
+		pr_err("cpu %d, no device found\n", cpu);
+		cpu_device = device;
 	}
 
-	ret = UCODE_ERROR;
-	if (*(u32 *)fw->data != UCODE_MAGIC) {
-		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
-		goto fw_release;
+	mrd->cpu = cpu;
+	sprintf(mrd->name, "amd-ucode/microcode_amd.bin");
+	if (c->x86 >= 0x15)
+		snprintf(mrd->name, sizeof(mrd->name),
+			 "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
+
+	if (request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
+				    mrd->name, cpu_device, GFP_KERNEL,
+				    (void *)mrd, amd_apply_firmware)) {
+		pr_info("data file %s load failed\n", mrd->name);
+		vfree(mrd);
+		return UCODE_NFOUND;
 	}
 
-	ret = load_microcode_amd(c->x86, fw->data, fw->size);
-
- fw_release:
-	release_firmware(fw);
-
- out:
-	return ret;
+	return UCODE_OK;
 }
 
 static enum ucode_state
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 15c9876..36351dc 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -171,8 +171,13 @@ static void apply_microcode_local(void *arg)
 static int apply_microcode_on_target(int cpu)
 {
 	struct apply_microcode_ctx ctx = { .err = 0 };
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	int ret;
 
+	ret = wait_for_completion_timeout(&uci->completion, HZ);
+	if (!ret)
+		pr_warn("microcode: cpu %d thread wait timed out\n", cpu);
+
 	ret = smp_call_function_single(cpu, apply_microcode_local, &ctx, 1);
 	if (!ret)
 		ret = ctx.err;
@@ -285,6 +290,7 @@ static int reload_for_cpu(int cpu)
 	if (!uci->valid)
 		return err;
 
+	init_completion(&uci->completion);
 	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, true);
 	if (ustate == UCODE_OK)
 		apply_microcode_on_target(cpu);
@@ -392,6 +398,7 @@ static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
 	if (system_state != SYSTEM_RUNNING)
 		return UCODE_NFOUND;
 
+	init_completion(&uci->completion);
 	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev,
 						     refresh_fw);
 
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 5fb2ceb..c63d7c0 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -78,6 +78,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/vmalloc.h>
+#include <linux/cpu.h>
 
 #include <asm/microcode_intel.h>
 #include <asm/processor.h>
@@ -267,28 +268,70 @@ static int get_ucode_fw(void *to, const void *from, size_t n)
 	return 0;
 }
 
+static void intel_apply_firmware(const struct firmware *fw, void *context)
+{
+	struct microcode_request_data *mrd =
+				       (struct microcode_request_data *)context;
+	int cpu = mrd->cpu;
+	int ret = UCODE_ERROR;
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	if (!fw) {
+		pr_warn("firmware %s not found\n", mrd->name);
+		goto out;
+	}
+
+	if (!fw->data || !fw->size) {
+		pr_warn("firmware %s invalid\n", mrd->name);
+		goto out;
+	}
+
+	ret = generic_load_microcode(cpu, (void *)fw->data,
+				     fw->size, &get_ucode_fw);
+	if (ret == UCODE_OK)
+		pr_info("firmware %s is ready for cpu %d\n", mrd->name, cpu);
+
+out:
+	complete_all(&uci->completion);
+	if (fw)
+		release_firmware(fw);
+	vfree(mrd);
+}
+
 static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 					     bool refresh_fw)
 {
-	char name[30];
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	const struct firmware *firmware;
-	enum ucode_state ret;
+	struct device *cpu_device;
+	struct microcode_request_data *mrd;
 
-	sprintf(name, "intel-ucode/%02x-%02x-%02x",
-		c->x86, c->x86_model, c->x86_mask);
+	if (!refresh_fw)
+		return UCODE_OK;
 
-	if (request_firmware(&firmware, name, device)) {
-		pr_debug("data file %s load failed\n", name);
-		return UCODE_NFOUND;
+	mrd = vmalloc(sizeof(*mrd));
+	if (!mrd)
+		return UCODE_ERROR;
+
+	cpu_device = get_cpu_device(cpu);
+	if (!cpu_device) {
+		pr_err("cpu %d, no device found\n", cpu);
+		cpu_device = device;
 	}
+	cpu_device = device;
 
-	ret = generic_load_microcode(cpu, (void *)firmware->data,
-				     firmware->size, &get_ucode_fw);
+	mrd->cpu = cpu;
+	sprintf(mrd->name, "intel-ucode/%02x-%02x-%02x",
+		c->x86, c->x86_model, c->x86_mask);
 
-	release_firmware(firmware);
+	if (request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
+				    mrd->name, cpu_device, GFP_KERNEL,
+				    (void *)mrd, intel_apply_firmware)) {
+		pr_info("data file %s load failed\n", mrd->name);
+		vfree(mrd);
+		return UCODE_NFOUND;
+	}
 
-	return ret;
+	return UCODE_OK;
 }
 
 static int get_ucode_user(void *to, const void *from, size_t n)
-- 
1.7.9.3

Re: [PATCH 0/2] Improve firmware loading times on AMD and Intel

[Andi Kleen]

Prarit Bhargava <prarit@redhat.com> writes:
>
> Using request_firmware_nowait() seems more appropriate here and then we
> can avoid these delays, resulting in very quick load times for the
> microcode.

It would  be probably also good to remember if the loading failed for a 
given CPU and then don't try it on any other.

Just need to cache the exact stepping too to handle different steppings.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing

[Ming Lei]

On Mon, Oct 21, 2013 at 5:35 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> If no firmware is found on the system that matches the processor, the
> microcode module can take hours to load.  For example on recent kernels
> when loading the microcode module on an Intel system:
>
> [  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
> [  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
> [  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
> [  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
> [  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
> ...
> [ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
> [ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
> [ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
> [ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
> [ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
> ...
> etc.
>
> Similarly there is a 60 second "hang" when loading the AMD module, which
> isn't as bad as the Intel situation, but it is a noticeable delay in the
> system boot and can be improved upon.
>
> Using request_firmware_nowait() seems more appropriate here and then we
> can avoid these delays, resulting in very quick load times for the
> microcode.

request_firmware_nowait() should be a good idea for the problem, but
why do you pass FW_ACTION_NOHOTPLUG as uevent?  It is used now
by drivers which requires their own user-space applications to handle
the request by polling the firmware device under sysfs.

So I am wondering if your microcode case belongs to the above case
of FW_ACTION_NOHOTPLUG?

And why don't you pass FW_ACTION_HOTPLUG? and you are sure
that udev isn't required to handle your microcode update request?


Thanks,
--
Ming Lei

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Ming Lei]

On Mon, Oct 21, 2013 at 5:35 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> If request_firmware_nowait() is called with uevent == NULL, the firmware
> completion is never marked complete resulting in a hang in the process.
>
> If uevent is undefined, that means we're not waiting on anything and the
> process should just clean up and complete.  While we're at it, add a
> debug dev_dbg() to indicate that the FW has not been found.
>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: x86@kernel.org
> Cc: herrmann.der.user@googlemail.com
> Cc: ming.lei@canonical.com
> Cc: tigran@aivazian.fsnet.co.uk
> ---
>  drivers/base/firmware_class.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 10a4467..95778dc 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -335,7 +335,8 @@ static bool fw_get_filesystem_firmware(struct device *device,
>                 set_bit(FW_STATUS_DONE, &buf->status);
>                 complete_all(&buf->completion);
>                 mutex_unlock(&fw_lock);
> -       }
> +       } else
> +               dev_dbg(device, "firmware: %s not found\n", buf->fw_id);
>
>         return success;
>  }
> @@ -886,6 +887,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>                         schedule_delayed_work(&fw_priv->timeout_work, timeout);
>
>                 kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
> +       } else {
> +               /* if there is no uevent then just cleanup */
> +               schedule_delayed_work(&fw_priv->timeout_work, 0);
>         }

This may not a good idea and might break current NOHOTPLUG
users, and how can you make sure the user space application can
complete the request during the timeout time?

Thanks,
--
Ming Lei

Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing

[Prarit Bhargava]

On 10/21/2013 08:20 AM, Ming Lei wrote:
> On Mon, Oct 21, 2013 at 5:35 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> If no firmware is found on the system that matches the processor, the
>> microcode module can take hours to load.  For example on recent kernels
>> when loading the microcode module on an Intel system:
>>
>> [  239.532116] microcode: CPU0 sig=0x306e4, pf=0x1, revision=0x413
>> [  299.693447] microcode: CPU1 sig=0x306e4, pf=0x1, revision=0x413
>> [  359.821972] microcode: CPU2 sig=0x306e4, pf=0x1, revision=0x413
>> [  419.960263] microcode: CPU3 sig=0x306e4, pf=0x1, revision=0x413
>> [  480.090024] microcode: CPU4 sig=0x306e4, pf=0x1, revision=0x413
>> ...
>> [ 2825.151364] microcode: CPU43 sig=0x306e4, pf=0x1, revision=0x413
>> [ 2885.280863] microcode: CPU44 sig=0x306e4, pf=0x1, revision=0x413
>> [ 2945.410719] microcode: CPU45 sig=0x306e4, pf=0x1, revision=0x413
>> [ 3005.540541] microcode: CPU46 sig=0x306e4, pf=0x1, revision=0x413
>> [ 3065.670405] microcode: CPU47 sig=0x306e4, pf=0x1, revision=0x413
>> ...
>> etc.
>>
>> Similarly there is a 60 second "hang" when loading the AMD module, which
>> isn't as bad as the Intel situation, but it is a noticeable delay in the
>> system boot and can be improved upon.
>>
>> Using request_firmware_nowait() seems more appropriate here and then we
>> can avoid these delays, resulting in very quick load times for the
>> microcode.
> 
> request_firmware_nowait() should be a good idea for the problem, but
> why do you pass FW_ACTION_NOHOTPLUG as uevent?  It is used now
> by drivers which requires their own user-space applications to handle
> the request by polling the firmware device under sysfs.

Hello Ming,

Oh, I see.

> 
> So I am wondering if your microcode case belongs to the above case
> of FW_ACTION_NOHOTPLUG?

You're right.  I can easily change that in v2.

> 
> And why don't you pass FW_ACTION_HOTPLUG? and you are sure
> that udev isn't required to handle your microcode update request?
> 

AFAICT in both cases, udev wasn't required to handle the cpu microcode update.
Both drivers use CMH to load the firmware which removes the need for udev to do
anything.  Admittedly maybe I've missed some odd use case but I don't think it
is necessary.

P.

Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing

[Ming Lei]

On Mon, Oct 21, 2013 at 8:26 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>
>> And why don't you pass FW_ACTION_HOTPLUG? and you are sure
>> that udev isn't required to handle your microcode update request?
>>
>
> AFAICT in both cases, udev wasn't required to handle the cpu microcode update.
> Both drivers use CMH to load the firmware which removes the need for udev to do
> anything.  Admittedly maybe I've missed some odd use case but I don't think it
> is necessary.

OK, so I guess the CMH still need uevent to get notified, right?

If yes, you should take FW_ACTION_HOTPLUG.


Thanks,
--
Ming Lei

Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing

[Prarit Bhargava]

On 10/21/2013 08:32 AM, Ming Lei wrote:
> On Mon, Oct 21, 2013 at 8:26 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>>
>>> And why don't you pass FW_ACTION_HOTPLUG? and you are sure
>>> that udev isn't required to handle your microcode update request?
>>>
>>
>> AFAICT in both cases, udev wasn't required to handle the cpu microcode update.
>> Both drivers use CMH to load the firmware which removes the need for udev to do
>> anything.  Admittedly maybe I've missed some odd use case but I don't think it
>> is necessary.
> 
> OK, so I guess the CMH still need uevent to get notified, right?

The code as it is _currently_ written does not use uevents to load the processor
firmware.  ie) call_usermodehelper does not need uevent to get notified, so I
think FW_ACTION_NOHOTPLUG is correct.

P.

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Prarit Bhargava]

On 10/21/2013 08:24 AM, Ming Lei wrote:
> On Mon, Oct 21, 2013 at 5:35 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> If request_firmware_nowait() is called with uevent == NULL, the firmware
>> completion is never marked complete resulting in a hang in the process.
>>
>> If uevent is undefined, that means we're not waiting on anything and the
>> process should just clean up and complete.  While we're at it, add a
>> debug dev_dbg() to indicate that the FW has not been found.
>>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: x86@kernel.org
>> Cc: herrmann.der.user@googlemail.com
>> Cc: ming.lei@canonical.com
>> Cc: tigran@aivazian.fsnet.co.uk
>> ---
>>  drivers/base/firmware_class.c |    6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 10a4467..95778dc 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -335,7 +335,8 @@ static bool fw_get_filesystem_firmware(struct device *device,
>>                 set_bit(FW_STATUS_DONE, &buf->status);
>>                 complete_all(&buf->completion);
>>                 mutex_unlock(&fw_lock);
>> -       }
>> +       } else
>> +               dev_dbg(device, "firmware: %s not found\n", buf->fw_id);
>>
>>         return success;
>>  }
>> @@ -886,6 +887,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>>                         schedule_delayed_work(&fw_priv->timeout_work, timeout);
>>
>>                 kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
>> +       } else {
>> +               /* if there is no uevent then just cleanup */
>> +               schedule_delayed_work(&fw_priv->timeout_work, 0);
>>         }
> 
> This may not a good idea and might break current NOHOTPLUG
> users, 

Ming,

The code is broken for all callers of request_firmware_nowait() with NOHOTPLUG
and CONFIG_FW_LOADER_USER_HELPER=y.  AFAICT with the two existing cases of this
usage in the kernel, both are broken and both are attempting to do the same
thing that I'm doing in the x86 microcode ATM.

This is the situation as I understand it and please correct me if I'm wrong
about the execution path.  If I call request_firmware_nowait() with NOHOTPLUG I
am essentially saying that there is no uevent associated with this firmware
load; that is uevent = 0.  request_firmware_work_func() is called as scheduled
task, which results in a call to _request_firmware().  _request_firmware() first
calls _request_firmware_prepare() which eventually results in a call to
__allocate_fw_buf() which does an init_completion(&buf->completion).

Returning back up the stack to _request_firmware() we eventually call
fw_get_filesystem_firmware().  _If the firmware does not exist_ success is false
and the if (success) loop is not executed, and it is important to note that the
complete_all(&buf->completion) is _not_ called.  fw_get_filesystem_firmware()
returns an error so that fw_load_from_user_helper() is called from
_request_firmware().

fw_load_from_user_helper() eventually calls _request_firmware_load() and this is
where we get into a problem.  fw_load_from_user_helper() calls all the file
creation, etc., and then hits this chunk of code:

        if (uevent) {
                dev_set_uevent_suppress(f_dev, false);
                dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
                if (timeout != MAX_SCHEDULE_TIMEOUT)
                        schedule_delayed_work(&fw_priv->timeout_work, timeout);

                kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
        }

        wait_for_completion(&buf->completion);

As I previously said, we've been called with NOHOTPLUG, ie) uevent = 0.  That
means we skip down to the wait_for_completion(&buf->completion) ... and we wait
... forever.

I can reproduce this by using a Dell PE 1850 & the dell_rbu module by doing the
following:

insmod dell_rbu.ko
echo init > /sys/devices/platform/dell_rbu/image_type
lsmod | grep dell_rbu

(after an hour)

[root@dell-pe1850-04 dell_rbu]# lsmod | grep dell_rbu
dell_rbu               14315  1
[root@dell-pe1850-04 dell_rbu]#

^^^ that use count is left because the thread is waiting with an existing module
ref count.  For kicks I put a printk in the dell_rbu code or instrument the
_request_firmware() code and did a reboot.  Since the completions are finished
on system shutdown, I see the code continue to execute at the end of boot.

> and how can you make sure the user space application can
> complete the request during the timeout time?

I see that your question really comes down to "are there additional
synchronizations needed in the two drivers that already call the code this way?"
 I realize that the answer to that is yes and I'll fix those up in a v2.  It
should be trivial to make those changes AFAICT.  I've introduced some additional
synchronization via a completion in the x86 microcode and will likely have to do
something similar in the other drivers ... although it may be easier to just
have the firmware code do all the synchronization.  I'll look into it.

Hope this explains things a bit better,

P.

> 
> Thanks,
> --
> Ming Lei

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Ming Lei]

On Tue, Oct 22, 2013 at 6:24 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>
>
> On 10/21/2013 08:24 AM, Ming Lei wrote:
>> On Mon, Oct 21, 2013 at 5:35 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>> If request_firmware_nowait() is called with uevent == NULL, the firmware
>>> completion is never marked complete resulting in a hang in the process.
>>>
>>> If uevent is undefined, that means we're not waiting on anything and the
>>> process should just clean up and complete.  While we're at it, add a
>>> debug dev_dbg() to indicate that the FW has not been found.
>>>
>>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>>> Cc: x86@kernel.org
>>> Cc: herrmann.der.user@googlemail.com
>>> Cc: ming.lei@canonical.com
>>> Cc: tigran@aivazian.fsnet.co.uk
>>> ---
>>>  drivers/base/firmware_class.c |    6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>>> index 10a4467..95778dc 100644
>>> --- a/drivers/base/firmware_class.c
>>> +++ b/drivers/base/firmware_class.c
>>> @@ -335,7 +335,8 @@ static bool fw_get_filesystem_firmware(struct device *device,
>>>                 set_bit(FW_STATUS_DONE, &buf->status);
>>>                 complete_all(&buf->completion);
>>>                 mutex_unlock(&fw_lock);
>>> -       }
>>> +       } else
>>> +               dev_dbg(device, "firmware: %s not found\n", buf->fw_id);
>>>
>>>         return success;
>>>  }
>>> @@ -886,6 +887,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>>>                         schedule_delayed_work(&fw_priv->timeout_work, timeout);
>>>
>>>                 kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
>>> +       } else {
>>> +               /* if there is no uevent then just cleanup */
>>> +               schedule_delayed_work(&fw_priv->timeout_work, 0);
>>>         }
>>
>> This may not a good idea and might break current NOHOTPLUG
>> users,
>
> Ming,
>
> The code is broken for all callers of request_firmware_nowait() with NOHOTPLUG
> and CONFIG_FW_LOADER_USER_HELPER=y.  AFAICT with the two existing cases of this
> usage in the kernel, both are broken and both are attempting to do the same
> thing that I'm doing in the x86 microcode ATM.
>
> This is the situation as I understand it and please correct me if I'm wrong
> about the execution path.  If I call request_firmware_nowait() with NOHOTPLUG I
> am essentially saying that there is no uevent associated with this firmware
> load; that is uevent = 0.  request_firmware_work_func() is called as scheduled
> task, which results in a call to _request_firmware().  _request_firmware() first
> calls _request_firmware_prepare() which eventually results in a call to
> __allocate_fw_buf() which does an init_completion(&buf->completion).
>
> Returning back up the stack to _request_firmware() we eventually call
> fw_get_filesystem_firmware().  _If the firmware does not exist_ success is false
> and the if (success) loop is not executed, and it is important to note that the
> complete_all(&buf->completion) is _not_ called.  fw_get_filesystem_firmware()
> returns an error so that fw_load_from_user_helper() is called from
> _request_firmware().
>
> fw_load_from_user_helper() eventually calls _request_firmware_load() and this is
> where we get into a problem.  fw_load_from_user_helper() calls all the file
> creation, etc., and then hits this chunk of code:
>
>         if (uevent) {
>                 dev_set_uevent_suppress(f_dev, false);
>                 dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
>                 if (timeout != MAX_SCHEDULE_TIMEOUT)
>                         schedule_delayed_work(&fw_priv->timeout_work, timeout);
>
>                 kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
>         }
>
>         wait_for_completion(&buf->completion);
>
> As I previously said, we've been called with NOHOTPLUG, ie) uevent = 0.  That
> means we skip down to the wait_for_completion(&buf->completion) ... and we wait
> ... forever.

Yes, it is exactly the previous design on NOHOTPLUG, because
firmware loader has to wait for the handling from user space, and
no one can predict when userspace comes because of no
notification. For example, the userspace may be 'some inputting
from shell by someone once he is free', :-) so it is difficult to set a
timeout explicitly for the handling.

But the requests can be killed before suspend & shutdown, so
it is still OK.

That is why NOHOTPLUG isn't encouraged to be taken, actually
I don't suggest you to do that too, :-)

You need to make sure your approach won't break micro-code
update application in current/previous distributions.

>
> I can reproduce this by using a Dell PE 1850 & the dell_rbu module by doing the
> following:
>
> insmod dell_rbu.ko
> echo init > /sys/devices/platform/dell_rbu/image_type
> lsmod | grep dell_rbu
>
> (after an hour)
>
> [root@dell-pe1850-04 dell_rbu]# lsmod | grep dell_rbu
> dell_rbu               14315  1
> [root@dell-pe1850-04 dell_rbu]#
>
> ^^^ that use count is left because the thread is waiting with an existing module
> ref count.  For kicks I put a printk in the dell_rbu code or instrument the
> _request_firmware() code and did a reboot.  Since the completions are finished
> on system shutdown, I see the code continue to execute at the end of boot.

Right, so no obvious problem from user view, isn't it?

>
>> and how can you make sure the user space application can
>> complete the request during the timeout time?
>
> I see that your question really comes down to "are there additional
> synchronizations needed in the two drivers that already call the code this way?"
>  I realize that the answer to that is yes and I'll fix those up in a v2.  It
> should be trivial to make those changes AFAICT.  I've introduced some additional
> synchronization via a completion in the x86 microcode and will likely have to do
> something similar in the other drivers ... although it may be easier to just
> have the firmware code do all the synchronization.  I'll look into it.
>
> Hope this explains things a bit better,

As I said above, setting a timeout may be not ok, and may break
current two drivers.


Thanks,
--
Ming Lei

Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing

[Ming Lei]

On Mon, Oct 21, 2013 at 10:25 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>
>
> On 10/21/2013 08:32 AM, Ming Lei wrote:
>> On Mon, Oct 21, 2013 at 8:26 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>>>
>>>> And why don't you pass FW_ACTION_HOTPLUG? and you are sure
>>>> that udev isn't required to handle your microcode update request?
>>>>
>>>
>>> AFAICT in both cases, udev wasn't required to handle the cpu microcode update.
>>> Both drivers use CMH to load the firmware which removes the need for udev to do
>>> anything.  Admittedly maybe I've missed some odd use case but I don't think it
>>> is necessary.
>>
>> OK, so I guess the CMH still need uevent to get notified, right?
>
> The code as it is _currently_ written does not use uevents to load the processor
> firmware.  ie) call_usermodehelper does not need uevent to get notified, so I
> think FW_ACTION_NOHOTPLUG is correct.

You need to make sure your patch won't break userspace in old
distribution with your _currently_ code.

Actually if udev isn't used in your user space, the timeout issue
won't be triggered because it is blocked by udev.

Thanks,
--
Ming Lei

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Prarit Bhargava]

On 10/21/2013 10:35 PM, Ming Lei wrote:
> On Tue, Oct 22, 2013 at 6:24 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>
>>
>> On 10/21/2013 08:24 AM, Ming Lei wrote:
>>> On Mon, Oct 21, 2013 at 5:35 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>>> If request_firmware_nowait() is called with uevent == NULL, the firmware
>>>> completion is never marked complete resulting in a hang in the process.
>>>>
>>>> If uevent is undefined, that means we're not waiting on anything and the
>>>> process should just clean up and complete.  While we're at it, add a
>>>> debug dev_dbg() to indicate that the FW has not been found.
>>>>
>>>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>>>> Cc: x86@kernel.org
>>>> Cc: herrmann.der.user@googlemail.com
>>>> Cc: ming.lei@canonical.com
>>>> Cc: tigran@aivazian.fsnet.co.uk
>>>> ---
>>>>  drivers/base/firmware_class.c |    6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>>>> index 10a4467..95778dc 100644
>>>> --- a/drivers/base/firmware_class.c
>>>> +++ b/drivers/base/firmware_class.c
>>>> @@ -335,7 +335,8 @@ static bool fw_get_filesystem_firmware(struct device *device,
>>>>                 set_bit(FW_STATUS_DONE, &buf->status);
>>>>                 complete_all(&buf->completion);
>>>>                 mutex_unlock(&fw_lock);
>>>> -       }
>>>> +       } else
>>>> +               dev_dbg(device, "firmware: %s not found\n", buf->fw_id);
>>>>
>>>>         return success;
>>>>  }
>>>> @@ -886,6 +887,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>>>>                         schedule_delayed_work(&fw_priv->timeout_work, timeout);
>>>>
>>>>                 kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
>>>> +       } else {
>>>> +               /* if there is no uevent then just cleanup */
>>>> +               schedule_delayed_work(&fw_priv->timeout_work, 0);
>>>>         }
>>>
>>> This may not a good idea and might break current NOHOTPLUG
>>> users,
>>
>> Ming,
>>
>> The code is broken for all callers of request_firmware_nowait() with NOHOTPLUG
>> and CONFIG_FW_LOADER_USER_HELPER=y.  AFAICT with the two existing cases of this
>> usage in the kernel, both are broken and both are attempting to do the same
>> thing that I'm doing in the x86 microcode ATM.
>>
>> This is the situation as I understand it and please correct me if I'm wrong
>> about the execution path.  If I call request_firmware_nowait() with NOHOTPLUG I
>> am essentially saying that there is no uevent associated with this firmware
>> load; that is uevent = 0.  request_firmware_work_func() is called as scheduled
>> task, which results in a call to _request_firmware().  _request_firmware() first
>> calls _request_firmware_prepare() which eventually results in a call to
>> __allocate_fw_buf() which does an init_completion(&buf->completion).
>>
>> Returning back up the stack to _request_firmware() we eventually call
>> fw_get_filesystem_firmware().  _If the firmware does not exist_ success is false
>> and the if (success) loop is not executed, and it is important to note that the
>> complete_all(&buf->completion) is _not_ called.  fw_get_filesystem_firmware()
>> returns an error so that fw_load_from_user_helper() is called from
>> _request_firmware().
>>
>> fw_load_from_user_helper() eventually calls _request_firmware_load() and this is
>> where we get into a problem.  fw_load_from_user_helper() calls all the file
>> creation, etc., and then hits this chunk of code:
>>
>>         if (uevent) {
>>                 dev_set_uevent_suppress(f_dev, false);
>>                 dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
>>                 if (timeout != MAX_SCHEDULE_TIMEOUT)
>>                         schedule_delayed_work(&fw_priv->timeout_work, timeout);
>>
>>                 kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
>>         }
>>
>>         wait_for_completion(&buf->completion);
>>
>> As I previously said, we've been called with NOHOTPLUG, ie) uevent = 0.  That
>> means we skip down to the wait_for_completion(&buf->completion) ... and we wait
>> ... forever.
> 
> Yes, it is exactly the previous design on NOHOTPLUG, because
> firmware loader has to wait for the handling from user space, and
> no one can predict when userspace comes because of no
> notification. For example, the userspace may be 'some inputting
> from shell by someone once he is free', :-) so it is difficult to set a
> timeout explicitly for the handling.
> 
> But the requests can be killed before suspend & shutdown, so
> it is still OK.
> 
> That is why NOHOTPLUG isn't encouraged to be taken, actually
> I don't suggest you to do that too, :-)
Okay ... I can certainly switch to HOTPLUG.

> 
> You need to make sure your approach won't break micro-code
> update application in current/previous distributions.

I've tested the following distributions today on a Dell PE 1850:  Ubuntu, SuSe,
Linux Mint, and of course Fedora.  I do not see any issues with either the
microcode update or the dell_rbu driver.  Unfortunately I do not have access to
a system that uses the lattice-ecp3-config, however, from code inspection it
looks like the driver looks at a specific place for the FW update and then
applies it via the call function in request_firmware_nowait() so it looks like
it is solid too.

I think maybe this patchset should be split into two separate submits, one for
the microcode and the second to figure out if the code really should wait
indefinitely.  AFAICT neither use case in the kernel expects an indefinite wait.

P.

> 
>>
>> I can reproduce this by using a Dell PE 1850 & the dell_rbu module by doing the
>> following:
>>
>> insmod dell_rbu.ko
>> echo init > /sys/devices/platform/dell_rbu/image_type
>> lsmod | grep dell_rbu
>>
>> (after an hour)
>>
>> [root@dell-pe1850-04 dell_rbu]# lsmod | grep dell_rbu
>> dell_rbu               14315  1
>> [root@dell-pe1850-04 dell_rbu]#
>>
>> ^^^ that use count is left because the thread is waiting with an existing module
>> ref count.  For kicks I put a printk in the dell_rbu code or instrument the
>> _request_firmware() code and did a reboot.  Since the completions are finished
>> on system shutdown, I see the code continue to execute at the end of boot.
> 
> Right, so no obvious problem from user view, isn't it?

Well, there is an issue that it is possible that the dell_rbu driver attempts to
load the update BEFORE the update is available.  I have written some additional
code to fix that.

P.

Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing

[Prarit Bhargava]

On 10/21/2013 10:43 PM, Ming Lei wrote:
> On Mon, Oct 21, 2013 at 10:25 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>
>>
>> On 10/21/2013 08:32 AM, Ming Lei wrote:
>>> On Mon, Oct 21, 2013 at 8:26 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>>>>
>>>>> And why don't you pass FW_ACTION_HOTPLUG? and you are sure
>>>>> that udev isn't required to handle your microcode update request?
>>>>>
>>>>
>>>> AFAICT in both cases, udev wasn't required to handle the cpu microcode update.
>>>> Both drivers use CMH to load the firmware which removes the need for udev to do
>>>> anything.  Admittedly maybe I've missed some odd use case but I don't think it
>>>> is necessary.
>>>
>>> OK, so I guess the CMH still need uevent to get notified, right?
>>
>> The code as it is _currently_ written does not use uevents to load the processor
>> firmware.  ie) call_usermodehelper does not need uevent to get notified, so I
>> think FW_ACTION_NOHOTPLUG is correct.
> 
> You need to make sure your patch won't break userspace in old
> distribution with your _currently_ code.
> 

AFAICT, Suse, Ubuntu, Linux Mint and Fedora all work with my changes.

P.

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Ming Lei]

On Wed, Oct 23, 2013 at 7:15 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> On 10/21/2013 10:35 PM, Ming Lei wrote:
>>
>> That is why NOHOTPLUG isn't encouraged to be taken, actually
>> I don't suggest you to do that too, :-)
> Okay ... I can certainly switch to HOTPLUG.

OK, that should be the right approach.

>
>>
>> You need to make sure your approach won't break micro-code
>> update application in current/previous distributions.
>
> I've tested the following distributions today on a Dell PE 1850:  Ubuntu, SuSe,
> Linux Mint, and of course Fedora.  I do not see any issues with either the
> microcode update or the dell_rbu driver.  Unfortunately I do not have access to

Actually I am wondering if your tests are enough because kernel
can't break user-space, which means lots of previous old version
distributions should surely be covered, :-)

If you keep HOTPLUG, only change to request_firmware_nowait(),
that should be OK since the loading protocol between userspace and
kernel won't change wrt. microcode updating.

> a system that uses the lattice-ecp3-config, however, from code inspection it
> looks like the driver looks at a specific place for the FW update and then
> applies it via the call function in request_firmware_nowait() so it looks like
> it is solid too.
>
> I think maybe this patchset should be split into two separate submits, one for
> the microcode and the second to figure out if the code really should wait
> indefinitely.  AFAICT neither use case in the kernel expects an indefinite wait.

If you switch to HOTPLUG, you needn't worry about waiting indefinitely,
need you?

Thanks,
--
Ming Lei

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Prarit Bhargava]

On 10/23/2013 12:16 AM, Ming Lei wrote:
> On Wed, Oct 23, 2013 at 7:15 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> On 10/21/2013 10:35 PM, Ming Lei wrote:
>>>
>>> That is why NOHOTPLUG isn't encouraged to be taken, actually
>>> I don't suggest you to do that too, :-)
>> Okay ... I can certainly switch to HOTPLUG.
> 
> OK, that should be the right approach.
> 
>>
>>>
>>> You need to make sure your approach won't break micro-code
>>> update application in current/previous distributions.
>>
>> I've tested the following distributions today on a Dell PE 1850:  Ubuntu, SuSe,
>> Linux Mint, and of course Fedora.  I do not see any issues with either the
>> microcode update or the dell_rbu driver.  Unfortunately I do not have access to
> 
> Actually I am wondering if your tests are enough because kernel
> can't break user-space, which means lots of previous old version
> distributions should surely be covered, :-)

I've tested an old version of Suse and a few older RHEL versions for kicks.  No
problems.  I'm testing an older version of Ubuntu ATM and will update with
details (it doesn't look like it does anything different FWIW so I'm not worried).

> 
> If you keep HOTPLUG, only change to request_firmware_nowait(),
> that should be OK since the loading protocol between userspace and
> kernel won't change wrt. microcode updating.
> 
>> a system that uses the lattice-ecp3-config, however, from code inspection it
>> looks like the driver looks at a specific place for the FW update and then
>> applies it via the call function in request_firmware_nowait() so it looks like
>> it is solid too.
>>
>> I think maybe this patchset should be split into two separate submits, one for
>> the microcode and the second to figure out if the code really should wait
>> indefinitely.  AFAICT neither use case in the kernel expects an indefinite wait.
> 
> If you switch to HOTPLUG, you needn't worry about waiting indefinitely,
> need you?

Nope ... I'll modify the code and retest.

Thanks for the input Ming :)

P.


> 
> Thanks,
> --
> Ming Lei

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Prarit Bhargava]

On 10/23/2013 06:36 AM, Prarit Bhargava wrote:
> 
> 
> On 10/23/2013 12:16 AM, Ming Lei wrote:
>> On Wed, Oct 23, 2013 at 7:15 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>> On 10/21/2013 10:35 PM, Ming Lei wrote:
>>>>
>>>> That is why NOHOTPLUG isn't encouraged to be taken, actually
>>>> I don't suggest you to do that too, :-)
>>> Okay ... I can certainly switch to HOTPLUG.
>>
>> OK, that should be the right approach.
>>
>>>
>>>>
>>>> You need to make sure your approach won't break micro-code
>>>> update application in current/previous distributions.
>>>
>>> I've tested the following distributions today on a Dell PE 1850:  Ubuntu, SuSe,
>>> Linux Mint, and of course Fedora.  I do not see any issues with either the
>>> microcode update or the dell_rbu driver.  Unfortunately I do not have access to
>>
>> Actually I am wondering if your tests are enough because kernel
>> can't break user-space, which means lots of previous old version
>> distributions should surely be covered, :-)
> 
> I've tested an old version of Suse and a few older RHEL versions for kicks.  No
> problems.  I'm testing an older version of Ubuntu ATM and will update with
> details (it doesn't look like it does anything different FWIW so I'm not worried).
> 
>>
>> If you keep HOTPLUG, only change to request_firmware_nowait(),
>> that should be OK since the loading protocol between userspace and
>> kernel won't change wrt. microcode updating.
>>
>>> a system that uses the lattice-ecp3-config, however, from code inspection it
>>> looks like the driver looks at a specific place for the FW update and then
>>> applies it via the call function in request_firmware_nowait() so it looks like
>>> it is solid too.
>>>
>>> I think maybe this patchset should be split into two separate submits, one for
>>> the microcode and the second to figure out if the code really should wait
>>> indefinitely.  AFAICT neither use case in the kernel expects an indefinite wait.
>>
>> If you switch to HOTPLUG, you needn't worry about waiting indefinitely,
>> need you?
> 
> Nope ... I'll modify the code and retest.  

After all this I completely forgot the problem I'm trying to solve here.  The
issue is that with HOTPLUG & request_microcode_nowait(), if the microcode image
is not found (that is the file is not found on disk), then EACH cpu waits 1
minute and it takes 2 hours for a 120 cpu box to load the microcode module.

Which is terrible... so HOTPLUG doesn't work here.

Let me back up Ming and see if you have a better solution for me.  I have a
system that does not have the x86 microcode loaded on disk.  I use the microcode
module which calls request_firmware_nowait() to load the microcode image and I
want it done as fast as possible.  The microcode loader does not have a uevent
so I'm not waiting on userspace for completion.

How do I avoid the 60 second delay/cpu introduced in the microcode path?  I
don't see one.  If I use HOTPLUG I'm waiting 60 seconds.  If I use NOHOTPLUG
AFAICT the loading function never will return.  AFAICT the same issue arises
with the dell_rbu code -- it appears to never load the dell_rbu firmware.

P.

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Ming Lei]

On Wed, Oct 23, 2013 at 8:02 PM, Prarit Bhargava <prarit@redhat.com> wrote:

>
> After all this I completely forgot the problem I'm trying to solve here.  The
> issue is that with HOTPLUG & request_microcode_nowait(), if the microcode image
> is not found (that is the file is not found on disk), then EACH cpu waits 1
> minute and it takes 2 hours for a 120 cpu box to load the microcode module.
>
> Which is terrible... so HOTPLUG doesn't work here.
>
> Let me back up Ming and see if you have a better solution for me.  I have a
> system that does not have the x86 microcode loaded on disk.  I use the microcode
> module which calls request_firmware_nowait() to load the microcode image and I
> want it done as fast as possible.  The microcode loader does not have a uevent
> so I'm not waiting on userspace for completion.
>
> How do I avoid the 60 second delay/cpu introduced in the microcode path?  I
> don't see one.  If I use HOTPLUG I'm waiting 60 seconds.  If I use NOHOTPLUG
> AFAICT the loading function never will return.  AFAICT the same issue arises
> with the dell_rbu code -- it appears to never load the dell_rbu firmware.

As I said, the 60 second delay is from udev, so that is the root cause.

There are some workarounds for your reference:

- fix udev

- disable CONFIG_FW_LOADER_USER_HELPER if you are sure
the microcode image is under the default firmware search paths, which
are defined in drivers/base/firmware_class.c

- fake a empty latest microcode image under your firmware path

- use request_firmware_nowait(HOTPLUG) to request firmware from
user space, and apply the microcode at a appropriate time after the
request is completed, and the approach is what we are discussing.


Thanks,
--
Ming Lei

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Prarit Bhargava]

On 10/23/2013 09:21 AM, Ming Lei wrote:
> On Wed, Oct 23, 2013 at 8:02 PM, Prarit Bhargava <prarit@redhat.com> wrote:
> 
>>
>> After all this I completely forgot the problem I'm trying to solve here.  The
>> issue is that with HOTPLUG & request_microcode_nowait(), if the microcode image
>> is not found (that is the file is not found on disk), then EACH cpu waits 1
>> minute and it takes 2 hours for a 120 cpu box to load the microcode module.
>>
>> Which is terrible... so HOTPLUG doesn't work here.
>>
>> Let me back up Ming and see if you have a better solution for me.  I have a
>> system that does not have the x86 microcode loaded on disk.  I use the microcode
>> module which calls request_firmware_nowait() to load the microcode image and I
>> want it done as fast as possible.  The microcode loader does not have a uevent
>> so I'm not waiting on userspace for completion.
>>
>> How do I avoid the 60 second delay/cpu introduced in the microcode path?  I
>> don't see one.  If I use HOTPLUG I'm waiting 60 seconds.  If I use NOHOTPLUG
>> AFAICT the loading function never will return.  AFAICT the same issue arises
>> with the dell_rbu code -- it appears to never load the dell_rbu firmware.
> 
> As I said, the 60 second delay is from udev, so that is the root cause.

Okay, so then my other option is to use NOHOTPLUG.  I've correctly modified it
to return and not wait for a NULL uevent.  The synchronization between the cont
function return and the actual application of the firmware is done in the driver
(in my 2/2 patch) where I've used a completion struct.  ... Am I still missing
something at this point?

I also have to make that change to the dell_rbu code because it too, is broken
the same way.  That is, I can load the dell_rbu module and it just hangs without
applying the firmware.

I'll submit a new version of these patches and we can continue from there.

P.
> 
> There are some workarounds for your reference:
> 

These are workarounds for an issue that arises in the kernel.

P.

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Ming Lei]

On Wed, Oct 23, 2013 at 10:08 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>
>
> On 10/23/2013 09:21 AM, Ming Lei wrote:
>> On Wed, Oct 23, 2013 at 8:02 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>
>>>
>>> After all this I completely forgot the problem I'm trying to solve here.  The
>>> issue is that with HOTPLUG & request_microcode_nowait(), if the microcode image
>>> is not found (that is the file is not found on disk), then EACH cpu waits 1
>>> minute and it takes 2 hours for a 120 cpu box to load the microcode module.
>>>
>>> Which is terrible... so HOTPLUG doesn't work here.
>>>
>>> Let me back up Ming and see if you have a better solution for me.  I have a
>>> system that does not have the x86 microcode loaded on disk.  I use the microcode
>>> module which calls request_firmware_nowait() to load the microcode image and I
>>> want it done as fast as possible.  The microcode loader does not have a uevent
>>> so I'm not waiting on userspace for completion.
>>>
>>> How do I avoid the 60 second delay/cpu introduced in the microcode path?  I
>>> don't see one.  If I use HOTPLUG I'm waiting 60 seconds.  If I use NOHOTPLUG
>>> AFAICT the loading function never will return.  AFAICT the same issue arises
>>> with the dell_rbu code -- it appears to never load the dell_rbu firmware.
>>
>> As I said, the 60 second delay is from udev, so that is the root cause.
>
> Okay, so then my other option is to use NOHOTPLUG.  I've correctly modified it
> to return and not wait for a NULL uevent.  The synchronization between the cont
> function return and the actual application of the firmware is done in the driver
> (in my 2/2 patch) where I've used a completion struct.  ... Am I still missing
> something at this point?

If you plan to use NOHOTPLUG to stop sending uevent to user space, you
need to make sure all distributions(include old ones) do not require
the notification previously, and you'd better to explain the microcode
upgrading principle in a bit detail so that we can make sure it won't
break the loading protocol between kernel and user space, at least
current code is using request_fimrware() and the uevent is surely
sent to userspace, right?

>
> I also have to make that change to the dell_rbu code because it too, is broken
> the same way.  That is, I can load the dell_rbu module and it just hangs without
> applying the firmware.

Because userspace does not handle the request and write fw data to driver,
how can you expect the driver to apply firmware?

Anyway, you need Cc dell_rbu guys to make sure the change.

> I'll submit a new version of these patches and we can continue from there.
>
> P.
>>
>> There are some workarounds for your reference:
>>
>
> These are workarounds for an issue that arises in the kernel.

Sorry, I don't understand, the root cause is surely from udev.

Thanks,
--
Ming Lei

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Henrique de Moraes Holschuh]

On Wed, 23 Oct 2013, Prarit Bhargava wrote:
> After all this I completely forgot the problem I'm trying to solve here.  The
> issue is that with HOTPLUG & request_microcode_nowait(), if the microcode image
> is not found (that is the file is not found on disk), then EACH cpu waits 1
> minute and it takes 2 hours for a 120 cpu box to load the microcode module.

The proper fix seems to be teaching the concept of negative caching to the
microcode core/drivers, as it was pointed out elsewhere in the thread.
Negative caching should have a lifetime of "the current update-all-cores
request".

This would fix the absurd compound timeout delays, as on most systems it
will result in just one timeout (the first one).

That first timeout can be fixed by the user if they disable the userspace
firmware loader helper.  IMHO that might well be the best choice, as it is
already the way forward.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent

[Prarit Bhargava]

On 10/24/2013 07:17 AM, Henrique de Moraes Holschuh wrote:
> On Wed, 23 Oct 2013, Prarit Bhargava wrote:
>> After all this I completely forgot the problem I'm trying to solve here.  The
>> issue is that with HOTPLUG & request_microcode_nowait(), if the microcode image
>> is not found (that is the file is not found on disk), then EACH cpu waits 1
>> minute and it takes 2 hours for a 120 cpu box to load the microcode module.
> 
> The proper fix seems to be teaching the concept of negative caching to the
> microcode core/drivers, as it was pointed out elsewhere in the thread.
> Negative caching should have a lifetime of "the current update-all-cores
> request".
> 

Yes, I'm implementing v2 to do this already; caching the microcode is obvious.
I was actually looking at the code to see if there was a reason that each
processor needs to do a load request but cannot see one.  I'm modifying the
microcode driver to do this, as I said, in v2.

> This would fix the absurd compound timeout delays, as on most systems it
> will result in just one timeout (the first one).
> 
> That first timeout can be fixed by the user if they disable the userspace
> firmware loader helper.  IMHO that might well be the best choice, as it is
> already the way forward.

The problem with that is I may have a configuration which depends on having the
userspace firmware loader helper for a device, but not the processors so IMO it
isn't a complete solution.

I've also toyed with the idea that there should be a request_firmware_timeout()
in which a timeout for HOTPLUG can be specified.

P.