[PATCH 0/2] User-friendly description for thermal zones

[PATCH 0/2] User-friendly description for thermal zones

[Prashanth Prakash]

ACPI 6.2 spec allows a _STR(string) object to be defined under a thermal
zone optionally, which return a string name/description for that thermal
zone. See section 11.4 of ACPI spec 6.2 for more details.

This series adds support to read the _STR object under a thermal zone and
expose it to the userspace via sysfs interface.


Prashanth Prakash (2):
  thermal: add a sysfs entry for thermal zone description
  ACPI/thermal: support for thermal zone description

 Documentation/thermal/sysfs-api.txt |  6 ++++++
 drivers/acpi/thermal.c              | 38 +++++++++++++++++++++++++++++++++++++
 drivers/thermal/thermal_sysfs.c     | 19 +++++++++++++++++++
 include/linux/thermal.h             |  4 ++++
 4 files changed, 67 insertions(+)

-- 
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] thermal: add a sysfs entry for thermal zone description

[Prashanth Prakash]

Add a method to thermal_zone_device_ops to read the description of a
thermal zone. Use the same method to expose the description to the
userspace via a sysfs entry.

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 Documentation/thermal/sysfs-api.txt |  6 ++++++
 drivers/thermal/thermal_sysfs.c     | 19 +++++++++++++++++++
 include/linux/thermal.h             |  4 ++++
 3 files changed, 29 insertions(+)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index bb9a0a5..b6aa417 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -62,6 +62,7 @@ temperature) and throttle appropriate devices.
 			will be fired.
 	.set_emul_temp: set the emulation temperature which helps in debugging
 			different threshold temperature points.
+	.get_desc: get a user friendly description for a thermal zone (optional)
     tzp: thermal zone platform parameters.
     passive_delay: number of milliseconds to wait between polls when
 	performing passive cooling.
@@ -280,6 +281,7 @@ Thermal zone device sys I/F, created once it's registered:
     |---integral_cutoff:        Offset above which errors are accumulated
     |---slope:                  Slope constant applied as linear extrapolation
     |---offset:                 Offset constant applied as linear extrapolation
+    |---desc			User friendly description/name
 
 Thermal cooling device sys I/F, created once it's registered:
 /sys/class/thermal/cooling_device[0-*]:
@@ -329,6 +331,10 @@ temp
 	Unit: millidegree Celsius
 	RO, Required
 
+desc
+	Provides a user friendly description/name for thermal zone
+        RO, Optional
+
 mode
 	One of the predefined values in [enabled, disabled].
 	This file gives information about the algorithm that is currently
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index a694de9..46d961d 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -34,6 +34,23 @@
 }
 
 static ssize_t
+desc_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	int ret;
+	char desc_str[THERMAL_MAX_DESC_STR_LEN];
+
+	if (!tz->ops->get_desc)
+		return scnprintf(buf, PAGE_SIZE, "<not supported>\n");
+
+	ret = tz->ops->get_desc(tz, desc_str, THERMAL_MAX_DESC_STR_LEN);
+	if (ret)
+		return ret;
+
+	return scnprintf(buf, THERMAL_MAX_DESC_STR_LEN, "%s\n", desc_str);
+}
+
+static ssize_t
 temp_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
@@ -397,6 +414,7 @@
  * present on the sysfs interface.
  */
 static DEVICE_ATTR(type, 0444, type_show, NULL);
+static DEVICE_ATTR(desc, 0444, desc_show, NULL);
 static DEVICE_ATTR(temp, 0444, temp_show, NULL);
 static DEVICE_ATTR(policy, S_IRUGO | S_IWUSR, policy_show, policy_store);
 static DEVICE_ATTR(available_policies, S_IRUGO, available_policies_show, NULL);
@@ -411,6 +429,7 @@ static DEVICE_ATTR(sustainable_power, S_IWUSR | S_IRUGO, sustainable_power_show,
 static struct attribute *thermal_zone_dev_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_temp.attr,
+	&dev_attr_desc.attr,
 #if (IS_ENABLED(CONFIG_THERMAL_EMULATION))
 	&dev_attr_emul_temp.attr,
 #endif
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index dab11f9..bd8377a 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -47,6 +47,9 @@
 /* use value, which < 0K, to indicate an invalid/uninitialized temperature */
 #define THERMAL_TEMP_INVALID	-274000
 
+/* Maximum length of thermal zone description string */
+#define THERMAL_MAX_DESC_STR_LEN 32
+
 /* Unit conversion macros */
 #define DECI_KELVIN_TO_CELSIUS(t)	({			\
 	long _t = (t);						\
@@ -127,6 +130,7 @@ struct thermal_zone_device_ops {
 			  enum thermal_trend *);
 	int (*notify) (struct thermal_zone_device *, int,
 		       enum thermal_trip_type);
+	int (*get_desc) (struct thermal_zone_device *, char *, int);
 };
 
 struct thermal_cooling_device_ops {
-- 
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] ACPI/thermal: support for thermal zone description

