[Patch] Adding threshold support to coretemp

[Patch] Adding threshold support to coretemp

[R, Durgadoss]

[-- Attachment #1: Type: text/plain, Size: 8231 bytes --]

Hi Jean/Fenghua,

I am submitting a patch to enable core thermal threshold
Support to coretemp.c. There are two core thermal thresholds
available through sysfs interfaces temp1_core_thresh[0/1].

The expectation is that thresh0 is lesser than the current temperature
and thresh1 is higher than the current temperature. Whenever the current
temperature crosses these limits, an interrupt is generated.
This interrupt is handles by the user space to do power
Management via CPU throttling, etc..

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Tue, 14 Dec 2010 05:10:27 +0530
Subject: [PATCH] Adding_threshold_support_to_coretemp

This patch adds the core thermal threshold support to coretemp.c.
These thresholds can be read/written using the sysfs interface
temp1_core_thresh[0/1]. These can be used to generate interrupts,
to do dynamic power management.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/msr-index.h |   12 ++++
 drivers/hwmon/coretemp.c         |  133 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..31cefad 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_OFFSET_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_OFFSET_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 42de98d..4c221b8 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -42,6 +42,9 @@
 typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
 		SHOW_NAME } SHOW;
 
+/* C indicates core thermal thresholds */
+enum thresholds { C_TTHRESH0, C_TTHRESH1} THRESH;
+
 /*
  * Functions declaration
  */
@@ -59,9 +62,13 @@ struct coretemp_data {
 	int temp;
 	int tjmax;
 	int ttarget;
+	int c_tthresh0;
+	int c_tthresh1;
 	u8 alarm;
 };
 
+static int set_core_threshold(struct coretemp_data *data, int val,
+						enum thresholds thresh);
 /*
  * Sysfs stuff
  */
@@ -104,6 +111,41 @@ static ssize_t show_temp(struct device *dev,
 	return err;
 }
 
