x86, cpufeature: Fix duplicate feature name "dts"
Subsystems (3)
PATCH 1/1 x86, cpufeature: Fix duplicate feature name "dts"
From: Jean Delvare <[email protected]>
Subject: x86, cpufeature: Fix duplicate feature name "dts"
We currently have two different x86 CPU features reported as "dts" in /proc/cpuinfo: Debug Store and Digital Thermal Sensor. Change the former to "ds" which makes more sense anyway, to clear up the confusion.
--- arch/x86/include/asm/cpufeature.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/include/asm/cpufeature.h +1 -1
@@ -35,7 +35,7 @@#define X86_FEATURE_PSE36 (0*32+17)/* 36-bit PSEs */ #define X86_FEATURE_PN (0*32+18)/* Processor serial number */ #define X86_FEATURE_CLFLSH (0*32+19)/* "clflush" CLFLUSH instruction */ -#define X86_FEATURE_DS (0*32+21)/* "dts" Debug Store */ +#define X86_FEATURE_DS (0*32+21)/* Debug Store */ #define X86_FEATURE_ACPI (0*32+22)/* ACPI via MSR */ #define X86_FEATURE_MMX (0*32+23)/* Multimedia Extensions */ #define X86_FEATURE_FXSR (0*32+24)/* FXSAVE/FXRSTOR, CR4.OSFXSR */
Signature · 1 line
Jean Delvare
Good spotting, but I'm worried this is the wrong fix.
This is a userspace ABI change, and the and we have used "dts" for debug store (debug trace store?) for a very long tie, whereas digital thermal sensor only has been used since 2010; another *major* question is which of these is more likely to be of interest to userspace.
Jan, do you have any feeling on this?
-hpa
Quoted 28 lines · Jean Delvare
From: Jean Delvare <[email protected]>
Subject: x86, cpufeature: Fix duplicate feature name "dts"We currently have two different x86 CPU features reported as "dts" in
/proc/cpuinfo: Debug Store and Digital Thermal Sensor. Change the former to "ds" which makes more sense anyway, to clear up the confusion.Signed-off-by Jean Delvare <[email protected]>Cc H. Peter Anvin <[email protected]>Cc Ingo Molnar <[email protected]>--- arch/x86/include/asm/cpufeature.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/include/asm/cpufeature.h +1
-1@@ -35,7 +35,7 @@#define X86_FEATURE_PSE36 (0*32+17)/* 36-bit PSEs */ #define X86_FEATURE_PN (0*32+18)/* Processor serial number */ #define X86_FEATURE_CLFLSH (0*32+19)/* "clflush" CLFLUSH instruction */ -#define X86_FEATURE_DS (0*32+21)/* "dts" Debug Store */ +#define X86_FEATURE_DS (0*32+21)/* Debug Store */ #define X86_FEATURE_ACPI (0*32+22)/* ACPI via MSR */ #define X86_FEATURE_MMX (0*32+23)/* Multimedia Extensions */ #define X86_FEATURE_FXSR (0*32+24)/* FXSAVE/FXRSTOR, CR4.OSFXSR */
Quoted 6 lines · H. Peter Anvin
Good spotting, but I'm worried this is the wrong fix.
This is a userspace ABI change, and the and we have used "dts" for debug store (debug trace store?) for a very long tie, whereas digital thermal sensor only has been used since 2010; another *major* question is which of these is more likely to be of interest to userspace.
I am aware of this problem. I found the bug because I was looking for the Digital Thermal Sensor feature, and /proc/cpuinfo said "dts" but the driver wouldn't work, this confused me. So at least the Digital Thermal Sensor is of interest. That being said I believe interest is more from humans than software, which is why I do not feel bad changing it. The current situation could already lead to false positives anyway, both for humans and software.
If your "right fix" is to find something else for the Digital Thermal Sensor, I doubt you can find something that won't confuse people even more. Especially when there is also the Package Thermal Sensor feature which is properly labeled "pts" already. "DTS" is widely used in the technical documentation to refer to this feature.
And FWIW, X86_FEATURE_DS has no user in the kernel. Doesn't look like a key feature to me, but maybe my view is biased...
Signature · 1 line
Jean Delvare
H. Peter Anvin
reviewerQuoted 4 lines · Jean Delvare
And FWIW,
X86_FEATURE_DShas no user in the kernel. Doesn't look like a key feature to me, but maybe my view is biased...
No, but apparently perfmon2 depends on it, in userspace, and it relies on the string.
So this is a nonstarter.
-hpa
Borislav Petkov
maintainerQuoted 10 lines · H. Peter Anvin
Quoted 4 lines · Jean Delvare
And FWIW,
X86_FEATURE_DShas no user in the kernel. Doesn't look like a key feature to me, but maybe my view is biased...No, but apparently perfmon2 depends on it, in userspace, and it relies on the string.
So this is a nonstarter.
Maybe call the sensor thing "dtsen" or something similarly short but not "dts"?
Signature · 2 lines
Regards/Gruss,
Boris.Quoted 7 lines · Borislav Petkov
Quoted 6 lines · H. Peter Anvin
Quoted 4 lines · Jean Delvare
And FWIW,
X86_FEATURE_DShas no user in the kernel. Doesn't look like a key feature to me, but maybe my view is biased...
Quoted 8 lines
Quoted 4 lines
No, but apparently perfmon2 depends on it, in userspace, and it relies on the string.
So this is a nonstarter.
Maybe call the sensor thing "dtsen" or something similarly short but not "dts"?
Yeah, that's what we have to do. Sigh.
-hpa
Quoted 11 lines · H. Peter Anvin
Quoted 10 lines · Borislav Petkov
Quoted 9 lines · H. Peter Anvin
Quoted 4 lines · Jean Delvare
And FWIW,
X86_FEATURE_DShas no user in the kernel. Doesn't look like a key feature to me, but maybe my view is biased...No, but apparently perfmon2 depends on it, in userspace, and it relies on the string.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
So this is a nonstarter.
Maybe call the sensor thing "dtsen" or something similarly short but not "dts"?
Yeah, that's what we have to do. Sigh.
Plus add a kernel build time check to avoid such string clashes in the future? These acronyms tend to be short and hard to read, I'd not be surprised if this repeated in the future.
Thanks,
Ingo
H. Peter Anvin
reviewerQuoted 5 lines · Ingo Molnar
Plus add a kernel build time check to avoid such string clashes in the future? These acronyms tend to be short and hard to read, I'd not be surprised if this repeated in the future.
Another *MASSIVE* fail is that an x86 cpufeature was added by a patch that went through the hwmon tree without any x86 maintainer ACK, and with a subject line that would not have made anyone aware of it:
a4659053 x86/hwmon: fix initialization of coretemp
We maintain x86/cpufeature as a separate -tip branch for a reason:
changes to that stuff mucks with *everything* as it is.
-hpa
Quoted 7 lines · H. Peter Anvin
Quoted 5 lines · Ingo Molnar
Plus add a kernel build time check to avoid such string clashes in the future? These acronyms tend to be short and hard to read, I'd not be surprised if this repeated in the future.
Quoted 8 lines
Another *MASSIVE* fail is that an x86 cpufeature was added by a patch that went through the hwmon tree without any x86 maintainer ACK, and with a subject line that would not have made anyone aware of it:
a4659053x86/hwmon: fix initialization of coretempWe maintain x86/cpufeature as a separate -tip branch for a reason:
changes to that stuff mucks with *everything* as it is.
My apologies. Hope I learned a bit since then.
Guenter
Quoted 9 lines
Quoted 1 line
Quoted 1 line · H. Peter Anvin
Good spotting, but I'm worried this is the wrong fix.
This is a userspace ABI change, and the and we have used "dts" for debug store (debug trace store?) for a very long tie, whereas digital thermal sensor only has been used since 2010; another *major* question is which of these is more likely to be of interest to userspace.
Jan, do you have any feeling on this?
Now that you already figured that there is user space code depending on the current (questionable) string, I don't think it matters much - I would have preferred to correct the string to match the CPUID documentation (albeit I seem to recall that early on the bit was indeed named DTS in the doc).
Wouldn't it be helpful if mkcapflags.pl detected duplicates?
Jan
H. Peter Anvin
reviewerDefinitely!
Quoted 22 lines · Jan Beulich
Quoted 4 lines
Quoted 1 line
Quoted 1 line · H. Peter Anvin
Good spotting, but I'm worried this is the wrong fix.
This is a userspace ABI change, and the and we have used "dts" for
debug
Quoted 1 line
store (debug trace store?) for a very long tie, whereas digital
thermal
Quoted 1 line
sensor only has been used since 2010; another *major* question is
which
Quoted 3 lines
of these is more likely to be of interest to userspace.
Jan, do you have any feeling on this?
Now that you already figured that there is user space code depending on the current (questionable) string, I don't think it matters much - I would have preferred to correct the string to match the CPUID documentation (albeit I seem to recall that early on the bit was indeed named DTS in the doc).
Wouldn't it be helpful if mkcapflags.pl detected duplicates?
Jan
Signature · 1 line
Sent from my mobile phone. Please excuse brevity and lack of formatting.
[tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
In reply to H. Peter AnvinCommit-ID: 3bac8715443fc83413e221a5cd9d5cf3d82b79b4 Gitweb: http://git.kernel.org/tip/3bac8715443fc83413e221a5cd9d5cf3d82b79b4 Author: H. Peter Anvin <[email protected]> AuthorDate: Fri, 22 Jun 2012 10:58:06 -0700 Committer: H. Peter Anvin <[email protected]> CommitDate: Fri, 22 Jun 2012 10:58:06 -0700
x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
It makes sense to label "Digital Thermal Store" as "DTS", but unfortunately the string "dts" was already used for "Debug Store", and /proc/cpuinfo is a user space ABI.
Therefore, rename this to "dtherm".
This conflict went into mainline via the hwmon tree without any x86 maintainer ack, and without any kind of hint in the subject.
a4659053 x86/hwmon: fix initialization of coretemp
--- arch/x86/include/asm/cpufeature.h | 2 +- arch/x86/kernel/cpu/scattered.c | 2 +- drivers/hwmon/coretemp.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
arch/x86/include/asm/cpufeature.h +1 -1
@@ -176,7 +176,7 @@#define X86_FEATURE_XSAVEOPT (7*32+ 4)/* Optimized Xsave */ #define X86_FEATURE_PLN (7*32+ 5)/* Intel Power Limit Notification */ #define X86_FEATURE_PTS (7*32+ 6)/* Intel Package Thermal Status */ -#define X86_FEATURE_DTS (7*32+ 7)/* Digital Thermal Sensor */ +#define X86_FEATURE_DTHERM (7*32+ 7)/* Digital Thermal Sensor */ #define X86_FEATURE_HW_PSTATE (7*32+ 8)/* AMD HW-PState */ /* Virtualization flags: Linux defined, word 8 */
arch/x86/kernel/cpu/scattered.c +1 -1
@@ -31,7 +31,7 @@ void __cpuinit init_scattered_cpuid_features(struct cpuinfo_x86 *c)const struct cpuid_bit *cb;static const struct cpuid_bit __cpuinitconst cpuid_bits[] = { - {X86_FEATURE_DTS ,CR_EAX ,0 ,0x00000006 ,0 }, + {X86_FEATURE_DTHERM ,CR_EAX ,0 ,0x00000006 ,0 }, {X86_FEATURE_IDA ,CR_EAX ,1 ,0x00000006 ,0 }, {X86_FEATURE_ARAT ,CR_EAX ,2 ,0x00000006 ,0 }, {X86_FEATURE_PLN ,CR_EAX ,4 ,0x00000006 ,0 },
drivers/hwmon/coretemp.c +2 -2
@@ -664,7 +664,7 @@ static void __cpuinit get_core_online(unsigned int cpu)* sensors. We check this bit only, all the early CPUs * without thermal sensors will be filtered out. */ -if (!cpu_has (c,X86_FEATURE_DTS )) +if (!cpu_has (c,X86_FEATURE_DTHERM ))return ;if (!pdev) {
@@ -765,7 +765,7 @@ static struct notifier_block coretemp_cpu_notifier __refdata = { };static const struct x86_cpu_id coretemp_ids[] = { - {X86_VENDOR_INTEL ,X86_FAMILY_ANY ,X86_MODEL_ANY ,X86_FEATURE_DTS }, + {X86_VENDOR_INTEL ,X86_FAMILY_ANY ,X86_MODEL_ANY ,X86_FEATURE_DTHERM }, {} };MODULE_DEVICE_TABLE (x86cpu, coretemp_ids);
Re: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
Quoted 10 lines · tip-bot for H. Peter Anvin
Commit-ID: 3bac8715443fc83413e221a5cd9d5cf3d82b79b4 Gitweb: http://git.kernel.org/tip/3bac8715443fc83413e221a5cd9d5cf3d82b79b4 Author: H. Peter Anvin <[email protected]> AuthorDate: Fri, 22 Jun 2012 10:58:06 -0700 Committer: H. Peter Anvin <[email protected]> CommitDate: Fri, 22 Jun 2012 10:58:06 -0700x86, cpufeature: Rename
X86_FEATURE_DTStoX86_FEATURE_DTHERMIt makes sense to label "Digital Thermal Store" as "DTS", but
You mean "Digital Thermal Sensor".
Quoted 4 lines
unfortunately the string "dts" was already used for "Debug Store", and
/proc/cpuinfois a user space ABI.Therefore, rename this to "dtherm".
I see the rationale for changing the string in /proc/cpuinfo, and "dtherm" is reasonably good. I fail to see the rationale for changing the X86_FEATURE_ name though, this is an API change we don't need. Plus X86_FEATURE_DTS has the merit of naming the feature the way it is commonly done in technical documentation, so readers know exactly what it refers too, which isn't the case of DTHERM. So can we please stick to X86_FEATURE_DTS?
Quoted 4 lines
This conflict went into mainline via the hwmon tree without any x86 maintainer ack, and without any kind of hint in the subject.
a4659053x86/hwmon: fix initialization of coretemp
All 3 x86 maintainers were Cc'd, none commented. And you know fairly well why the patch went through the hwmon tree. So please stop the finger-pointing. It's unfortunate that it happened, but it did, and we try to fix it now, period.
Quoted 4 lines
Reported-by Jean Delvare <[email protected]>Cc Jan Beulich <[email protected]>Cc <[email protected]> v2.6.36..v3.4
No Signed-off-by?
Not sure why you want this to go to stable trees?
Quoted 5 lines
--- arch/x86/include/asm/cpufeature.h | 2 +- arch/x86/kernel/cpu/scattered.c | 2 +- drivers/hwmon/coretemp.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
Signature · 1 line
Jean Delvare
H. Peter Anvin
reviewerRe: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
Quoted 11 lines · Jean Delvare
Quoted 2 lines
Therefore, rename this to "dtherm".
I see the rationale for changing the string in
/proc/cpuinfo, and "dtherm" is reasonably good. I fail to see the rationale for changing theX86_FEATURE_name though, this is an API change we don't need. PlusX86_FEATURE_DTShas the merit of naming the feature the way it is commonly done in technical documentation, so readers know exactly what it refers too, which isn't the case of DTHERM. So can we please stick toX86_FEATURE_DTS?
Except that *really* seems like begging for similar problems in the future.
Quoted 10 lines
Quoted 4 lines
This conflict went into mainline via the hwmon tree without any x86 maintainer ack, and without any kind of hint in the subject.
a4659053x86/hwmon: fix initialization of coretempAll 3 x86 maintainers were Cc'd, none commented. And you know fairly well why the patch went through the hwmon tree. So please stop the finger-pointing. It's unfortunate that it happened, but it did, and we try to fix it now, period.
Quoted 9 lines
Quoted 4 lines
Reported-by Jean Delvare <[email protected]>Cc Jan Beulich <[email protected]>Cc <[email protected]> v2.6.36..v3.4No Signed-off-by?
Not sure why you want this to go to stable trees?
I think we want to minimize the ABI divergence here.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Re: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
Quoted 12 lines · H. Peter Anvin
Quoted 10 lines · Jean Delvare
Quoted 2 lines
Therefore, rename this to "dtherm".
I see the rationale for changing the string in
/proc/cpuinfo, and "dtherm" is reasonably good. I fail to see the rationale for changing theX86_FEATURE_name though, this is an API change we don't need. PlusX86_FEATURE_DTShas the merit of naming the feature the way it is commonly done in technical documentation, so readers know exactly what it refers too, which isn't the case of DTHERM. So can we please stick toX86_FEATURE_DTS?
Quoted 1 line
Except that *really* seems like begging for similar problems in the future.
With your other patch to catch such collisions, I think we should be just safe from now on?
Quoted 15 lines
Quoted 15 lines
Quoted 4 lines
This conflict went into mainline via the hwmon tree without any x86 maintainer ack, and without any kind of hint in the subject.
a4659053x86/hwmon: fix initialization of coretempAll 3 x86 maintainers were Cc'd, none commented. And you know fairly well why the patch went through the hwmon tree. So please stop the finger-pointing. It's unfortunate that it happened, but it did, and we try to fix it now, period.
Quoted 4 lines
Reported-by Jean Delvare <[email protected]>Cc Jan Beulich <[email protected]>Cc <[email protected]> v2.6.36..v3.4
Quoted 6 lines
Quoted 4 lines
No Signed-off-by?
Not sure why you want this to go to stable trees?
I think we want to minimize the ABI divergence here.
The ABI divergence already exists and will have to be dealt with anyway. All you're doing by pushing the changes to stable trees is making its shape more complex, and increasing the risk of conflict. Such a conflict already happened, BTW...
Signature · 1 line
Jean Delvare
H. Peter Anvin
reviewerRe: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
In reply to Jean DelvareQuoted 11 lines · Jean Delvare
Quoted 5 lines
This conflict went into mainline via the hwmon tree without any x86 maintainer ack, and without any kind of hint in the subject.
a4659053x86/hwmon: fix initialization of coretempAll 3 x86 maintainers were Cc'd, none commented. And you know fairly well why the patch went through the hwmon tree. So please stop the finger-pointing. It's unfortunate that it happened, but it did, and we try to fix it now, period.
It isn't about finger-pointing, it is about a failure of process; we had a process that would have caught this problem and the process failed. There are certainly blame enough to go around, but I think one of the things here is that lack of response isn't necessary an ACK - it may actually be that the patch looked at first glance like it had no impact on our area (incorrectly as it turned out.)
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
H. Peter Anvin
reviewerRe: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
Quoted 8 lines · H. Peter Anvin
It isn't about finger-pointing, it is about a failure of process; we had a process that would have caught this problem and the process failed. There are certainly blame enough to go around, but I think one of the things here is that lack of response isn't necessary an ACK - it may actually be that the patch looked at first glance like it had no impact on our area (incorrectly as it turned out.)
Let me be clear: I'm not blaming you, or anyone else, personally. Like many other failures, there were a number of steps in the chain here, one of them being not checking this in the script from the beginning, and that one is mine.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
[tip:x86/urgent] x86, cpufeature: Catch duplicate CPU feature strings
In reply to H. Peter AnvinCommit-ID: d6a99b549bc2d52ecefe06e59ba4fdcc0e59714b Gitweb: http://git.kernel.org/tip/d6a99b549bc2d52ecefe06e59ba4fdcc0e59714b Author: H. Peter Anvin <[email protected]> AuthorDate: Fri, 22 Jun 2012 11:47:15 -0700 Committer: H. Peter Anvin <[email protected]> CommitDate: Fri, 22 Jun 2012 11:47:15 -0700
x86, cpufeature: Catch duplicate CPU feature strings
We had a case of duplicate CPU feature strings, a user space ABI violation, for almost two years. Make it a build error so that doesn't happen again.
--- arch/x86/kernel/cpu/mkcapflags.pl | 23 ++++++++++++++++++----- 1 files changed, 18 insertions(+), 5 deletions(-)
arch/x86/kernel/cpu/mkcapflags.pl +18 -5
@@ -11,22 +11,35 @@ open(OUT, "> $out\0") or die "$0: cannot create: $out: $!\n"; print OUT "#include <asm/cpufeature.h>\n\n"; print OUT "const char * const x86_cap_flags[NCAPINTS*32] = {\n"; +%features = (); +$err = 0; + while (defined($line = <IN>)) { if ($line =~ /^\s*\#\s*define\s+(X86_FEATURE_(\S+))\s+(.*)$/) { $macro = $1; - $feature = $2; + $feature = "\L$2"; $tail = $3; if ($tail =~ /\/\*\s*\"([^"]*)\".*\*\//) { - $feature = $1; + $feature = "\L$1"; } - if ($feature ne '') { - printf OUT "\t%-32s = \"%s\",\n", - "[$macro]", "\L$feature"; + next if ($feature eq ''); + + if ($features{$feature}++) { + print STDERR "$in: duplicate feature name: $feature\n"; + $err++; } + printf OUT "\t%-32s = \"%s\",%s\n", "[$macro]", $feature; } } print OUT "};\n"; close(IN); close(OUT); + +if ($err) { + unlink($out); + exit(1); +} + +exit(0);
[tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
In reply to H. Peter AnvinCommit-ID: 4ad33411308596f2f918603509729922a1ec4411 Gitweb: http://git.kernel.org/tip/4ad33411308596f2f918603509729922a1ec4411 Author: H. Peter Anvin <[email protected]> AuthorDate: Fri, 22 Jun 2012 10:58:06 -0700 Committer: H. Peter Anvin <[email protected]> CommitDate: Mon, 25 Jun 2012 09:01:15 -0700
x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM
It makes sense to label "Digital Thermal Sensor" as "DTS", but unfortunately the string "dts" was already used for "Debug Store", and /proc/cpuinfo is a user space ABI.
Therefore, rename this to "dtherm".
This conflict went into mainline via the hwmon tree without any x86 maintainer ack, and without any kind of hint in the subject.
a4659053 x86/hwmon: fix initialization of coretemp
--- arch/x86/include/asm/cpufeature.h | 2 +- arch/x86/kernel/cpu/scattered.c | 2 +- drivers/hwmon/coretemp.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
arch/x86/include/asm/cpufeature.h +1 -1
@@ -176,7 +176,7 @@#define X86_FEATURE_XSAVEOPT (7*32+ 4)/* Optimized Xsave */ #define X86_FEATURE_PLN (7*32+ 5)/* Intel Power Limit Notification */ #define X86_FEATURE_PTS (7*32+ 6)/* Intel Package Thermal Status */ -#define X86_FEATURE_DTS (7*32+ 7)/* Digital Thermal Sensor */ +#define X86_FEATURE_DTHERM (7*32+ 7)/* Digital Thermal Sensor */ #define X86_FEATURE_HW_PSTATE (7*32+ 8)/* AMD HW-PState */ /* Virtualization flags: Linux defined, word 8 */
arch/x86/kernel/cpu/scattered.c +1 -1
@@ -31,7 +31,7 @@ void __cpuinit init_scattered_cpuid_features(struct cpuinfo_x86 *c)const struct cpuid_bit *cb;static const struct cpuid_bit __cpuinitconst cpuid_bits[] = { - {X86_FEATURE_DTS ,CR_EAX ,0 ,0x00000006 ,0 }, + {X86_FEATURE_DTHERM ,CR_EAX ,0 ,0x00000006 ,0 }, {X86_FEATURE_IDA ,CR_EAX ,1 ,0x00000006 ,0 }, {X86_FEATURE_ARAT ,CR_EAX ,2 ,0x00000006 ,0 }, {X86_FEATURE_PLN ,CR_EAX ,4 ,0x00000006 ,0 },
drivers/hwmon/coretemp.c +2 -2
@@ -664,7 +664,7 @@ static void __cpuinit get_core_online(unsigned int cpu)* sensors. We check this bit only, all the early CPUs * without thermal sensors will be filtered out. */ -if (!cpu_has (c,X86_FEATURE_DTS )) +if (!cpu_has (c,X86_FEATURE_DTHERM ))return ;if (!pdev) {
@@ -765,7 +765,7 @@ static struct notifier_block coretemp_cpu_notifier __refdata = { };static const struct x86_cpu_id coretemp_ids[] = { - {X86_VENDOR_INTEL ,X86_FAMILY_ANY ,X86_MODEL_ANY ,X86_FEATURE_DTS }, + {X86_VENDOR_INTEL ,X86_FAMILY_ANY ,X86_MODEL_ANY ,X86_FEATURE_DTHERM }, {} };MODULE_DEVICE_TABLE (x86cpu, coretemp_ids);
[tip:x86/urgent] x86, cpufeature: Catch duplicate CPU feature strings
In reply to H. Peter AnvinCommit-ID: 55f6cb9d0b364e7e8cb65b51193f5e4743c44fde Gitweb: http://git.kernel.org/tip/55f6cb9d0b364e7e8cb65b51193f5e4743c44fde Author: H. Peter Anvin <[email protected]> AuthorDate: Fri, 22 Jun 2012 11:47:15 -0700 Committer: H. Peter Anvin <[email protected]> CommitDate: Mon, 25 Jun 2012 09:02:13 -0700
x86, cpufeature: Catch duplicate CPU feature strings
We had a case of duplicate CPU feature strings, a user space ABI violation, for almost two years. Make it a build error so that doesn't happen again.
--- arch/x86/kernel/cpu/mkcapflags.pl | 23 ++++++++++++++++++----- 1 files changed, 18 insertions(+), 5 deletions(-)
arch/x86/kernel/cpu/mkcapflags.pl +18 -5
@@ -11,22 +11,35 @@ open(OUT, "> $out\0") or die "$0: cannot create: $out: $!\n"; print OUT "#include <asm/cpufeature.h>\n\n"; print OUT "const char * const x86_cap_flags[NCAPINTS*32] = {\n"; +%features = (); +$err = 0; + while (defined($line = <IN>)) { if ($line =~ /^\s*\#\s*define\s+(X86_FEATURE_(\S+))\s+(.*)$/) { $macro = $1; - $feature = $2; + $feature = "\L$2"; $tail = $3; if ($tail =~ /\/\*\s*\"([^"]*)\".*\*\//) { - $feature = $1; + $feature = "\L$1"; } - if ($feature ne '') { - printf OUT "\t%-32s = \"%s\",\n", - "[$macro]", "\L$feature"; + next if ($feature eq ''); + + if ($features{$feature}++) { + print STDERR "$in: duplicate feature name: $feature\n"; + $err++; } + printf OUT "\t%-32s = \"%s\",%s\n", "[$macro]", $feature; } } print OUT "};\n"; close(IN); close(OUT); + +if ($err) { + unlink($out); + exit(1); +} + +exit(0);
Jean Delvare
Acked-byRe: [tip:x86/urgent] x86, cpufeature: Catch duplicate CPU feature strings
Quoted 42 lines · tip-bot for H. Peter Anvin
Commit-ID: 55f6cb9d0b364e7e8cb65b51193f5e4743c44fde Gitweb: http://git.kernel.org/tip/55f6cb9d0b364e7e8cb65b51193f5e4743c44fde Author: H. Peter Anvin <[email protected]> AuthorDate: Fri, 22 Jun 2012 11:47:15 -0700 Committer: H. Peter Anvin <[email protected]> CommitDate: Mon, 25 Jun 2012 09:02:13 -0700x86, cpufeature: Catch duplicate CPU feature strings
We had a case of duplicate CPU feature strings, a user space ABI violation, for almost two years. Make it a build error so that doesn't happen again.
Cc Jan Beulich <[email protected]>Cc Jean Delvare <[email protected]>--- arch/x86/kernel/cpu/mkcapflags.pl | 23 ++++++++++++++++++----- 1 files changed, 18 insertions(+), 5 deletions(-)arch/x86/kernel/cpu/mkcapflags.pl +5
-2@@ -11,22 +11,35 @@ open(OUT, "> $out\0") or die "$0: cannot create: $out: $!\n"; print OUT "#include <asm/cpufeature.h>\n\n"; print OUT "const char * const x86_cap_flags[NCAPINTS*32] = {\n"; +%features = (); +$err = 0; + while (defined($line = <IN>)) { if ($line =~ /^\s*\#\s*define\s+(X86_FEATURE_(\S+))\s+(.*)$/) { $macro = $1; - $feature = $2; + $feature = "\L$2"; $tail = $3; if ($tail =~ /\/\*\s*\"([^"]*)\".*\*\//) { - $feature = $1; + $feature = "\L$1"; }
arch/x86/kernel/cpu/mkcapflags.pl open(OUT, "> $out\0") or die "$0: cannot create: $out: $!\n"; · line 32
Quoted 10 lines
arch/x86/kernel/cpu/mkcapflags.pl +6
-3- if ($feature ne '') { - printf OUT "\t%-32s = \"%s\",\n", - "[$macro]", "\L$feature"; + next if ($feature eq ''); + + if ($features{$feature}++) { + print STDERR "$in: duplicate feature name: $feature\n"; + $err++; } + printf OUT "\t%-32s = \"%s\",%s\n", "[$macro]", $feature;
You added an extra %s in the string, can't think of a good reason for it. perl -w would have told you, I think we should add it.
Other than this, the code looks good and the improvement reasonable, so you can add:
after fixing it.
Quoted 13 lines
arch/x86/kernel/cpu/mkcapflags.pl +7
-0} } print OUT "};\n"; close(IN); close(OUT); + +if ($err) { + unlink($out); + exit(1); +} + +exit(0);
Signature · 1 line
Jean Delvare
H. Peter Anvin
reviewerRe: [tip:x86/urgent] x86, cpufeature: Catch duplicate CPU feature strings
Quoted 4 lines · Jean Delvare
You added an extra %s in the string, can't think of a good reason for it. perl -w would have told you, I think we should add it.
Ah yes, a leftover from testing.
Quoted 5 lines
Other than this, the code looks good and the improvement reasonable, so you can add:
Acked-by Jean Delvare <[email protected]>
Should probably be reviewed-by: but nevertheless...
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
tip-bot for H. Peter Anvin
reviewer[tip:x86/urgent] x86, cpufeature: Remove stray %s, add -w to mkcapflags.pl
In reply to Jean DelvareCommit-ID: 1b6b7c9ff3514772958c075f8c89e42dddf6a4d8 Gitweb: http://git.kernel.org/tip/1b6b7c9ff3514772958c075f8c89e42dddf6a4d8 Author: H. Peter Anvin <[email protected]> AuthorDate: Tue, 26 Jun 2012 07:58:23 -0700 Committer: H. Peter Anvin <[email protected]> CommitDate: Tue, 26 Jun 2012 08:02:48 -0700
x86, cpufeature: Remove stray %s, add -w to mkcapflags.pl
There was a stray %s left from testing, remove it.
Add -w to the #! line (which is parsed by Perl even if the Perl interpreter is invoked explicitly on the command line) to catch these kinds of errors in the future.
--- arch/x86/kernel/cpu/mkcapflags.pl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/mkcapflags.pl +2 -2
@@ -1,4 +1,4 @@ -#!/usr/bin/perl +#!/usr/bin/perl -w # # Generate the x86_cap_flags[] array from include/asm-x86/cpufeature.h #
@@ -29,7 +29,7 @@ while (defined($line = <IN>)) { print STDERR "$in: duplicate feature name: $feature\n"; $err++; } - printf OUT "\t%-32s = \"%s\",%s\n", "[$macro]", $feature; + printf OUT "\t%-32s = \"%s\",\n", "[$macro]", $feature; } } print OUT "};\n";