[Prashanth Prakash]

Per ACPI 6.2 spec, platforms can optionally add a string(_STR)
object within each thermal zone package which provides a user
friendly name/description.

Add support to parse the string object, which will be exposed
to userspace by thermal framework.

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 drivers/acpi/thermal.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 1d0417b..6ab6480 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -41,6 +41,7 @@
 #include <linux/acpi.h>
 #include <linux/workqueue.h>
 #include <linux/uaccess.h>
+#include <linux/nls.h>
 
 #define PREFIX "ACPI: "
 
@@ -188,6 +189,7 @@ struct acpi_thermal {
 	int tz_enabled;
 	int kelvin_offset;
 	struct work_struct thermal_check_work;
+	char desc[THERMAL_MAX_DESC_STR_LEN];
 };
 
 /* --------------------------------------------------------------------------
@@ -543,6 +545,15 @@ static int thermal_get_temp(struct thermal_zone_device *thermal, int *temp)
 	return 0;
 }
 
+static int thermal_get_desc(struct thermal_zone_device *thermal, char *desc,
+			int size)
+{
+	struct acpi_thermal *tz = thermal->devdata;
+
+	strlcpy(desc, tz->desc, size);
+	return 0;
+}
+
 static int thermal_get_mode(struct thermal_zone_device *thermal,
 				enum thermal_device_mode *mode)
 {
@@ -880,6 +891,7 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
 	.get_crit_temp = thermal_get_crit_temp,
 	.get_trend = thermal_get_trend,
 	.notify = thermal_notify,
+	.get_desc = thermal_get_desc,
 };
 
 static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
@@ -1014,6 +1026,29 @@ static void acpi_thermal_aml_dependency_fix(struct acpi_thermal *tz)
 	acpi_evaluate_integer(handle, "_TMP", NULL, &value);
 }
 
+static void acpi_thermal_get_desc(struct acpi_thermal *tz)
+{
+	acpi_handle handle = tz->device->handle;
+	acpi_status status;
+	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+
+	status = acpi_evaluate_object(handle, "_STR", NULL, &buffer);
+
+	if (ACPI_FAILURE(status))
+		strlcpy(tz->desc, "<not supported>", THERMAL_MAX_DESC_STR_LEN);
+	else {
+		union acpi_object *str;
+		int result;
+
+		str = buffer.pointer;
+		result = utf16s_to_utf8s((wchar_t *)str->string.pointer,
+					str->string.length, UTF16_LITTLE_ENDIAN,
+					tz->desc, THERMAL_MAX_DESC_STR_LEN-1);
+		tz->desc[result] = 0;
+		kfree(buffer.pointer);
+	}
+}
+
 static int acpi_thermal_get_info(struct acpi_thermal *tz)
 {
 	int result = 0;
@@ -1045,6 +1080,9 @@ static int acpi_thermal_get_info(struct acpi_thermal *tz)
 	else
 		acpi_thermal_get_polling_frequency(tz);
 
+	/* Get thermal zone description [_STR] (optional) */
+	acpi_thermal_get_desc(tz);
+
 	return 0;
 }
 
-- 
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Zhang Rui]

On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
> Per ACPI 6.2 spec, platforms can optionally add a string(_STR)
> object within each thermal zone package which provides a user
> friendly name/description.
> 
> Add support to parse the string object, which will be exposed
> to userspace by thermal framework.
> 

is there any real request for this?

_STR is a generic control method for all the ACPI devices.
Thus I'm wondering, if really needed, should we expose this in acpi bus
instead?

thanks,
rui

> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> ---
>  drivers/acpi/thermal.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 1d0417b..6ab6480 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -41,6 +41,7 @@
>  #include <linux/acpi.h>
>  #include <linux/workqueue.h>
>  #include <linux/uaccess.h>
> +#include <linux/nls.h>
>  
>  #define PREFIX "ACPI: "
>  
> @@ -188,6 +189,7 @@ struct acpi_thermal {
>  	int tz_enabled;
>  	int kelvin_offset;
>  	struct work_struct thermal_check_work;
> +	char desc[THERMAL_MAX_DESC_STR_LEN];
>  };
>  
>  /* ---------------------------------------------------------------
> -----------
> @@ -543,6 +545,15 @@ static int thermal_get_temp(struct
> thermal_zone_device *thermal, int *temp)
>  	return 0;
>  }
>  
> +static int thermal_get_desc(struct thermal_zone_device *thermal,
> char *desc,
> +			int size)
> +{
> +	struct acpi_thermal *tz = thermal->devdata;
> +
> +	strlcpy(desc, tz->desc, size);
> +	return 0;
> +}
> +
>  static int thermal_get_mode(struct thermal_zone_device *thermal,
>  				enum thermal_device_mode *mode)
>  {
> @@ -880,6 +891,7 @@ static int acpi_thermal_cooling_device_cb(struct
> thermal_zone_device *thermal,
>  	.get_crit_temp = thermal_get_crit_temp,
>  	.get_trend = thermal_get_trend,
>  	.notify = thermal_notify,
> +	.get_desc = thermal_get_desc,
>  };
>  
>  static int acpi_thermal_register_thermal_zone(struct acpi_thermal
> *tz)
> @@ -1014,6 +1026,29 @@ static void
> acpi_thermal_aml_dependency_fix(struct acpi_thermal *tz)
>  	acpi_evaluate_integer(handle, "_TMP", NULL, &value);
>  }
>  
> +static void acpi_thermal_get_desc(struct acpi_thermal *tz)
> +{
> +	acpi_handle handle = tz->device->handle;
> +	acpi_status status;
> +	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> +
> +	status = acpi_evaluate_object(handle, "_STR", NULL,
> &buffer);
> +
> +	if (ACPI_FAILURE(status))
> +		strlcpy(tz->desc, "<not supported>",
> THERMAL_MAX_DESC_STR_LEN);
> +	else {
> +		union acpi_object *str;
> +		int result;
> +
> +		str = buffer.pointer;
> +		result = utf16s_to_utf8s((wchar_t *)str-
> >string.pointer,
> +					str->string.length,
> UTF16_LITTLE_ENDIAN,
> +					tz->desc,
> THERMAL_MAX_DESC_STR_LEN-1);
> +		tz->desc[result] = 0;
> +		kfree(buffer.pointer);
> +	}
> +}
> +
>  static int acpi_thermal_get_info(struct acpi_thermal *tz)
>  {
>  	int result = 0;
> @@ -1045,6 +1080,9 @@ static int acpi_thermal_get_info(struct
> acpi_thermal *tz)
>  	else
>  		acpi_thermal_get_polling_frequency(tz);
>  
> +	/* Get thermal zone description [_STR] (optional) */
> +	acpi_thermal_get_desc(tz);
> +
>  	return 0;
>  }
>  

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Prakash, Prashanth]

Hi Rui,

On 8/8/2017 2:23 AM, Zhang Rui wrote:
> On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
>> Per ACPI 6.2 spec, platforms can optionally add a string(_STR)
>> object within each thermal zone package which provides a user
>> friendly name/description.
>>
>> Add support to parse the string object, which will be exposed
>> to userspace by thermal framework.
>>
> is there any real request for this?

Yes, Qualcomm server platforms adds these description strings.
> _STR is a generic control method for all the ACPI devices.
> Thus I'm wondering, if really needed, should we expose this in acpi bus
> instead?

AFAIK, adding a _STR to any package was not explicitly allowed by the spec.
Updates in APCI 6.2 made it legal to add an _STR object to thermal zone
specifically, so added this support only to thermal zone.

Thanks,
Prashanth

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Zhang Rui]

Hi, Prakash,

On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
> Hi Rui,
> 
> On 8/8/2017 2:23 AM, Zhang Rui wrote:
> > 
> > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
> > > 
> > > Per ACPI 6.2 spec, platforms can optionally add a string(_STR)
> > > object within each thermal zone package which provides a user
> > > friendly name/description.
> > > 
> > > Add support to parse the string object, which will be exposed
> > > to userspace by thermal framework.
> > > 
> > is there any real request for this?
> Yes, Qualcomm server platforms adds these description strings.
> > 
> > _STR is a generic control method for all the ACPI devices.
> > Thus I'm wondering, if really needed, should we expose this in acpi
> > bus
> > instead?
> AFAIK, adding a _STR to any package was not explicitly allowed by the
> spec.
> Updates in APCI 6.2 made it legal to add an _STR object to thermal
> zone
> specifically, so added this support only to thermal zone.
> 

I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2, but
according to section 6.1.10, "The _STR object evaluates to a Unicode
string that describes the device or thermal zone. "
_STR is still a generic control method that can exist in any other
device scope.

so to me, this is a optional but generic feature for all the ACPI
devices, and we don't have a solid reason that it should be part of
thermal sysfs I/F, thus a better solution to me is to expose this as an
attribute of ACPI device, and we can link to the ACPI device from
thermal sysfs I/F in userspace.

what do you think, Rafael?

thanks,
rui
> Thanks,
> Prashanth
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Rafael J. Wysocki]

