Skip to contentSkip to conversation

x86: kernel/microcode_core.c simple_strtoul cleanup

PATCH RESEND Superseded Shuah Khan 5 participants 9 messages May 6, 2012 – May 9, 2012 x86

lore
Share & tools
lore

Shuah Khan

author
#0 lore raw

PATCH RESEND 1/1 x86: kernel/microcode_core.c simple_strtoul cleanup

Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul() instead of calling obsoleted simple_strtoul().

---
 arch/x86/kernel/microcode_core.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
arch/x86/kernel/microcode_core.c +4 -5
index c9bda6d..fbdfc69 100644
@@ -299,12 +299,11 @@ static ssize_t reload_store(struct device *dev,
 {
 	unsigned long val;
 	int cpu = dev->id;
-	int ret = 0;
-	char *end;
+	ssize_t ret = 0;
 
-	val = simple_strtoul(buf, &end, 0);
-	if (end == buf)
-		return -EINVAL;
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	if (val == 1) {
 		get_online_cpus();
Signature · 1 line
1.7.9.5
maintainer
#1 lore raw
Quoted 4 lines · Shuah Khan

Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul() instead of calling obsoleted simple_strtoul().

Ok, much better, thanks.

@Ingo: I'll pick this one up unless you want to do that :-).

Quoted 27 lines
---
 arch/x86/kernel/microcode_core.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
arch/x86/kernel/microcode_core.c +4 -5
index c9bda6d..fbdfc69 100644
@@ -299,12 +299,11 @@ static ssize_t reload_store(struct device *dev,
 {
 	unsigned long val;
 	int cpu = dev->id;
-	int ret = 0;
-	char *end;
+	ssize_t ret = 0;
 
-	val = simple_strtoul(buf, &end, 0);
-	if (end == buf)
-		return -EINVAL;
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	if (val == 1) {
 		get_online_cpus();
Signature · 1 line
1.7.9.5
Signature · 2 lines
Regards/Gruss,
Boris.
#2 lore raw
Quoted 10 lines · Borislav Petkov
Quoted 4 lines · Shuah Khan

Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul() instead of calling obsoleted simple_strtoul().

Ok, much better, thanks.

@Ingo: I'll pick this one up unless you want to do that :-).

I'll pick it up. I presume I have your ack on it?

-hpa

maintainer
#3 lore raw
Quoted 1 line · H. Peter Anvin

I'll pick it up. I presume I have your ack on it?

Yep, sure.

Btw, while at it, I gave the whole sysfs reload thing a critical look and whether it is at all that useful - this thing gives you microcode reloading on a single CPU. And what you actually wanna do is reload the microcode on the whole system, i.e. all cores in succession.

And we don't use the reload thing on AMD, so I was wondering, if you guys don't find it useful on Intel hw, maybe we can remove it completely in favor of

$ rmmod microcode; modprobe microcode

which reloads the ucode on each core.

Of course, one can iterate over each core in a shell-loop and write into the reload file to reload ucode after having updated the ucode image in /lib/firmware but removing and then modprobing the module is shorter :-)

Hmm?

Signature · 2 lines
Regards/Gruss,
    Boris.
#4 lore raw
Quoted 14 lines · Borislav Petkov
Quoted 1 line · H. Peter Anvin

I'll pick it up. I presume I have your ack on it?

Yep, sure.

Btw, while at it, I gave the whole sysfs reload thing a critical look and whether it is at all that useful - this thing gives you microcode reloading on a single CPU. And what you actually wanna do is reload the microcode on the whole system, i.e. all cores in succession.

And we don't use the reload thing on AMD, so I was wondering, if you guys don't find it useful on Intel hw, maybe we can remove it completely in favor of

Quoted 7 lines
$ rmmod microcode; modprobe microcode

which reloads the ucode on each core.

Of course, one can iterate over each core in a shell-loop and write into the reload file to reload ucode after having updated the ucode image in /lib/firmware but removing and then modprobing the module is shorter :-)

Can we PLEASE fix it properly by adding a new node (which is _not_ per-cpu) that requests the microcode core to refresh all cpus? Preferably by invalidating the microcode cache, THEN fetching each required microcode just once for the first core that needs it, and caching it for use the other cores. You can leave the (IMHO mostly useless) per-cpu sysfs nodes alone, so as to not break ABI, or deprecate them for an year or something.

I am speaking this with my userland maintainer hat. I *do NOT* want to rmmod crap in a production server to update microcode. And I want to be able to support static-compiled microcode.

Signature · 4 lines
  "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
#5 lore raw
Quoted 14 lines · Henrique de Moraes Holschuh
Quoted 4 lines

Of course, one can iterate over each core in a shell-loop and write into the reload file to reload ucode after having updated the ucode image in /lib/firmware but removing and then modprobing the module is shorter :-)

Can we PLEASE fix it properly by adding a new node (which is _not_ per-cpu) that requests the microcode core to refresh all cpus? Preferably by invalidating the microcode cache, THEN fetching each required microcode just once for the first core that needs it, and caching it for use the other cores. You can leave the (IMHO mostly useless) per-cpu sysfs nodes alone, so as to not break ABI, or deprecate them for an year or something.

Quoted 4 lines

I am speaking this with my userland maintainer hat. I *do NOT* want to rmmod crap in a production server to update microcode. And I want to be able to support static-compiled microcode.

Seconded. There's also the ability to disable module unloading.

Thanks,

Ingo

author
#6 lore raw
Quoted 16 lines · Ingo Molnar
Quoted 14 lines · Henrique de Moraes Holschuh
Quoted 4 lines

Of course, one can iterate over each core in a shell-loop and write into the reload file to reload ucode after having updated the ucode image in /lib/firmware but removing and then modprobing the module is shorter :-)

Can we PLEASE fix it properly by adding a new node (which is _not_ per-cpu) that requests the microcode core to refresh all cpus? Preferably by invalidating the microcode cache, THEN fetching each required microcode just once for the first core that needs it, and caching it for use the other cores. You can leave the (IMHO mostly useless) per-cpu sysfs nodes alone, so as to not break ABI, or deprecate them for an year or something.

Quoted 6 lines
Quoted 4 lines

I am speaking this with my userland maintainer hat. I *do NOT* want to rmmod crap in a production server to update microcode. And I want to be able to support static-compiled microcode.

Seconded. There's also the ability to disable module unloading.

I will take a stab at the disabling module unloading if I may. The rest of the tasks are a bit out of my league at this time. :)

Thanks,
-- Shuah

maintainer
#7 lore raw
In reply to Ingo Molnar
Quoted 11 lines · Ingo Molnar
Quoted 8 lines · Henrique de Moraes Holschuh
Quoted 4 lines

Of course, one can iterate over each core in a shell-loop and write into the reload file to reload ucode after having updated the ucode image in /lib/firmware but removing and then modprobing the module is shorter :-)

