[lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold Support to

[lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold Support to

[Durgadoss R]

This patch adds the core and pkg support to coretemp.
These thresholds can be configured via the sysfs interfaces tempX_max
and tempX_max_hyst. An interrupt is generated when CPU temperature reaches
or crosses above tempX_max OR drops below tempX_max_hyst.

This patch is based on the documentation in IA Manual vol 3A, that can be
downloaded from here:
http://download.intel.com/design/processor/manuals/253668.pdf

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
v1
* Initial code to add core/pkg threshold support
v2
* Rearranged the attrs in rd/wr arrays in create_core_attrs function
* Added code to conditionally create the ttarget/tmin/max_alarm interfaces
* Changed the MAX_ATTRS definition
* Added attr_size variable to temp_data structure
v3
* Fixed some spelling mistakes
* Optimized if-else in create_core_attrs function
v4
* Report max_alarm based on STATUS bit instead of LOG bit
* Corrected bound checking in store_ttarget/tmin functions
 Documentation/hwmon/coretemp |    7 ++
 drivers/hwmon/coretemp.c     |  177 +++++++++++++++++++++++++++++++-----------
 2 files changed, 139 insertions(+), 45 deletions(-)

diff --git a/Documentation/hwmon/coretemp b/Documentation/hwmon/coretemp
index f85e913..fa8776a 100644
--- a/Documentation/hwmon/coretemp
+++ b/Documentation/hwmon/coretemp
@@ -35,6 +35,13 @@ the Out-Of-Spec bit. Following table summarizes the exported sysfs files:
 All Sysfs entries are named with their core_id (represented here by 'X').
 tempX_input	 - Core temperature (in millidegrees Celsius).
 tempX_max	 - All cooling devices should be turned on (on Core2).
+		   Initialized with IA32_THERM_INTERRUPT. When the CPU
+		   temperature reaches this temperature, an interrupt is
+		   generated and tempX_max_alarm is set.
+tempX_max_hyst   - If the CPU temperature falls below than temperature,
+		   an interrupt is generated and tempX_max_alarm is reset.
+tempX_max_alarm  - Set if the temperature reaches or exceeds tempX_max.
+		   Reset if the temperature drops to or below tempX_max_hyst.
 tempX_crit	 - Maximum junction temperature (in millidegrees Celsius).
 tempX_crit_alarm - Set when Out-of-spec bit is set, never clears.
 		   Correct CPU operation is no longer guaranteed.
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index de3d246..53ed50d 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -44,7 +44,9 @@
 #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
 #define NUM_REAL_CORES		16	/* Number of Real cores per cpu */
 #define CORETEMP_NAME_LENGTH	17	/* String Length of attrs */
-#define MAX_ATTRS		5	/* Maximum no of per-core attrs */
+#define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
+#define MAX_THRESH_ATTRS	3	/* Maximum no of Threshold attrs */
+#define TOTAL_ATTRS		(MAX_CORE_ATTRS + MAX_THRESH_ATTRS)
 #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
 
 #ifdef CONFIG_SMP
@@ -67,6 +69,9 @@
  *		This value is passed as "id" field to rdmsr/wrmsr functions.
  * @status_reg: One of IA32_THERM_STATUS or IA32_PACKAGE_THERM_STATUS,
  *		from where the temperature values should be read.
+ * @intrpt_reg: One of IA32_THERM_INTERRUPT or IA32_PACKAGE_THERM_INTERRUPT,
+ *		from where the thresholds are read.
+ * @attr_size:  Total number of pre-core attrs displayed in the sysfs.
  * @is_pkg_data: If this is 1, the temp_data holds pkgtemp data.
  *		Otherwise, temp_data holds coretemp data.
  * @valid: If this is 1, the current temperature is valid.
@@ -74,15 +79,18 @@
 struct temp_data {
 	int temp;
 	int ttarget;
+	int tmin;
 	int tjmax;
 	unsigned long last_updated;
 	unsigned int cpu;
 	u32 cpu_core_id;
 	u32 status_reg;
+	u32 intrpt_reg;
+	int attr_size;
 	bool is_pkg_data;
 	bool valid;
-	struct sensor_device_attribute sd_attrs[MAX_ATTRS];
-	char attr_name[MAX_ATTRS][CORETEMP_NAME_LENGTH];
+	struct sensor_device_attribute sd_attrs[TOTAL_ATTRS];
+	char attr_name[TOTAL_ATTRS][CORETEMP_NAME_LENGTH];
 	struct mutex update_lock;
 };
 
@@ -137,6 +145,19 @@ static ssize_t show_crit_alarm(struct device *dev,
 	return sprintf(buf, "%d\n", (eax >> 5) & 1);
 }
 
+static ssize_t show_max_alarm(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	u32 eax, edx;
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct temp_data *tdata = pdata->core_data[attr->index];
+
+	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
+
+	return sprintf(buf, "%d\n", !!(eax & THERM_STATUS_THRESHOLD1));
+}
+
 static ssize_t show_tjmax(struct device *dev,
 			struct device_attribute *devattr, char *buf)
 {
@@ -155,6 +176,83 @@ static ssize_t show_ttarget(struct device *dev,
 	return sprintf(buf, "%d\n", pdata->core_data[attr->index]->ttarget);
 }
 
+static ssize_t store_ttarget(struct device *dev,
+				struct device_attribute *devattr,
+				const char *buf, size_t count)
+{
+	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct temp_data *tdata = pdata->core_data[attr->index];
+	u32 eax, edx;
+	unsigned long val;
+	int diff;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	/*
+	 * THERM_MASK_THRESHOLD1 is 7 bits wide. Values are entered in terms
+	 * of milli degree celsius. Hence don't accept val > (127 * 1000)
+	 */
+	if (val > tdata->tjmax || val > 127000)
+		return -EINVAL;
+
+	diff = (tdata->tjmax - val) / 1000;
+
+	mutex_lock(&tdata->update_lock);
+	rdmsr_on_cpu(tdata->cpu, tdata->intrpt_reg, &eax, &edx);
+	eax = (eax & ~THERM_MASK_THRESHOLD1) |
+				(diff << THERM_SHIFT_THRESHOLD1);
+	wrmsr_on_cpu(tdata->cpu, tdata->intrpt_reg, eax, edx);
+	tdata->ttarget = val;
+	mutex_unlock(&tdata->update_lock);
+
+	return count;
+}
+
+static ssize_t show_tmin(struct device *dev,
+			struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct platform_data *pdata = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", pdata->core_data[attr->index]->tmin);
+}
+
+static ssize_t store_tmin(struct device *dev,
+				struct device_attribute *devattr,
+				const char *buf, size_t count)
+{
+	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct temp_data *tdata = pdata->core_data[attr->index];
+	u32 eax, edx;
+	unsigned long val;
+	int diff;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	/*
+	 * THERM_MASK_THRESHOLD0 is 7 bits wide. Values are entered in terms
+	 * of milli degree celsius. Hence don't accept val > (127 * 1000)
+	 */
+	if (val > tdata->tjmax || val > 127000)
+		return -EINVAL;
+
+	diff = (tdata->tjmax - val) / 1000;
+
+	mutex_lock(&tdata->update_lock);
+	rdmsr_on_cpu(tdata->cpu, tdata->intrpt_reg, &eax, &edx);
+	eax = (eax & ~THERM_MASK_THRESHOLD0) |
+				(diff << THERM_SHIFT_THRESHOLD0);
+	wrmsr_on_cpu(tdata->cpu, tdata->intrpt_reg, eax, edx);
+	tdata->tmin = val;
+	mutex_unlock(&tdata->update_lock);
+
+	return count;
+}
+
 static ssize_t show_temp(struct device *dev,
 			struct device_attribute *devattr, char *buf)
 {
@@ -361,23 +459,31 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev,
 				int attr_no)
 {
 	int err, i;
-	static ssize_t (*rd_ptr[MAX_ATTRS]) (struct device *dev,
+	static ssize_t (*rd_ptr[TOTAL_ATTRS]) (struct device *dev,
 			struct device_attribute *devattr, char *buf) = {
-			show_label, show_crit_alarm, show_ttarget,
-			show_temp, show_tjmax };
-	static const char *names[MAX_ATTRS] = {
+			show_label, show_crit_alarm, show_temp, show_tjmax,
+			show_max_alarm, show_ttarget, show_tmin };
+	static ssize_t (*rw_ptr[TOTAL_ATTRS]) (struct device *dev,
+			struct device_attribute *devattr, const char *buf,
+			size_t count) = { NULL, NULL, NULL, NULL, NULL,
+					store_ttarget, store_tmin };
+	static const char *names[TOTAL_ATTRS] = {
 					"temp%d_label", "temp%d_crit_alarm",
-					"temp%d_max", "temp%d_input",
-					"temp%d_crit" };
+					"temp%d_input", "temp%d_crit",
+					"temp%d_max_alarm", "temp%d_max",
+					"temp%d_max_hyst" };
 
-	for (i = 0; i < MAX_ATTRS; i++) {
+	for (i = 0; i < tdata->attr_size; i++) {
 		snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH, names[i],
 			attr_no);
 		sysfs_attr_init(&tdata->sd_attrs[i].dev_attr.attr);
 		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
 		tdata->sd_attrs[i].dev_attr.attr.mode = S_IRUGO;
+		if (rw_ptr[i]) {
+			tdata->sd_attrs[i].dev_attr.attr.mode |= S_IWUSR;
+			tdata->sd_attrs[i].dev_attr.store = rw_ptr[i];
+		}
 		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
-		tdata->sd_attrs[i].dev_attr.store = NULL;
 		tdata->sd_attrs[i].index = attr_no;
 		err = device_create_file(dev, &tdata->sd_attrs[i].dev_attr);
 		if (err)
@@ -391,38 +497,6 @@ exit_free:
 	return err;
 }
 
-static void update_ttarget(__u8 cpu_model, struct temp_data *tdata,
-				struct device *dev)
-{
-	int err;
-	u32 eax, edx;
-
-	/*
-	 * Initialize ttarget value. Eventually this will be
-	 * initialized with the value from MSR_IA32_THERM_INTERRUPT
-	 * register. If IA32_TEMPERATURE_TARGET is supported, this
-	 * value will be over written below.
-	 * To Do: Patch to initialize ttarget from MSR_IA32_THERM_INTERRUPT
-	 */
-	tdata->ttarget = tdata->tjmax - 20000;
-
-	/*
-	 * Read the still undocumented IA32_TEMPERATURE_TARGET. It exists
-	 * on older CPUs but not in this register,
-	 * Atoms don't have it either.
-	 */
-	if (cpu_model > 0xe && cpu_model != 0x1c) {
-		err = rdmsr_safe_on_cpu(tdata->cpu,
-				MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
-		if (err) {
-			dev_warn(dev,
-			"Unable to read IA32_TEMPERATURE_TARGET MSR\n");
-		} else {
-			tdata->ttarget = tdata->tjmax -
-					((eax >> 8) & 0xff) * 1000;
-		}
-	}
-}
 
 static int __devinit chk_ucode_version(struct platform_device *pdev)
 {
@@ -481,9 +555,12 @@ static struct temp_data *init_temp_data(unsigned int cpu, int pkg_flag)
 
 	tdata->status_reg = pkg_flag ? MSR_IA32_PACKAGE_THERM_STATUS :
 							MSR_IA32_THERM_STATUS;
+	tdata->intrpt_reg = pkg_flag ? MSR_IA32_PACKAGE_THERM_INTERRUPT :
+						MSR_IA32_THERM_INTERRUPT;
 	tdata->is_pkg_data = pkg_flag;
 	tdata->cpu = cpu;
 	tdata->cpu_core_id = TO_CORE_ID(cpu);
+	tdata->attr_size = MAX_CORE_ATTRS;
 	mutex_init(&tdata->update_lock);
 	return tdata;
 }
@@ -533,7 +610,17 @@ static int create_core_data(struct platform_data *pdata,
 	else
 		tdata->tjmax = get_tjmax(c, cpu, &pdev->dev);
 
-	update_ttarget(c->x86_model, tdata, &pdev->dev);
+	/*
+	 * Test if we can access the intrpt register. If so, increase the
+	 * 'size' enough to have ttarget/tmin/max_alarm interfaces.
+	 * Initialize ttarget with bits 16:22 of MSR_IA32_THERM_INTERRUPT
+	 */
+	err = rdmsr_safe_on_cpu(cpu, tdata->intrpt_reg, &eax, &edx);
+	if (!err) {
+		tdata->attr_size += MAX_THRESH_ATTRS;
+		tdata->ttarget = tdata->tjmax - ((eax >> 16) & 0x7f) * 1000;
+	}
+
 	pdata->core_data[attr_no] = tdata;
 
 	/* Create sysfs interfaces */
@@ -570,7 +657,7 @@ static void coretemp_remove_core(struct platform_data *pdata,
 	struct temp_data *tdata = pdata->core_data[indx];
 
 	/* Remove the sysfs attributes */
-	for (i = 0; i < MAX_ATTRS; i++)
+	for (i = 0; i < tdata->attr_size; i++)
 		device_remove_file(dev, &tdata->sd_attrs[i].dev_attr);
 
 	kfree(pdata->core_data[indx]);
-- 
1.6.1


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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Tue, 2011-07-12 at 07:07 -0400, Durgadoss R wrote: 
> This patch adds the core and pkg support to coretemp.
> These thresholds can be configured via the sysfs interfaces tempX_max
> and tempX_max_hyst. An interrupt is generated when CPU temperature reaches
> or crosses above tempX_max OR drops below tempX_max_hyst.
> 
> This patch is based on the documentation in IA Manual vol 3A, that can be
> downloaded from here:
> http://download.intel.com/design/processor/manuals/253668.pdf
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

Looks like it is working now, so I applied the patch to -next.

Thanks,
Guenter





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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

Hi Guenter, Durgadoss,

Sorry for the very late comment.

On Wed, 13 Jul 2011 13:48:11 -0700, Guenter Roeck wrote:
> On Tue, 2011-07-12 at 07:07 -0400, Durgadoss R wrote: 
> > This patch adds the core and pkg support to coretemp.
> > These thresholds can be configured via the sysfs interfaces tempX_max
> > and tempX_max_hyst. An interrupt is generated when CPU temperature reaches
> > or crosses above tempX_max OR drops below tempX_max_hyst.
> > 
> > This patch is based on the documentation in IA Manual vol 3A, that can be
> > downloaded from here:
> > http://download.intel.com/design/processor/manuals/253668.pdf
> > 
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
> Looks like it is working now, so I applied the patch to -next.

Working, really? That's not how it looks like from here.

On my Xeon E5520, the output looked like this before the patch:

coretemp-isa-0000
Core 0:       +61.0°C  (high = +87.0°C, crit = +97.0°C)
Core 1:       +56.0°C  (high = +87.0°C, crit = +97.0°C)
Core 2:       +57.0°C  (high = +87.0°C, crit = +97.0°C)
Core 3:       +56.0°C  (high = +87.0°C, crit = +97.0°C)

And now it looks like this :

coretemp-isa-0000
Core 0:       +61.0°C  (high = +97.0°C, hyst =  +0.0°C)
                       (crit = +97.0°C)
Core 1:       +57.0°C  (high = +97.0°C, hyst =  +0.0°C)
                       (crit = +97.0°C)
Core 2:       +60.0°C  (high = +97.0°C, hyst =  +0.0°C)
                       (crit = +97.0°C)
Core 3:       +56.0°C  (high = +97.0°C, hyst =  +0.0°C)
                       (crit = +97.0°C)

High = crit doesn't make much sense, and neither does hyst = 0.

Looking at the code, it seems that the hyst value (tmin) is simply
never read from the CPU. I wrote a fix, I'll send it shortly. After
properly initializing tmin, I get the following:

coretemp-isa-0000
Core 0:       +61.0°C  (high = +97.0°C, hyst = +97.0°C)
                       (crit = +97.0°C)
Core 1:       +57.0°C  (high = +97.0°C, hyst = +97.0°C)
                       (crit = +97.0°C)
Core 2:       +59.0°C  (high = +97.0°C, hyst = +97.0°C)
                       (crit = +97.0°C)
Core 3:       +56.0°C  (high = +97.0°C, hyst = +97.0°C)
                       (crit = +97.0°C)

I am still suspicious... Does this mean that the thermal interrupt
mechanism is disabled by default?

What happened to the old high limit value (87°C)? I don't know what
meaning it had physically but at least the value was reasonable.

How comes that the driver doesn't use THERM_INT_THRESHOLD0_ENABLE and
THERM_INT_THRESHOLD1_ENABLE? If the thresholds can be disabled, then
the driver should certainly handle these cases.

On my Core Duo T2600n the output looks like:

coretemp-isa-0000
Core 0:      +54.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
                      (crit = +100.0°C)
Core 1:      +55.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
                      (crit = +100.0°C)

High at 47°C by default seems unreasonable, and hyst > high even more
so. But at least the ALARM flags are consistent with these limits.

Lastly, on Core2 Duo E6400, I get this :

coretemp-isa-0000
Core 0:       +71.0°C  (high = +100.0°C, hyst = +100.0°C)
                       (crit = +100.0°C)
Core 1:       +69.0°C  (high = +100.0°C, hyst = +100.0°C)
                       (crit = +100.0°C)

So problem is the same as on my Xeon E5520: it looks like the
thresholds are disabled?

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Mon, 2011-09-12 at 12:18 -0400, Jean Delvare wrote:
> Hi Guenter, Durgadoss,
> 
> Sorry for the very late comment.
> 
> On Wed, 13 Jul 2011 13:48:11 -0700, Guenter Roeck wrote:
> > On Tue, 2011-07-12 at 07:07 -0400, Durgadoss R wrote: 
> > > This patch adds the core and pkg support to coretemp.
> > > These thresholds can be configured via the sysfs interfaces tempX_max
> > > and tempX_max_hyst. An interrupt is generated when CPU temperature reaches
> > > or crosses above tempX_max OR drops below tempX_max_hyst.
> > > 
> > > This patch is based on the documentation in IA Manual vol 3A, that can be
> > > downloaded from here:
> > > http://download.intel.com/design/processor/manuals/253668.pdf
> > > 
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > 
> > Looks like it is working now, so I applied the patch to -next.
> 
> Working, really? That's not how it looks like from here.
> 
Frankly, I don't remember how I tested it - though I remember that I
did. I re-tested it on a Xeon C5528 and got the same results :(. Maybe I
was mentally unstable at the time or something. Sorry for that - should
not happen.

Question is what to do - back it out ? I don't have time to look into it
more right now.

Guenter



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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Mon, 12 Sep 2011 10:13:29 -0700, Guenter Roeck wrote:
> On Mon, 2011-09-12 at 12:18 -0400, Jean Delvare wrote:
> > Hi Guenter, Durgadoss,
> > 
> > Sorry for the very late comment.
> > 
> > On Wed, 13 Jul 2011 13:48:11 -0700, Guenter Roeck wrote:
> > > Looks like it is working now, so I applied the patch to -next.
> > 
> > Working, really? That's not how it looks like from here.
> > 
> Frankly, I don't remember how I tested it - though I remember that I
> did. I re-tested it on a Xeon C5528 and got the same results :(. Maybe I
> was mentally unstable at the time or something. Sorry for that - should
> not happen.
> 
> Question is what to do - back it out ? I don't have time to look into it
> more right now.

I'll send a patch for the most obvious bug. For the rest, I'd like to
hear Durgadoss before we decide what do to. Certainly he had a good
reason for posting the patch in the first place, let's avoid a bold
revert if possible.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[R, Durgadoss]

Hi Jean,

[snip] 
> I am still suspicious... Does this mean that the thermal interrupt
> mechanism is disabled by default?
[snip]

As you rightly pointed out, the Interrupts for the thresholds are disabled,
by default. We have to enable them if we decide to do so.
If we enable them, we will get interrupts when the input temperature
Crosses temp1_max and temp1_max_hyst in either directions. 

The temp1_max is supposed to be more than temp1_input and
temp1_max_hyst is supposed to be lesser than temp1_input.
By default the temp1_max_hyst is left at 0.

I wanted to use the ABI names temp1_thresh1 and temp1_thresh0.
But Guenter pointed out those are new ABI names, which nobody
is aware of. So, I reused the existing ABI.

The basic idea is this:
Consider, the input temperature is 40. 
We configure the max to 45 and max_hyst to 35.
Case 1:
------
After a while, the temperature reaches 45. We get an interrupt,
and throttle the factors that cause an increase in temperature.
(For example, reduce CPU frequency)
Case 2:
------
After a while, the temperature reaches 35. We get an interrupt,
and release the throttle (if we had any) or increase the
performance.

Our coretemp driver, on getting these interrupts, will
notify the user space (possibly through Uevent or something)
Then the user space will take appropriate actions.

This is what I had in mind, when I submitted this patch.
I am willing to add code to coretemp that will do this.

Kindly let me know your thoughts on this.

Thanks,
Durga

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Tue, Sep 13, 2011 at 05:22:40AM -0400, R, Durgadoss wrote:
> Hi Jean,
> 
> [snip] 
> > I am still suspicious... Does this mean that the thermal interrupt
> > mechanism is disabled by default?
> [snip]
> 
> As you rightly pointed out, the Interrupts for the thresholds are disabled,
> by default. We have to enable them if we decide to do so.
> If we enable them, we will get interrupts when the input temperature
> Crosses temp1_max and temp1_max_hyst in either directions. 
> 
> The temp1_max is supposed to be more than temp1_input and
> temp1_max_hyst is supposed to be lesser than temp1_input.
> By default the temp1_max_hyst is left at 0.
> 
Should be set to something useful. Also, it does not have to be less
than temp1_input, as long as it is less than temp1_max (which was my
understanding). Worst case you would get another interrupt (when the
temperature crosses temp1_max_hyst from below) which you would ignore.

Interrupts should be enabled, and you can send uevents and sysfs notifications
whenever a threshold is crossed. All you would have to do is to keep the old alarm
state and send events whenever it changes.

Example:
	temp@, max_thresholdP, max`

1st interrupt when temp reaches 50 -> ignore
2nd interrupt when temp reaches 60 -> set alarm, send uevent and sysfs notifications
3rd interrupt when temp reaches 60 (from above) -> ignore
4th interrupt when temp reaches 50 (from above) -> reset alarm, send uevent and sysfs notifications 

Thanks,
Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[R, Durgadoss]

Hi Guenter,

> > The temp1_max is supposed to be more than temp1_input and
> > temp1_max_hyst is supposed to be lesser than temp1_input.
> > By default the temp1_max_hyst is left at 0.
> >
> Should be set to something useful. Also, it does not have to be less
> than temp1_input, as long as it is less than temp1_max (which was my
> understanding). Worst case you would get another interrupt (when the
> temperature crosses temp1_max_hyst from below) which you would ignore.

Yes. I agree with you.
I have no clue on what exact value to set to max and max_hyst initially.
Can we set temp1_max to 70 and temp1_max_hyst to 50 ?

> Interrupts should be enabled, and you can send uevents and sysfs notifications
> whenever a threshold is crossed. All you would have to do is to keep the old
> alarm
> state and send events whenever it changes.
> 
> Example:
> 	temp@, max_thresholdP, max`
> 
> 1st interrupt when temp reaches 50 -> ignore
> 2nd interrupt when temp reaches 60 -> set alarm, send uevent and sysfs
> notifications
> 3rd interrupt when temp reaches 60 (from above) -> ignore
> 4th interrupt when temp reaches 50 (from above) -> reset alarm, send uevent and
> sysfs notifications
> 

Thank you for the example.
The example is very clear. I will submit a patch to add this functionality
to coretemp.

Thanks,
Durga

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Tue, Sep 13, 2011 at 09:28:26AM -0400, R, Durgadoss wrote:
> Hi Guenter,
> 
> > > The temp1_max is supposed to be more than temp1_input and
> > > temp1_max_hyst is supposed to be lesser than temp1_input.
> > > By default the temp1_max_hyst is left at 0.
> > >
> > Should be set to something useful. Also, it does not have to be less
> > than temp1_input, as long as it is less than temp1_max (which was my
> > understanding). Worst case you would get another interrupt (when the
> > temperature crosses temp1_max_hyst from below) which you would ignore.
> 
> Yes. I agree with you.
> I have no clue on what exact value to set to max and max_hyst initially.
> Can we set temp1_max to 70 and temp1_max_hyst to 50 ?
> 
Some constant below Tjmax for each ? Old value for max was 20 C below Tjmax,
as far as I recall. Maybe another 10 C less for max_hyst.
That would make it (64, 74, 94) for my Xeon which seems to make sense.

Jean, any suggestion ?

Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Tue, 13 Sep 2011 18:58:26 +0530, R, Durgadoss wrote:
> Hi Guenter,
> 
> > > The temp1_max is supposed to be more than temp1_input and
> > > temp1_max_hyst is supposed to be lesser than temp1_input.
> > > By default the temp1_max_hyst is left at 0.
> > >
> > Should be set to something useful. Also, it does not have to be less
> > than temp1_input, as long as it is less than temp1_max (which was my
> > understanding). Worst case you would get another interrupt (when the
> > temperature crosses temp1_max_hyst from below) which you would ignore.
> 
> Yes. I agree with you.
> I have no clue on what exact value to set to max and max_hyst initially.
> Can we set temp1_max to 70 and temp1_max_hyst to 50 ?

If anything, then the answer depends on the CPU, as it depends on
TjMax. But more likely, no, we don't want to set these values, as we do
NOT initialize limits in hwmon drivers [1]. The whole point of the
hwmon sysfs interface is to let the user set the limits they want in
sensors.conf. Ideally each CPU would have sane defaults, and failing
that, the BIOS should set appropriate values.

> > Interrupts should be enabled, and you can send uevents and sysfs notifications
> > whenever a threshold is crossed. All you would have to do is to keep the old
> > alarm
> > state and send events whenever it changes.

But who should enable interrupts? And who will receive them? It really
sounds like a task for thermal management, NOT hardware monitoring.
Seems the boundary is getting blurrier every day...

I would like to avoid the case where merely loading the coretemp driver
results in CPU throttling or worse simply because the threshold
settings are wrong.

> > 
> > Example:
> > 	temp@, max_thresholdP, max`
> > 
> > 1st interrupt when temp reaches 50 -> ignore
> > 2nd interrupt when temp reaches 60 -> set alarm, send uevent and sysfs
> > notifications
> > 3rd interrupt when temp reaches 60 (from above) -> ignore
> > 4th interrupt when temp reaches 50 (from above) -> reset alarm, send uevent and
> > sysfs notifications
> 
> Thank you for the example.
> The example is very clear. I will submit a patch to add this functionality
> to coretemp.

Please do, it can only help us move forward. However I suspect that
more discussions and experiments will be needed to come up with the
right solution.

[1] One exception being a few chips which have high limits set to 0 as the
hardware default; in this case we change it to the maximum possible
value at driver initialization time.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Tue, 13 Sep 2011 06:45:37 -0700, Guenter Roeck wrote:
> On Tue, Sep 13, 2011 at 09:28:26AM -0400, R, Durgadoss wrote:
> > Hi Guenter,
> > 
> > > > The temp1_max is supposed to be more than temp1_input and
> > > > temp1_max_hyst is supposed to be lesser than temp1_input.
> > > > By default the temp1_max_hyst is left at 0.

FWIW my experiments contradict this. Since I fixed the bug in the
coretemp driver that was causing tempX_max_hyst to never be read, I've
seen no machine where temp1_max_hyst was 0.

> > > Should be set to something useful. Also, it does not have to be less
> > > than temp1_input, as long as it is less than temp1_max (which was my
> > > understanding). Worst case you would get another interrupt (when the
> > > temperature crosses temp1_max_hyst from below) which you would ignore.
> > 
> > Yes. I agree with you.
> > I have no clue on what exact value to set to max and max_hyst initially.
> > Can we set temp1_max to 70 and temp1_max_hyst to 50 ?
> > 
> Some constant below Tjmax for each ? Old value for max was 20 C below Tjmax,
> as far as I recall. Maybe another 10 C less for max_hyst.
> That would make it (64, 74, 94) for my Xeon which seems to make sense.

I never liked this arbitrary -20°C thing. It never made any sense
anyway, as it was only a software number returned by the driver with no
connection to the physical reality as I understand it. I see no reason
to reuse this number.

OTOH I have no idea what the old temp1_max value (up to kernel 2.6.39)
was supposed to represent. This bits it was read from (bit 14-8 of
MSR_IA32_TEMPERATURE_TARGET) are marked as reserved in the SDM, i.e.
the value is undocumented. It was added by Rudolf (Cc'd) in January
2008, and in the commit message he wrote: "temperature at which all
fans should spin full speed". I have no idea how real this is, nor
whether this was only indicative for software, or if something would
really happen physically if that limit is reached (difficult in the
general case, as the CPU has clue how the fans are controlled.)

Rudolf, was temp1_max doing anything for you? Do you remember?

Durgadoss, your change dropped the use of these bits, was it on
purpose? If the register exists and is useful then Intel please
document it.

> Jean, any suggestion ?

Many suggestions, although neither authoritative nor complete.

I would suggest that we leave the threshold values alone, at least as
long as they don't seem obviously wrong (e.g. hyst > max).

If we change the threshold values, it should be clearly notified in the
kernel log.

The key question is probably whether we want to enable the interrupts
or not, and if we do, where. IMHO the same piece of code enabling the
interrupts should be handling them when they fire.

If we do not enable interrupts ourselves then I see no rationale for
setting the thresholds: this should either be done earlier, when the
interrupts are enabled (BIOS or thermal management code) or later
(user-space, through sensors.conf.)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

Hi Jean,

On Tue, 2011-09-13 at 10:13 -0400, Jean Delvare wrote:
> On Tue, 13 Sep 2011 18:58:26 +0530, R, Durgadoss wrote:
> > Hi Guenter,
> > 
> > > > The temp1_max is supposed to be more than temp1_input and
> > > > temp1_max_hyst is supposed to be lesser than temp1_input.
> > > > By default the temp1_max_hyst is left at 0.
> > > >
> > > Should be set to something useful. Also, it does not have to be less
> > > than temp1_input, as long as it is less than temp1_max (which was my
> > > understanding). Worst case you would get another interrupt (when the
> > > temperature crosses temp1_max_hyst from below) which you would ignore.
> > 
> > Yes. I agree with you.
> > I have no clue on what exact value to set to max and max_hyst initially.
> > Can we set temp1_max to 70 and temp1_max_hyst to 50 ?
> 
> If anything, then the answer depends on the CPU, as it depends on
> TjMax. But more likely, no, we don't want to set these values, as we do
> NOT initialize limits in hwmon drivers [1]. The whole point of the
> hwmon sysfs interface is to let the user set the limits they want in
> sensors.conf. Ideally each CPU would have sane defaults, and failing
> that, the BIOS should set appropriate values.
> 
Looking at the code again, we did have ttarget for max, didn't we ? Any
objections against using it ?

As for max_hyst, looks like we'll have the option to leave it at 0, or
to set it to something. You had a problem with leaving it at 0, so I
guess it will be up to you to suggest an alternative. Setting it to the
same value as max might be an alternative (and maybe keep interrupts
disabled in that case).

> > > Interrupts should be enabled, and you can send uevents and sysfs notifications
> > > whenever a threshold is crossed. All you would have to do is to keep the old
> > > alarm
> > > state and send events whenever it changes.
> 
> But who should enable interrupts? And who will receive them? It really
> sounds like a task for thermal management, NOT hardware monitoring.
> Seems the boundary is getting blurrier every day...
> 
> I would like to avoid the case where merely loading the coretemp driver
> results in CPU throttling or worse simply because the threshold
> settings are wrong.
> 
That is not how it works. The hwmon driver doesn't actually do anything.
Best current example is gpio-fan.c: If thresholds are crossed,
sysfs_notify is called for the affected alarm attributes, and a uevent
is generated for the hwmon kobject. The new EXYNOS4 driver has a similar
mechanism, though I had them take out sysfs notifications since those
only covered 0->1 alarm transitions and we don't have guidelines for how
to handle that (see below).

I don't see a problem with the notifications - it pretty much lets
userland decide what if anything to do, and does not handle anything in
the kernel. It enables the use of poll() on sysfs alarm attributes which
I think is useful and desirable, and it generates a uevent to enable
script handling, which I also think makes sense.

But on the other side I _did_ ask a couple of times on the list (I am
sure I copied you) if we should have guidelines for the use of
sysfs_notify() and kobject_uevent() in hwmon drivers. Unfortunately, I
didn't get any replies.

As for getting and handling interrupts, there another example for that.
The lm90 driver already implements alert handling. Today it only
generates a log message, but one could imagine generating a uevent
and/or sysfs notifications. Problem with sysfs notifications, though, is
that smbus alerts are only generated for 0->1 transitions, not for 1->0
transitions. This again leads to the question if and how to use
sysfs_notify() in such cases.

Guenter



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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Mon, 12 Sep 2011 18:18:00 +0200, Jean Delvare wrote:
> On my Core Duo T2600, the output looks like:
> 
> coretemp-isa-0000
> Core 0:      +54.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
>                       (crit = +100.0°C)
> Core 1:      +55.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
>                       (crit = +100.0°C)
> 
> High at 47°C by default seems unreasonable, and hyst > high even more
> so. But at least the ALARM flags are consistent with these limits.

BTW, the high and hyst values on this CPU appear to be random. Today
they are 89°C and 96°C. Very odd.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Fri, 2011-09-16 at 13:00 -0400, Jean Delvare wrote:
> On Mon, 12 Sep 2011 18:18:00 +0200, Jean Delvare wrote:
> > On my Core Duo T2600, the output looks like:
> > 
> > coretemp-isa-0000
> > Core 0:      +54.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
> >                       (crit = +100.0°C)
> > Core 1:      +55.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
> >                       (crit = +100.0°C)
> > 
> > High at 47°C by default seems unreasonable, and hyst > high even more
> > so. But at least the ALARM flags are consistent with these limits.
> 
> BTW, the high and hyst values on this CPU appear to be random. Today
> they are 89°C and 96°C. Very odd.
> 
Not initialized, maybe ? Still odd, though.

Guenter



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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Fri, 16 Sep 2011 10:48:55 -0700, Guenter Roeck wrote:
> On Fri, 2011-09-16 at 13:00 -0400, Jean Delvare wrote:
> > On Mon, 12 Sep 2011 18:18:00 +0200, Jean Delvare wrote:
> > > On my Core Duo T2600, the output looks like:
> > > 
> > > coretemp-isa-0000
> > > Core 0:      +54.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
> > >                       (crit = +100.0°C)
> > > Core 1:      +55.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
> > >                       (crit = +100.0°C)
> > > 
> > > High at 47°C by default seems unreasonable, and hyst > high even more
> > > so. But at least the ALARM flags are consistent with these limits.
> > 
> > BTW, the high and hyst values on this CPU appear to be random. Today
> > they are 89°C and 96°C. Very odd.
> > 
> Not initialized, maybe ? Still odd, though.

Worse than this. Reloading the driver changes the values.

I think I finally understood what is going on. The threshold values are
adjusted dynamically based on the measured temperature. This is on
kernel 2.6.32 where the kernel has no clue about these thresholds, so
the only possibility I can think of is that the BIOS is doing it. And
"hyst" is consistently higher than "high", which means that the BIOS has
decided on an opposite convention to what the coretemp driver is doing.

So our driver makes many assumptions which aren't verified in my case:

* The driver shouldn't assume that the threshold values are under his
  sole control. Reading the values once at initialization time and
  never again after that is not correct.

* The driver assumes that threshold0 is higher than threshold1. Looking
  at the SDM, there is no such asymmetry, both thresholds are
  equivalent. So my laptop's BIOS is in its own right when deciding that
  threshold1 is high and threshold0 is low. Given that 0 < 1, their
  decision makes even more sense than ours. It's an IBM/Lenovo Thinkpad
  T60p, a pretty popular series, so we can't just ignore this problem.
  A lot of users will be affected.

* The driver artificially binds the two thresholds by making one the
  _hyst of the other. I see no such relation in the datasheet though,
  both thresholds appear to be completely independent. I know that this
  wasn't Durgadoss' original implementation and we had him change to
  that, but retrospectively this seems to have been a mistake.

I presume that my BIOS leverages the interrupts associated with the
thresholds to do dynamic thermal management, either by fan speed control
or by CPU throttling, or anything else, or a mix of all these.

Durgadoss, please speak up if anything I wrote above isn't correct.

This brings up a question I asked before but never got an answer to,
and it seems I can't find the answer in the SDM either: where are the
interrupts going? Are these by any chance SMIs which the kernel has no
way to deal with?

The first 2 wrong assumptions listed above can easily get fixed. First
one is fixed by always reading the values from the MSR instead of the
cache. Second one is fixed by testing the threshold values at
initialization time to determine which direction the BIOS went with
(might be racy though.)

The last assumption however seems very difficult to fix. It would be
valid to use one of the thresholds as a real low limit (e.g. to enable
a heating system if the system is about to freeze, or more
realistically, to enable turbo mode on low temperatures). In a way
that's what my laptop's BIOS is doing, although the threshold value and
presumably its effect change dynamically.

The fact that each threshold can be used for anything makes it very
difficult to make them fit in our standard hwmon interface. On one
machine the BIOS may expect the temperature to be below both thresholds
when the system is idle, while on others it will expect that the
current temperature is between the thresholds (as is the case on my
laptop.) This means that there is no unique semantics attached to these
thresholds, while our standard interface wants semantics attached
always.

I admit I am not sure how to deal with all this. Suggestions are
welcome. What I'm sure of is that we don't want to let the coretemp
driver in the state it currently is... We will get a flood of user
complaints or at least questions if we do.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Fri, 2011-09-16 at 15:21 -0400, Jean Delvare wrote:
> On Fri, 16 Sep 2011 10:48:55 -0700, Guenter Roeck wrote:
> > On Fri, 2011-09-16 at 13:00 -0400, Jean Delvare wrote:
> > > On Mon, 12 Sep 2011 18:18:00 +0200, Jean Delvare wrote:
> > > > On my Core Duo T2600, the output looks like:
> > > > 
> > > > coretemp-isa-0000
> > > > Core 0:      +54.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
> > > >                       (crit = +100.0°C)
> > > > Core 1:      +55.0°C  (high = +47.0°C, hyst = +64.0°C)  ALARM
> > > >                       (crit = +100.0°C)
> > > > 
> > > > High at 47°C by default seems unreasonable, and hyst > high even more
> > > > so. But at least the ALARM flags are consistent with these limits.
> > > 
> > > BTW, the high and hyst values on this CPU appear to be random. Today
> > > they are 89°C and 96°C. Very odd.
> > > 
> > Not initialized, maybe ? Still odd, though.
> 
> Worse than this. Reloading the driver changes the values.
> 
> I think I finally understood what is going on. The threshold values are
> adjusted dynamically based on the measured temperature. This is on
> kernel 2.6.32 where the kernel has no clue about these thresholds, so
> the only possibility I can think of is that the BIOS is doing it. And
> "hyst" is consistently higher than "high", which means that the BIOS has
> decided on an opposite convention to what the coretemp driver is doing.
> 
> So our driver makes many assumptions which aren't verified in my case:
> 
> * The driver shouldn't assume that the threshold values are under his
>   sole control. Reading the values once at initialization time and
>   never again after that is not correct.
> 
> * The driver assumes that threshold0 is higher than threshold1. Looking
>   at the SDM, there is no such asymmetry, both thresholds are
>   equivalent. So my laptop's BIOS is in its own right when deciding that
>   threshold1 is high and threshold0 is low. Given that 0 < 1, their
>   decision makes even more sense than ours. It's an IBM/Lenovo Thinkpad
>   T60p, a pretty popular series, so we can't just ignore this problem.
>   A lot of users will be affected.
> 
> * The driver artificially binds the two thresholds by making one the
>   _hyst of the other. I see no such relation in the datasheet though,
>   both thresholds appear to be completely independent. I know that this
>   wasn't Durgadoss' original implementation and we had him change to
>   that, but retrospectively this seems to have been a mistake.
> 
Do you have a better idea ? In the context, the lower threshold was
supposed to be used to turn off thermal throttling, which in my
understanding matches the semantics of _hyst - ie it is the value at
which an alarm is turned off.

> I presume that my BIOS leverages the interrupts associated with the
> thresholds to do dynamic thermal management, either by fan speed control
> or by CPU throttling, or anything else, or a mix of all these.
> 
> Durgadoss, please speak up if anything I wrote above isn't correct.
> 
> This brings up a question I asked before but never got an answer to,
> and it seems I can't find the answer in the SDM either: where are the
> interrupts going? Are these by any chance SMIs which the kernel has no
> way to deal with?
> 
Looks like the BIOS would get those interrupts, at least on your
system ? Some entity must change the register contents, after all.

> The first 2 wrong assumptions listed above can easily get fixed. First
> one is fixed by always reading the values from the MSR instead of the
> cache. Second one is fixed by testing the threshold values at
> initialization time to determine which direction the BIOS went with
> (might be racy though.)
> 
> The last assumption however seems very difficult to fix. It would be
> valid to use one of the thresholds as a real low limit (e.g. to enable
> a heating system if the system is about to freeze, or more
> realistically, to enable turbo mode on low temperatures). In a way
> that's what my laptop's BIOS is doing, although the threshold value and
> presumably its effect change dynamically.
> 
> The fact that each threshold can be used for anything makes it very
> difficult to make them fit in our standard hwmon interface. On one
> machine the BIOS may expect the temperature to be below both thresholds
> when the system is idle, while on others it will expect that the
> current temperature is between the thresholds (as is the case on my
> laptop.) This means that there is no unique semantics attached to these
> thresholds, while our standard interface wants semantics attached
> always.
> 
> I admit I am not sure how to deal with all this. Suggestions are
> welcome. What I'm sure of is that we don't want to let the coretemp
> driver in the state it currently is... We will get a flood of user
> complaints or at least questions if we do.
> 
Me not either ... I did not expect that a "third party" would access and
use the values. Sounds almost as bad as the ACPI interaction with some
of the drivers.

Given that we don't know how the BIOS (or some other entity) uses the
limits, one possible solution that comes to my mind would be to extend
the ABI to specifically permit non-specific threshold attributes (such
as tempX_thresholdY). Not sure if that is a good idea, though.

Guenter



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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[R, Durgadoss]

Hi Jean,

Some clarifications from my side.
I am testing the coretemp driver code on Linux-3.0-rc6
Kernel on a Core i5 machine running Fedora 12.
All my observations are w.r.t this configuration.

> 
> Worse than this. Reloading the driver changes the values.
> 
> I think I finally understood what is going on. The threshold values are
> adjusted dynamically based on the measured temperature. This is on
> kernel 2.6.32 where the kernel has no clue about these thresholds, so
> the only possibility I can think of is that the BIOS is doing it. And
> "hyst" is consistently higher than "high", which means that the BIOS has
> decided on an opposite convention to what the coretemp driver is doing.
> 

I do not think the BIOS is changing the threshold values (temp1_max
and temp1_max_hyst) in my case. They are both 0 when I insmod the 
driver, and remain 0 until I 'echo' some values into it.
As you rightly pointed out below, the SDM does not restrict the
values for _max or max_hyst. I can very well make max_hyst higher than _max.

> So our driver makes many assumptions which aren't verified in my case:
> 
> * The driver shouldn't assume that the threshold values are under his
>   sole control. Reading the values once at initialization time and
>   never again after that is not correct.
> 

In My BIOS it is not changing anything. So, unless the driver changes it
the values remain the same.

That said, I do not know the behaviour on a different kernel version,
(especially older kernels) with a different BIOS. I do not see any
mention of 'BIOS configuring (or can configure) these thresholds' in
the SDM. For a double check, I am referring to IA Manual Vol-3A.Chapter-14:
Download Link: 
http://download.intel.com/design/processor/manuals/253668.pdf

In Section 14.5 it says:
"On-die digital thermal sensor and interrupt mechanisms permit the OS to
manage thermal conditions natively without relying on BIOS or other system
board components."
This is also one more reason why I believe BIOS is not changing
these thresholds. But it might be wrong too.

In older kernels (before Linux 3.0), the temp1_max sysfs interface
reads the value from IA32_TEMPERATURE_TARGET register.
But in recent machines, I don't think this register is supported.
This is the reason, I wanted the sysfs interface names to be
tempX_threshold[0/1] (or something but not interfere with temp1_max).
Due to ABI considerations, we had to make it 'temp1_max'.
May be we should leave temp1_max as such, and add temp1_threshold[0/1] ?

So, Jean, if your kernel's temp1_max interface is being read from
IA32_TEMPERATURE_TARGET, I can imagine the change in values you
reported.

Could you please tell us, whether the change in values are seen
in a newer kernel(Linux 3.0+) also ? Hope you find some time..

Moreover, I do not see a mention of IA32_TEMPERATURE_TARGET in the SDM
I am referring to. May be all of us don't see :-(

> * The driver assumes that threshold0 is higher than threshold1. Looking
>   at the SDM, there is no such asymmetry, both thresholds are
>   equivalent. So my laptop's BIOS is in its own right when deciding that
>   threshold1 is high and threshold0 is low. Given that 0 < 1, their
>   decision makes even more sense than ours. It's an IBM/Lenovo Thinkpad
>   T60p, a pretty popular series, so we can't just ignore this problem.
>   A lot of users will be affected.
> 

I feel the driver thinks the other way only. The store_ttarget function
corresponding to temp1_max interface, reads values for Threshold1
and store_tmin corresponding to temp1_max_hyst interface reads values
for Threshold0. Kindly correct me if I am wrong here.

> * The driver artificially binds the two thresholds by making one the
>   _hyst of the other. I see no such relation in the datasheet though,

I completely agree with you that the thresholds are independent.
So, we might have to remove the _hyst from the interface name.
More than that, changing the interface names would be clean I believe.

>   both thresholds appear to be completely independent. I know that this
>   wasn't Durgadoss' original implementation and we had him change to
>   that, but retrospectively this seems to have been a mistake.
> 

I also feel the same. But I am all for fixing this.

> I presume that my BIOS leverages the interrupts associated with the
> thresholds to do dynamic thermal management, either by fan speed control
> or by CPU throttling, or anything else, or a mix of all these.
> 
> Durgadoss, please speak up if anything I wrote above isn't correct.
> 
> This brings up a question I asked before but never got an answer to,
> and it seems I can't find the answer in the SDM either: where are the
> interrupts going? Are these by any chance SMIs which the kernel has no
> way to deal with?
> 

Alright, here comes the million dollar question:
where are the interrupts going?

Honestly I do not have a complete answer. I am writing
Whatever I know, from the kernel code base.

If we look at the file therm_throt.c, inside arch/x86/kernel/
cpu/mcheck/ I think we will get some idea.

There is some code which registers for thermal related interrupts

asmlinkage void smp_thermal_interrupt(struct pt_regs *regs)
{
        exit_idle();
        irq_enter();
        inc_irq_stat(irq_thermal_count);
        smp_thermal_vector();
        irq_exit();
        /* Ack only at the end to avoid potential reentry */
        ack_APIC_irq();
}

When our threshold interrupt occurs, the control comes here.
(We should enable the interrupt for this..)
And as of now, there is no code inside therm_throt that can
handle our threshold interrupts. For the past two days, I had
been working on a patch, to add this functionality, to therm_throt.

But the patch has to go to linux-x86_64 mailing list. I will copy
Jean & Guenter, while submitting this patch.

This is what I am planning to try out:
1. Catch the threshold interrupts in therm_throt
2. Notify coretemp from therm_throt
   (There is a function pointer exported for this)
3. Send UEvent from coretemp to the user space
4. Let the user space deal with the UEvent
   (i.e let it take appropriate actions)

> I admit I am not sure how to deal with all this. Suggestions are
> welcome. What I'm sure of is that we don't want to let the coretemp
> driver in the state it currently is... We will get a flood of user
> complaints or at least questions if we do.

I agree. I plan to do the 4 steps that I mentioned above.
Kindly correct/suggest alternatives.

And Sorry Jean, I think we are in different time zones.
That's why could not respond to this mail early.
I woke up this morning reading this email..and replying now.

Thanks,
Durga

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

Hi Durgadoss,

On Sat, Sep 17, 2011 at 01:40:25AM -0400, R, Durgadoss wrote:
> Hi Jean,
> 
> Some clarifications from my side.
> I am testing the coretemp driver code on Linux-3.0-rc6
> Kernel on a Core i5 machine running Fedora 12.
> All my observations are w.r.t this configuration.
> 
[ ... ]

> Alright, here comes the million dollar question:
> where are the interrupts going?
> 
> Honestly I do not have a complete answer. I am writing
> Whatever I know, from the kernel code base.
> 
> If we look at the file therm_throt.c, inside arch/x86/kernel/
> cpu/mcheck/ I think we will get some idea.
> 
> There is some code which registers for thermal related interrupts
> 
> asmlinkage void smp_thermal_interrupt(struct pt_regs *regs)
> {
>         exit_idle();
>         irq_enter();
>         inc_irq_stat(irq_thermal_count);
>         smp_thermal_vector();
>         irq_exit();
>         /* Ack only at the end to avoid potential reentry */
>         ack_APIC_irq();
> }
> 
> When our threshold interrupt occurs, the control comes here.
> (We should enable the interrupt for this..)
> And as of now, there is no code inside therm_throt that can
> handle our threshold interrupts. For the past two days, I had
> been working on a patch, to add this functionality, to therm_throt.
> 
> But the patch has to go to linux-x86_64 mailing list. I will copy
> Jean & Guenter, while submitting this patch.
> 
Maybe I misunderstand something, but I don't think that will work.
We can not have one module set parameters for interrupts to be handled
by another. If the thermal throttling module is going to use
the interrupts, the thresholds should be set there, not in the
hwmon driver.

Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

Hi Guenter,

Late reply...

On Tue, 13 Sep 2011 08:20:06 -0700, Guenter Roeck wrote:
> On Tue, 2011-09-13 at 10:13 -0400, Jean Delvare wrote:
> > If anything, then the answer depends on the CPU, as it depends on
> > TjMax. But more likely, no, we don't want to set these values, as we do
> > NOT initialize limits in hwmon drivers [1]. The whole point of the
> > hwmon sysfs interface is to let the user set the limits they want in
> > sensors.conf. Ideally each CPU would have sane defaults, and failing
> > that, the BIOS should set appropriate values.
> > 
> Looking at the code again, we did have ttarget for max, didn't we ? Any
> objections against using it ?

It's pretty confusing. If we stick to the SDM, MSR_TEMPERATURE_TARGET
is a register (MSR 0x1a2) and "Temperature Target" is a 7-bit read-only
value at 23:16 in this register. This value is what the 2.6.39 driver
is calling tjmax (a name the SDM never uses.)

What the 2.6.39 driver calls ttarget is another, undocumented 7-bit
value at 15:8 in the same register. We should definitely have used a
different name for it, but finding the proper name for something which
is undocumented isn't easy. Rudolf Marek documented this temperature as
the "temperature at which all fans should spin full speed". I have no
idea where he got the information from, all I can say is that the
reported value is 10°C below TjMax on my Xeon E5520 and 16°C below
TjMax on my wife's Core2 Duo E6400, which seems realistic. But I don't
think we ever reached these limits so I have no idea what would happen
then (if anything.)

Reusing the name ttarget for threshold1 in kernel 3.1 was definitely a
bad idea, as it only added to the confusion, and also hid the fact that
we were removing a feature from the driver in the process.

> As for max_hyst, looks like we'll have the option to leave it at 0, or
> to set it to something. You had a problem with leaving it at 0, so I
> guess it will be up to you to suggest an alternative. Setting it to the
> same value as max might be an alternative (and maybe keep interrupts
> disabled in that case).

My own experiments since then suggest that we shouldn't touch it at all
(at least not at driver initialization time.)

> > > > Interrupts should be enabled, and you can send uevents and sysfs notifications
> > > > whenever a threshold is crossed. All you would have to do is to keep the old
> > > > alarm
> > > > state and send events whenever it changes.
> > 
> > But who should enable interrupts? And who will receive them? It really
> > sounds like a task for thermal management, NOT hardware monitoring.
> > Seems the boundary is getting blurrier every day...
> > 
> > I would like to avoid the case where merely loading the coretemp driver
> > results in CPU throttling or worse simply because the threshold
> > settings are wrong.
> > 
> That is not how it works. The hwmon driver doesn't actually do anything.
> Best current example is gpio-fan.c: If thresholds are crossed,
> sysfs_notify is called for the affected alarm attributes, and a uevent
> is generated for the hwmon kobject. The new EXYNOS4 driver has a similar
> mechanism, though I had them take out sysfs notifications since those
> only covered 0->1 alarm transitions and we don't have guidelines for how
> to handle that (see below).
> 
> I don't see a problem with the notifications - it pretty much lets
> userland decide what if anything to do, and does not handle anything in
> the kernel. It enables the use of poll() on sysfs alarm attributes which
> I think is useful and desirable, and it generates a uevent to enable
> script handling, which I also think makes sense.

Yes, I'm totally fine with this as well, it all seems pretty sane and
useful. It would be good to have this documented somewhere under
Documentation/hwmon, but I don't think this is the case at the moment?

> But on the other side I _did_ ask a couple of times on the list (I am
> sure I copied you) if we should have guidelines for the use of
> sysfs_notify() and kobject_uevent() in hwmon drivers. Unfortunately, I
> didn't get any replies.

I'm sorry about that... I'm way too busy to be helpful, I am aware of
that, but unfortunately I don't expect much improvement in the near
future, and maybe not even in the not so near future :(

> As for getting and handling interrupts, there another example for that.
> The lm90 driver already implements alert handling. Today it only
> generates a log message, but one could imagine generating a uevent
> and/or sysfs notifications. Problem with sysfs notifications, though, is
> that smbus alerts are only generated for 0->1 transitions, not for 1->0
> transitions. This again leads to the question if and how to use
> sysfs_notify() in such cases.

My work on the lm90 driver was essentially a proof of concept, or an
example of SMBus alert support implementation if you will. It wasn't
meant to be terribly useful in its original implementation. I
completely agree that the log messages could be replaced by uevents or
sysfs notifications or both.

The fact that only 0->1 transitions are reported by the hardware is
likely to be a frequent case. If I understand correctly, the idea is
that the monitoring software can be passive as long as no notification
is received, but once a notification is received, it should switch to
active mode, taking action to solve the problem and then repeatedly
checking the temperatures (for example) until the bad condition is
gone. At which point it can go to passive mode again.

I don't think it makes sense to request that both 0->1 and 1->0
transitions are reported by drivers. If the hardware doesn't do it,
then it doesn't do it. Instead, whatever user-space tool is in charge
should expect (at least as a default behavior) that 1->0 transitions
aren't notified. Does it answer your question?

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

Hi Guenter,

On Fri, 16 Sep 2011 12:40:34 -0700, Guenter Roeck wrote:
> On Fri, 2011-09-16 at 15:21 -0400, Jean Delvare wrote:
> > So our driver makes many assumptions which aren't verified in my case:
> > 
> > * The driver shouldn't assume that the threshold values are under his
> >   sole control. Reading the values once at initialization time and
> >   never again after that is not correct.
> > 
> > * The driver assumes that threshold0 is higher than threshold1. Looking
> >   at the SDM, there is no such asymmetry, both thresholds are
> >   equivalent. So my laptop's BIOS is in its own right when deciding that
> >   threshold1 is high and threshold0 is low. Given that 0 < 1, their
> >   decision makes even more sense than ours. It's an IBM/Lenovo Thinkpad
> >   T60p, a pretty popular series, so we can't just ignore this problem.
> >   A lot of users will be affected.
> > 
> > * The driver artificially binds the two thresholds by making one the
> >   _hyst of the other. I see no such relation in the datasheet though,
> >   both thresholds appear to be completely independent. I know that this
> >   wasn't Durgadoss' original implementation and we had him change to
> >   that, but retrospectively this seems to have been a mistake.
> > 
> Do you have a better idea ? In the context, the lower threshold was
> supposed to be used to turn off thermal throttling, which in my
> understanding matches the semantics of _hyst - ie it is the value at
> which an alarm is turned off.

This was a driver decision, as the hardware doesn't put semantics on
the limits. And that was not a bad decision per se, it would be OK if
the driver had full control over the thresholds and interrupts, but as
it turns out, that won't be the case on every machine.

> > I presume that my BIOS leverages the interrupts associated with the
> > thresholds to do dynamic thermal management, either by fan speed control
> > or by CPU throttling, or anything else, or a mix of all these.
> > 
> > Durgadoss, please speak up if anything I wrote above isn't correct.
> > 
> > This brings up a question I asked before but never got an answer to,
> > and it seems I can't find the answer in the SDM either: where are the
> > interrupts going? Are these by any chance SMIs which the kernel has no
> > way to deal with?
> > 
> Looks like the BIOS would get those interrupts, at least on your
> system ? Some entity must change the register contents, after all.

Yes, that's my guess as well.

> > The first 2 wrong assumptions listed above can easily get fixed. First
> > one is fixed by always reading the values from the MSR instead of the
> > cache. Second one is fixed by testing the threshold values at
> > initialization time to determine which direction the BIOS went with
> > (might be racy though.)
> > 
> > The last assumption however seems very difficult to fix. It would be
> > valid to use one of the thresholds as a real low limit (e.g. to enable
> > a heating system if the system is about to freeze, or more
> > realistically, to enable turbo mode on low temperatures). In a way
> > that's what my laptop's BIOS is doing, although the threshold value and
> > presumably its effect change dynamically.
> > 
> > The fact that each threshold can be used for anything makes it very
> > difficult to make them fit in our standard hwmon interface. On one
> > machine the BIOS may expect the temperature to be below both thresholds
> > when the system is idle, while on others it will expect that the
> > current temperature is between the thresholds (as is the case on my
> > laptop.) This means that there is no unique semantics attached to these
> > thresholds, while our standard interface wants semantics attached
> > always.
> > 
> > I admit I am not sure how to deal with all this. Suggestions are
> > welcome. What I'm sure of is that we don't want to let the coretemp
> > driver in the state it currently is... We will get a flood of user
> > complaints or at least questions if we do.
>
> Me not either ... I did not expect that a "third party" would access and
> use the values. Sounds almost as bad as the ACPI interaction with some
> of the drivers.

Not really. Actually it sounds quite sane that the OS is not
responsible for thermal safety of the system. It's definitely the job
of the hardware itself, or failing that, the BIOS. Of course I would
prefer if the BIOS could just program a monitoring chip at boot time
and leave it alone after that, but on a laptop with no such smart
hardware monitoring chip, it makes some sense to use tricks like this.

As with ACPI interaction, the real problem isn't the BIOS poking at the
hardware... The problem is that we often don't know if it does or not.

> Given that we don't know how the BIOS (or some other entity) uses the
> limits, one possible solution that comes to my mind would be to extend
> the ABI to specifically permit non-specific threshold attributes (such
> as tempX_thresholdY). Not sure if that is a good idea, though.

This was more or less my conclusion after writing my report yesterday
evening. Just like you, I am not sure if it is a good idea. It's always
difficult to come up with an abstracted interface when we have a single
device implementing it for now (although I seem to remember a recent
new driver where 4 thresholds were needed?)

But anyway, the current state of the coretemp driver is simply not
good, so we have to do something. Short of a better proposal, I'd say
yes, let's go with tempX_threshold[0-7] and their associated alarms.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Sat, Sep 17, 2011 at 08:00:21AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Fri, 16 Sep 2011 12:40:34 -0700, Guenter Roeck wrote:
> > On Fri, 2011-09-16 at 15:21 -0400, Jean Delvare wrote:
> > > So our driver makes many assumptions which aren't verified in my case:
> > > 
> > > * The driver shouldn't assume that the threshold values are under his
> > >   sole control. Reading the values once at initialization time and
> > >   never again after that is not correct.
> > > 
> > > * The driver assumes that threshold0 is higher than threshold1. Looking
> > >   at the SDM, there is no such asymmetry, both thresholds are
> > >   equivalent. So my laptop's BIOS is in its own right when deciding that
> > >   threshold1 is high and threshold0 is low. Given that 0 < 1, their
> > >   decision makes even more sense than ours. It's an IBM/Lenovo Thinkpad
> > >   T60p, a pretty popular series, so we can't just ignore this problem.
> > >   A lot of users will be affected.
> > > 
> > > * The driver artificially binds the two thresholds by making one the
> > >   _hyst of the other. I see no such relation in the datasheet though,
> > >   both thresholds appear to be completely independent. I know that this
> > >   wasn't Durgadoss' original implementation and we had him change to
> > >   that, but retrospectively this seems to have been a mistake.
> > > 
> > Do you have a better idea ? In the context, the lower threshold was
> > supposed to be used to turn off thermal throttling, which in my
> > understanding matches the semantics of _hyst - ie it is the value at
> > which an alarm is turned off.
> 
> This was a driver decision, as the hardware doesn't put semantics on
> the limits. And that was not a bad decision per se, it would be OK if
> the driver had full control over the thresholds and interrupts, but as
> it turns out, that won't be the case on every machine.
> 
Agreed.

> > > I presume that my BIOS leverages the interrupts associated with the
> > > thresholds to do dynamic thermal management, either by fan speed control
> > > or by CPU throttling, or anything else, or a mix of all these.
> > > 
> > > Durgadoss, please speak up if anything I wrote above isn't correct.
> > > 
> > > This brings up a question I asked before but never got an answer to,
> > > and it seems I can't find the answer in the SDM either: where are the
> > > interrupts going? Are these by any chance SMIs which the kernel has no
> > > way to deal with?
> > > 
> > Looks like the BIOS would get those interrupts, at least on your
> > system ? Some entity must change the register contents, after all.
> 
> Yes, that's my guess as well.
> 
> > > The first 2 wrong assumptions listed above can easily get fixed. First
> > > one is fixed by always reading the values from the MSR instead of the
> > > cache. Second one is fixed by testing the threshold values at
> > > initialization time to determine which direction the BIOS went with
> > > (might be racy though.)
> > > 
> > > The last assumption however seems very difficult to fix. It would be
> > > valid to use one of the thresholds as a real low limit (e.g. to enable
> > > a heating system if the system is about to freeze, or more
> > > realistically, to enable turbo mode on low temperatures). In a way
> > > that's what my laptop's BIOS is doing, although the threshold value and
> > > presumably its effect change dynamically.
> > > 
> > > The fact that each threshold can be used for anything makes it very
> > > difficult to make them fit in our standard hwmon interface. On one
> > > machine the BIOS may expect the temperature to be below both thresholds
> > > when the system is idle, while on others it will expect that the
> > > current temperature is between the thresholds (as is the case on my
> > > laptop.) This means that there is no unique semantics attached to these
> > > thresholds, while our standard interface wants semantics attached
> > > always.
> > > 
> > > I admit I am not sure how to deal with all this. Suggestions are
> > > welcome. What I'm sure of is that we don't want to let the coretemp
> > > driver in the state it currently is... We will get a flood of user
> > > complaints or at least questions if we do.
> >
> > Me not either ... I did not expect that a "third party" would access and
> > use the values. Sounds almost as bad as the ACPI interaction with some
> > of the drivers.
> 
> Not really. Actually it sounds quite sane that the OS is not
> responsible for thermal safety of the system. It's definitely the job
> of the hardware itself, or failing that, the BIOS. Of course I would

Maybe, but I don't think we can assume or expect that this is always
the case either.

> prefer if the BIOS could just program a monitoring chip at boot time
> and leave it alone after that, but on a laptop with no such smart
> hardware monitoring chip, it makes some sense to use tricks like this.
> 
> As with ACPI interaction, the real problem isn't the BIOS poking at the
> hardware... The problem is that we often don't know if it does or not.
> 
> > Given that we don't know how the BIOS (or some other entity) uses the
> > limits, one possible solution that comes to my mind would be to extend
> > the ABI to specifically permit non-specific threshold attributes (such
> > as tempX_thresholdY). Not sure if that is a good idea, though.
> 
> This was more or less my conclusion after writing my report yesterday
> evening. Just like you, I am not sure if it is a good idea. It's always
> difficult to come up with an abstracted interface when we have a single
> device implementing it for now (although I seem to remember a recent
> new driver where 4 thresholds were needed?)
> 
Those were directional attributes, though, with a clear notion of
"triggered if value exceeds threshold or reaches it from below".

> But anyway, the current state of the coretemp driver is simply not
> good, so we have to do something. Short of a better proposal, I'd say
> yes, let's go with tempX_threshold[0-7] and their associated alarms.
> 
Alarms is tricky. Keep in mind that those thresholds are non-directional and 
trigger each time the temperature is reached, from above or from below.
Unless relationship between the threshold is well defined, such as with
<hyst, max> as we tried before, an "alarm" attribute on a single threshold
does not have a well defined meaning. An attribute named "triggered"
or similar might make more sense - something that causes a notification
if triggered and auto-resets after being read.

Still, with Durgadoss' other email, I am not sure I like where this is going.
I thought the idea was to trigger interrupts which would result in a notification
on the alarm attribute. As it is, it looks like the idea is to have the
interrupts handled by arch/x86/kernel/cpu/mcheck/therm_throt.c. If so,
I think it might be better to handle thresholds completely in the thermal
throttling code.

Thanks,
Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

Hi Jean,

On Sat, Sep 17, 2011 at 06:20:24AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> Late reply...
> 
no problem - me busy too ;).

> On Tue, 13 Sep 2011 08:20:06 -0700, Guenter Roeck wrote:
> > On Tue, 2011-09-13 at 10:13 -0400, Jean Delvare wrote:
> > > If anything, then the answer depends on the CPU, as it depends on
> > > TjMax. But more likely, no, we don't want to set these values, as we do
> > > NOT initialize limits in hwmon drivers [1]. The whole point of the
> > > hwmon sysfs interface is to let the user set the limits they want in
> > > sensors.conf. Ideally each CPU would have sane defaults, and failing
> > > that, the BIOS should set appropriate values.
> > > 
> > Looking at the code again, we did have ttarget for max, didn't we ? Any
> > objections against using it ?
> 
> It's pretty confusing. If we stick to the SDM, MSR_TEMPERATURE_TARGET
> is a register (MSR 0x1a2) and "Temperature Target" is a 7-bit read-only
> value at 23:16 in this register. This value is what the 2.6.39 driver
> is calling tjmax (a name the SDM never uses.)
> 
> What the 2.6.39 driver calls ttarget is another, undocumented 7-bit
> value at 15:8 in the same register. We should definitely have used a
> different name for it, but finding the proper name for something which
> is undocumented isn't easy. Rudolf Marek documented this temperature as
> the "temperature at which all fans should spin full speed". I have no
> idea where he got the information from, all I can say is that the
> reported value is 10°C below TjMax on my Xeon E5520 and 16°C below
> TjMax on my wife's Core2 Duo E6400, which seems realistic. But I don't
> think we ever reached these limits so I have no idea what would happen
> then (if anything.)
> 
> Reusing the name ttarget for threshold1 in kernel 3.1 was definitely a
> bad idea, as it only added to the confusion, and also hid the fact that
> we were removing a feature from the driver in the process.
> 
I agree.

> > As for max_hyst, looks like we'll have the option to leave it at 0, or
> > to set it to something. You had a problem with leaving it at 0, so I
> > guess it will be up to you to suggest an alternative. Setting it to the
> > same value as max might be an alternative (and maybe keep interrupts
> > disabled in that case).
> 
> My own experiments since then suggest that we shouldn't touch it at all
> (at least not at driver initialization time.)
> 
Yes.

> > > > > Interrupts should be enabled, and you can send uevents and sysfs notifications
> > > > > whenever a threshold is crossed. All you would have to do is to keep the old
> > > > > alarm
> > > > > state and send events whenever it changes.
> > > 
> > > But who should enable interrupts? And who will receive them? It really
> > > sounds like a task for thermal management, NOT hardware monitoring.
> > > Seems the boundary is getting blurrier every day...
> > > 
> > > I would like to avoid the case where merely loading the coretemp driver
> > > results in CPU throttling or worse simply because the threshold
> > > settings are wrong.
> > > 
> > That is not how it works. The hwmon driver doesn't actually do anything.
> > Best current example is gpio-fan.c: If thresholds are crossed,
> > sysfs_notify is called for the affected alarm attributes, and a uevent
> > is generated for the hwmon kobject. The new EXYNOS4 driver has a similar
> > mechanism, though I had them take out sysfs notifications since those
> > only covered 0->1 alarm transitions and we don't have guidelines for how
> > to handle that (see below).
> > 
> > I don't see a problem with the notifications - it pretty much lets
> > userland decide what if anything to do, and does not handle anything in
> > the kernel. It enables the use of poll() on sysfs alarm attributes which
> > I think is useful and desirable, and it generates a uevent to enable
> > script handling, which I also think makes sense.
> 
> Yes, I'm totally fine with this as well, it all seems pretty sane and
> useful. It would be good to have this documented somewhere under
> Documentation/hwmon, but I don't think this is the case at the moment?
> 
I wanted to get some feedback before I start documenting it. I should just
submit a patch describing how I think it should be done instead.

> > But on the other side I _did_ ask a couple of times on the list (I am
> > sure I copied you) if we should have guidelines for the use of
> > sysfs_notify() and kobject_uevent() in hwmon drivers. Unfortunately, I
> > didn't get any replies.
> 
> I'm sorry about that... I'm way too busy to be helpful, I am aware of
> that, but unfortunately I don't expect much improvement in the near
> future, and maybe not even in the not so near future :(
> 
> > As for getting and handling interrupts, there another example for that.
> > The lm90 driver already implements alert handling. Today it only
> > generates a log message, but one could imagine generating a uevent
> > and/or sysfs notifications. Problem with sysfs notifications, though, is
> > that smbus alerts are only generated for 0->1 transitions, not for 1->0
> > transitions. This again leads to the question if and how to use
> > sysfs_notify() in such cases.
> 
> My work on the lm90 driver was essentially a proof of concept, or an
> example of SMBus alert support implementation if you will. It wasn't
> meant to be terribly useful in its original implementation. I
> completely agree that the log messages could be replaced by uevents or
> sysfs notifications or both.
> 
I understand - I'll need to make it useful for the max6696 in our HW, though.

> The fact that only 0->1 transitions are reported by the hardware is
> likely to be a frequent case. If I understand correctly, the idea is
> that the monitoring software can be passive as long as no notification
> is received, but once a notification is received, it should switch to
> active mode, taking action to solve the problem and then repeatedly
> checking the temperatures (for example) until the bad condition is
> gone. At which point it can go to passive mode again.
> 
> I don't think it makes sense to request that both 0->1 and 1->0
> transitions are reported by drivers. If the hardware doesn't do it,
> then it doesn't do it. Instead, whatever user-space tool is in charge
> should expect (at least as a default behavior) that 1->0 transitions
> aren't notified. Does it answer your question?
> 
Not really, unfortunately. Like in the lm90 driver, there are devices 
which don't handle SMBalerts correctly. For PMBus devices, ADM1275, ADM1276,
and the entire UCD92xx series are among those (ADM127x trigger SMBalert every few
ms while there is an alarm condirion, and UCD92XX simply hold the SMBalert pin low).
As soon as an alarm is triggered, I have to disable alerts completely and revert
to polling to catch state updates. While this conveniently covers the 1->0 state
change, I need it to be able to re-enable alerts after all alarms have gone away.

The lm90 driver with its LM90_HAVE_BROKEN_ALERT flag has the same problem.
Sure, one can simply disable alerts forever after the first alert is triggered,
as the driver does right now, but that is really just as helpful as having
no alerts at all.

My current solution is to revert to in-kernel polling while alarms are active.
This way I can re-enable alerts after the alarms are no longer active,
and the additional overhead is quite low since the polling thread only runs
while there are active alarms.

If we declare that applications can only depend on 0->1 transitions, I would
still need in-kernel polling to be able to re-enable alerts. As a result, we
would end up with double polling - by the kernel _and_ by the application.
That would really be undesirable.

Thanks,
Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Sat, Sep 17, 2011 at 12:58:40PM -0400, R, Durgadoss wrote:
> Hi Guenter/Jean,
> 
> [snip..] 
> > > But anyway, the current state of the coretemp driver is simply not
> > > good, so we have to do something. Short of a better proposal, I'd say
> > > yes, let's go with tempX_threshold[0-7] and their associated alarms.
> > >
> > Alarms is tricky. Keep in mind that those thresholds are non-directional and
> > trigger each time the temperature is reached, from above or from below.
> > Unless relationship between the threshold is well defined, such as with
> > <hyst, max> as we tried before, an "alarm" attribute on a single threshold
> > does not have a well defined meaning. An attribute named "triggered"
> > or similar might make more sense - something that causes a notification
> > if triggered and auto-resets after being read.
> > 
> > Still, with Durgadoss' other email, I am not sure I like where this is going.
> > I thought the idea was to trigger interrupts which would result in a
> > notification
> > on the alarm attribute. As it is, it looks like the idea is to have the
> > interrupts handled by arch/x86/kernel/cpu/mcheck/therm_throt.c. If so,
> > I think it might be better to handle thresholds completely in the thermal
> > throttling code.
> > 
> 
> It was just a suggestion from me that we can handle the interrupts from
> within therm_throt since there was code in there, doing this already.
> I did not know (in fact do not know..) how to catch these interrupts
> inside coretemp. That's why I thought of adding code in therm_throt.
> 
> If it's not the right way to go, I am open to other ideas.
> 
> On the other hand, I am fine with the idea of having tempX_threshold[0-7]
> and their alarms. We can use the 'status' bits in the THERM_STATUS 
> register to represent the alarm. The logic can be something like this:
> 
> //Log bits indicate the input temperature reached the configured threshold;
> //but we do not know from which direction.
> if (msr_val & THERM_LOG_THRESHOLD0) {
> 	//if the status bit is one, the input temperature is higher than the
> 	//configured threshold. If it is zero, the input temperature is lower
> 	//than the configured threshold.
> 	bool alarm = msr_val & THERM_STATUS_THRESHOLD0;
> 		print: alarm
> 	//Let the user space take care of 0/1 from the *_alarm interfaces.
> }
> 
So there is a clear notion of "exceed" associated with those thresholds ?
I thought there was just an interrupt whenever the threshold is reached
from either side. Looks like I missed that one.

Personally, I don't think "alarm" would be appropriate here, since we don't
know if the threshold is supposed to be a lower or an upper limit, and if
it reflects an alarm to start with. If we define a new set of attributes for 
unspecified thresholds, I would prefer something like "tempX_thresholdY_triggered".

Thanks,
Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[R, Durgadoss]

Hi Guenter/Jean,

[snip..] 
> > But anyway, the current state of the coretemp driver is simply not
> > good, so we have to do something. Short of a better proposal, I'd say
> > yes, let's go with tempX_threshold[0-7] and their associated alarms.
> >
> Alarms is tricky. Keep in mind that those thresholds are non-directional and
> trigger each time the temperature is reached, from above or from below.
> Unless relationship between the threshold is well defined, such as with
> <hyst, max> as we tried before, an "alarm" attribute on a single threshold
> does not have a well defined meaning. An attribute named "triggered"
> or similar might make more sense - something that causes a notification
> if triggered and auto-resets after being read.
> 
> Still, with Durgadoss' other email, I am not sure I like where this is going.
> I thought the idea was to trigger interrupts which would result in a
> notification
> on the alarm attribute. As it is, it looks like the idea is to have the
> interrupts handled by arch/x86/kernel/cpu/mcheck/therm_throt.c. If so,
> I think it might be better to handle thresholds completely in the thermal
> throttling code.
> 

It was just a suggestion from me that we can handle the interrupts from
within therm_throt since there was code in there, doing this already.
I did not know (in fact do not know..) how to catch these interrupts
inside coretemp. That's why I thought of adding code in therm_throt.

If it's not the right way to go, I am open to other ideas.

On the other hand, I am fine with the idea of having tempX_threshold[0-7]
and their alarms. We can use the 'status' bits in the THERM_STATUS 
register to represent the alarm. The logic can be something like this:

//Log bits indicate the input temperature reached the configured threshold;
//but we do not know from which direction.
if (msr_val & THERM_LOG_THRESHOLD0) {
	//if the status bit is one, the input temperature is higher than the
	//configured threshold. If it is zero, the input temperature is lower
	//than the configured threshold.
	bool alarm = msr_val & THERM_STATUS_THRESHOLD0;
		print: alarm
	//Let the user space take care of 0/1 from the *_alarm interfaces.
}

Does this look Ok ? 
Or, kindly suggest alternatives.

Thanks,
Durga

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[R, Durgadoss]

Hi Guenter,

[snip..]
> > It was just a suggestion from me that we can handle the interrupts from
> > within therm_throt since there was code in there, doing this already.
> > I did not know (in fact do not know..) how to catch these interrupts
> > inside coretemp. That's why I thought of adding code in therm_throt.
> >
> > If it's not the right way to go, I am open to other ideas.
> >
> > On the other hand, I am fine with the idea of having tempX_threshold[0-7]
> > and their alarms. We can use the 'status' bits in the THERM_STATUS
> > register to represent the alarm. The logic can be something like this:
> >
> > //Log bits indicate the input temperature reached the configured threshold;
> > //but we do not know from which direction.
> > if (msr_val & THERM_LOG_THRESHOLD0) {
> > 	//if the status bit is one, the input temperature is higher than the
> > 	//configured threshold. If it is zero, the input temperature is lower
> > 	//than the configured threshold.
> > 	bool alarm = msr_val & THERM_STATUS_THRESHOLD0;
> > 		print: alarm
> > 	//Let the user space take care of 0/1 from the *_alarm interfaces.
> > }
> >
> So there is a clear notion of "exceed" associated with those thresholds ?

Sorry Guenter. I don't get what you mean by this :-(

> I thought there was just an interrupt whenever the threshold is reached
> from either side. Looks like I missed that one.
> 
> Personally, I don't think "alarm" would be appropriate here, since we don't
> know if the threshold is supposed to be a lower or an upper limit, and if
> it reflects an alarm to start with. If we define a new set of attributes for
> unspecified thresholds, I would prefer something like
> "tempX_thresholdY_triggered".
> 

For me, it looks like, we need not know whether the threshold is upper or lower.
Anyway, for every threshold, we will get two interrupts (for either direction)
So, the user space can assume either a lower threshold and look for 0 in the
Corresponding alarm interface Or a higher threshold and look for 1 in the
alarm interface. Will this not work ?

Thanks,
Durga


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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Sat, Sep 17, 2011 at 01:23:27PM -0400, R, Durgadoss wrote:
> Hi Guenter,
> 
> [snip..]
> > > It was just a suggestion from me that we can handle the interrupts from
> > > within therm_throt since there was code in there, doing this already.
> > > I did not know (in fact do not know..) how to catch these interrupts
> > > inside coretemp. That's why I thought of adding code in therm_throt.
> > >
> > > If it's not the right way to go, I am open to other ideas.
> > >
> > > On the other hand, I am fine with the idea of having tempX_threshold[0-7]
> > > and their alarms. We can use the 'status' bits in the THERM_STATUS
> > > register to represent the alarm. The logic can be something like this:
> > >
> > > //Log bits indicate the input temperature reached the configured threshold;
> > > //but we do not know from which direction.
> > > if (msr_val & THERM_LOG_THRESHOLD0) {
> > > 	//if the status bit is one, the input temperature is higher than the
> > > 	//configured threshold. If it is zero, the input temperature is lower
> > > 	//than the configured threshold.
> > > 	bool alarm = msr_val & THERM_STATUS_THRESHOLD0;
> > > 		print: alarm
> > > 	//Let the user space take care of 0/1 from the *_alarm interfaces.
> > > }
> > >
> > So there is a clear notion of "exceed" associated with those thresholds ?
> 
> Sorry Guenter. I don't get what you mean by this :-(
> 
How about "It is known that the temperature is at or above the threshold" ?

> > I thought there was just an interrupt whenever the threshold is reached
> > from either side. Looks like I missed that one.
> > 
> > Personally, I don't think "alarm" would be appropriate here, since we don't
> > know if the threshold is supposed to be a lower or an upper limit, and if
> > it reflects an alarm to start with. If we define a new set of attributes for
> > unspecified thresholds, I would prefer something like
> > "tempX_thresholdY_triggered".
> > 
> 
> For me, it looks like, we need not know whether the threshold is upper or lower.
> Anyway, for every threshold, we will get two interrupts (for either direction)
> So, the user space can assume either a lower threshold and look for 0 in the
> Corresponding alarm interface Or a higher threshold and look for 1 in the
> alarm interface. Will this not work ?
> 
For a lower threshold, "alarm" implies "temperature is at or below threshold",
In other words, "alarm" can mean that a value is above or below a given threshold -
it has a semantics that depends on its context.

This context is not known in the case of a generic threshold. This is why I suggested
to use a more neutral term, such as "triggered", which would imply "at or above
threshold" (or possibly just "threshold triggered" if the direction is not known)
without attaching a semantics to it.

Thanks,
Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[R, Durgadoss]

Hi Guenter,

[snip..]
> > > > //but we do not know from which direction.
> > > > if (msr_val & THERM_LOG_THRESHOLD0) {
> > > > 	//if the status bit is one, the input temperature is higher than
> the
> > > > 	//configured threshold. If it is zero, the input temperature is
> lower
> > > > 	//than the configured threshold.
> > > > 	bool alarm = msr_val & THERM_STATUS_THRESHOLD0;
> > > > 		print: alarm
> > > > 	//Let the user space take care of 0/1 from the *_alarm interfaces.
> > > > }
> > > >
> > > So there is a clear notion of "exceed" associated with those thresholds ?
> >
> > Sorry Guenter. I don't get what you mean by this :-(
> >
> How about "It is known that the temperature is at or above the threshold" ?

Oh. I get it. Thank you..

> > > I thought there was just an interrupt whenever the threshold is reached
> > > from either side. Looks like I missed that one.
> > >
> > > Personally, I don't think "alarm" would be appropriate here, since we don't
> > > know if the threshold is supposed to be a lower or an upper limit, and if
> > > it reflects an alarm to start with. If we define a new set of attributes
> for
> > > unspecified thresholds, I would prefer something like
> > > "tempX_thresholdY_triggered".
> > >
> >
> > For me, it looks like, we need not know whether the threshold is upper or
> lower.
> > Anyway, for every threshold, we will get two interrupts (for either
> direction)
> > So, the user space can assume either a lower threshold and look for 0 in the
> > Corresponding alarm interface Or a higher threshold and look for 1 in the
> > alarm interface. Will this not work ?
> >
> For a lower threshold, "alarm" implies "temperature is at or below threshold",
> In other words, "alarm" can mean that a value is above or below a given
> threshold -
> it has a semantics that depends on its context.
> 
> This context is not known in the case of a generic threshold. This is why I
> suggested
> to use a more neutral term, such as "triggered", which would imply "at or above
> threshold" (or possibly just "threshold triggered" if the direction is not
> known)
> without attaching a semantics to it.
> 

Ok. I agree. We can use tempX_thresholdY_triggered.

Thanks,
Durga

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

Hi Durga,

On Sat, Sep 17, 2011 at 01:40:41PM -0400, R, Durgadoss wrote:
> Hi Guenter,
> 
[ ... ]

> > > For me, it looks like, we need not know whether the threshold is upper or
> > lower.
> > > Anyway, for every threshold, we will get two interrupts (for either
> > direction)
> > > So, the user space can assume either a lower threshold and look for 0 in the
> > > Corresponding alarm interface Or a higher threshold and look for 1 in the
> > > alarm interface. Will this not work ?
> > >
> > For a lower threshold, "alarm" implies "temperature is at or below threshold",
> > In other words, "alarm" can mean that a value is above or below a given
> > threshold -
> > it has a semantics that depends on its context.
> > 
> > This context is not known in the case of a generic threshold. This is why I
> > suggested
> > to use a more neutral term, such as "triggered", which would imply "at or above
> > threshold" (or possibly just "threshold triggered" if the direction is not
> > known)
> > without attaching a semantics to it.
> > 
> 
> Ok. I agree. We can use tempX_thresholdY_triggered.
> 
I'd like to hear Jean's opinion first. Also, if we introduce new attributes,
we should probably reinstate the old "max".

Thanks,
Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

Hi Guenter,

On Sat, 17 Sep 2011 09:31:38 -0700, Guenter Roeck wrote:
> The lm90 driver with its LM90_HAVE_BROKEN_ALERT flag has the same problem.
> Sure, one can simply disable alerts forever after the first alert is triggered,
> as the driver does right now, but that is really just as helpful as having
> no alerts at all.

This isn't what the lm90 driver is doing. Look at the end of
lm90_update_device(), alarms are re-enabled if needed. I agree that it
isn't bullet proof, as it only works if user-space is reading at least
one value repeatedly, but it's way better than what you described.

> My current solution is to revert to in-kernel polling while alarms are active.
> This way I can re-enable alerts after the alarms are no longer active,
> and the additional overhead is quite low since the polling thread only runs
> while there are active alarms.
> 
> If we declare that applications can only depend on 0->1 transitions, I would
> still need in-kernel polling to be able to re-enable alerts. As a result, we
> would end up with double polling - by the kernel _and_ by the application.
> That would really be undesirable.

I agree. I suggested user-space polling because I didn't know there was
a need for in-kernel polling. If you think that this will be the case
for most drivers, then I have no objection to specifying that drivers
should notify both directions or neither, i.e. no user-space polling.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Sun, Sep 18, 2011 at 09:30:22AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Sat, 17 Sep 2011 09:31:38 -0700, Guenter Roeck wrote:
> > The lm90 driver with its LM90_HAVE_BROKEN_ALERT flag has the same problem.
> > Sure, one can simply disable alerts forever after the first alert is triggered,
> > as the driver does right now, but that is really just as helpful as having
> > no alerts at all.
> 
> This isn't what the lm90 driver is doing. Look at the end of
> lm90_update_device(), alarms are re-enabled if needed. I agree that it
> isn't bullet proof, as it only works if user-space is reading at least
> one value repeatedly, but it's way better than what you described.
> 
Ah, I missed that. Not much better, though. It means that applications
would have to start polling after the first alarm is seen on a device,
and keep doing so until the last alarm is gone.

> > My current solution is to revert to in-kernel polling while alarms are active.
> > This way I can re-enable alerts after the alarms are no longer active,
> > and the additional overhead is quite low since the polling thread only runs
> > while there are active alarms.
> > 
> > If we declare that applications can only depend on 0->1 transitions, I would
> > still need in-kernel polling to be able to re-enable alerts. As a result, we
> > would end up with double polling - by the kernel _and_ by the application.
> > That would really be undesirable.
> 
> I agree. I suggested user-space polling because I didn't know there was
> a need for in-kernel polling. If you think that this will be the case
> for most drivers, then I have no objection to specifying that drivers
> should notify both directions or neither, i.e. no user-space polling.
> 
Ok.

Thanks,
Guenter

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Sat, 17 Sep 2011 11:09:43 -0700, Guenter Roeck wrote:
> Hi Durga,
> 
> On Sat, Sep 17, 2011 at 01:40:41PM -0400, R, Durgadoss wrote:
> > Hi Guenter,
> > 
> [ ... ]
> 
> > > > For me, it looks like, we need not know whether the threshold is upper or
> > > lower.
> > > > Anyway, for every threshold, we will get two interrupts (for either
> > > direction)
> > > > So, the user space can assume either a lower threshold and look for 0 in the
> > > > Corresponding alarm interface Or a higher threshold and look for 1 in the
> > > > alarm interface. Will this not work ?
> > > >
> > > For a lower threshold, "alarm" implies "temperature is at or below threshold",
> > > In other words, "alarm" can mean that a value is above or below a given
> > > threshold -
> > > it has a semantics that depends on its context.
> > > 
> > > This context is not known in the case of a generic threshold. This is why I
> > > suggested
> > > to use a more neutral term, such as "triggered", which would imply "at or above
> > > threshold" (or possibly just "threshold triggered" if the direction is not
> > > known)
> > > without attaching a semantics to it.
> > > 
> > 
> > Ok. I agree. We can use tempX_thresholdY_triggered.
> > 
> I'd like to hear Jean's opinion first.

I'm fine with your proposal, yes.

> Also, if we introduce new attributes, we should probably reinstate the old "max".

Definitely. And preferably the 2.6.39 variant rather than the 3.0
variant, i.e. no magical -20°C offset, unless someone can explain where
it comes from.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Sat, 17 Sep 2011 11:10:25 +0530, R, Durgadoss wrote:
> That said, I do not know the behaviour on a different kernel version,
> (especially older kernels) with a different BIOS. I do not see any
> mention of 'BIOS configuring (or can configure) these thresholds' in
> the SDM. For a double check, I am referring to IA Manual Vol-3A.Chapter-14:
> Download Link: 
> http://download.intel.com/design/processor/manuals/253668.pdf

Yes, I'm reading the same...

> In Section 14.5 it says:
> "On-die digital thermal sensor and interrupt mechanisms permit the OS to
> manage thermal conditions natively without relying on BIOS or other system
> board components."
> This is also one more reason why I believe BIOS is not changing
> these thresholds. But it might be wrong too.

Maybe the BIOS shouldn't do it if you stick to the SDM wording, by I
have a real world case on my desk where it does. As I said, this is a
popular series of machines, so we will have to deal with it.

> In older kernels (before Linux 3.0), the temp1_max sysfs interface
> reads the value from IA32_TEMPERATURE_TARGET register.
> But in recent machines, I don't think this register is supported.

I thought it was the other way around and it was not supported on older
CPUs, but it was on newer ones. Pretty hard to say though, given that
it's still undocumented.

> This is the reason, I wanted the sysfs interface names to be
> tempX_threshold[0/1] (or something but not interfere with temp1_max).
> Due to ABI considerations, we had to make it 'temp1_max'.
> May be we should leave temp1_max as such, and add temp1_threshold[0/1] ?
> 
> So, Jean, if your kernel's temp1_max interface is being read from
> IA32_TEMPERATURE_TARGET, I can imagine the change in values you
> reported.

No, my report was with the latest coretemp driver (backported to my
older kernel), so tempX_max not read from IA32_TEMPERATURE_TARGET, and
thresholds mapping to tempX_max and tempX_max_hyst.

> Could you please tell us, whether the change in values are seen
> in a newer kernel(Linux 3.0+) also ? Hope you find some time..

I can try, but I doubt it will change anything, as I'm testing with the
latest version of the coretemp driver anyway.

> Moreover, I do not see a mention of IA32_TEMPERATURE_TARGET in the SDM
> I am referring to. May be all of us don't see :-(

Not sure what you mean here, but if you mean bits 14-8 of MSR
IA32_TEMPERATURE_TARGET, then yes, as I said these are undocumented.
You can argue that we shouldn't use undocumented registers in our
driver, but Rudolf thought it was useful, and honestly, if we only used
documented information, the coretemp driver would not have existed in
the first place.

> > * The driver assumes that threshold0 is higher than threshold1. Looking
> >   at the SDM, there is no such asymmetry, both thresholds are
> >   equivalent. So my laptop's BIOS is in its own right when deciding that
> >   threshold1 is high and threshold0 is low. Given that 0 < 1, their
> >   decision makes even more sense than ours. It's an IBM/Lenovo Thinkpad
> >   T60p, a pretty popular series, so we can't just ignore this problem.
> >   A lot of users will be affected.
> 
> I feel the driver thinks the other way only. The store_ttarget function
> corresponding to temp1_max interface, reads values for Threshold1
> and store_tmin corresponding to temp1_max_hyst interface reads values
> for Threshold0. Kindly correct me if I am wrong here.

I know what the driver does, I was simply saying that it shouldn't do
it. This is pointless now anyway, as we agreed to rework the whole
interface.

> (...)
> Alright, here comes the million dollar question:
> where are the interrupts going?
> 
> Honestly I do not have a complete answer. I am writing
> Whatever I know, from the kernel code base.
> 
> If we look at the file therm_throt.c, inside arch/x86/kernel/
> cpu/mcheck/ I think we will get some idea.
> 
> There is some code which registers for thermal related interrupts
> 
> asmlinkage void smp_thermal_interrupt(struct pt_regs *regs)
> {
>         exit_idle();
>         irq_enter();
>         inc_irq_stat(irq_thermal_count);
>         smp_thermal_vector();
>         irq_exit();
>         /* Ack only at the end to avoid potential reentry */
>         ack_APIC_irq();
> }
> 
> When our threshold interrupt occurs, the control comes here.
> (We should enable the interrupt for this..)

I doubt it. On my Thinkpad, the threshold interrupts are enabled, I see
the threshold values change under heavy load, but the TRM counters
in /proc/interrupts do not change. So at least in my case it really
seems that the interrupts are SMIs and not thermal event interrupts.

I find it really odd that the SDM doesn't seem to explain this. Or
maybe I am just not looking at the right place. 1874 pages is a lot.

> And as of now, there is no code inside therm_throt that can
> handle our threshold interrupts. For the past two days, I had
> been working on a patch, to add this functionality, to therm_throt.
> 
> But the patch has to go to linux-x86_64 mailing list. I will copy
> Jean & Guenter, while submitting this patch.
> 
> This is what I am planning to try out:
> 1. Catch the threshold interrupts in therm_throt

If we can at all...

> 2. Notify coretemp from therm_throt
>    (There is a function pointer exported for this)
> 3. Send UEvent from coretemp to the user space
> 4. Let the user space deal with the UEvent
>    (i.e let it take appropriate actions)

As Guenter wrote, it's questionable then why coretemp would be
responsible for this.

> > I admit I am not sure how to deal with all this. Suggestions are
> > welcome. What I'm sure of is that we don't want to let the coretemp
> > driver in the state it currently is... We will get a flood of user
> > complaints or at least questions if we do.
> 
> I agree. I plan to do the 4 steps that I mentioned above.
> Kindly correct/suggest alternatives.
> 
> And Sorry Jean, I think we are in different time zones.

Me being very busy is worse than the time zone difference, I'm afraid.

> That's why could not respond to this mail early.
> I woke up this morning reading this email..and replying now.

Take it easy :)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Sat, 17 Sep 2011 03:12:12 -0700, Guenter Roeck wrote:
> Hi Durgadoss,
> 
> On Sat, Sep 17, 2011 at 01:40:25AM -0400, R, Durgadoss wrote:
> > If we look at the file therm_throt.c, inside arch/x86/kernel/
> > cpu/mcheck/ I think we will get some idea.
> > 
> > There is some code which registers for thermal related interrupts
> > 
> > asmlinkage void smp_thermal_interrupt(struct pt_regs *regs)
> > {
> >         exit_idle();
> >         irq_enter();
> >         inc_irq_stat(irq_thermal_count);
> >         smp_thermal_vector();
> >         irq_exit();
> >         /* Ack only at the end to avoid potential reentry */
> >         ack_APIC_irq();
> > }
> > 
> > When our threshold interrupt occurs, the control comes here.
> > (We should enable the interrupt for this..)
> > And as of now, there is no code inside therm_throt that can
> > handle our threshold interrupts. For the past two days, I had
> > been working on a patch, to add this functionality, to therm_throt.
> > 
> > But the patch has to go to linux-x86_64 mailing list. I will copy
> > Jean & Guenter, while submitting this patch.
> > 
> Maybe I misunderstand something, but I don't think that will work.
> We can not have one module set parameters for interrupts to be handled
> by another. If the thermal throttling module is going to use
> the interrupts, the thresholds should be set there, not in the
> hwmon driver.

Actually it can be done using notification chains (coretemp registers a
callback function when loaded.) As I recall the mechanism is already in
place but unused at the moment. But that doesn't mean we want to do
this. I see no reason to make things more complex that they need to be,
if therm_throt registers the interrupt then it should deal with them by
itself. After all, we are discussing a brand new interface to deal with
this case, so it doesn't have to be in hwmon, it can be anywhere else.

But anyway, this discussion is pointless if we're talking about SMIs,
which is my impression so far (at least on my Thinkpad laptop - I need
to experiment further on the other machines.)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Jean Delvare]

On Sun, 18 Sep 2011 09:46:32 -0700, Guenter Roeck wrote:
> On Sun, Sep 18, 2011 at 09:30:22AM -0400, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > On Sat, 17 Sep 2011 09:31:38 -0700, Guenter Roeck wrote:
> > > The lm90 driver with its LM90_HAVE_BROKEN_ALERT flag has the same problem.
> > > Sure, one can simply disable alerts forever after the first alert is triggered,
> > > as the driver does right now, but that is really just as helpful as having
> > > no alerts at all.
> > 
> > This isn't what the lm90 driver is doing. Look at the end of
> > lm90_update_device(), alarms are re-enabled if needed. I agree that it
> > isn't bullet proof, as it only works if user-space is reading at least
> > one value repeatedly, but it's way better than what you described.
>
> Ah, I missed that. Not much better, though. It means that applications
> would have to start polling after the first alarm is seen on a device,
> and keep doing so until the last alarm is gone.

Correct. Following your idea, this could be replaced by a kernel
thread / tasklet / whatever that would only run when needed.

I wonder if someday hardware vendors will start doing their work
properly so that we don't have to bloat our driver code? Probably not :(

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

On Sun, 2011-09-18 at 13:24 -0400, Jean Delvare wrote:
> On Sat, 17 Sep 2011 11:09:43 -0700, Guenter Roeck wrote:
> > Hi Durga,
> > 
> > On Sat, Sep 17, 2011 at 01:40:41PM -0400, R, Durgadoss wrote:
> > > Hi Guenter,
> > > 
> > [ ... ]
> > 
> > > > > For me, it looks like, we need not know whether the threshold is upper or
> > > > lower.
> > > > > Anyway, for every threshold, we will get two interrupts (for either
> > > > direction)
> > > > > So, the user space can assume either a lower threshold and look for 0 in the
> > > > > Corresponding alarm interface Or a higher threshold and look for 1 in the
> > > > > alarm interface. Will this not work ?
> > > > >
> > > > For a lower threshold, "alarm" implies "temperature is at or below threshold",
> > > > In other words, "alarm" can mean that a value is above or below a given
> > > > threshold -
> > > > it has a semantics that depends on its context.
> > > > 
> > > > This context is not known in the case of a generic threshold. This is why I
> > > > suggested
> > > > to use a more neutral term, such as "triggered", which would imply "at or above
> > > > threshold" (or possibly just "threshold triggered" if the direction is not
> > > > known)
> > > > without attaching a semantics to it.
> > > > 
> > > 
> > > Ok. I agree. We can use tempX_thresholdY_triggered.
> > > 
> > I'd like to hear Jean's opinion first.
> 
> I'm fine with your proposal, yes.
> 
> > Also, if we introduce new attributes, we should probably reinstate the old "max".
> 
> Definitely. And preferably the 2.6.39 variant rather than the 3.0
> variant, i.e. no magical -20°C offset, unless someone can explain where
> it comes from.
> 
Sounds good.

Durga, do you have time to make those changes ?

Thanks,
GUenter



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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[R, Durgadoss]

Hi Guenter,

> -----Original Message-----
> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> Sent: Monday, September 19, 2011 10:54 PM
> To: Jean Delvare
> Cc: R, Durgadoss; lm-sensors@lm-sensors.org
> Subject: Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold Support
> to Coretemp
> 
> On Sun, 2011-09-18 at 13:24 -0400, Jean Delvare wrote:
> > On Sat, 17 Sep 2011 11:09:43 -0700, Guenter Roeck wrote:
> > > Hi Durga,
> > >
> > > On Sat, Sep 17, 2011 at 01:40:41PM -0400, R, Durgadoss wrote:
> > > > Hi Guenter,
> > > >
> > > [ ... ]
> > >
> > > > > > For me, it looks like, we need not know whether the threshold is
> upper or
> > > > > lower.
> > > > > > Anyway, for every threshold, we will get two interrupts (for either
> > > > > direction)
> > > > > > So, the user space can assume either a lower threshold and look for 0
> in the
> > > > > > Corresponding alarm interface Or a higher threshold and look for 1 in
> the
> > > > > > alarm interface. Will this not work ?
> > > > > >
> > > > > For a lower threshold, "alarm" implies "temperature is at or below
> threshold",
> > > > > In other words, "alarm" can mean that a value is above or below a given
> > > > > threshold -
> > > > > it has a semantics that depends on its context.
> > > > >
> > > > > This context is not known in the case of a generic threshold. This is
> why I
> > > > > suggested
> > > > > to use a more neutral term, such as "triggered", which would imply "at
> or above
> > > > > threshold" (or possibly just "threshold triggered" if the direction is
> not
> > > > > known)
> > > > > without attaching a semantics to it.
> > > > >
> > > >
> > > > Ok. I agree. We can use tempX_thresholdY_triggered.
> > > >
> > > I'd like to hear Jean's opinion first.
> >
> > I'm fine with your proposal, yes.
> >
> > > Also, if we introduce new attributes, we should probably reinstate the old
> "max".
> >
> > Definitely. And preferably the 2.6.39 variant rather than the 3.0
> > variant, i.e. no magical -20°C offset, unless someone can explain where
> > it comes from.
> >
> Sounds good.
> 
> Durga, do you have time to make those changes ?
> 

I hope I can find some time in a couple of days.
So I Will submit a patch for this.

Thanks,
Durga 

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

Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold

[Guenter Roeck]

Hi Durga,

On Mon, Sep 19, 2011 at 11:27:56PM -0400, R, Durgadoss wrote:
> Hi Guenter,
> 
> > -----Original Message-----
> > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> > Sent: Monday, September 19, 2011 10:54 PM
> > To: Jean Delvare
> > Cc: R, Durgadoss; lm-sensors@lm-sensors.org
> > Subject: Re: [lm-sensors] [PATCHv4 1/1] Hwmon: Add core/pkg Threshold Support
> > to Coretemp
> > 
> > On Sun, 2011-09-18 at 13:24 -0400, Jean Delvare wrote:
> > > On Sat, 17 Sep 2011 11:09:43 -0700, Guenter Roeck wrote:
> > > > Hi Durga,
> > > >
> > > > On Sat, Sep 17, 2011 at 01:40:41PM -0400, R, Durgadoss wrote:
> > > > > Hi Guenter,
> > > > >
> > > > [ ... ]
> > > >
> > > > > > > For me, it looks like, we need not know whether the threshold is
> > upper or
> > > > > > lower.
> > > > > > > Anyway, for every threshold, we will get two interrupts (for either
> > > > > > direction)
> > > > > > > So, the user space can assume either a lower threshold and look for 0
> > in the
> > > > > > > Corresponding alarm interface Or a higher threshold and look for 1 in
> > the
> > > > > > > alarm interface. Will this not work ?
> > > > > > >
> > > > > > For a lower threshold, "alarm" implies "temperature is at or below
> > threshold",
> > > > > > In other words, "alarm" can mean that a value is above or below a given
> > > > > > threshold -
> > > > > > it has a semantics that depends on its context.
> > > > > >
> > > > > > This context is not known in the case of a generic threshold. This is
> > why I
> > > > > > suggested
> > > > > > to use a more neutral term, such as "triggered", which would imply "at
> > or above
> > > > > > threshold" (or possibly just "threshold triggered" if the direction is
> > not
> > > > > > known)
> > > > > > without attaching a semantics to it.
> > > > > >
> > > > >
> > > > > Ok. I agree. We can use tempX_thresholdY_triggered.
> > > > >
> > > > I'd like to hear Jean's opinion first.
> > >
> > > I'm fine with your proposal, yes.
> > >
> > > > Also, if we introduce new attributes, we should probably reinstate the old
> > "max".
> > >
> > > Definitely. And preferably the 2.6.39 variant rather than the 3.0
> > > variant, i.e. no magical -20°C offset, unless someone can explain where
> > > it comes from.
> > >
> > Sounds good.
> > 
> > Durga, do you have time to make those changes ?
> > 
> 
> I hope I can find some time in a couple of days.
> So I Will submit a patch for this.
> 
Never mind - I needed distraction and coded it in the last hour or so.

I'll test it tomorrow and send a patch unless I hit a major snag.

Thanks,
Guenter

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