On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> Hi, Prakash,
>
> On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
>> Hi Rui,
>>
>> On 8/8/2017 2:23 AM, Zhang Rui wrote:
>> >
>> > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
>> > >
>> > > Per ACPI 6.2 spec, platforms can optionally add a string(_STR)
>> > > object within each thermal zone package which provides a user
>> > > friendly name/description.
>> > >
>> > > Add support to parse the string object, which will be exposed
>> > > to userspace by thermal framework.
>> > >
>> > is there any real request for this?
>> Yes, Qualcomm server platforms adds these description strings.
>> >
>> > _STR is a generic control method for all the ACPI devices.
>> > Thus I'm wondering, if really needed, should we expose this in acpi
>> > bus
>> > instead?
>> AFAIK, adding a _STR to any package was not explicitly allowed by the
>> spec.
>> Updates in APCI 6.2 made it legal to add an _STR object to thermal
>> zone
>> specifically, so added this support only to thermal zone.
>>
>
> I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2, but
> according to section 6.1.10, "The _STR object evaluates to a Unicode
> string that describes the device or thermal zone. "
> _STR is still a generic control method that can exist in any other
> device scope.
>
> so to me, this is a optional but generic feature for all the ACPI
> devices, and we don't have a solid reason that it should be part of
> thermal sysfs I/F, thus a better solution to me is to expose this as an
> attribute of ACPI device, and we can link to the ACPI device from
> thermal sysfs I/F in userspace.
>
> what do you think, Rafael?

Since you have a ->get_desc method, I don't have a big problem with
the approach here.

I'm not particularly liking the "<not supported>" thing returned if
_STR is not present, though.

Thanks,
Rafael

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Zhang Rui]

On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
> On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
> wrote:
> > 
> > Hi, Prakash,
> > 
> > On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
> > > 
> > > Hi Rui,
> > > 
> > > On 8/8/2017 2:23 AM, Zhang Rui wrote:
> > > > 
> > > > 
> > > > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
> > > > > 
> > > > > 
> > > > > Per ACPI 6.2 spec, platforms can optionally add a
> > > > > string(_STR)
> > > > > object within each thermal zone package which provides a user
> > > > > friendly name/description.
> > > > > 
> > > > > Add support to parse the string object, which will be exposed
> > > > > to userspace by thermal framework.
> > > > > 
> > > > is there any real request for this?
> > > Yes, Qualcomm server platforms adds these description strings.
> > > > 
> > > > 
> > > > _STR is a generic control method for all the ACPI devices.
> > > > Thus I'm wondering, if really needed, should we expose this in
> > > > acpi
> > > > bus
> > > > instead?
> > > AFAIK, adding a _STR to any package was not explicitly allowed by
> > > the
> > > spec.
> > > Updates in APCI 6.2 made it legal to add an _STR object to
> > > thermal
> > > zone
> > > specifically, so added this support only to thermal zone.
> > > 
> > I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2, but
> > according to section 6.1.10, "The _STR object evaluates to a
> > Unicode
> > string that describes the device or thermal zone. "
> > _STR is still a generic control method that can exist in any other
> > device scope.
> > 
> > so to me, this is a optional but generic feature for all the ACPI
> > devices, and we don't have a solid reason that it should be part of
> > thermal sysfs I/F, thus a better solution to me is to expose this
> > as an
> > attribute of ACPI device, and we can link to the ACPI device from
> > thermal sysfs I/F in userspace.
> > 
> > what do you think, Rafael?
> Since you have a ->get_desc method, I don't have a big problem with
> the approach here.
> 
> I'm not particularly liking the "<not supported>" thing returned if
> _STR is not present, though.

No, actually I mean adding a new sysfs attribute under ACPI device
node, just like path/hid/status/adr, etc.

Of course the attribute should be optional, depends on if the _STR
control methods exist or not.

thanks,
rui
> 
> Thanks,
> Rafael

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Rafael J. Wysocki]

On Friday, August 18, 2017 4:14:47 AM CEST Zhang Rui wrote:
> On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
> > On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > > 
> > > Hi, Prakash,
> > > 
> > > On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
> > > > 
> > > > Hi Rui,
> > > > 
> > > > On 8/8/2017 2:23 AM, Zhang Rui wrote:
> > > > > 
> > > > > 
> > > > > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
> > > > > > 
> > > > > > 
> > > > > > Per ACPI 6.2 spec, platforms can optionally add a
> > > > > > string(_STR)
> > > > > > object within each thermal zone package which provides a user
> > > > > > friendly name/description.
> > > > > > 
> > > > > > Add support to parse the string object, which will be exposed
> > > > > > to userspace by thermal framework.
> > > > > > 
> > > > > is there any real request for this?
> > > > Yes, Qualcomm server platforms adds these description strings.
> > > > > 
> > > > > 
> > > > > _STR is a generic control method for all the ACPI devices.
> > > > > Thus I'm wondering, if really needed, should we expose this in
> > > > > acpi
> > > > > bus
> > > > > instead?
> > > > AFAIK, adding a _STR to any package was not explicitly allowed by
> > > > the
> > > > spec.
> > > > Updates in APCI 6.2 made it legal to add an _STR object to
> > > > thermal
> > > > zone
> > > > specifically, so added this support only to thermal zone.
> > > > 
> > > I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2, but
> > > according to section 6.1.10, "The _STR object evaluates to a
> > > Unicode
> > > string that describes the device or thermal zone. "
> > > _STR is still a generic control method that can exist in any other
> > > device scope.
> > > 
> > > so to me, this is a optional but generic feature for all the ACPI
> > > devices, and we don't have a solid reason that it should be part of
> > > thermal sysfs I/F, thus a better solution to me is to expose this
> > > as an
> > > attribute of ACPI device, and we can link to the ACPI device from
> > > thermal sysfs I/F in userspace.
> > > 
> > > what do you think, Rafael?
> > Since you have a ->get_desc method, I don't have a big problem with
> > the approach here.
> > 
> > I'm not particularly liking the "<not supported>" thing returned if
> > _STR is not present, though.
> 
> No, actually I mean adding a new sysfs attribute under ACPI device
> node, just like path/hid/status/adr, etc.