+static ssize_t show_threshold(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+
+	if (!data->valid)
+		return -EINVAL;
+
+	switch (attr->index) {
+	case C_TTHRESH0:
+		return sprintf(buf, "%d\n", data->c_tthresh0);
+	case C_TTHRESH1:
+		return sprintf(buf, "%d\n", data->c_tthresh1);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t set_threshold(struct device *dev,
+		struct device_attribute *devattr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+	unsigned long val;
+	int err;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	err = set_core_threshold(data, val, attr->index);
+
+	return (err) ? -EINVAL : count;
+}
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
 			  SHOW_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
@@ -114,12 +156,19 @@ static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
 static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
 static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
 
+static SENSOR_DEVICE_ATTR(temp1_core_thresh0, S_IWUSR | S_IRUGO,
+				show_threshold,	set_threshold, C_TTHRESH0);
+static SENSOR_DEVICE_ATTR(temp1_core_thresh1, S_IWUSR | S_IRUGO,
+				show_threshold, set_threshold, C_TTHRESH1);
+
 static struct attribute *coretemp_attributes[] = {
 	&sensor_dev_attr_name.dev_attr.attr,
 	&sensor_dev_attr_temp1_label.dev_attr.attr,
 	&dev_attr_temp1_crit_alarm.attr,
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh0.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh1.dev_attr.attr,
 	NULL
 };
 
@@ -298,6 +347,77 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
 	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
 }
 
+static void configure_apic(void *info)
+{
+	u32 l;
+	int *flag = (int *)info;
+
+	l = apic_read(APIC_LVTTHMR);
+
+	if (*flag)	/* Non-Zero flag Masks the APIC */
+		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
+	else		/* Zero flag UnMasks the APIC */
+		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
+}
+
+static int set_core_threshold(struct coretemp_data *data, int temp,
+						enum thresholds thresh)
+{
+	u32 eax, edx;
+	int diff;
+	int flag = 1;
+
+	if (temp > data->tjmax)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	diff = (data->tjmax - temp)/1000;
+
+	/* Mask the APIC */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	if (thresh == C_TTHRESH0) {
+		eax = (eax & ~THERM_MASK_THRESHOLD0) |
+					(diff << THERM_OFFSET_THRESHOLD0);
+		data->c_tthresh0 = temp;
+	} else {
+		eax = (eax & ~THERM_MASK_THRESHOLD1) |
+					(diff << THERM_OFFSET_THRESHOLD1);
+		data->c_tthresh1 = temp;
+	}
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	/* Unmask the APIC */
+	flag = 0;
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	mutex_unlock(&data->update_lock);
+	return 0;
+}
+
+static int __devinit enable_thresh_support(struct coretemp_data *data)
+{
+	u32 eax, edx;
+	int flag = 1; /* Non-Zero Flag masks the apic */
+
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	flag = 0; /*Flag should be zero to unmask the apic */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	return 0;
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -353,6 +473,15 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
 	platform_set_drvdata(pdev, data);
 
+	/* Enable threshold support */
+	enable_thresh_support(data);
+
+	/* Set Initial Core thresholds.
+	 * The lower and upper threshold values here are assumed
+	 */
+	set_core_threshold(data, 0, C_TTHRESH0);
+	set_core_threshold(data, 90000, C_TTHRESH1);
+
 	/*
 	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
 	 * on older CPUs but not in this register,
@@ -405,6 +534,10 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
 	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh0.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh1.dev_attr);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 	return 0;
-- 
1.6.5.2



[-- Attachment #2: 0001-Adding_threshold_support_to_coretemp.patch --]
[-- Type: application/octet-stream, Size: 7328 bytes --]

From: Durgadoss R <durgadoss.r@intel.com>
Date: Tue, 14 Dec 2010 05:10:27 +0530
Subject: [PATCH] Adding_threshold_support_to_coretemp

This patch adds the core thermal threshold support to coretemp.c.
These thresholds can be read/written using the sysfs interface
temp1_core_thresh[0/1]. These can be used to generate interrupts,
to do dynamic power management.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 arch/x86/include/asm/msr-index.h |   12 ++++
 drivers/hwmon/coretemp.c         |  133 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..31cefad 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_OFFSET_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_OFFSET_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 42de98d..4c221b8 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -42,6 +42,9 @@
 typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
 		SHOW_NAME } SHOW;
 
+/* C indicates core thermal thresholds */
+enum thresholds { C_TTHRESH0, C_TTHRESH1} THRESH;
+
 /*
  * Functions declaration
  */
@@ -59,9 +62,13 @@ struct coretemp_data {
 	int temp;
 	int tjmax;
 	int ttarget;
+	int c_tthresh0;
+	int c_tthresh1;
 	u8 alarm;
 };
 
+static int set_core_threshold(struct coretemp_data *data, int val,
+						enum thresholds thresh);
 /*
  * Sysfs stuff
  */
@@ -104,6 +111,41 @@ static ssize_t show_temp(struct device *dev,
 	return err;
 }
 
+static ssize_t show_threshold(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+
+	if (!data->valid)
+		return -EINVAL;
+
+	switch (attr->index) {
+	case C_TTHRESH0:
+		return sprintf(buf, "%d\n", data->c_tthresh0);
+	case C_TTHRESH1:
+		return sprintf(buf, "%d\n", data->c_tthresh1);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t set_threshold(struct device *dev,
+		struct device_attribute *devattr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+	unsigned long val;
+	int err;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	err = set_core_threshold(data, val, attr->index);
+
+	return (err) ? -EINVAL : count;
+}
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
 			  SHOW_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
@@ -114,12 +156,19 @@ static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
 static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
 static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
 
+static SENSOR_DEVICE_ATTR(temp1_core_thresh0, S_IWUSR | S_IRUGO,
+				show_threshold,	set_threshold, C_TTHRESH0);
+static SENSOR_DEVICE_ATTR(temp1_core_thresh1, S_IWUSR | S_IRUGO,
+				show_threshold, set_threshold, C_TTHRESH1);
+
 static struct attribute *coretemp_attributes[] = {
 	&sensor_dev_attr_name.dev_attr.attr,
 	&sensor_dev_attr_temp1_label.dev_attr.attr,
 	&dev_attr_temp1_crit_alarm.attr,
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh0.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh1.dev_attr.attr,
 	NULL
 };
 
@@ -298,6 +347,77 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
 	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
 }
 
+static void configure_apic(void *info)
+{
+	u32 l;
+	int *flag = (int *)info;
+
+	l = apic_read(APIC_LVTTHMR);
+
+	if (*flag)	/* Non-Zero flag Masks the APIC */
+		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
+	else		/* Zero flag UnMasks the APIC */
+		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
+}
+
+static int set_core_threshold(struct coretemp_data *data, int temp,
+						enum thresholds thresh)
+{
+	u32 eax, edx;
+	int diff;
+	int flag = 1;
+
+	if (temp > data->tjmax)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	diff = (data->tjmax - temp)/1000;
+
+	/* Mask the APIC */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	if (thresh == C_TTHRESH0) {
+		eax = (eax & ~THERM_MASK_THRESHOLD0) |
+					(diff << THERM_OFFSET_THRESHOLD0);
+		data->c_tthresh0 = temp;
+	} else {
+		eax = (eax & ~THERM_MASK_THRESHOLD1) |
+					(diff << THERM_OFFSET_THRESHOLD1);
+		data->c_tthresh1 = temp;
+	}
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	/* Unmask the APIC */
+	flag = 0;
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	mutex_unlock(&data->update_lock);
+	return 0;
+}
+
+static int __devinit enable_thresh_support(struct coretemp_data *data)
+{
+	u32 eax, edx;
+	int flag = 1; /* Non-Zero Flag masks the apic */
+
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	flag = 0; /*Flag should be zero to unmask the apic */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	return 0;
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -353,6 +473,15 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
 	platform_set_drvdata(pdev, data);
 
+	/* Enable threshold support */
+	enable_thresh_support(data);
+
+	/* Set Initial Core thresholds.
+	 * The lower and upper threshold values here are assumed
+	 */
+	set_core_threshold(data, 0, C_TTHRESH0);
+	set_core_threshold(data, 90000, C_TTHRESH1);
+
 	/*
 	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
 	 * on older CPUs but not in this register,
@@ -405,6 +534,10 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
 	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh0.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh1.dev_attr);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 	return 0;
-- 
1.6.5.2

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Tue, Dec 14, 2010 at 06:55:35AM -0500, R, Durgadoss wrote:
> Hi Jean/Fenghua,
> 
> I am submitting a patch to enable core thermal threshold
> Support to coretemp.c. There are two core thermal thresholds
> available through sysfs interfaces temp1_core_thresh[0/1].
> 
> The expectation is that thresh0 is lesser than the current temperature
> and thresh1 is higher than the current temperature. Whenever the current
> temperature crosses these limits, an interrupt is generated.
> This interrupt is handles by the user space to do power
> Management via CPU throttling, etc..
> 
> This patch is generated against stable Linux-2.6 kernel.
> 
> Kindly review and merge.
> ------------------------------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Tue, 14 Dec 2010 05:10:27 +0530
> Subject: [PATCH] Adding_threshold_support_to_coretemp
> 
> This patch adds the core thermal threshold support to coretemp.c.
> These thresholds can be read/written using the sysfs interface
> temp1_core_thresh[0/1]. These can be used to generate interrupts,
> to do dynamic power management.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
I am not inclined to look further into this patch until the ABI questions are resolved.
As written, the patch gets my NACK simply because it introduces random driver specific
new ABI attributes.

Guenter

RE: [lm-sensors] [Patch] Adding threshold support to coretemp

[R, Durgadoss]

Hi Guenter,

> > ------------------------------------------------------------------
> > From: Durgadoss R <durgadoss.r@intel.com>
> >
> > Date: Tue, 14 Dec 2010 05:10:27 +0530
> > Subject: [PATCH] Adding_threshold_support_to_coretemp
> >
> > This patch adds the core thermal threshold support to coretemp.c.
> > These thresholds can be read/written using the sysfs interface
> > temp1_core_thresh[0/1]. These can be used to generate interrupts,
> > to do dynamic power management.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >
> I am not inclined to look further into this patch until the ABI questions are
> resolved.
> As written, the patch gets my NACK simply because it introduces random driver
> specific
> new ABI attributes.
> 

Since the Hardware supports the programming of thresholds, I added this support
in the driver. I had to add new attributes, since there are no existing attributes,
that support these thresholds.
Could you please suggest any better way(s) of enabling this support ?
I can add documentation for these attributes in Doc/hwmon/sysfs-interface.

Thanks,
Durga

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Wed, Dec 15, 2010 at 07:29:15AM -0500, R, Durgadoss wrote:
> Hi Guenter,
> 
> > > ------------------------------------------------------------------
> > > From: Durgadoss R <durgadoss.r@intel.com>
> > >
> > > Date: Tue, 14 Dec 2010 05:10:27 +0530
> > > Subject: [PATCH] Adding_threshold_support_to_coretemp
> > >
> > > This patch adds the core thermal threshold support to coretemp.c.
> > > These thresholds can be read/written using the sysfs interface
> > > temp1_core_thresh[0/1]. These can be used to generate interrupts,
> > > to do dynamic power management.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > >
> > I am not inclined to look further into this patch until the ABI questions are
> > resolved.
> > As written, the patch gets my NACK simply because it introduces random driver
> > specific
> > new ABI attributes.
> > 
> 
> Since the Hardware supports the programming of thresholds, I added this support
> in the driver. I had to add new attributes, since there are no existing attributes,
> that support these thresholds.
> Could you please suggest any better way(s) of enabling this support ?
> I can add documentation for these attributes in Doc/hwmon/sysfs-interface.
> 
At least two.

1) Use existing ABI attributes. The coretemp driver doesn't use all available attributes
for low/high temperatures, so you could pick unused ones (and if necessary redefine the ones 
already used). We have
	lcrit
	min
	max
	crit
	emergency

All those attributes are ultimately thresholds; no reason not to use them.

Furthermore, the two thresholds are really upper/lower temperature targets which
result in requesting a specific action. As such, it is really just one (upper) threshold
with an associated hysteresis value to undo the action caused by the upper threshold.
So you could use
	tempX_max
	tempX_max_hyst
or
	tempX_crit
	tempX_crit_hyst
or
	tempX_emergency
	tempX_emergency_hyst
instead to reflect this meaning.

I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each other.
Maybe you could simply use tempX_max as upper threshold (where it exists),
with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use tempX_max_hyst
to calculate the lower threshold.

2) Define new attribute names which can be used as generic ABI, such as
	tempX_threshold
	tempX_threshold_hyst

Right now I don't really see the need for additional attributes, since there are spare ones
available, so 2) is less likely to be accepted. You would have to make a really good case
for it, and not just to me but convince Jean as well.

Thanks,
Guenter

RE: [lm-sensors] [Patch] Adding threshold support to coretemp

[R, Durgadoss]

Hi Guenter,

Thanks for the suggestions.
I need clarification on some of the things you mentioned here.
Kindly help.

> > Since the Hardware supports the programming of thresholds, I added this
> support
> > in the driver. I had to add new attributes, since there are no existing
> attributes,
> > that support these thresholds.
> > Could you please suggest any better way(s) of enabling this support ?
> > I can add documentation for these attributes in Doc/hwmon/sysfs-interface.
> >
> At least two.
> 
> 1) Use existing ABI attributes. The coretemp driver doesn't use all available
> attributes
> for low/high temperatures, so you could pick unused ones (and if necessary
> redefine the ones
> already used). We have
> 	lcrit
> 	min
> 	max
> 	crit
> 	emergency
> 
> All those attributes are ultimately thresholds; no reason not to use them.

These are thresholds only. But their corresponding documentation in sysfs-interface
reflects completely different meaning from what these thresholds do. When we program
these thresholds, we get an interrupt, if the current temperature goes below the
lower threshold (thresh0) or above the upper threshold (thresh1). As per the documentation,
none of these existing attributes generate interrupt like this. In fact, I had an
idea to use _max and _min. Then the meaning will be contradictory. Hence I refrained,
from using that.

> 
> Furthermore, the two thresholds are really upper/lower temperature targets
> which
> result in requesting a specific action. As such, it is really just one (upper)
> threshold
> with an associated hysteresis value to undo the action caused by the upper
> threshold.
> So you could use
> 	tempX_max
> 	tempX_max_hyst
> or
> 	tempX_crit
> 	tempX_crit_hyst
> or
> 	tempX_emergency
> 	tempX_emergency_hyst
> instead to reflect this meaning.
> 

Here also, the _max and _crit have already been used. Even if we redefine,
it will not adhere to the Documentation. Since the thresh0 and thresh1 both,
are lesser than _crit, I feel it's not really nice to use _emergency.
(But if that's the only way and we all are Ok with that, I can use it,
with a bit of change in Documentation..)

> I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each
> other.
> Maybe you could simply use tempX_max as upper threshold (where it exists),
> with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use
> tempX_max_hyst
> to calculate the lower threshold.

The _thresh value is lower than _max, which in turn is lower than _crit.
The newer generation of CPU's don't support the IA32_TEMP_TARGET register,
and hence the _max is almost never used. But since it has already been used,
as "ttarget", I can't use it for this upper threshold. Otherwise, I do not
have any problem in using _min and _max. Though, we have to modify the
documentation. I am proposing this idea, since you said "redefine" old ones if
necessary. Please let me know your opinion here.

> 
> 2) Define new attribute names which can be used as generic ABI, such as
> 	tempX_threshold
> 	tempX_threshold_hyst
> 
> Right now I don't really see the need for additional attributes, since there
> are spare ones
> available, so 2) is less likely to be accepted. You would have to make a really
> good case
> for it, and not just to me but convince Jean as well.

I understand there are available spare attributes; but seems like they have been
defined with some other meaning. I also wanted to refrain from introducing
new attributes as much as possible. But, because of the above-mentioned issues,
I had to arrive at this solution.

Thanks,
Durga

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Thu, Dec 16, 2010 at 05:48:58AM -0500, R, Durgadoss wrote:
[ ... ]
> 
> These are thresholds only. But their corresponding documentation in sysfs-interface
> reflects completely different meaning from what these thresholds do. When we program
> these thresholds, we get an interrupt, if the current temperature goes below the
> lower threshold (thresh0) or above the upper threshold (thresh1). As per the documentation,
> none of these existing attributes generate interrupt like this. In fact, I had an
> idea to use _max and _min. Then the meaning will be contradictory. Hence I refrained,
> from using that.
> 
temp[1-*]_min   Temperature min value.
temp[1-*]_max   Temperature max value.
temp[1-*]_max_hyst
                Temperature hysteresis value for max limit.

I don't see a limitation in use here. It only says min/max/hysteresis value, nor what
that means or implies, nor if there can an interrupt be associated with it.
I don't see anything contradictory.

> Here also, the _max and _crit have already been used. Even if we redefine,
> it will not adhere to the Documentation. Since the thresh0 and thresh1 both,

Again, why ? Where would it not adhere to the documentation ?

> are lesser than _crit, I feel it's not really nice to use _emergency.
> (But if that's the only way and we all are Ok with that, I can use it,
> with a bit of change in Documentation..)
> 
Not really.

> > I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each
> > other.
> > Maybe you could simply use tempX_max as upper threshold (where it exists),
> > with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use
> > tempX_max_hyst
> > to calculate the lower threshold.
> 
> The _thresh value is lower than _max, which in turn is lower than _crit.
> The newer generation of CPU's don't support the IA32_TEMP_TARGET register,
> and hence the _max is almost never used. But since it has already been used,
> as "ttarget", I can't use it for this upper threshold. Otherwise, I do not
> have any problem in using _min and _max. Though, we have to modify the
> documentation. I am proposing this idea, since you said "redefine" old ones if
> necessary. Please let me know your opinion here.
> 
As you state yourself, IA32_TEMP_TARGET is no longer supported, and it is only used
to set the value of _max (ie to instantiate ttarget). What is done with it subsequently
is another question. And if the CPUs supporting IA32_TEMP_TARGET don't support the
threshold registers, the question does not even arise.

> I understand there are available spare attributes; but seems like they have been
> defined with some other meaning. I also wanted to refrain from introducing
> new attributes as much as possible. But, because of the above-mentioned issues,
> I had to arrive at this solution.
> 
My recommendation would be to use _max as upper threshold, instantiate it with IA32_TEMP_TARGET
if supported, and make it writable. Then use _max_threshold for the lower threshold.
I don't see any problem with that. Then explain in Documentation/sysfs/coretemp what 
exactly happens if the limits are reached.

Guenter