* [PATCH 0/2] Improve firmware loading times on AMD and Intel @ 2013-10-20 21:35 Prarit Bhargava 2013-10-20 21:35 ` [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent Prarit Bhargava ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Prarit Bhargava @ 2013-10-20 21:35 UTC (permalink / raw) To: linux-kernel; +Cc: Prarit Bhargava, x86, herrmann.der.user, ming.lei, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-20 21:35 [PATCH 0/2] Improve firmware loading times on AMD and Intel Prarit Bhargava @ 2013-10-20 21:35 ` Prarit Bhargava 2013-10-21 12:24 ` Ming Lei 2013-10-20 21:35 ` [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing Prarit Bhargava 2013-10-20 22:58 ` [PATCH 0/2] Improve firmware loading times on AMD and Intel Andi Kleen 2 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-20 21:35 UTC (permalink / raw) To: linux-kernel; +Cc: Prarit Bhargava, x86, herrmann.der.user, ming.lei, tigran 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-20 21:35 ` [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent Prarit Bhargava @ 2013-10-21 12:24 ` Ming Lei 2013-10-21 22:24 ` Prarit Bhargava 0 siblings, 1 reply; 22+ messages in thread From: Ming Lei @ 2013-10-21 12:24 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, herrmann.der.user, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-21 12:24 ` Ming Lei @ 2013-10-21 22:24 ` Prarit Bhargava 2013-10-22 2:35 ` Ming Lei 0 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-21 22:24 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, herrmann.der.user, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-21 22:24 ` Prarit Bhargava @ 2013-10-22 2:35 ` Ming Lei 2013-10-22 23:15 ` Prarit Bhargava 0 siblings, 1 reply; 22+ messages in thread From: Ming Lei @ 2013-10-22 2:35 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-22 2:35 ` Ming Lei @ 2013-10-22 23:15 ` Prarit Bhargava 2013-10-23 4:16 ` Ming Lei 0 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-22 23:15 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-22 23:15 ` Prarit Bhargava @ 2013-10-23 4:16 ` Ming Lei 2013-10-23 10:36 ` Prarit Bhargava 0 siblings, 1 reply; 22+ messages in thread From: Ming Lei @ 2013-10-23 4:16 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-23 4:16 ` Ming Lei @ 2013-10-23 10:36 ` Prarit Bhargava 2013-10-23 12:02 ` Prarit Bhargava 0 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-23 10:36 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-23 10:36 ` Prarit Bhargava @ 2013-10-23 12:02 ` Prarit Bhargava 2013-10-23 13:21 ` Ming Lei 2013-10-24 11:17 ` Henrique de Moraes Holschuh 0 siblings, 2 replies; 22+ messages in thread From: Prarit Bhargava @ 2013-10-23 12:02 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-23 12:02 ` Prarit Bhargava @ 2013-10-23 13:21 ` Ming Lei 2013-10-23 14:08 ` Prarit Bhargava 2013-10-24 11:17 ` Henrique de Moraes Holschuh 1 sibling, 1 reply; 22+ messages in thread From: Ming Lei @ 2013-10-23 13:21 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-23 13:21 ` Ming Lei @ 2013-10-23 14:08 ` Prarit Bhargava 2013-10-24 1:54 ` Ming Lei 0 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-23 14:08 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-23 14:08 ` Prarit Bhargava @ 2013-10-24 1:54 ` Ming Lei 0 siblings, 0 replies; 22+ messages in thread From: Ming Lei @ 2013-10-24 1:54 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-23 12:02 ` Prarit Bhargava 2013-10-23 13:21 ` Ming Lei @ 2013-10-24 11:17 ` Henrique de Moraes Holschuh 2013-10-24 12:05 ` Prarit Bhargava 1 sibling, 1 reply; 22+ messages in thread From: Henrique de Moraes Holschuh @ 2013-10-24 11:17 UTC (permalink / raw) To: Prarit Bhargava Cc: Ming Lei, Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent 2013-10-24 11:17 ` Henrique de Moraes Holschuh @ 2013-10-24 12:05 ` Prarit Bhargava 0 siblings, 0 replies; 22+ messages in thread From: Prarit Bhargava @ 2013-10-24 12:05 UTC (permalink / raw) To: Henrique de Moraes Holschuh Cc: Ming Lei, Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing 2013-10-20 21:35 [PATCH 0/2] Improve firmware loading times on AMD and Intel Prarit Bhargava 2013-10-20 21:35 ` [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent Prarit Bhargava @ 2013-10-20 21:35 ` Prarit Bhargava 2013-10-21 12:20 ` Ming Lei 2013-10-20 22:58 ` [PATCH 0/2] Improve firmware loading times on AMD and Intel Andi Kleen 2 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-20 21:35 UTC (permalink / raw) To: linux-kernel; +Cc: Prarit Bhargava, x86, herrmann.der.user, ming.lei, tigran 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, µcode_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, µcode_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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing 2013-10-20 21:35 ` [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing Prarit Bhargava @ 2013-10-21 12:20 ` Ming Lei 2013-10-21 12:26 ` Prarit Bhargava 0 siblings, 1 reply; 22+ messages in thread From: Ming Lei @ 2013-10-21 12:20 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, herrmann.der.user, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing 2013-10-21 12:20 ` Ming Lei @ 2013-10-21 12:26 ` Prarit Bhargava 2013-10-21 12:32 ` Ming Lei 0 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-21 12:26 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, herrmann.der.user, tigran 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing 2013-10-21 12:26 ` Prarit Bhargava @ 2013-10-21 12:32 ` Ming Lei 2013-10-21 14:25 ` Prarit Bhargava 0 siblings, 1 reply; 22+ messages in thread From: Ming Lei @ 2013-10-21 12:32 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, herrmann.der.user, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing 2013-10-21 12:32 ` Ming Lei @ 2013-10-21 14:25 ` Prarit Bhargava 2013-10-22 2:43 ` Ming Lei 0 siblings, 1 reply; 22+ messages in thread From: Prarit Bhargava @ 2013-10-21 14:25 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, herrmann.der.user, tigran 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing 2013-10-21 14:25 ` Prarit Bhargava @ 2013-10-22 2:43 ` Ming Lei 2013-10-22 23:16 ` Prarit Bhargava 0 siblings, 1 reply; 22+ messages in thread From: Ming Lei @ 2013-10-22 2:43 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing 2013-10-22 2:43 ` Ming Lei @ 2013-10-22 23:16 ` Prarit Bhargava 0 siblings, 0 replies; 22+ messages in thread From: Prarit Bhargava @ 2013-10-22 23:16 UTC (permalink / raw) To: Ming Lei; +Cc: Linux Kernel Mailing List, x86, Andreas Herrmann, tigran 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] Improve firmware loading times on AMD and Intel 2013-10-20 21:35 [PATCH 0/2] Improve firmware loading times on AMD and Intel Prarit Bhargava 2013-10-20 21:35 ` [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent Prarit Bhargava 2013-10-20 21:35 ` [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing Prarit Bhargava @ 2013-10-20 22:58 ` Andi Kleen 2 siblings, 0 replies; 22+ messages in thread From: Andi Kleen @ 2013-10-20 22:58 UTC (permalink / raw) To: Prarit Bhargava; +Cc: linux-kernel, x86, herrmann.der.user, ming.lei, tigran 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-10-24 12:06 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-10-20 21:35 [PATCH 0/2] Improve firmware loading times on AMD and Intel Prarit Bhargava 2013-10-20 21:35 ` [PATCH 1/2] firmware, fix request_firmware_nowait() freeze with no uevent Prarit Bhargava 2013-10-21 12:24 ` Ming Lei 2013-10-21 22:24 ` Prarit Bhargava 2013-10-22 2:35 ` Ming Lei 2013-10-22 23:15 ` Prarit Bhargava 2013-10-23 4:16 ` Ming Lei 2013-10-23 10:36 ` Prarit Bhargava 2013-10-23 12:02 ` Prarit Bhargava 2013-10-23 13:21 ` Ming Lei 2013-10-23 14:08 ` Prarit Bhargava 2013-10-24 1:54 ` Ming Lei 2013-10-24 11:17 ` Henrique de Moraes Holschuh 2013-10-24 12:05 ` Prarit Bhargava 2013-10-20 21:35 ` [PATCH 2/2] intel_microcode, Fix long microcode load time when firmware file is missing Prarit Bhargava 2013-10-21 12:20 ` Ming Lei 2013-10-21 12:26 ` Prarit Bhargava 2013-10-21 12:32 ` Ming Lei 2013-10-21 14:25 ` Prarit Bhargava 2013-10-22 2:43 ` Ming Lei 2013-10-22 23:16 ` Prarit Bhargava 2013-10-20 22:58 ` [PATCH 0/2] Improve firmware loading times on AMD and Intel Andi Kleen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).