Can we PLEASE fix it properly by adding a new node (which is _not_ per-cpu) that requests the microcode core to refresh all cpus?

Ok, where do you want to have it:

$ find /sys -iname "microcode"
/sys/bus/platform/devices/microcode
/sys/devices/system/cpu/cpu0/microcode
/sys/devices/system/cpu/cpu1/microcode
/sys/devices/virtual/misc/microcode
/sys/devices/platform/microcode
/sys/class/misc/microcode
/sys/module/microcode

I'm all for the level at

/sys/devices/system/cpu/

where CPU-related stuff can go in - not per-CPU but per whole system. Ingo?

Quoted 3 lines
Quoted 3 lines

Preferably by invalidating the microcode cache, THEN fetching each required microcode just once for the first core that needs it, and caching it for use the other cores.

That should be easy, the most part is already there.

Quoted 2 lines
Quoted 2 lines

You can leave the (IMHO mostly useless) per-cpu sysfs nodes alone, so as to not break ABI, or deprecate them for an year or something.

Yeah, about that, what is not breaking the ABI? Having the sysfs node but reading/writing it returns -E<something>, having the sysfs node and reading/writing it does nothing, or...?

Quoted 3 lines
Quoted 3 lines

I *do NOT* want to rmmod crap in a production server to update microcode. And I want to be able to support static-compiled microcode.

I'd need this more explained, why?

If by "static-compiled microcode" you mean the microcode.ko module should be built-in that can't fly because for request_firmware we need userspace to actually find the ucode image.

Thanks.

Signature · 2 lines
Regards/Gruss,
    Boris.
#8 lore raw

[tip:x86/microcode] x86, microcode: microcode_core. c simple_strtoul cleanup

In reply to Shuah Khan
Commit-ID:  e826abd523913f63eb03b59746ffb16153c53dc4
Gitweb:     http://git.kernel.org/tip/e826abd523913f63eb03b59746ffb16153c53dc4
Author:     Shuah Khan <[email protected]>
AuthorDate: Sun, 6 May 2012 11:11:04 -0600
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Mon, 7 May 2012 11:36:49 -0700

x86, microcode: microcode_core.c simple_strtoul cleanup

Change reload_for_cpu() in kernel/microcode_core.c to call kstrtoul() instead of calling obsoleted simple_strtoul().

---
 arch/x86/kernel/microcode_core.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
arch/x86/kernel/microcode_core.c +4 -5
index c9bda6d..fbdfc69 100644
@@ -299,12 +299,11 @@ static ssize_t reload_store(struct device *dev,
 {
 	unsigned long val;
 	int cpu = dev->id;
-	int ret = 0;
-	char *end;
+	ssize_t ret = 0;
 
-	val = simple_strtoul(buf, &end, 0);
-	if (end == buf)
-		return -EINVAL;
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	if (val == 1) {
 		get_online_cpus();