[lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Jan Beulich]

According to the documentation, simply reading the respective MSR
isn't sufficient: It should be written with zeros, cpuid(1) be
executed, and then read (see arch/x86/kernel/cpu/intel.c for an
example).

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Chen Gong <gong.chen@linux.intel.com>

---
 drivers/hwmon/coretemp.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

--- linux-2.6.36-rc7/drivers/hwmon/coretemp.c
+++ 2.6.36-rc7-coretemp-ucode/drivers/hwmon/coretemp.c
@@ -292,6 +292,15 @@ static int __devinit get_tjmax(struct cp
 	}
 }
 
+static void __devinit get_ucode_rev_on_cpu(void *edx)
+{
+	u32 eax;
+
+	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+	sync_core();
+	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -327,8 +336,13 @@ static int __devinit coretemp_probe(stru
 
 	if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
 		/* check for microcode update */
-		rdmsr_on_cpu(data->id, MSR_IA32_UCODE_REV, &eax, &edx);
-		if (edx < 0x39) {
+		err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
+					       &edx, 1);
+		if (err)
+			dev_warn(&pdev->dev,
+				 "Cannot determine microcode revision "
+				 "of the CPU!\n");
+		else if (edx < 0x39) {
 			err = -ENODEV;
 			dev_err(&pdev->dev,
 				"Errata AE18 not fixed, update BIOS or "




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Fenghua Yu]

On Thu, Oct 07, 2010 at 02:36:01AM -0700, Jan Beulich wrote:
> According to the documentation, simply reading the respective MSR
> isn't sufficient: It should be written with zeros, cpuid(1) be
> executed, and then read (see arch/x86/kernel/cpu/intel.c for an
> example).
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Chen Gong <gong.chen@linux.intel.com>
> 
> ---
>  drivers/hwmon/coretemp.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.36-rc7/drivers/hwmon/coretemp.c
> +++ 2.6.36-rc7-coretemp-ucode/drivers/hwmon/coretemp.c
> @@ -292,6 +292,15 @@ static int __devinit get_tjmax(struct cp
>  	}
>  }
>  
> +static void __devinit get_ucode_rev_on_cpu(void *edx)
> +{
> +	u32 eax;
> +
> +	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> +	sync_core();
> +	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
> +}
> +
>  static int __devinit coretemp_probe(struct platform_device *pdev)
>  {
>  	struct coretemp_data *data;
> @@ -327,8 +336,13 @@ static int __devinit coretemp_probe(stru
>  
>  	if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
>  		/* check for microcode update */
> -		rdmsr_on_cpu(data->id, MSR_IA32_UCODE_REV, &eax, &edx);
> -		if (edx < 0x39) {
> +		err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
> +					       &edx, 1);
> +		if (err)
> +			dev_warn(&pdev->dev,
> +				 "Cannot determine microcode revision "
> +				 "of the CPU!\n");

When err, need to call dev_err and go to exit_free. This error handling should
be same as edx < 0x39 case.

> +		else if (edx < 0x39) {
>  			err = -ENODEV;
>  			dev_err(&pdev->dev,
>  				"Errata AE18 not fixed, update BIOS or "
> 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Jan Beulich]

>>> On 07.10.10 at 20:46, Fenghua Yu <fenghua.yu@intel.com> wrote:
> On Thu, Oct 07, 2010 at 02:36:01AM -0700, Jan Beulich wrote:
>> @@ -327,8 +336,13 @@ static int __devinit coretemp_probe(stru
>>  
>>  	if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
>>  		/* check for microcode update */
>> -		rdmsr_on_cpu(data->id, MSR_IA32_UCODE_REV, &eax, &edx);
>> -		if (edx < 0x39) {
>> +		err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
>> +					       &edx, 1);
>> +		if (err)
>> +			dev_warn(&pdev->dev,
>> +				 "Cannot determine microcode revision "
>> +				 "of the CPU!\n");
> 
> When err, need to call dev_err and go to exit_free. This error handling 
> should
> be same as edx < 0x39 case.

Hmm, not sure - I'd prefer to consider the machine usable in this
(theoretical only anyway) case.

Jan


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Jean Delvare]

Hi Jan,

On Fri, 08 Oct 2010 08:05:21 +0100, Jan Beulich wrote:
> >>> On 07.10.10 at 20:46, Fenghua Yu <fenghua.yu@intel.com> wrote:
> > On Thu, Oct 07, 2010 at 02:36:01AM -0700, Jan Beulich wrote:
> >> @@ -327,8 +336,13 @@ static int __devinit coretemp_probe(stru
> >>  
> >>  	if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
> >>  		/* check for microcode update */
> >> -		rdmsr_on_cpu(data->id, MSR_IA32_UCODE_REV, &eax, &edx);
> >> -		if (edx < 0x39) {
> >> +		err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
> >> +					       &edx, 1);
> >> +		if (err)
> >> +			dev_warn(&pdev->dev,
> >> +				 "Cannot determine microcode revision "
> >> +				 "of the CPU!\n");
> > 
> > When err, need to call dev_err and go to exit_free. This error handling 
> > should
> > be same as edx < 0x39 case.
> 
> Hmm, not sure - I'd prefer to consider the machine usable in this
> (theoretical only anyway) case.

I tend to prefer Fenghua's approach, sorry. If a bad microcode revision
is so wrong that we fail the probe when we see it, then a failure to
determine the microcode revision should lead to the same result. As
always, missing monitoring information is preferable to unreliable
monitoring information.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Jan Beulich]

According to the documentation, simply reading the respective MSR
isn't sufficient: It should be written with zeros, cpuid(1) be
executed, and then read (see arch/x86/kernel/cpu/intel.c for an
example).

v2: Fail probe when microcode revision cannot be determined, but is
needed to check for proper operation.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Chen Gong <gong.chen@linux.intel.com>
Cc: Jean Delvare <khali@linux-fr.org>

---
 drivers/hwmon/coretemp.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

--- linux-2.6.36-rc7/drivers/hwmon/coretemp.c
+++ 2.6.36-rc7-coretemp-ucode/drivers/hwmon/coretemp.c
@@ -292,6 +292,15 @@ static int __devinit get_tjmax(struct cp
 	}
 }
 
+static void __devinit get_ucode_rev_on_cpu(void *edx)
+{
+	u32 eax;
+
+	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+	sync_core();
+	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -327,8 +336,15 @@ static int __devinit coretemp_probe(stru
 
 	if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
 		/* check for microcode update */
-		rdmsr_on_cpu(data->id, MSR_IA32_UCODE_REV, &eax, &edx);
-		if (edx < 0x39) {
+		err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
+					       &edx, 1);
+		if (err) {
+			dev_err(&pdev->dev,
+				"Cannot determine microcode revision of "
+				"CPU#%u (%d)!\n", data->id, err);
+			err = -ENODEV;
+			goto exit_free;
+		} else if (edx < 0x39) {
 			err = -ENODEV;
 			dev_err(&pdev->dev,
 				"Errata AE18 not fixed, update BIOS or "




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Henrique de Moraes Holschuh]

On Fri, 08 Oct 2010, Jean Delvare wrote:
> On Fri, 08 Oct 2010 08:05:21 +0100, Jan Beulich wrote:
> > >>> On 07.10.10 at 20:46, Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > On Thu, Oct 07, 2010 at 02:36:01AM -0700, Jan Beulich wrote:
> > >> @@ -327,8 +336,13 @@ static int __devinit coretemp_probe(stru
> > >>  
> > >>  	if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
> > >>  		/* check for microcode update */
> > >> -		rdmsr_on_cpu(data->id, MSR_IA32_UCODE_REV, &eax, &edx);
> > >> -		if (edx < 0x39) {
> > >> +		err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
> > >> +					       &edx, 1);
> > >> +		if (err)
> > >> +			dev_warn(&pdev->dev,
> > >> +				 "Cannot determine microcode revision "
> > >> +				 "of the CPU!\n");
> > > 
> > > When err, need to call dev_err and go to exit_free. This error handling 
> > > should
> > > be same as edx < 0x39 case.
> > 
> > Hmm, not sure - I'd prefer to consider the machine usable in this
> > (theoretical only anyway) case.
> 
> I tend to prefer Fenghua's approach, sorry. If a bad microcode revision
> is so wrong that we fail the probe when we see it, then a failure to
> determine the microcode revision should lead to the same result. As
> always, missing monitoring information is preferable to unreliable
> monitoring information.

That is correct.  On a related note, this is the second place in the kernel
that needs to determine the microcode revision.  Shouldn't we have this as
common arch-specific code?

The nasty thing about microcode revisions is that they can increase at any
point in time (although we CAN get notified of it), but I don't think we 
ensure that they cannot go BACK after suspend/resume cycles.

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode

[Henrique de Moraes Holschuh]

I still think this should be common code shared with microcode_intel.

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Guenter Roeck]

On Fri, Oct 08, 2010 at 08:33:22AM -0400, Henrique de Moraes Holschuh wrote:
[ ... ]
> 
> The nasty thing about microcode revisions is that they can increase at any
> point in time (although we CAN get notified of it), but I don't think we 
> ensure that they cannot go BACK after suspend/resume cycles.
> 
Not sure if it would be worthwhile dealing with that. This sounds a lot like
"let's try to make the OS's life miserable", and you end up with a lot of
nasty code trying to deal with such situations. Next would be to try to verify
if the microcode is really the version it claims to be, or if someone hacked it.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode revision

[Henrique de Moraes Holschuh]

On Fri, 08 Oct 2010, Guenter Roeck wrote:
> On Fri, Oct 08, 2010 at 08:33:22AM -0400, Henrique de Moraes Holschuh wrote:
> [ ... ]
> > The nasty thing about microcode revisions is that they can increase at any
> > point in time (although we CAN get notified of it), but I don't think we 
> > ensure that they cannot go BACK after suspend/resume cycles.

Actually, I just checked the code and we do.  There are some misleading
comments on top of microcode_intel.c.

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode

[Guenter Roeck]

On Fri, Oct 08, 2010 at 04:59:38AM -0400, Jan Beulich wrote:
> According to the documentation, simply reading the respective MSR
> isn't sufficient: It should be written with zeros, cpuid(1) be
> executed, and then read (see arch/x86/kernel/cpu/intel.c for an
> example).
> 
> v2: Fail probe when microcode revision cannot be determined, but is
> needed to check for proper operation.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Chen Gong <gong.chen@linux.intel.com>
> Cc: Jean Delvare <khali@linux-fr.org>
> 
> ---
>  drivers/hwmon/coretemp.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.36-rc7/drivers/hwmon/coretemp.c
> +++ 2.6.36-rc7-coretemp-ucode/drivers/hwmon/coretemp.c
> @@ -292,6 +292,15 @@ static int __devinit get_tjmax(struct cp
>  	}
>  }
>  
> +static void __devinit get_ucode_rev_on_cpu(void *edx)
> +{
> +	u32 eax;
> +
> +	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> +	sync_core();
> +	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
> +}
> +
>  static int __devinit coretemp_probe(struct platform_device *pdev)
>  {
>  	struct coretemp_data *data;
> @@ -327,8 +336,15 @@ static int __devinit coretemp_probe(stru
>  
>  	if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
>  		/* check for microcode update */
> -		rdmsr_on_cpu(data->id, MSR_IA32_UCODE_REV, &eax, &edx);
> -		if (edx < 0x39) {
> +		err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
> +					       &edx, 1);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"Cannot determine microcode revision of "
> +				"CPU#%u (%d)!\n", data->id, err);
> +			err = -ENODEV;
> +			goto exit_free;
> +		} else if (edx < 0x39) {
>  			err = -ENODEV;
>  			dev_err(&pdev->dev,
>  				"Errata AE18 not fixed, update BIOS or "
> 
Do we have a conclusion on this ? Looks like no one Acked it, and there was
a suggestion to provice a generic piece of code to retrieve the ucode version.
Turns out the ucode revision is already available via ucode_cpu_info[cpu].cpu_sig.rev
unless I am missing something.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode

[Jan Beulich]

>>> On 09.10.10 at 20:44, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> Turns out the ucode revision is already available via 
> ucode_cpu_info[cpu].cpu_sig.rev
> unless I am missing something.

But you're not suggesting to make coretemp.ko depend on
microcode.ko, are you?

Jan


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode

[Guenter Roeck]

On Mon, Oct 11, 2010 at 02:51:00AM -0400, Jan Beulich wrote:
> >>> On 09.10.10 at 20:44, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > Turns out the ucode revision is already available via 
> > ucode_cpu_info[cpu].cpu_sig.rev
> > unless I am missing something.
> 
> But you're not suggesting to make coretemp.ko depend on
> microcode.ko, are you?
> 
I am not really suggesting anything. Personally, I think the code is just fine
as it is. I would just like someone to Ack it before I would want to apply
it, and I happened to notice the above while I was looking for other places
where the microcode version is retrieved.

On the contrary, the added dependency (which I had not realized) would be
a good argument against using ucode_cpu_info[].

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode

[Fenghua Yu]

On Mon, Oct 11, 2010 at 05:40:28AM -0700, Guenter Roeck wrote:
> On Mon, Oct 11, 2010 at 02:51:00AM -0400, Jan Beulich wrote:
> > >>> On 09.10.10 at 20:44, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > Turns out the ucode revision is already available via 
> > > ucode_cpu_info[cpu].cpu_sig.rev
> > > unless I am missing something.
> > 
> > But you're not suggesting to make coretemp.ko depend on
> > microcode.ko, are you?
> > 
> I am not really suggesting anything. Personally, I think the code is just fine
> as it is. I would just like someone to Ack it before I would want to apply
> it, and I happened to notice the above while I was looking for other places
> where the microcode version is retrieved.
> 
> On the contrary, the added dependency (which I had not realized) would be
> a good argument against using ucode_cpu_info[].
> 
> Guenter
> 
Acked-by: Fenghua Yu <fenghua.yu@intel.com>

---
We can apply Jan's clean v2 patch for coretemp.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] coretemp: fix reading of microcode

[Guenter Roeck]

On Mon, Oct 11, 2010 at 11:13:00AM -0400, Fenghua Yu wrote:
> On Mon, Oct 11, 2010 at 05:40:28AM -0700, Guenter Roeck wrote:
> > On Mon, Oct 11, 2010 at 02:51:00AM -0400, Jan Beulich wrote:
> > > >>> On 09.10.10 at 20:44, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > > Turns out the ucode revision is already available via 
> > > > ucode_cpu_info[cpu].cpu_sig.rev
> > > > unless I am missing something.
> > > 
> > > But you're not suggesting to make coretemp.ko depend on
> > > microcode.ko, are you?
> > > 
> > I am not really suggesting anything. Personally, I think the code is just fine
> > as it is. I would just like someone to Ack it before I would want to apply
> > it, and I happened to notice the above while I was looking for other places
> > where the microcode version is retrieved.
> > 
> > On the contrary, the added dependency (which I had not realized) would be
> > a good argument against using ucode_cpu_info[].
> > 
> > Guenter
> > 
> Acked-by: Fenghua Yu <fenghua.yu@intel.com>
> 
Applied.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors