[PATCH] x86 microcode: revert some work_on_cpu

[PATCH] x86 microcode: revert some work_on_cpu

[Hugh Dickins]

Revert part of af5c820a3169e81af869c113e18ec7588836cd50
x86: cpumask: use work_on_cpu in arch/x86/kernel/microcode_core.c

That change is causing only one Intel CPU's microcode to be updated e.g.
microcode: CPU3 updated from revision 0x9 to 0x17, date = 2005-04-22 
where before it announced that also for CPU0 and CPU1 and CPU2.

We cannot use work_on_cpu() in the CONFIG_MICROCODE_OLD_INTERFACE code,
because Intel's request_microcode_user() involves a copy_from_user() from
/sbin/microcode_ctl, which therefore needs to be on that CPU at the time.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
This may be not the only problem with that commit: I've seen lockdep
warnings from s2ram when suspending; but I think there have been other
work_on_cpu() lockdep issues, and you may already be on to them?

 arch/x86/kernel/microcode_core.c |   33 +++++++++--------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

--- 2.6.30-rc1/arch/x86/kernel/microcode_core.c	2009-04-08 18:19:28.000000000 +0100
+++ linux/arch/x86/kernel/microcode_core.c	2009-04-12 23:06:57.000000000 +0100
@@ -108,40 +108,29 @@ struct ucode_cpu_info		ucode_cpu_info[NR
 EXPORT_SYMBOL_GPL(ucode_cpu_info);
 
 #ifdef CONFIG_MICROCODE_OLD_INTERFACE
-struct update_for_cpu {
-	const void __user	*buf;
-	size_t			size;
-};
-
-static long update_for_cpu(void *_ufc)
-{
-	struct update_for_cpu *ufc = _ufc;
-	int error;
-
-	error = microcode_ops->request_microcode_user(smp_processor_id(),
-						      ufc->buf, ufc->size);
-	if (error < 0)
-		return error;
-	if (!error)
-		microcode_ops->apply_microcode(smp_processor_id());
-	return error;
-}
-
 static int do_microcode_update(const void __user *buf, size_t size)
 {
+	cpumask_t old;
 	int error = 0;
 	int cpu;
-	struct update_for_cpu ufc = { .buf = buf, .size = size };
+
+	old = current->cpus_allowed;
 
 	for_each_online_cpu(cpu) {
 		struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
 		if (!uci->valid)
 			continue;
-		error = work_on_cpu(cpu, update_for_cpu, &ufc);
+
+		set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
+		error = microcode_ops->request_microcode_user(cpu, buf, size);
 		if (error < 0)
-			break;
+			goto out;
+		if (!error)
+			microcode_ops->apply_microcode(cpu);
 	}
+out:
+	set_cpus_allowed_ptr(current, &old);
 	return error;
 }
 

Re: [PATCH] x86 microcode: revert some work_on_cpu

[Rusty Russell]

On Wed, 15 Apr 2009 03:55:42 am Hugh Dickins wrote:
> Revert part of af5c820a3169e81af869c113e18ec7588836cd50
> x86: cpumask: use work_on_cpu in arch/x86/kernel/microcode_core.c
> 
> That change is causing only one Intel CPU's microcode to be updated e.g.
> microcode: CPU3 updated from revision 0x9 to 0x17, date = 2005-04-22 
> where before it announced that also for CPU0 and CPU1 and CPU2.
> 
> We cannot use work_on_cpu() in the CONFIG_MICROCODE_OLD_INTERFACE code,
> because Intel's request_microcode_user() involves a copy_from_user() from
> /sbin/microcode_ctl, which therefore needs to be on that CPU at the time.

Erk.  Ack the reversion, but this needs to be fixed properly.

We can't just mug a process's affinity.  I'll look at this code again and
see what I can do.

> May be not the only problem with that commit: I've seen lockdep
> warnings from s2ram when suspending; but I think there have been other
> work_on_cpu() lockdep issues, and you may already be on to them?

Yep.

Thanks,
Rusty.

Re: [PATCH] x86 microcode: revert some work_on_cpu

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 17019 bytes --]

2009/4/15 Rusty Russell <rusty@rustcorp.com.au>:
> On Wed, 15 Apr 2009 03:55:42 am Hugh Dickins wrote:
>> Revert part of af5c820a3169e81af869c113e18ec7588836cd50
>> x86: cpumask: use work_on_cpu in arch/x86/kernel/microcode_core.c
>>
>> That change is causing only one Intel CPU's microcode to be updated e.g.
>> microcode: CPU3 updated from revision 0x9 to 0x17, date = 2005-04-22
>> where before it announced that also for CPU0 and CPU1 and CPU2.
>>
>> We cannot use work_on_cpu() in the CONFIG_MICROCODE_OLD_INTERFACE code,
>> because Intel's request_microcode_user() involves a copy_from_user() from
>> /sbin/microcode_ctl, which therefore needs to be on that CPU at the time.
>
> Erk.  Ack the reversion, but this needs to be fixed properly.
>
> We can't just mug a process's affinity.  I'll look at this code again and
> see what I can do.

Rusty,


what's about something like below?

Disclaimer: it's not even compilation-tested, consider it merely as an
illustration of an approach. I'll be able to test it later.

- run collect_cpu_info() and apply_microcode() on a target  cpu, the
rest of callbacks are just fine to run on any cpu;