Yes, I understood that, but since the power supply framework has a
description callback, why not to use it really?

> Of course the attribute should be optional, depends on if the _STR
> control methods exist or not.

So what's the exact benefit from doing this over the approach the $subject
patch?

Thanks,
Rafael

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Zhang Rui]

On Fri, 2017-08-18 at 14:34 +0200, Rafael J. Wysocki wrote:
> On Friday, August 18, 2017 4:14:47 AM CEST Zhang Rui wrote:
> > 
> > On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
> > > 
> > > On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > 
> > > > Hi, Prakash,
> > > > 
> > > > On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
> > > > > 
> > > > > 
> > > > > Hi Rui,
> > > > > 
> > > > > On 8/8/2017 2:23 AM, Zhang Rui wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Per ACPI 6.2 spec, platforms can optionally add a
> > > > > > > string(_STR)
> > > > > > > object within each thermal zone package which provides a
> > > > > > > user
> > > > > > > friendly name/description.
> > > > > > > 
> > > > > > > Add support to parse the string object, which will be
> > > > > > > exposed
> > > > > > > to userspace by thermal framework.
> > > > > > > 
> > > > > > is there any real request for this?
> > > > > Yes, Qualcomm server platforms adds these description
> > > > > strings.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > _STR is a generic control method for all the ACPI devices.
> > > > > > Thus I'm wondering, if really needed, should we expose this
> > > > > > in
> > > > > > acpi
> > > > > > bus
> > > > > > instead?
> > > > > AFAIK, adding a _STR to any package was not explicitly
> > > > > allowed by
> > > > > the
> > > > > spec.
> > > > > Updates in APCI 6.2 made it legal to add an _STR object to
> > > > > thermal
> > > > > zone
> > > > > specifically, so added this support only to thermal zone.
> > > > > 
> > > > I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2,
> > > > but
> > > > according to section 6.1.10, "The _STR object evaluates to a
> > > > Unicode
> > > > string that describes the device or thermal zone. "
> > > > _STR is still a generic control method that can exist in any
> > > > other
> > > > device scope.
> > > > 
> > > > so to me, this is a optional but generic feature for all the
> > > > ACPI
> > > > devices, and we don't have a solid reason that it should be
> > > > part of
> > > > thermal sysfs I/F, thus a better solution to me is to expose
> > > > this
> > > > as an
> > > > attribute of ACPI device, and we can link to the ACPI device
> > > > from
> > > > thermal sysfs I/F in userspace.
> > > > 
> > > > what do you think, Rafael?
> > > Since you have a ->get_desc method, I don't have a big problem
> > > with
> > > the approach here.
> > > 
> > > I'm not particularly liking the "<not supported>" thing returned
> > > if
> > > _STR is not present, though.
> > No, actually I mean adding a new sysfs attribute under ACPI device
> > node, just like path/hid/status/adr, etc.
> Yes, I understood that, but since the power supply framework has a
> description callback, why not to use it really?
> 
I just checked the code, and ACPI devices indeed have 'description'
sysfs attribute if there is _STR. Cool, that's what I'm proposing.

But your statement, "the power supply framework has a description
callback" still confuses me.

> > 
> > Of course the attribute should be optional, depends on if the _STR
> > control methods exist or not.
> So what's the exact benefit from doing this over the approach the
> $subject
> patch?

hmmm, the subject patch introduces kernel code to get _STR content from
ACPI and then export it to userspace via a new thermal sysfs attribute.

And what I'm proposing is that, if the content of _STR is available
under ACPI device node, then we can easily get the information from
thermal sysfs I/F using symbol links, thus we don't need kernel changes
or new thermal sysfs attribute.

