[PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[Jean Delvare]

From: Jean Delvare <khali@linux-fr.org>
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 <khali@linux-fr.org>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/cpufeature.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-3.5-rc3.orig/arch/x86/include/asm/cpufeature.h	2012-06-21 15:57:17.061585016 +0200
+++ linux-3.5-rc3/arch/x86/include/asm/cpufeature.h	2012-06-21 15:57:42.087585387 +0200
@@ -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 */


-- 
Jean Delvare

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[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?

	-hpa


On 06/21/2012 07:28 AM, Jean Delvare wrote:
> From: Jean Delvare <khali@linux-fr.org>
> 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 <khali@linux-fr.org>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/include/asm/cpufeature.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-3.5-rc3.orig/arch/x86/include/asm/cpufeature.h	2012-06-21 15:57:17.061585016 +0200
> +++ linux-3.5-rc3/arch/x86/include/asm/cpufeature.h	2012-06-21 15:57:42.087585387 +0200
> @@ -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 */
> 
> 

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[Jean Delvare]

On Thu, 21 Jun 2012 09:28:59 -0700, H. Peter Anvin wrote:
> 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...

-- 
Jean Delvare

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[H. Peter Anvin]

On 06/21/2012 10:07 AM, Jean Delvare wrote:
> 
> 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...
> 

No, but apparently perfmon2 depends on it, in userspace, and it relies
on the string.

So this is a nonstarter.

	-hpa

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[Borislav Petkov]

On Thu, Jun 21, 2012 at 11:06:04AM -0700, H. Peter Anvin wrote:
> On 06/21/2012 10:07 AM, Jean Delvare wrote:
> > 
> > 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...
> > 
> 
> 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"?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[H. Peter Anvin]

On 06/21/2012 11:44 AM, Borislav Petkov wrote:
> On Thu, Jun 21, 2012 at 11:06:04AM -0700, H. Peter Anvin wrote:
>> On 06/21/2012 10:07 AM, Jean Delvare wrote:
>>>
>>> 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...
>>>
>>
>> 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

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[Ingo Molnar]

* H. Peter Anvin <hpa@linux.intel.com> wrote:

> On 06/21/2012 11:44 AM, Borislav Petkov wrote:
> > On Thu, Jun 21, 2012 at 11:06:04AM -0700, H. Peter Anvin wrote:
> >> On 06/21/2012 10:07 AM, Jean Delvare wrote:
> >>>
> >>> 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...
> >>>
> >>
> >> 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.

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

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[Jan Beulich]

>>> On 21.06.12 at 18:28, "H. Peter Anvin" <hpa@linux.intel.com> wrote:
> 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

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[H. Peter Anvin]

Definitely!

Jan Beulich <JBeulich@suse.com> wrote:

>>>> On 21.06.12 at 18:28, "H. Peter Anvin" <hpa@linux.intel.com> wrote:
>> 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

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[H. Peter Anvin]

On 06/21/2012 11:59 PM, Ingo Molnar wrote:
> 
> 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

Re: [PATCH] x86, cpufeature: Fix duplicate feature name "dts"

[Guenter Roeck]

On Fri, 2012-06-22 at 14:01 -0400, H. Peter Anvin wrote:
> On 06/21/2012 11:59 PM, Ingo Molnar wrote:
> > 
> > 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.

My apologies. Hope I learned a bit since then.

Guenter

[tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM

[tip-bot for H. Peter Anvin]

Commit-ID:  3bac8715443fc83413e221a5cd9d5cf3d82b79b4
Gitweb:     http://git.kernel.org/tip/3bac8715443fc83413e221a5cd9d5cf3d82b79b4
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 22 Jun 2012 10:58:06 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
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

Reported-by: Jean Delvare <khali@linux-fr.org>
Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
Cc: Jan Beulich <JBeulich@suse.com>
Cc: <stable@vger.kernel.org> v2.6.36..v3.4
---
 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(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 340ee49..f91e80f 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -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 */
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index addf9e8..ee8e9ab 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -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 },
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index b9d5123..0f52799 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -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

[tip-bot for H. Peter Anvin]

Commit-ID:  d6a99b549bc2d52ecefe06e59ba4fdcc0e59714b
Gitweb:     http://git.kernel.org/tip/d6a99b549bc2d52ecefe06e59ba4fdcc0e59714b
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 22 Jun 2012 11:47:15 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
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.

Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Jean Delvare <khali@linux-fr.org>
---
 arch/x86/kernel/cpu/mkcapflags.pl |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/mkcapflags.pl b/arch/x86/kernel/cpu/mkcapflags.pl
index dfea390..0c5b549 100644
--- a/arch/x86/kernel/cpu/mkcapflags.pl
+++ b/arch/x86/kernel/cpu/mkcapflags.pl
@@ -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);

Re: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM

[Jean Delvare]

On Fri, 22 Jun 2012 13:38:21 -0700, tip-bot for H. Peter Anvin wrote:
> Commit-ID:  3bac8715443fc83413e221a5cd9d5cf3d82b79b4
> Gitweb:     http://git.kernel.org/tip/3bac8715443fc83413e221a5cd9d5cf3d82b79b4
> Author:     H. Peter Anvin <hpa@linux.intel.com>
> AuthorDate: Fri, 22 Jun 2012 10:58:06 -0700
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> 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

You mean "Digital Thermal Sensor".

> unfortunately the string "dts" was already used for "Debug Store", and
> /proc/cpuinfo is 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?

> 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

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.

> Reported-by: Jean Delvare <khali@linux-fr.org>
> Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: <stable@vger.kernel.org> v2.6.36..v3.4

No Signed-off-by?

Not sure why you want this to go to stable trees?

> ---
>  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(-)

-- 
Jean Delvare

Re: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM

[H. Peter Anvin]

On 06/24/2012 12:49 PM, Jean Delvare wrote:
>>
>> 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?
>

Except that *really* seems like begging for similar problems in the future.

>> 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
>
> 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.
>
>> Reported-by: Jean Delvare <khali@linux-fr.org>
>> Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
>> Cc: Jan Beulich <JBeulich@suse.com>
>> Cc: <stable@vger.kernel.org> v2.6.36..v3.4
>
> 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.

	-hpa

-- 
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

[H. Peter Anvin]

On 06/24/2012 12:49 PM, Jean Delvare wrote:
>
>> 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
>
> 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.
>

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

-- 
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

[H. Peter Anvin]

On 06/24/2012 01:39 PM, H. Peter Anvin wrote:
>
> 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

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM

[tip-bot for H. Peter Anvin]

Commit-ID:  4ad33411308596f2f918603509729922a1ec4411
Gitweb:     http://git.kernel.org/tip/4ad33411308596f2f918603509729922a1ec4411
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 22 Jun 2012 10:58:06 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
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

Reported-by: Jean Delvare <khali@linux-fr.org>
Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
Cc: Jan Beulich <JBeulich@suse.com>
Cc: <stable@vger.kernel.org> v2.6.36..v3.4
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 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(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 340ee49..f91e80f 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -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 */
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index addf9e8..ee8e9ab 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -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 },
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index b9d5123..0f52799 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -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

[tip-bot for H. Peter Anvin]

Commit-ID:  55f6cb9d0b364e7e8cb65b51193f5e4743c44fde
Gitweb:     http://git.kernel.org/tip/55f6cb9d0b364e7e8cb65b51193f5e4743c44fde
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 22 Jun 2012 11:47:15 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
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.

Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Jean Delvare <khali@linux-fr.org>
---
 arch/x86/kernel/cpu/mkcapflags.pl |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/mkcapflags.pl b/arch/x86/kernel/cpu/mkcapflags.pl
index dfea390..0c5b549 100644
--- a/arch/x86/kernel/cpu/mkcapflags.pl
+++ b/arch/x86/kernel/cpu/mkcapflags.pl
@@ -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);

Re: [tip:x86/urgent] x86, cpufeature: Rename X86_FEATURE_DTS to X86_FEATURE_DTHERM

[Jean Delvare]

On Sun, 24 Jun 2012 13:37:00 -0700, H. Peter Anvin wrote:
> On 06/24/2012 12:49 PM, Jean Delvare wrote:
> >>
> >> 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?
> 
> 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?

> >> 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
> >
> > 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.
> >
> >> Reported-by: Jean Delvare <khali@linux-fr.org>
> >> Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
> >> Cc: Jan Beulich <JBeulich@suse.com>
> >> Cc: <stable@vger.kernel.org> v2.6.36..v3.4
> >
> > 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...

-- 
Jean Delvare

Re: [tip:x86/urgent] x86, cpufeature: Catch duplicate CPU feature strings

[Jean Delvare]

On Mon, 25 Jun 2012 09:14:29 -0700, tip-bot for H. Peter Anvin wrote:
> Commit-ID:  55f6cb9d0b364e7e8cb65b51193f5e4743c44fde
> Gitweb:     http://git.kernel.org/tip/55f6cb9d0b364e7e8cb65b51193f5e4743c44fde
> Author:     H. Peter Anvin <hpa@linux.intel.com>
> AuthorDate: Fri, 22 Jun 2012 11:47:15 -0700
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> 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.
> 
> Link: http://lkml.kernel.org/r/4FE34BCB.5050305@linux.intel.com
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Jean Delvare <khali@linux-fr.org>
> ---
>  arch/x86/kernel/cpu/mkcapflags.pl |   23 ++++++++++++++++++-----
>  1 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mkcapflags.pl b/arch/x86/kernel/cpu/mkcapflags.pl
> index dfea390..0c5b549 100644
> --- a/arch/x86/kernel/cpu/mkcapflags.pl
> +++ b/arch/x86/kernel/cpu/mkcapflags.pl
> @@ -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;

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:

Acked-by: Jean Delvare <khali@linux-fr.org>

after fixing it.

>  	}
>  }
>  print OUT "};\n";
>  
>  close(IN);
>  close(OUT);
> +
> +if ($err) {
> +	unlink($out);
> +	exit(1);
> +}
> +
> +exit(0);


