| Message ID | 1500054535-975-3-git-send-email-pprakash@codeaurora.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Zhang Rui |
| Headers | show |
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; > } >
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
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
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
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
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
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
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 >
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
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
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
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; }
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(+)