- some synchronization changes (based on
http://lkml.indiana.edu/hypermail/linux/kernel/0903.1/00525.html)
See the "Synchronization" note in microcode_core.c

- removed sysfs_remove_group() from mc_sysdev_add() to avoid warnings
being triggered in some cases (there were a few reports on lkml) --
perhaps, to be reworked.

- some things perhaps need to be changed, e.g. add a return code to
apply_microcode(), ...

(the non-white-space damaged version is enclosed)

diff -upr linux-2.6.git/arch/x86/include/asm/microcode.h
linux-2.6.git.my/arch/x86/include/asm/microcode.h
--- linux-2.6.git/arch/x86/include/asm/microcode.h      2009-04-14
22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/include/asm/microcode.h   2009-04-15
11:49:47.000000000 +0200
@@ -13,10 +13,16 @@ struct microcode_ops {
        int  (*request_microcode_user) (int cpu, const void __user
*buf, size_t size);
        int  (*request_microcode_fw) (int cpu, struct device *device);

-       void (*apply_microcode) (int cpu);
+       void (*microcode_fini_cpu) (int cpu);

+       /*
+        * The generic 'microcode_core' part guarantees that
+        * the callbacks below run on a target cpu when they
+        * are being called.
+        * See also the "Synchronization" section in microcode_core.c.
+        */
+       void (*apply_microcode) (int cpu);
        int  (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
-       void (*microcode_fini_cpu) (int cpu);
 };

 struct ucode_cpu_info {
diff -upr linux-2.6.git/arch/x86/kernel/microcode_amd.c
linux-2.6.git.my/arch/x86/kernel/microcode_amd.c
--- linux-2.6.git/arch/x86/kernel/microcode_amd.c       2009-04-14
22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/kernel/microcode_amd.c    2009-04-15
11:46:38.000000000 +0200
@@ -17,7 +17,6 @@
 #include <linux/capability.h>
 #include <linux/miscdevice.h>
 #include <linux/firmware.h>
-#include <linux/spinlock.h>
 #include <linux/cpumask.h>
 #include <linux/pci_ids.h>
 #include <linux/uaccess.h>
@@ -79,9 +78,6 @@ struct microcode_amd {
 #define UCODE_CONTAINER_SECTION_HDR    8
 #define UCODE_CONTAINER_HEADER_SIZE    12

-/* serialize access to the physical write */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
 static struct equiv_cpu_entry *equiv_cpu_table;

 static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
@@ -158,11 +154,9 @@ static void apply_microcode_amd(int cpu)
        if (mc_amd == NULL)
                return;

-       spin_lock_irqsave(&microcode_update_lock, flags);
        wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
        /* get patch id after patching */
        rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-       spin_unlock_irqrestore(&microcode_update_lock, flags);

        /* check current patch id and patch's id for match */
        if (rev != mc_amd->hdr.patch_id) {
@@ -327,9 +321,6 @@ static int request_microcode_fw(int cpu,
        const struct firmware *firmware;
        int ret;

-       /* We should bind the task to the CPU */
-       BUG_ON(cpu != raw_smp_processor_id());
-
        ret = request_firmware(&firmware, fw_name, device);
        if (ret) {
                printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
diff -upr linux-2.6.git/arch/x86/kernel/microcode_core.c
linux-2.6.git.my/arch/x86/kernel/microcode_core.c
--- linux-2.6.git/arch/x86/kernel/microcode_core.c      2009-04-14
22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/kernel/microcode_core.c   2009-04-15
11:45:48.000000000 +0200
@@ -101,36 +101,89 @@ MODULE_LICENSE("GPL");

 static struct microcode_ops    *microcode_ops;

-/* no concurrent ->write()s are allowed on /dev/cpu/microcode */
+/*
+ * Synchronization.
+ *
+ * All non cpu-hotplug-callback call sites use:
+ *
+ * - microcode_mutex to synchronize with each other;
+ * - get/put_online_cpus() to synchronize with
+ *   the cpu-hotplug-callback call sites.
+ *
+ * We guarantee that only a single cpu is being
+ * updated at any particular moment of time.
+ */
 static DEFINE_MUTEX(microcode_mutex);

 struct ucode_cpu_info          ucode_cpu_info[NR_CPUS];
 EXPORT_SYMBOL_GPL(ucode_cpu_info);

+/*
+ * Operations that are run on a target cpu:
+ */
+
+struct collect_for_cpu {
+       struct cpu_signature    cpu_sig;
+       int                     cpu;
+};
+
+static long collect_cpu_info_local(void *arg)
+{
+       struct collect_for_cpu *cfc = arg;
+
+       BUG_ON(cfc->cpu != smp_processor_id());
+
+       return microcode_ops->collect_cpu_info(cfc->cpu, &cfc->cpu_sig);
+}
+
+static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
+{
+       return work_on_cpu(cpu, collect_cpu_info_local, cpu_sig);
+}
+
+static void collect_cpu_info(int cpu)
+{
+       struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+       memset(uci, 0, sizeof(*uci));
+       if (!collect_cpu_info_on_target(cpu, &uci->cpu_sig))
+               uci->valid = 1;
+}
+
+static long apply_microcode_local(void *arg)
+{
+       int cpu = (int)arg;
+
+       BUG_ON(cpu != smp_processor_id());
+       microcode_ops->apply_microcode(cpu);
+
+       return 0;
+}
+
+static int apply_microcode_on_target(int cpu)
+{
+       return work_on_cpu(cpu, apply_microcode_local, (void *)cpu);
+}
+
 #ifdef CONFIG_MICROCODE_OLD_INTERFACE
 static int do_microcode_update(const void __user *buf, size_t size)
 {
-       cpumask_t old;
        int error = 0;
        int cpu;

-       old = current->cpus_allowed;
-
        for_each_online_cpu(cpu) {
                struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

                if (!uci->valid)
                        continue;

-               set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
                error = microcode_ops->request_microcode_user(cpu, buf, size);
                if (error < 0)
-                       goto out;
+                       break;
                if (!error)
-                       microcode_ops->apply_microcode(cpu);
+                       apply_microcode_on_target(cpu);
        }
-out:
-       set_cpus_allowed_ptr(current, &old);
+
        return error;
 }

@@ -205,17 +258,16 @@ MODULE_ALIAS_MISCDEV(MICROCODE_MINOR);
 /* fake device for request_firmware */
 static struct platform_device  *microcode_pdev;

-static long reload_for_cpu(void *unused)
+static int reload_for_cpu(int cpu)
 {
-       struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+       struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
        int err = 0;

        mutex_lock(&microcode_mutex);
        if (uci->valid) {
-               err = microcode_ops->request_microcode_fw(smp_processor_id(),
-                                                         &microcode_pdev->dev);
+               err = microcode_ops->request_microcode_fw(cpu,
&microcode_pdev->dev);
                if (!err)
-                       microcode_ops->apply_microcode(smp_processor_id());
+                       apply_microcode_on_target(cpu);
        }
        mutex_unlock(&microcode_mutex);
        return err;
@@ -235,7 +287,7 @@ static ssize_t reload_store(struct sys_d
        if (val == 1) {
                get_online_cpus();
                if (cpu_online(cpu))
-                       err = work_on_cpu(cpu, reload_for_cpu, NULL);
+                       err = reload_for_cpu(cpu);
                put_online_cpus();
        }
        if (err)
@@ -275,7 +327,7 @@ static struct attribute_group mc_attr_gr
        .name           = "microcode",
 };

-static void __microcode_fini_cpu(int cpu)
+static void microcode_fini_cpu(int cpu)
 {
        struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

@@ -283,22 +335,6 @@ static void __microcode_fini_cpu(int cpu
        uci->valid = 0;
 }

-static void microcode_fini_cpu(int cpu)
-{
-       mutex_lock(&microcode_mutex);
-       __microcode_fini_cpu(cpu);
-       mutex_unlock(&microcode_mutex);
-}
-
-static void collect_cpu_info(int cpu)
-{
-       struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
-       memset(uci, 0, sizeof(*uci));
-       if (!microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig))
-               uci->valid = 1;
-}
-
 static int microcode_resume_cpu(int cpu)
 {
        struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -313,15 +349,15 @@ static int microcode_resume_cpu(int cpu)
         * Let's verify that the 'cached' ucode does belong
         * to this cpu (a bit of paranoia):
         */
-       if (microcode_ops->collect_cpu_info(cpu, &nsig)) {
-               __microcode_fini_cpu(cpu);
+       if (collect_cpu_info_on_target(cpu, &nsig)) {
+               microcode_fini_cpu(cpu);
                printk(KERN_ERR "failed to collect_cpu_info for
resuming cpu #%d\n",
                                cpu);
                return -1;
        }

        if ((nsig.sig != uci->cpu_sig.sig) || (nsig.pf != uci->cpu_sig.pf)) {
-               __microcode_fini_cpu(cpu);
+               microcode_fini_cpu(cpu);
                printk(KERN_ERR "cached ucode doesn't match the
resuming cpu #%d\n",
                                cpu);
                /* Should we look for a new ucode here? */
@@ -331,9 +367,9 @@ static int microcode_resume_cpu(int cpu)
        return 0;
 }

-static long microcode_update_cpu(void *unused)
+static int microcode_init_cpu(int cpu)
 {
-       struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+       struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
        int err = 0;

        /*
@@ -341,25 +377,16 @@ static long microcode_update_cpu(void *u
         * otherwise just request a firmware:
         */
        if (uci->valid) {
-               err = microcode_resume_cpu(smp_processor_id());
+               err = microcode_resume_cpu(cpu);
        } else {
-               collect_cpu_info(smp_processor_id());
+               collect_cpu_info(cpu);
                if (uci->valid && system_state == SYSTEM_RUNNING)
                        err = microcode_ops->request_microcode_fw(
-                                       smp_processor_id(),
+                                       cpu,
                                        &microcode_pdev->dev);
        }
        if (!err)
-               microcode_ops->apply_microcode(smp_processor_id());
-       return err;
-}
-
-static int microcode_init_cpu(int cpu)
-{
-       int err;
-       mutex_lock(&microcode_mutex);
-       err = work_on_cpu(cpu, microcode_update_cpu, NULL);
-       mutex_unlock(&microcode_mutex);
+               apply_microcode_on_target(cpu);

        return err;
 }
@@ -380,8 +407,6 @@ static int mc_sysdev_add(struct sys_devi
                return err;

        err = microcode_init_cpu(cpu);
-       if (err)
-               sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);

        return err;
 }
@@ -406,8 +431,17 @@ static int mc_sysdev_resume(struct sys_d
        if (!cpu_online(cpu))
                return 0;

-       /* only CPU 0 will apply ucode here */
-       microcode_update_cpu(NULL);
+       /*
+        * All non-bootup cpus are still disabled,
+        * so only CPU 0 will apply ucode here.
+        *
+        * Moreover, there can be no concurrent
+        * updates from any other places at this point.
+        */
+       WARN_ON(cpu != 0);
+
+       microcode_init_cpu(cpu);
+
        return 0;
 }

@@ -471,9 +505,6 @@ static int __init microcode_init(void)
                return -ENODEV;
        }

-       error = microcode_dev_init();
-       if (error)
-               return error;
        microcode_pdev = platform_device_register_simple("microcode", -1,
                                                         NULL, 0);
        if (IS_ERR(microcode_pdev)) {
@@ -482,14 +513,26 @@ static int __init microcode_init(void)
        }

        get_online_cpus();
+       /*
+        * --dimm. Hmm, we can avoid it if we perhaps first
+        * try to apply ucode in mc_sysdev_add() and only
+        * then create a sysfs group.
+        */
+       mutex_lock(&microcode_mutex);
        error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
+       mutex_unlock(&microcode_mutex);
+
        put_online_cpus();
+
        if (error) {
-               microcode_dev_exit();
                platform_device_unregister(microcode_pdev);
                return error;
        }

+       error = microcode_dev_init();
+       if (error)
+               return error;
+
        register_hotcpu_notifier(&mc_cpu_notifier);

        printk(KERN_INFO
@@ -507,7 +550,9 @@ static void __exit microcode_exit(void)
        unregister_hotcpu_notifier(&mc_cpu_notifier);

        get_online_cpus();
+       mutex_lock(&microcode_mutex);
        sysdev_driver_unregister(&cpu_sysdev_class, &mc_sysdev_driver);
+       mutex_unlock(&microcode_mutex);
        put_online_cpus();

        platform_device_unregister(microcode_pdev);
diff -upr linux-2.6.git/arch/x86/kernel/microcode_intel.c
linux-2.6.git.my/arch/x86/kernel/microcode_intel.c
--- linux-2.6.git/arch/x86/kernel/microcode_intel.c     2009-04-14
22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/kernel/microcode_intel.c  2009-04-15
11:47:21.000000000 +0200
@@ -75,7 +75,6 @@
 #include <linux/miscdevice.h>
 #include <linux/firmware.h>
 #include <linux/smp_lock.h>
-#include <linux/spinlock.h>
 #include <linux/cpumask.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
@@ -150,9 +149,6 @@ struct extended_sigtable {

 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)

-/* serialize access to the physical write to MSR 0x79 */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
 static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 {
        struct cpuinfo_x86 *c = &cpu_data(cpu_num);
@@ -176,15 +172,11 @@ static int collect_cpu_info(int cpu_num,
                csig->pf = 1 << ((val[1] >> 18) & 7);
        }

-       /* serialize access to the physical write to MSR 0x79 */
-       spin_lock_irqsave(&microcode_update_lock, flags);
-
        wrmsr(MSR_IA32_UCODE_REV, 0, 0);
        /* see notes above for revision 1.07.  Apparent chip bug */
        sync_core();
        /* get the current revision from MSR 0x8B */
        rdmsr(MSR_IA32_UCODE_REV, val[0], csig->rev);
-       spin_unlock_irqrestore(&microcode_update_lock, flags);

        pr_debug("microcode: collect_cpu_info : sig=0x%x, pf=0x%x, rev=0x%x\n",
                        csig->sig, csig->pf, csig->rev);
@@ -336,9 +328,6 @@ static void apply_microcode(int cpu)
        if (mc_intel == NULL)
                return;

-       /* serialize access to the physical write to MSR 0x79 */
-       spin_lock_irqsave(&microcode_update_lock, flags);
-
        /* write microcode via MSR 0x79 */
        wrmsr(MSR_IA32_UCODE_WRITE,
              (unsigned long) mc_intel->bits,
@@ -351,7 +340,6 @@ static void apply_microcode(int cpu)
        /* get the current revision from MSR 0x8B */
        rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

-       spin_unlock_irqrestore(&microcode_update_lock, flags);
        if (val[1] != mc_intel->hdr.rev) {
                printk(KERN_ERR "microcode: CPU%d update from revision "
                                "0x%x to 0x%x failed\n",
@@ -445,8 +433,6 @@ static int request_microcode_fw(int cpu,
        const struct firmware *firmware;
        int ret;

-       /* We should bind the task to the CPU */
-       BUG_ON(cpu != raw_smp_processor_id());
        sprintf(name, "intel-ucode/%02x-%02x-%02x",
                c->x86, c->x86_model, c->x86_mask);
        ret = request_firmware(&firmware, name, device);
@@ -470,9 +456,6 @@ static int get_ucode_user(void *to, cons

 static int request_microcode_user(int cpu, const void __user *buf, size_t size)
 {
-       /* We should bind the task to the CPU */
-       BUG_ON(cpu != raw_smp_processor_id());
-
        return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
 }



---





-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: microcode.patch --]
[-- Type: text/x-patch, Size: 13246 bytes --]

diff -upr linux-2.6.git/arch/x86/include/asm/microcode.h linux-2.6.git.my/arch/x86/include/asm/microcode.h
--- linux-2.6.git/arch/x86/include/asm/microcode.h	2009-04-14 22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/include/asm/microcode.h	2009-04-15 11:49:47.000000000 +0200
@@ -13,10 +13,16 @@ struct microcode_ops {
 	int  (*request_microcode_user) (int cpu, const void __user *buf, size_t size);
 	int  (*request_microcode_fw) (int cpu, struct device *device);
 
-	void (*apply_microcode) (int cpu);
+	void (*microcode_fini_cpu) (int cpu);
 
+	/* 
+	 * The generic 'microcode_core' part guarantees that
+	 * the callbacks below run on a target cpu when they
+	 * are being called.
+	 * See also the "Synchronization" section in microcode_core.c.
+	 */
+	void (*apply_microcode) (int cpu);
 	int  (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
-	void (*microcode_fini_cpu) (int cpu);
 };
 
 struct ucode_cpu_info {
diff -upr linux-2.6.git/arch/x86/kernel/microcode_amd.c linux-2.6.git.my/arch/x86/kernel/microcode_amd.c
--- linux-2.6.git/arch/x86/kernel/microcode_amd.c	2009-04-14 22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/kernel/microcode_amd.c	2009-04-15 11:46:38.000000000 +0200
@@ -17,7 +17,6 @@
 #include <linux/capability.h>
 #include <linux/miscdevice.h>
 #include <linux/firmware.h>
-#include <linux/spinlock.h>
 #include <linux/cpumask.h>
 #include <linux/pci_ids.h>
 #include <linux/uaccess.h>
@@ -79,9 +78,6 @@ struct microcode_amd {
 #define UCODE_CONTAINER_SECTION_HDR	8
 #define UCODE_CONTAINER_HEADER_SIZE	12
 
-/* serialize access to the physical write */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
 static struct equiv_cpu_entry *equiv_cpu_table;
 
 static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
@@ -158,11 +154,9 @@ static void apply_microcode_amd(int cpu)
 	if (mc_amd == NULL)
 		return;
 
-	spin_lock_irqsave(&microcode_update_lock, flags);
 	wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
 	/* get patch id after patching */
 	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-	spin_unlock_irqrestore(&microcode_update_lock, flags);
 
 	/* check current patch id and patch's id for match */
 	if (rev != mc_amd->hdr.patch_id) {
@@ -327,9 +321,6 @@ static int request_microcode_fw(int cpu,
 	const struct firmware *firmware;
 	int ret;
 
-	/* We should bind the task to the CPU */
-	BUG_ON(cpu != raw_smp_processor_id());
-
 	ret = request_firmware(&firmware, fw_name, device);
 	if (ret) {
 		printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
diff -upr linux-2.6.git/arch/x86/kernel/microcode_core.c linux-2.6.git.my/arch/x86/kernel/microcode_core.c
--- linux-2.6.git/arch/x86/kernel/microcode_core.c	2009-04-14 22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/kernel/microcode_core.c	2009-04-15 11:45:48.000000000 +0200
@@ -101,36 +101,89 @@ MODULE_LICENSE("GPL");
 
 static struct microcode_ops	*microcode_ops;
 
-/* no concurrent ->write()s are allowed on /dev/cpu/microcode */
+/*
+ * Synchronization.
+ *
+ * All non cpu-hotplug-callback call sites use:
+ *
+ * - microcode_mutex to synchronize with each other;
+ * - get/put_online_cpus() to synchronize with
+ *   the cpu-hotplug-callback call sites.
+ *
+ * We guarantee that only a single cpu is being
+ * updated at any particular moment of time.
+ */
 static DEFINE_MUTEX(microcode_mutex);
 
 struct ucode_cpu_info		ucode_cpu_info[NR_CPUS];
 EXPORT_SYMBOL_GPL(ucode_cpu_info);
 
+/*
+ * Operations that are run on a target cpu: 
+ */
+
+struct collect_for_cpu {
+	struct cpu_signature	cpu_sig;
+	int			cpu;
+};
+
+static long collect_cpu_info_local(void *arg)
+{
+	struct collect_for_cpu *cfc = arg;
+
+	BUG_ON(cfc->cpu != smp_processor_id());
+
+	return microcode_ops->collect_cpu_info(cfc->cpu, &cfc->cpu_sig);
+}
+
+static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
+{
+	return work_on_cpu(cpu, collect_cpu_info_local, cpu_sig);
+}
+
+static void collect_cpu_info(int cpu)
+{
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	memset(uci, 0, sizeof(*uci));
+	if (!collect_cpu_info_on_target(cpu, &uci->cpu_sig))
+		uci->valid = 1;
+}
+
+static long apply_microcode_local(void *arg)
+{
+	int cpu = (int)arg;
+
+	BUG_ON(cpu != smp_processor_id());
+	microcode_ops->apply_microcode(cpu);
+
+	return 0;
+}
+
+static int apply_microcode_on_target(int cpu)
+{
+	return work_on_cpu(cpu, apply_microcode_local, (void *)cpu);
+}
+
 #ifdef CONFIG_MICROCODE_OLD_INTERFACE
 static int do_microcode_update(const void __user *buf, size_t size)
 {
-	cpumask_t old;
 	int error = 0;
 	int cpu;
 
-	old = current->cpus_allowed;
-
 	for_each_online_cpu(cpu) {
 		struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
 		if (!uci->valid)
 			continue;
 
-		set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
 		error = microcode_ops->request_microcode_user(cpu, buf, size);
 		if (error < 0)
-			goto out;
+			break;
 		if (!error)
-			microcode_ops->apply_microcode(cpu);
+			apply_microcode_on_target(cpu);
 	}
-out:
-	set_cpus_allowed_ptr(current, &old);
+
 	return error;
 }
 
@@ -205,17 +258,16 @@ MODULE_ALIAS_MISCDEV(MICROCODE_MINOR);
 /* fake device for request_firmware */
 static struct platform_device	*microcode_pdev;
 
-static long reload_for_cpu(void *unused)
+static int reload_for_cpu(int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	int err = 0;
 
 	mutex_lock(&microcode_mutex);
 	if (uci->valid) {
-		err = microcode_ops->request_microcode_fw(smp_processor_id(),
-							  &microcode_pdev->dev);
+		err = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
 		if (!err)
-			microcode_ops->apply_microcode(smp_processor_id());
+			apply_microcode_on_target(cpu);
 	}
 	mutex_unlock(&microcode_mutex);
 	return err;
@@ -235,7 +287,7 @@ static ssize_t reload_store(struct sys_d
 	if (val == 1) {
 		get_online_cpus();
 		if (cpu_online(cpu))
-			err = work_on_cpu(cpu, reload_for_cpu, NULL);
+			err = reload_for_cpu(cpu);
 		put_online_cpus();
 	}
 	if (err)
@@ -275,7 +327,7 @@ static struct attribute_group mc_attr_gr
 	.name		= "microcode",
 };
 
-static void __microcode_fini_cpu(int cpu)
+static void microcode_fini_cpu(int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
@@ -283,22 +335,6 @@ static void __microcode_fini_cpu(int cpu
 	uci->valid = 0;
 }
 
-static void microcode_fini_cpu(int cpu)
-{
-	mutex_lock(&microcode_mutex);
-	__microcode_fini_cpu(cpu);
-	mutex_unlock(&microcode_mutex);
-}
-
-static void collect_cpu_info(int cpu)
-{
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
-	memset(uci, 0, sizeof(*uci));
-	if (!microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig))
-		uci->valid = 1;
-}
-
 static int microcode_resume_cpu(int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -313,15 +349,15 @@ static int microcode_resume_cpu(int cpu)
 	 * Let's verify that the 'cached' ucode does belong
 	 * to this cpu (a bit of paranoia):
 	 */
-	if (microcode_ops->collect_cpu_info(cpu, &nsig)) {
-		__microcode_fini_cpu(cpu);
+	if (collect_cpu_info_on_target(cpu, &nsig)) {
+		microcode_fini_cpu(cpu);
 		printk(KERN_ERR "failed to collect_cpu_info for resuming cpu #%d\n",
 				cpu);
 		return -1;
 	}
 
 	if ((nsig.sig != uci->cpu_sig.sig) || (nsig.pf != uci->cpu_sig.pf)) {
-		__microcode_fini_cpu(cpu);
+		microcode_fini_cpu(cpu);
 		printk(KERN_ERR "cached ucode doesn't match the resuming cpu #%d\n",
 				cpu);
 		/* Should we look for a new ucode here? */
@@ -331,9 +367,9 @@ static int microcode_resume_cpu(int cpu)
 	return 0;
 }
 
-static long microcode_update_cpu(void *unused)
+static int microcode_init_cpu(int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	int err = 0;
 
 	/*
@@ -341,25 +377,16 @@ static long microcode_update_cpu(void *u
 	 * otherwise just request a firmware:
 	 */
 	if (uci->valid) {
-		err = microcode_resume_cpu(smp_processor_id());
+		err = microcode_resume_cpu(cpu);
 	} else {
-		collect_cpu_info(smp_processor_id());
+		collect_cpu_info(cpu);
 		if (uci->valid && system_state == SYSTEM_RUNNING)
 			err = microcode_ops->request_microcode_fw(
-					smp_processor_id(),
+					cpu,
 					&microcode_pdev->dev);
 	}
 	if (!err)
-		microcode_ops->apply_microcode(smp_processor_id());
-	return err;
-}
-
-static int microcode_init_cpu(int cpu)
-{
-	int err;
-	mutex_lock(&microcode_mutex);
-	err = work_on_cpu(cpu, microcode_update_cpu, NULL);
-	mutex_unlock(&microcode_mutex);
+		apply_microcode_on_target(cpu);
 
 	return err;
 }
@@ -380,8 +407,6 @@ static int mc_sysdev_add(struct sys_devi
 		return err;
 
 	err = microcode_init_cpu(cpu);
-	if (err)
-		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
 
 	return err;
 }
@@ -406,8 +431,17 @@ static int mc_sysdev_resume(struct sys_d
 	if (!cpu_online(cpu))
 		return 0;
 
-	/* only CPU 0 will apply ucode here */
-	microcode_update_cpu(NULL);
+	/*
+	 * All non-bootup cpus are still disabled,
+	 * so only CPU 0 will apply ucode here.
+	 *
+	 * Moreover, there can be no concurrent
+	 * updates from any other places at this point.
+	 */
+	WARN_ON(cpu != 0);
+
+	microcode_init_cpu(cpu);
+
 	return 0;
 }
 
@@ -471,9 +505,6 @@ static int __init microcode_init(void)
 		return -ENODEV;
 	}
 
-	error = microcode_dev_init();
-	if (error)
-		return error;
 	microcode_pdev = platform_device_register_simple("microcode", -1,
 							 NULL, 0);
 	if (IS_ERR(microcode_pdev)) {
@@ -482,14 +513,26 @@ static int __init microcode_init(void)
 	}
 
 	get_online_cpus();
+	/*
+	 * --dimm. Hmm, we can avoid it if we perhaps first
+	 * try to apply ucode in mc_sysdev_add() and only 
+	 * then create a sysfs group.
+	 */
+	mutex_lock(&microcode_mutex);
 	error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
+	mutex_unlock(&microcode_mutex);
+
 	put_online_cpus();
+
 	if (error) {
-		microcode_dev_exit();
 		platform_device_unregister(microcode_pdev);
 		return error;
 	}
 
+	error = microcode_dev_init();
+	if (error)
+		return error;
+
 	register_hotcpu_notifier(&mc_cpu_notifier);
 
 	printk(KERN_INFO
@@ -507,7 +550,9 @@ static void __exit microcode_exit(void)
 	unregister_hotcpu_notifier(&mc_cpu_notifier);
 
 	get_online_cpus();
+	mutex_lock(&microcode_mutex);
 	sysdev_driver_unregister(&cpu_sysdev_class, &mc_sysdev_driver);
+	mutex_unlock(&microcode_mutex);
 	put_online_cpus();
 
 	platform_device_unregister(microcode_pdev);
diff -upr linux-2.6.git/arch/x86/kernel/microcode_intel.c linux-2.6.git.my/arch/x86/kernel/microcode_intel.c
--- linux-2.6.git/arch/x86/kernel/microcode_intel.c	2009-04-14 22:51:48.000000000 +0200
+++ linux-2.6.git.my/arch/x86/kernel/microcode_intel.c	2009-04-15 11:47:21.000000000 +0200
@@ -75,7 +75,6 @@
 #include <linux/miscdevice.h>
 #include <linux/firmware.h>
 #include <linux/smp_lock.h>
-#include <linux/spinlock.h>
 #include <linux/cpumask.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
@@ -150,9 +149,6 @@ struct extended_sigtable {
 
 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
 
-/* serialize access to the physical write to MSR 0x79 */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
 static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
@@ -176,15 +172,11 @@ static int collect_cpu_info(int cpu_num,
 		csig->pf = 1 << ((val[1] >> 18) & 7);
 	}
 
-	/* serialize access to the physical write to MSR 0x79 */
-	spin_lock_irqsave(&microcode_update_lock, flags);
-
 	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 	/* see notes above for revision 1.07.  Apparent chip bug */
 	sync_core();
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], csig->rev);
-	spin_unlock_irqrestore(&microcode_update_lock, flags);
 
 	pr_debug("microcode: collect_cpu_info : sig=0x%x, pf=0x%x, rev=0x%x\n",
 			csig->sig, csig->pf, csig->rev);
@@ -336,9 +328,6 @@ static void apply_microcode(int cpu)
 	if (mc_intel == NULL)
 		return;
 
-	/* serialize access to the physical write to MSR 0x79 */
-	spin_lock_irqsave(&microcode_update_lock, flags);
-
 	/* write microcode via MSR 0x79 */
 	wrmsr(MSR_IA32_UCODE_WRITE,
 	      (unsigned long) mc_intel->bits,
@@ -351,7 +340,6 @@ static void apply_microcode(int cpu)
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
 
-	spin_unlock_irqrestore(&microcode_update_lock, flags);
 	if (val[1] != mc_intel->hdr.rev) {
 		printk(KERN_ERR "microcode: CPU%d update from revision "
 				"0x%x to 0x%x failed\n",
@@ -445,8 +433,6 @@ static int request_microcode_fw(int cpu,
 	const struct firmware *firmware;
 	int ret;
 
-	/* We should bind the task to the CPU */
-	BUG_ON(cpu != raw_smp_processor_id());
 	sprintf(name, "intel-ucode/%02x-%02x-%02x",
 		c->x86, c->x86_model, c->x86_mask);
 	ret = request_firmware(&firmware, name, device);
@@ -470,9 +456,6 @@ static int get_ucode_user(void *to, cons
 
 static int request_microcode_user(int cpu, const void __user *buf, size_t size)
 {
-	/* We should bind the task to the CPU */
-	BUG_ON(cpu != raw_smp_processor_id());
-
 	return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
 }

Re: [PATCH] x86 microcode: revert some work_on_cpu

[Dmitry Adamushko]

> Disclaimer: it's not even compilation-tested, consider it merely as an
> illustration of an approach. I'll be able to test it later.

And here is the version that works for me.

(Perhaps, will still take another round as some parts seem vague to me.)

---

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index c882664..0389381 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -13,10 +13,16 @@ struct microcode_ops {
 	int  (*request_microcode_user) (int cpu, const void __user *buf, size_t size);
 	int  (*request_microcode_fw) (int cpu, struct device *device);
 
-	void (*apply_microcode) (int cpu);
+	void (*microcode_fini_cpu) (int cpu);
 
+	/* 
+	 * The generic 'microcode_core' part guarantees that
+	 * the callbacks below run on a target cpu when they
+	 * are being called.
+	 * See also the "Synchronization" section in microcode_core.c.
+	 */
+	void (*apply_microcode) (int cpu);
 	int  (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
-	void (*microcode_fini_cpu) (int cpu);
 };
 
 struct ucode_cpu_info {
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 453b579..0900d9c 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -17,7 +17,6 @@
 #include <linux/capability.h>
 #include <linux/miscdevice.h>
 #include <linux/firmware.h>
-#include <linux/spinlock.h>
 #include <linux/cpumask.h>
 #include <linux/pci_ids.h>
 #include <linux/uaccess.h>
@@ -79,9 +78,6 @@ struct microcode_amd {
 #define UCODE_CONTAINER_SECTION_HDR	8
 #define UCODE_CONTAINER_HEADER_SIZE	12
 
-/* serialize access to the physical write */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
 static struct equiv_cpu_entry *equiv_cpu_table;
 
 static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
@@ -146,7 +142,6 @@ static int get_matching_microcode(int cpu, void *mc, int rev)
 
 static void apply_microcode_amd(int cpu)
 {
-	unsigned long flags;
 	u32 rev, dummy;
 	int cpu_num = raw_smp_processor_id();
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
@@ -158,11 +153,9 @@ static void apply_microcode_amd(int cpu)
 	if (mc_amd == NULL)
 		return;
 
-	spin_lock_irqsave(&microcode_update_lock, flags);
 	wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
 	/* get patch id after patching */
 	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-	spin_unlock_irqrestore(&microcode_update_lock, flags);
 
 	/* check current patch id and patch's id for match */
 	if (rev != mc_amd->hdr.patch_id) {
@@ -327,9 +320,6 @@ static int request_microcode_fw(int cpu, struct device *device)
 	const struct firmware *firmware;
 	int ret;
 
-	/* We should bind the task to the CPU */
-	BUG_ON(cpu != raw_smp_processor_id());
-
 	ret = request_firmware(&firmware, fw_name, device);
 	if (ret) {
 		printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 2e0eb41..9ee151f 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -101,36 +101,97 @@ MODULE_LICENSE("GPL");
 
 static struct microcode_ops	*microcode_ops;
 
-/* no concurrent ->write()s are allowed on /dev/cpu/microcode */
+/*
+ * Synchronization.
+ *
+ * All non cpu-hotplug-callback call sites use:
+ *
+ * - microcode_mutex to synchronize with each other;
+ * - get/put_online_cpus() to synchronize with
+ *   the cpu-hotplug-callback call sites.
+ *
+ * We guarantee that only a single cpu is being
+ * updated at any particular moment of time.
+ */
 static DEFINE_MUTEX(microcode_mutex);
 
 struct ucode_cpu_info		ucode_cpu_info[NR_CPUS];
 EXPORT_SYMBOL_GPL(ucode_cpu_info);
 
+/*
+ * Operations that are run on a target cpu: 
+ */
+
+struct collect_for_cpu {
+	struct cpu_signature	*cpu_sig;
+	int			cpu;
+};
+
+static long collect_cpu_info_local(void *arg)
+{
+	struct collect_for_cpu *cfc = arg;
+
+	BUG_ON(cfc->cpu != raw_smp_processor_id());
+
+	return microcode_ops->collect_cpu_info(cfc->cpu, cfc->cpu_sig);
+}
+
+static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
+{
+	struct collect_for_cpu cfc = { .cpu_sig = cpu_sig, .cpu = cpu };
+
+	return work_on_cpu(cpu, collect_cpu_info_local, &cfc);
+}
+
+static void collect_cpu_info(int cpu)
+{
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	memset(uci, 0, sizeof(*uci));
+	if (!collect_cpu_info_on_target(cpu, &uci->cpu_sig))
+		uci->valid = 1;
+}
+
+struct apply_for_cpu {
+	int cpu;
+};
+
+static long apply_microcode_local(void *arg)
+{
+	struct apply_for_cpu *afc = arg;
+
+	BUG_ON(afc->cpu != raw_smp_processor_id());
+	microcode_ops->apply_microcode(afc->cpu);
+
+	return 0;
+}
+
+static int apply_microcode_on_target(int cpu)
+{
+	struct apply_for_cpu afc = { .cpu = cpu };
+
+	return work_on_cpu(cpu, apply_microcode_local, &afc);
+}
+
 #ifdef CONFIG_MICROCODE_OLD_INTERFACE
 static int do_microcode_update(const void __user *buf, size_t size)
 {
-	cpumask_t old;
 	int error = 0;
 	int cpu;
 
-	old = current->cpus_allowed;
-
 	for_each_online_cpu(cpu) {
 		struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
 		if (!uci->valid)
 			continue;
 
-		set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
 		error = microcode_ops->request_microcode_user(cpu, buf, size);
 		if (error < 0)
-			goto out;
+			break;
 		if (!error)
-			microcode_ops->apply_microcode(cpu);
+			apply_microcode_on_target(cpu);
 	}
-out:
-	set_cpus_allowed_ptr(current, &old);
+
 	return error;
 }
 
@@ -205,17 +266,16 @@ MODULE_ALIAS_MISCDEV(MICROCODE_MINOR);
 /* fake device for request_firmware */
 static struct platform_device	*microcode_pdev;
 
-static long reload_for_cpu(void *unused)
+static int reload_for_cpu(int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	int err = 0;
 
 	mutex_lock(&microcode_mutex);
 	if (uci->valid) {
-		err = microcode_ops->request_microcode_fw(smp_processor_id(),
-							  &microcode_pdev->dev);
+		err = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
 		if (!err)
-			microcode_ops->apply_microcode(smp_processor_id());
+			apply_microcode_on_target(cpu);
 	}
 	mutex_unlock(&microcode_mutex);
 	return err;
@@ -235,7 +295,7 @@ static ssize_t reload_store(struct sys_device *dev,
 	if (val == 1) {
 		get_online_cpus();
 		if (cpu_online(cpu))
-			err = work_on_cpu(cpu, reload_for_cpu, NULL);
+			err = reload_for_cpu(cpu);
 		put_online_cpus();
 	}
 	if (err)
@@ -275,7 +335,7 @@ static struct attribute_group mc_attr_group = {
 	.name		= "microcode",
 };
 
-static void __microcode_fini_cpu(int cpu)
+static void microcode_fini_cpu(int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
@@ -283,85 +343,44 @@ static void __microcode_fini_cpu(int cpu)
 	uci->valid = 0;
 }
 
-static void microcode_fini_cpu(int cpu)
-{
-	mutex_lock(&microcode_mutex);
-	__microcode_fini_cpu(cpu);
-	mutex_unlock(&microcode_mutex);
-}
-
-static void collect_cpu_info(int cpu)
+static void microcode_resume_cpu(int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
-	memset(uci, 0, sizeof(*uci));
-	if (!microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig))
-		uci->valid = 1;
+	if (!uci->valid || !uci->mc)
+		return;
+
+	pr_debug("microcode: CPU%d updated upon resume\n", cpu);
+	apply_microcode_on_target(cpu);
 }
 
-static int microcode_resume_cpu(int cpu)
+static void microcode_init_cpu(int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	struct cpu_signature nsig;
+	int err;
 
-	pr_debug("microcode: CPU%d resumed\n", cpu);
+	collect_cpu_info(cpu);
 
-	if (!uci->mc)
-		return 1;
+	if (!uci->valid)
+		return;
+	if (system_state != SYSTEM_RUNNING)
+		return;
 
-	/*
-	 * Let's verify that the 'cached' ucode does belong
-	 * to this cpu (a bit of paranoia):
-	 */
-	if (microcode_ops->collect_cpu_info(cpu, &nsig)) {
-		__microcode_fini_cpu(cpu);
-		printk(KERN_ERR "failed to collect_cpu_info for resuming cpu #%d\n",
-				cpu);
-		return -1;
+	err = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
+	if (!err) {
+		pr_debug("microcode: CPU%d updated upon init\n", cpu);
+		apply_microcode_on_target(cpu);
 	}
-
-	if ((nsig.sig != uci->cpu_sig.sig) || (nsig.pf != uci->cpu_sig.pf)) {
-		__microcode_fini_cpu(cpu);
-		printk(KERN_ERR "cached ucode doesn't match the resuming cpu #%d\n",
-				cpu);
-		/* Should we look for a new ucode here? */
-		return 1;
-	}
-
-	return 0;
 }
 
-static long microcode_update_cpu(void *unused)
+static void microcode_update_cpu(int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
-	int err = 0;
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
-	/*
-	 * Check if the system resume is in progress (uci->valid != NULL),
-	 * otherwise just request a firmware:
-	 */
-	if (uci->valid) {
-		err = microcode_resume_cpu(smp_processor_id());
-	} else {
-		collect_cpu_info(smp_processor_id());
-		if (uci->valid && system_state == SYSTEM_RUNNING)
-			err = microcode_ops->request_microcode_fw(
-					smp_processor_id(),
-					&microcode_pdev->dev);
-	}
-	if (!err)
-		microcode_ops->apply_microcode(smp_processor_id());
-	return err;
-}
-
-static int microcode_init_cpu(int cpu)
-{
-	int err;
-	mutex_lock(&microcode_mutex);
-	err = work_on_cpu(cpu, microcode_update_cpu, NULL);
-	mutex_unlock(&microcode_mutex);
-
-	return err;
+	if (uci->valid)
+		microcode_resume_cpu(cpu);
+	else
+		microcode_init_cpu(cpu);
 }
 
 static int mc_sysdev_add(struct sys_device *sys_dev)
@@ -379,9 +398,10 @@ static int mc_sysdev_add(struct sys_device *sys_dev)
 	if (err)
 		return err;
 
-	err = microcode_init_cpu(cpu);
-	if (err)
-		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
+	microcode_init_cpu(cpu);
+	if (!uci->valid)
+		/* can't work with this cpu */
+		err = -1;
 
 	return err;
 }
@@ -402,12 +422,23 @@ static int mc_sysdev_remove(struct sys_device *sys_dev)
 static int mc_sysdev_resume(struct sys_device *dev)
 {
 	int cpu = dev->id;
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
 	if (!cpu_online(cpu))
 		return 0;
 
-	/* only CPU 0 will apply ucode here */
-	microcode_update_cpu(NULL);
+	/*
+	 * All non-bootup cpus are still disabled,
+	 * so only CPU 0 will apply ucode here.
+	 *
+	 * Moreover, there can be no concurrent
+	 * updates from any other places at this point.
+	 */
+	WARN_ON(cpu != 0);
+
+	if (uci->valid && uci->mc)
+		microcode_ops->apply_microcode(cpu);
+
 	return 0;
 }
 
@@ -427,9 +458,7 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
 	switch (action) {
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
-		if (microcode_init_cpu(cpu))
-			printk(KERN_ERR "microcode: failed to init CPU%d\n",
-			       cpu);
+		microcode_update_cpu(cpu);
 	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:
 		pr_debug("microcode: CPU%d added\n", cpu);
@@ -471,9 +500,6 @@ static int __init microcode_init(void)
 		return -ENODEV;
 	}
 
-	error = microcode_dev_init();
-	if (error)
-		return error;
 	microcode_pdev = platform_device_register_simple("microcode", -1,
 							 NULL, 0);
 	if (IS_ERR(microcode_pdev)) {
@@ -482,14 +508,26 @@ static int __init microcode_init(void)
 	}
 
 	get_online_cpus();
+	/*
+	 * --dimm. Hmm, we can avoid it if we perhaps first
+	 * try to apply ucode in mc_sysdev_add() and only 
+	 * then create a sysfs group.
+	 */
+	mutex_lock(&microcode_mutex);
 	error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
+	mutex_unlock(&microcode_mutex);
+
 	put_online_cpus();
+
 	if (error) {
-		microcode_dev_exit();
 		platform_device_unregister(microcode_pdev);
 		return error;
 	}
 
+	error = microcode_dev_init();
+	if (error)
+		return error;
+
 	register_hotcpu_notifier(&mc_cpu_notifier);
 
 	printk(KERN_INFO
@@ -507,7 +545,9 @@ static void __exit microcode_exit(void)
 	unregister_hotcpu_notifier(&mc_cpu_notifier);
 
 	get_online_cpus();
+	mutex_lock(&microcode_mutex);
 	sysdev_driver_unregister(&cpu_sysdev_class, &mc_sysdev_driver);
+	mutex_unlock(&microcode_mutex);
 	put_online_cpus();
 
 	platform_device_unregister(microcode_pdev);
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 149b9ec..bc824bb 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -75,7 +75,6 @@
 #include <linux/miscdevice.h>
 #include <linux/firmware.h>
 #include <linux/smp_lock.h>
-#include <linux/spinlock.h>
 #include <linux/cpumask.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
@@ -150,13 +149,9 @@ struct extended_sigtable {
 
 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
 
-/* serialize access to the physical write to MSR 0x79 */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
 static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
-	unsigned long flags;
 	unsigned int val[2];
 
 	memset(csig, 0, sizeof(*csig));
@@ -176,15 +171,11 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 		csig->pf = 1 << ((val[1] >> 18) & 7);
 	}
 
-	/* serialize access to the physical write to MSR 0x79 */
-	spin_lock_irqsave(&microcode_update_lock, flags);
-
 	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 	/* see notes above for revision 1.07.  Apparent chip bug */
 	sync_core();
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], csig->rev);
-	spin_unlock_irqrestore(&microcode_update_lock, flags);
 
 	pr_debug("microcode: collect_cpu_info : sig=0x%x, pf=0x%x, rev=0x%x\n",
 			csig->sig, csig->pf, csig->rev);
@@ -322,7 +313,6 @@ static void apply_microcode(int cpu)
 {
 	struct microcode_intel *mc_intel;
 	struct ucode_cpu_info *uci;
-	unsigned long flags;
 	unsigned int val[2];
 	int cpu_num;
 
@@ -336,9 +326,6 @@ static void apply_microcode(int cpu)
 	if (mc_intel == NULL)
 		return;
 
-	/* serialize access to the physical write to MSR 0x79 */
-	spin_lock_irqsave(&microcode_update_lock, flags);
-
 	/* write microcode via MSR 0x79 */
 	wrmsr(MSR_IA32_UCODE_WRITE,
 	      (unsigned long) mc_intel->bits,
@@ -351,7 +338,6 @@ static void apply_microcode(int cpu)
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
 
-	spin_unlock_irqrestore(&microcode_update_lock, flags);
 	if (val[1] != mc_intel->hdr.rev) {
 		printk(KERN_ERR "microcode: CPU%d update from revision "
 				"0x%x to 0x%x failed\n",
@@ -445,8 +431,6 @@ static int request_microcode_fw(int cpu, struct device *device)
 	const struct firmware *firmware;
 	int ret;
 
-	/* We should bind the task to the CPU */
-	BUG_ON(cpu != raw_smp_processor_id());
 	sprintf(name, "intel-ucode/%02x-%02x-%02x",
 		c->x86, c->x86_model, c->x86_mask);
 	ret = request_firmware(&firmware, name, device);
@@ -470,9 +454,6 @@ static int get_ucode_user(void *to, const void *from, size_t n)
 
 static int request_microcode_user(int cpu, const void __user *buf, size_t size)
 {
-	/* We should bind the task to the CPU */
-	BUG_ON(cpu != raw_smp_processor_id());
-
 	return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
 }