thanks,
rui
> 
> Thanks,
> Rafael
> 

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 4:28 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> On Fri, 2017-08-18 at 14:34 +0200, Rafael J. Wysocki wrote:
>> On Friday, August 18, 2017 4:14:47 AM CEST Zhang Rui wrote:
>> >
>> > On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
>> > >
>> > > On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
>> > > wrote:
>> > > >
>> > > >
>> > > > Hi, Prakash,
>> > > >
>> > > > On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
>> > > > >
>> > > > >
>> > > > > Hi Rui,
>> > > > >
>> > > > > On 8/8/2017 2:23 AM, Zhang Rui wrote:
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > Per ACPI 6.2 spec, platforms can optionally add a
>> > > > > > > string(_STR)
>> > > > > > > object within each thermal zone package which provides a
>> > > > > > > user
>> > > > > > > friendly name/description.
>> > > > > > >
>> > > > > > > Add support to parse the string object, which will be
>> > > > > > > exposed
>> > > > > > > to userspace by thermal framework.
>> > > > > > >
>> > > > > > is there any real request for this?
>> > > > > Yes, Qualcomm server platforms adds these description
>> > > > > strings.
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > _STR is a generic control method for all the ACPI devices.
>> > > > > > Thus I'm wondering, if really needed, should we expose this
>> > > > > > in
>> > > > > > acpi
>> > > > > > bus
>> > > > > > instead?
>> > > > > AFAIK, adding a _STR to any package was not explicitly
>> > > > > allowed by
>> > > > > the
>> > > > > spec.
>> > > > > Updates in APCI 6.2 made it legal to add an _STR object to
>> > > > > thermal
>> > > > > zone
>> > > > > specifically, so added this support only to thermal zone.
>> > > > >
>> > > > I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2,
>> > > > but
>> > > > according to section 6.1.10, "The _STR object evaluates to a
>> > > > Unicode
>> > > > string that describes the device or thermal zone. "
>> > > > _STR is still a generic control method that can exist in any
>> > > > other
>> > > > device scope.
>> > > >
>> > > > so to me, this is a optional but generic feature for all the
>> > > > ACPI
>> > > > devices, and we don't have a solid reason that it should be
>> > > > part of
>> > > > thermal sysfs I/F, thus a better solution to me is to expose
>> > > > this
>> > > > as an
>> > > > attribute of ACPI device, and we can link to the ACPI device
>> > > > from
>> > > > thermal sysfs I/F in userspace.
>> > > >
>> > > > what do you think, Rafael?
>> > > Since you have a ->get_desc method, I don't have a big problem
>> > > with
>> > > the approach here.
>> > >
>> > > I'm not particularly liking the "<not supported>" thing returned
>> > > if
>> > > _STR is not present, though.
>> > No, actually I mean adding a new sysfs attribute under ACPI device
>> > node, just like path/hid/status/adr, etc.
>> Yes, I understood that, but since the power supply framework has a
>> description callback, why not to use it really?
>>
> I just checked the code, and ACPI devices indeed have 'description'
> sysfs attribute if there is _STR. Cool, that's what I'm proposing.

OK

> But your statement, "the power supply framework has a description
> callback" still confuses me.

That's because I was confused, sorry about that.

But as you said, since we have the description thing already, it
should be sufficient for the use case at hand.

Thanks,
Rafael

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Prakash, Prashanth]

Hi Rafael/Rui,

On 8/17/2017 8:14 PM, Zhang Rui wrote:
> On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
>> On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
>> wrote:
>>> Hi, Prakash,
>>>
>>> On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
>>>> Hi Rui,
>>>>
>>>> On 8/8/2017 2:23 AM, Zhang Rui wrote:
>>>>>
>>>>> On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
>>>>>>
>>>>>> Per ACPI 6.2 spec, platforms can optionally add a
>>>>>> string(_STR)
>>>>>> object within each thermal zone package which provides a user
>>>>>> friendly name/description.
>>>>>>
>>>>>> Add support to parse the string object, which will be exposed
>>>>>> to userspace by thermal framework.
>>>>>>
>>>>> is there any real request for this?
>>>> Yes, Qualcomm server platforms adds these description strings.
>>>>>
>>>>> _STR is a generic control method for all the ACPI devices.
>>>>> Thus I'm wondering, if really needed, should we expose this in
>>>>> acpi
>>>>> bus
>>>>> instead?
>>>> AFAIK, adding a _STR to any package was not explicitly allowed by
>>>> the
>>>> spec.
>>>> Updates in APCI 6.2 made it legal to add an _STR object to
>>>> thermal
>>>> zone
>>>> specifically, so added this support only to thermal zone.
>>>>
>>> I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2, but
>>> according to section 6.1.10, "The _STR object evaluates to a
>>> Unicode
>>> string that describes the device or thermal zone. "
>>> _STR is still a generic control method that can exist in any other
>>> device scope.
>>>
>>> so to me, this is a optional but generic feature for all the ACPI
>>> devices, and we don't have a solid reason that it should be part of
>>> thermal sysfs I/F, thus a better solution to me is to expose this
>>> as an
>>> attribute of ACPI device, and we can link to the ACPI device from
>>> thermal sysfs I/F in userspace.
>>>
>>> what do you think, Rafael?
>> Since you have a ->get_desc method, I don't have a big problem with
>> the approach here.
>>
>> I'm not particularly liking the "<not supported>" thing returned if
>> _STR is not present, though.
I will change the implementation such that if _STR object was not found then
thermal_get_desc would return -ENXIO (or should it be different errno?).