-- 
Jean Delvare

Re: [tip:x86/urgent] x86, cpufeature: Catch duplicate CPU feature strings

[H. Peter Anvin]

On 06/26/2012 05:32 AM, Jean Delvare wrote:
>
> 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.

> Other than this, the code looks good and the improvement reasonable, so
> you can add:
>
> Acked-by: Jean Delvare <khali@linux-fr.org>
>

Should probably be reviewed-by: but nevertheless...

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[tip:x86/urgent] x86, cpufeature: Remove stray %s, add -w to mkcapflags.pl

[tip-bot for H. Peter Anvin]

Commit-ID:  1b6b7c9ff3514772958c075f8c89e42dddf6a4d8
Gitweb:     http://git.kernel.org/tip/1b6b7c9ff3514772958c075f8c89e42dddf6a4d8
Author:     H. Peter Anvin <hpa@zytor.com>
AuthorDate: Tue, 26 Jun 2012 07:58:23 -0700
Committer:  H. Peter Anvin <hpa@zytor.com>
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.

Reported-by: Jean Delvare <khali@linux-fr.org>
Link: http://lkml.kernel.org/r/20120626143246.0c9bf301@endymion.delvare
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/cpu/mkcapflags.pl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mkcapflags.pl b/arch/x86/kernel/cpu/mkcapflags.pl
index 0c5b549..c7b3fe2 100644
--- a/arch/x86/kernel/cpu/mkcapflags.pl
+++ b/arch/x86/kernel/cpu/mkcapflags.pl
@@ -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";