Re: [lm-sensors] [PATCH] coretemp: Add TEMP_TARGET support

[Jean Delvare <khali@linux-fr.org>]

Hi Rudolf,

On Sat, 01 Dec 2007 21:37:39 +0100, Rudolf Marek wrote:
> Attached patch enables driver to export the target temperature. This temperature
> is calibrated into each CPU, and serves as some indicator to cooling platform.
> All fans should spin at full speed if the temperature is reached.
> 
> Signed-off-by: Rudolf Marek <r.marek@assembler.cz>
> 
> I managed to get answer from Intel at:
> 
> http://softwarecommunity.intel.com/isn/Community/en-US/forums/permalink/30228436/30230975/ShowThread.aspx#30230975
> 
> Can someone test if it works please? The lines in the patch are longer than 80
> chars, imho it is allowed now?

Only if folding the line in question would hinder code readability.
This doesn't seem to be the case here so I think you should fold (most
of) the lines.

> coretemp-isa-0000
> Adapter: ISA adapter
> Core 0:      +36.0°C  (high = +65.0°C, crit = +85.0°C)
> 
> I have in plan to add Penryn CPUs to driver once I know the details.

I can only test on my T2600 which does not have the feature. So all I
can say is that there is no regression there.

> Index: linux-2.6.23.9/drivers/hwmon/coretemp.c
> =================================> --- linux-2.6.23.9.orig/drivers/hwmon/coretemp.c	2007-12-01 12:28:08.000000000 +0100
> +++ linux-2.6.23.9/drivers/hwmon/coretemp.c	2007-12-01 21:29:10.000000000 +0100
> @@ -38,7 +38,7 @@
>  
>  #define DRVNAME	"coretemp"
>  
> -typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_LABEL, SHOW_NAME } SHOW;
> +typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL, SHOW_NAME } SHOW;
>  
>  /*
>   * Functions declaration
> @@ -55,6 +55,7 @@
>  	unsigned long last_updated;	/* in jiffies */
>  	int temp;
>  	int tjmax;
> +	int ttarget;
>  	u8 alarm;
>  };
>  
> @@ -95,9 +96,10 @@
>  
>  	if (attr->index = SHOW_TEMP)
>  		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
> -	else
> +	else if (attr->index = SHOW_TJMAX)
>  		err = sprintf(buf, "%d\n", data->tjmax);
> -
> +	else
> +		err = sprintf(buf, "%d\n", data->ttarget);
>  	return err;
>  }
>  
> @@ -105,6 +107,8 @@
>  			  SHOW_TEMP);
>  static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
>  			  SHOW_TJMAX);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
> +			  SHOW_TTARGET);
>  static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
>  static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
>  static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
> @@ -223,10 +227,27 @@
>  			 "temperature might be wrong!\n");
>  	}
>  
> +	data->ttarget = data->tjmax;

This initialization isn't needed. data->ttarget is only read if the
temp1_max is created, and in this case you overwrite the value below.

> +
> +	/* read the still undocumented IA32_TEMPERATURE_TARGET it exists
> +	   on older CPUs but not here */

I can't really make sense of the second part of this comment, please
clarify.

> +	if (c->x86_model > 0xe) {
> +		err = rdmsr_safe_on_cpu(data->id, 0x1a2, &eax, &edx);
> +		if (err) {
> +			dev_warn(&pdev->dev, "Unable to read IA32_TEMPERATURE_TARGET MSR\n");
> +		} else {
> +			data->ttarget = data->tjmax - (((eax >> 8) & 0xff) * 1000);
> +			err = device_create_file(&pdev->dev,
> +							&sensor_dev_attr_temp1_max.dev_attr);
> +			if (err)
> +				goto exit_free;
> +		}
> +	}
> +
>  	platform_set_drvdata(pdev, data);

You have a race condition here, the driver data should be set before
creating any sysfs file, otherwise dev_get_drvdata() might be called
before it is set.

>  
>  	if ((err = sysfs_create_group(&pdev->dev.kobj, &coretemp_group)))
> -		goto exit_free;
> +		goto exit_dev;
>  
>  	data->class_dev = hwmon_device_register(&pdev->dev);
>  	if (IS_ERR(data->class_dev)) {
> @@ -240,6 +261,8 @@
>  
>  exit_class:
>  	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> +exit_dev:
> +	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
>  exit_free:
>  	kfree(data);
>  exit:
> @@ -252,6 +275,7 @@
>  
>  	hwmon_device_unregister(data->class_dev);
>  	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> +	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
>  	platform_set_drvdata(pdev, NULL);
>  	kfree(data);
>  	return 0;
> Index: linux-2.6.23.9/Documentation/hwmon/coretemp
> =================================> --- linux-2.6.23.9.orig/Documentation/hwmon/coretemp	2007-12-01 21:25:17.000000000 +0100
> +++ linux-2.6.23.9/Documentation/hwmon/coretemp	2007-12-01 21:28:35.000000000 +0100
> @@ -25,7 +25,8 @@
>  the Out-Of-Spec bit. Following table summarizes the exported sysfs files:
>  
>  temp1_input	 - Core temperature (in millidegrees Celsius).
> -temp1_crit	 - Maximum junction temperature  (in millidegrees Celsius).
> +temp1_max	 - All cooling devices should be turned on (on Core2).
> +temp1_crit	 - Maximum junction temperature (in millidegrees Celsius).
>  temp1_crit_alarm - Set when Out-of-spec bit is set, never clears.
>  		   Correct CPU operation is no longer guaranteed.
>  temp1_label	 - Contains string "Core X", where X is processor

-- 
Jean Delvare

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