> No, actually I mean adding a new sysfs attribute under ACPI device
> node, just like path/hid/status/adr, etc.
Sorry Rui, I didn't read your earlier comment correctly. Thermal zone's _STR is
useful in couple of scenarios(even if ACPI device containing the thermal_zone
had a _STR object):
- When we have more than 1 thermal sensors/zones under a device then this will
allow us to differentiate them
- When we have some thermal sensors that doesn't have ACPI device associated
with it. For example: a shared L3 cache, an abstract region on SoC. In these cases
we can add a thermal zone object in an appropriate place in dsdt and the
associated _STR will allow us to provide a user friendly name/description.
> Of course the attribute should be optional, depends on if the _STR
> control methods exist or not.
>
> thanks,
> rui
>> Thanks,
>> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Thanks for your feedback!

--
Regards,
Prashanth

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Zhang Rui]

On Fri, 2017-08-18 at 16:31 -0600, Prakash, Prashanth wrote:
> Hi Rafael/Rui,
> 
> On 8/17/2017 8:14 PM, Zhang Rui wrote:
> > 
> > On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
> > > 
> > > On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > Hi, Prakash,
> > > > 
> > > > On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
> > > > > 
> > > > > Hi Rui,
> > > > > 
> > > > > On 8/8/2017 2:23 AM, Zhang Rui wrote:
> > > > > > 
> > > > > > 
> > > > > > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > Per ACPI 6.2 spec, platforms can optionally add a
> > > > > > > string(_STR)
> > > > > > > object within each thermal zone package which provides a
> > > > > > > user
> > > > > > > friendly name/description.
> > > > > > > 
> > > > > > > Add support to parse the string object, which will be
> > > > > > > exposed
> > > > > > > to userspace by thermal framework.
> > > > > > > 
> > > > > > is there any real request for this?
> > > > > Yes, Qualcomm server platforms adds these description
> > > > > strings.
> > > > > > 
> > > > > > 
> > > > > > _STR is a generic control method for all the ACPI devices.
> > > > > > Thus I'm wondering, if really needed, should we expose this
> > > > > > in
> > > > > > acpi
> > > > > > bus
> > > > > > instead?
> > > > > AFAIK, adding a _STR to any package was not explicitly
> > > > > allowed by
> > > > > the
> > > > > spec.
> > > > > Updates in APCI 6.2 made it legal to add an _STR object to
> > > > > thermal
> > > > > zone
> > > > > specifically, so added this support only to thermal zone.
> > > > > 
> > > > I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2,
> > > > but
> > > > according to section 6.1.10, "The _STR object evaluates to a
> > > > Unicode
> > > > string that describes the device or thermal zone. "
> > > > _STR is still a generic control method that can exist in any
> > > > other
> > > > device scope.
> > > > 
> > > > so to me, this is a optional but generic feature for all the
> > > > ACPI
> > > > devices, and we don't have a solid reason that it should be
> > > > part of
> > > > thermal sysfs I/F, thus a better solution to me is to expose
> > > > this
> > > > as an
> > > > attribute of ACPI device, and we can link to the ACPI device
> > > > from
> > > > thermal sysfs I/F in userspace.
> > > > 
> > > > what do you think, Rafael?
> > > Since you have a ->get_desc method, I don't have a big problem
> > > with
> > > the approach here.
> > > 
> > > I'm not particularly liking the "<not supported>" thing returned
> > > if
> > > _STR is not present, though.
> I will change the implementation such that if _STR object was not
> found then
> thermal_get_desc would return -ENXIO (or should it be different
> errno?).
> 
> > 
> > No, actually I mean adding a new sysfs attribute under ACPI device
> > node, just like path/hid/status/adr, etc.
> Sorry Rui, I didn't read your earlier comment correctly. Thermal
> zone's _STR is
> useful in couple of scenarios(even if ACPI device containing the
> thermal_zone
> had a _STR object):
> - When we have more than 1 thermal sensors/zones under a device then
> this will
> allow us to differentiate them

Yes I agree.
>From userspace point of view,
with you patch, userspace can get the content of _STR by
cat /sys/class/thermal/thermal_zoneX/desc

And what I mean is that, userspace can already get the same information
by
cat /sys/class/thermal/thermal_zoneX/device/description
even without the patch.


> - When we have some thermal sensors that doesn't have ACPI device
> associated
> with it. For example: a shared L3 cache, an abstract region on SoC.
> In these cases
> we can add a thermal zone object in an appropriate place in dsdt and
> the
> associated _STR will allow us to provide a user friendly
> name/description.

if the sensor is registered by native driver, I think .get_desc() is
useful.
But if you want to hack the dsdt to get it enumerated via ACPI, then my
approach still works without the patch. :)

thanks,
rui
> > 
> > Of course the attribute should be optional, depends on if the _STR
> > control methods exist or not.
> > 
> > thanks,
> > rui
> > > 
> > > Thanks,
> > > Rafael
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> Thanks for your feedback!
> 
> --
> Regards,
> Prashanth
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Prakash, Prashanth]

Hi Rui,

On 8/21/2017 8:37 AM, Zhang Rui wrote:
> On Fri, 2017-08-18 at 16:31 -0600, Prakash, Prashanth wrote:
>> Hi Rafael/Rui,
>>
>> On 8/17/2017 8:14 PM, Zhang Rui wrote:
>>> On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
>>>> On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
>>>> wrote:
>>>>> Hi, Prakash,
>>>>>
>>>>> On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
>>>>>> Hi Rui,
>>>>>>
>>>>>> On 8/8/2017 2:23 AM, Zhang Rui wrote:
>>>>>>>
>>>>>>> On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
>>>>>>>>
>>>>>>>> Per ACPI 6.2 spec, platforms can optionally add a
>>>>>>>> string(_STR)
>>>>>>>> object within each thermal zone package which provides a
>>>>>>>> user
>>>>>>>> friendly name/description.
>>>>>>>>
>>>>>>>> Add support to parse the string object, which will be
>>>>>>>> exposed
>>>>>>>> to userspace by thermal framework.
>>>>>>>>
>>>>>>> is there any real request for this?
>>>>>> Yes, Qualcomm server platforms adds these description
>>>>>> strings.
>>>>>>>
>>>>>>> _STR is a generic control method for all the ACPI devices.
>>>>>>> Thus I'm wondering, if really needed, should we expose this
>>>>>>> in
>>>>>>> acpi
>>>>>>> bus
>>>>>>> instead?
>>>>>> AFAIK, adding a _STR to any package was not explicitly
>>>>>> allowed by
>>>>>> the
>>>>>> spec.
>>>>>> Updates in APCI 6.2 made it legal to add an _STR object to
>>>>>> thermal
>>>>>> zone
>>>>>> specifically, so added this support only to thermal zone.
>>>>>>
>>>>> I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2,
>>>>> but
>>>>> according to section 6.1.10, "The _STR object evaluates to a
>>>>> Unicode
>>>>> string that describes the device or thermal zone. "
>>>>> _STR is still a generic control method that can exist in any
>>>>> other
>>>>> device scope.
>>>>>
>>>>> so to me, this is a optional but generic feature for all the
>>>>> ACPI
>>>>> devices, and we don't have a solid reason that it should be
>>>>> part of
>>>>> thermal sysfs I/F, thus a better solution to me is to expose
>>>>> this
>>>>> as an
>>>>> attribute of ACPI device, and we can link to the ACPI device
>>>>> from
>>>>> thermal sysfs I/F in userspace.
>>>>>
>>>>> what do you think, Rafael?
>>>> Since you have a ->get_desc method, I don't have a big problem
>>>> with
>>>> the approach here.
>>>>
>>>> I'm not particularly liking the "<not supported>" thing returned
>>>> if
>>>> _STR is not present, though.
>> I will change the implementation such that if _STR object was not
>> found then
>> thermal_get_desc would return -ENXIO (or should it be different
>> errno?).
>>
>>> No, actually I mean adding a new sysfs attribute under ACPI device
>>> node, just like path/hid/status/adr, etc.
>> Sorry Rui, I didn't read your earlier comment correctly. Thermal
>> zone's _STR is
>> useful in couple of scenarios(even if ACPI device containing the
>> thermal_zone
>> had a _STR object):
>> - When we have more than 1 thermal sensors/zones under a device then
>> this will
>> allow us to differentiate them
> Yes I agree.
> From userspace point of view,
> with you patch, userspace can get the content of _STR by
> cat /sys/class/thermal/thermal_zoneX/desc
>
> And what I mean is that, userspace can already get the same information
> by
> cat /sys/class/thermal/thermal_zoneX/device/description
> even without the patch.
>
>
>> - When we have some thermal sensors that doesn't have ACPI device
>> associated
>> with it. For example: a shared L3 cache, an abstract region on SoC.
>> In these cases
>> we can add a thermal zone object in an appropriate place in dsdt and
>> the
>> associated _STR will allow us to provide a user friendly
>> name/description.
> if the sensor is registered by native driver, I think .get_desc() is
> useful.
> But if you want to hack the dsdt to get it enumerated via ACPI, then my
> approach still works without the patch. :)

Yes, it works :) and agree we should drop these patch.
Sorry, I should have checked more thoroughly before posting.

Thanks for the feedback :)

--
Regards,
Prashanth