Thermal zone names

Thermal zone names

[Jean Delvare]

Hi Rui,

The ACPI thermal zones in /proc/acpi/thermal_zone have a name. The ACPI
thermal zones in /sys/class/thermal do not. Would it be possible to add
it there? When these thermal zones are exported to libsensors, the
temperatures appear with their default labels (temp1, temp2, etc...)
Users sometimes wonder what these temperatures correspond to. The ACPI
names are short and not always clear, but sometimes they could help
figuring out what the temperature corresponds to.

-- 
Jean Delvare

Re: Thermal zone names

[Zhang Rui]

On Thu, 2008-08-21 at 04:34 +0800, Jean Delvare wrote:
> Hi Rui,
> 
> The ACPI thermal zones in /proc/acpi/thermal_zone have a name. The
> ACPI
> thermal zones in /sys/class/thermal do not.
First, the name used in procfs doesn't make sense.
It just uses the arbitrary stings exported by BIOS. Some of them is
meaningless, and even there may be duplicate names.
The only benefit is that we can easily figure out which device in the
ACPI namespace this interface is for.

Second, /sys/class/thermal/thermal_zoneX/device is the symbol link to
the real device node, and there is a sysfs I/F named "path" which can be
used for the same purpose (find corresponding devices in ACPI namespace)

so it's okay that the ACPI thermal zones in /sys/class/thermal doesn't
have a name.

>  Would it be possible to add
> it there? When these thermal zones are exported to libsensors, the
> temperatures appear with their default labels (temp1, temp2, etc...)
> Users sometimes wonder what these temperatures correspond to.
Do you mean making use of "temp[1-*]_label"?
As we already know the BIOS name (procfs name) of an ACPI thermal sysfs
class device, we can export the name of thermal sysfs class device in
"temp[1-*]_label", something like "thermal_zoneX".
what do you think?

thanks,
rui

--
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: Thermal zone names

[Jean Delvare]

Hi Rui,

On Thu, 21 Aug 2008 09:36:20 +0800, Zhang Rui wrote:
> On Thu, 2008-08-21 at 04:34 +0800, Jean Delvare wrote:
> > Hi Rui,
> > 
> > The ACPI thermal zones in /proc/acpi/thermal_zone have a name. The
> > ACPI
> > thermal zones in /sys/class/thermal do not.
> First, the name used in procfs doesn't make sense.
> It just uses the arbitrary stings exported by BIOS. Some of them is
> meaningless, and even there may be duplicate names.

I thought that these names were unique identifiers for ACPI?

> The only benefit is that we can easily figure out which device in the
> ACPI namespace this interface is for.
> 
> Second, /sys/class/thermal/thermal_zoneX/device is the symbol link to
> the real device node, and there is a sysfs I/F named "path" which can be
> used for the same purpose (find corresponding devices in ACPI namespace)
> 
> so it's okay that the ACPI thermal zones in /sys/class/thermal doesn't
> have a name.

For lm-sensors users, the fact that the information is available
somewhere in sysfs isn't too interesting. Either the name is used as
the default label, or it's not.

> >  Would it be possible to add
> > it there? When these thermal zones are exported to libsensors, the
> > temperatures appear with their default labels (temp1, temp2, etc...)
> > Users sometimes wonder what these temperatures correspond to.
> Do you mean making use of "temp[1-*]_label"?

Yes, exactly.

> As we already know the BIOS name (procfs name) of an ACPI thermal sysfs
> class device, we can export the name of thermal sysfs class device in
> "temp[1-*]_label", something like "thermal_zoneX".
> what do you think?

The name "thermal_zoneX" is not useful at all for the user. It's no
more informative than "tempX". What I wanted to print was the ACPI
name. Now you say that it's not meaningful. In that case there's simply
nothing we can do, sorry for the noise.

Thanks,
-- 
Jean Delvare
--
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: Thermal zone names

[Thomas Renninger]

On Thursday 21 August 2008 05:48:52 pm Jean Delvare wrote:
> Hi Rui,
>
> On Thu, 21 Aug 2008 09:36:20 +0800, Zhang Rui wrote:
> > On Thu, 2008-08-21 at 04:34 +0800, Jean Delvare wrote:
> > > Hi Rui,
> > >
> > > The ACPI thermal zones in /proc/acpi/thermal_zone have a name. The
> > > ACPI
> > > thermal zones in /sys/class/thermal do not.
> >
> > First, the name used in procfs doesn't make sense.
> > It just uses the arbitrary stings exported by BIOS. Some of them is
> > meaningless, and even there may be duplicate names.
>
> I thought that these names were unique identifiers for ACPI?
Theoretically no: You can have the same device name in different scopes.
e.g. XYZ.THERM and ZYX.THERM
while THERM could always be a thermal device.

In practice they must not be identical or we have duplicate names in:
/proc/acpi/thermal_zone/*
The only place where we really saw duplicate entries of the same device type
was for video devices. But this showed the broken design of /proc/acpi which
cannot happen for /sys/bus/acpi/devices/* anymore.

But Zhang is right, the assumption in /proc/ that the names are always unique
was wrong.
>
> > The only benefit is that we can easily figure out which device in the
> > ACPI namespace this interface is for.
> >
> > Second, /sys/class/thermal/thermal_zoneX/device is the symbol link to
> > the real device node, and there is a sysfs I/F named "path" which can be
> > used for the same purpose (find corresponding devices in ACPI namespace)
> >
> > so it's okay that the ACPI thermal zones in /sys/class/thermal doesn't
> > have a name.
>
> For lm-sensors users, the fact that the information is available
> somewhere in sysfs isn't too interesting. Either the name is used as
> the default label, or it's not.
I found a function inside a thermal zone on an HP which seem to be the name.
It is not mentioned in the spec, but we could document that and tell vendors 
to use it. Hmm, it should start with "_" if this should get specified at 
later time?

I post a little RFC patch which makes use of this one as a hwmon thermal 
device/zone name.
Totally untested...

        Thomas

[RFC PATCH] Read name/location of thermal zone from ACPI function and pass it to hwmon

[Thomas Renninger]

The first patch is enabling a acpi_evaluate_string helper function.

Both patches are for discussion only and beside the fact that
they should compile they are untested.

If you think this works, please give it a review and I can
rework the patch and ask the guy with this BIOS/machine to give
it a test.

      Thomas

[PATCH 1/2] patch acpi_introduce_evaluate_string.patch

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/utils.c    |   11 +++++------
 include/acpi/acpi_bus.h |    4 ++++
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 1009261..250d7f1 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -295,15 +295,14 @@ acpi_evaluate_integer(acpi_handle handle,
 
 EXPORT_SYMBOL(acpi_evaluate_integer);
 
-#if 0
 acpi_status
 acpi_evaluate_string(acpi_handle handle,
 		     acpi_string pathname,
-		     acpi_object_list * arguments, acpi_string * data)
+		     struct acpi_object_list *arguments, acpi_string *data)
 {
 	acpi_status status = AE_OK;
-	acpi_object *element = NULL;
-	acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *element = NULL;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
 
 	if (!data)
@@ -315,7 +314,7 @@ acpi_evaluate_string(acpi_handle handle,
 		return status;
 	}
 
-	element = (acpi_object *) buffer.pointer;
+	element = (union acpi_object *) buffer.pointer;
 
 	if ((element->type != ACPI_TYPE_STRING)
 	    || (element->type != ACPI_TYPE_BUFFER)
@@ -338,7 +337,7 @@ acpi_evaluate_string(acpi_handle handle,
 
 	return AE_OK;
 }
-#endif
+EXPORT_SYMBOL(acpi_evaluate_string);
 
 acpi_status
 acpi_evaluate_reference(acpi_handle handle,
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a5ac0bc..08a191f 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -48,6 +48,10 @@ acpi_evaluate_integer(acpi_handle handle,
 		      acpi_string pathname,
 		      struct acpi_object_list *arguments, unsigned long *data);
 acpi_status
+acpi_evaluate_string(acpi_handle handle,
+		     acpi_string pathname,
+		     struct acpi_object_list *arguments, acpi_string *data);
+acpi_status
 acpi_evaluate_reference(acpi_handle handle,
 			acpi_string pathname,
 			struct acpi_object_list *arguments,
-- 
1.5.4.5

Re: [PATCH 1/2] patch acpi_introduce_evaluate_string.patch

[Zhao Yakui]

On Mon, 2008-08-25 at 00:11 +0200, Thomas Renninger wrote:
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> ---
>  drivers/acpi/utils.c    |   11 +++++------
>  include/acpi/acpi_bus.h |    4 ++++
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 1009261..250d7f1 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -295,15 +295,14 @@ acpi_evaluate_integer(acpi_handle handle,
>  
>  EXPORT_SYMBOL(acpi_evaluate_integer);
>  
It seems reasonable. 
But the returned buffer object will be appropriate. IMO it is
unnecessary to add the function of acpi_evaluate_string.

Thanks.



> -#if 0
>  acpi_status
>  acpi_evaluate_string(acpi_handle handle,
>  		     acpi_string pathname,
> -		     acpi_object_list * arguments, acpi_string * data)
> +		     struct acpi_object_list *arguments, acpi_string *data)
>  {
>  	acpi_status status = AE_OK;
> -	acpi_object *element = NULL;
> -	acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *element = NULL;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  
> 
>  	if (!data)
> @@ -315,7 +314,7 @@ acpi_evaluate_string(acpi_handle handle,
>  		return status;
>  	}
>  
> -	element = (acpi_object *) buffer.pointer;
> +	element = (union acpi_object *) buffer.pointer;
>  
>  	if ((element->type != ACPI_TYPE_STRING)
>  	    || (element->type != ACPI_TYPE_BUFFER)
> @@ -338,7 +337,7 @@ acpi_evaluate_string(acpi_handle handle,
>  
>  	return AE_OK;
>  }
> -#endif
> +EXPORT_SYMBOL(acpi_evaluate_string);
>  
>  acpi_status
>  acpi_evaluate_reference(acpi_handle handle,
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index a5ac0bc..08a191f 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -48,6 +48,10 @@ acpi_evaluate_integer(acpi_handle handle,
>  		      acpi_string pathname,
>  		      struct acpi_object_list *arguments, unsigned long *data);
>  acpi_status
> +acpi_evaluate_string(acpi_handle handle,
> +		     acpi_string pathname,
> +		     struct acpi_object_list *arguments, acpi_string *data);
> +acpi_status
>  acpi_evaluate_reference(acpi_handle handle,
>  			acpi_string pathname,
>  			struct acpi_object_list *arguments,

[PATCH 2/2] Give ACPI hwmon thermal devices a name if BIOS provides one

[Thomas Renninger]

On a HP tx2500z laptop one thermal device provides this function:
            Name (REGN, "Processor Thermal Zone")

Evaluate it and pass it as a name to hwmon.

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/thermal.c        |   15 ++++++++++++++-
 drivers/thermal/thermal_sys.c |   11 +++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 9127036..8095e9e 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -1600,11 +1600,24 @@ static void acpi_thermal_notify(acpi_handle handle, u32 event, void *data)
 static int acpi_thermal_get_info(struct acpi_thermal *tz)
 {
 	int result = 0;
-
+	char *name = NULL;
+	acpi_status status;
 
 	if (!tz)
 		return -EINVAL;
 
+	/* Get optional name of the thermal zone */
+	status = acpi_evaluate_string(tz->device->handle, "REGN", NULL,
+				      &name);
+	if (ACPI_SUCCESS(status)){
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Thermal zone %s has name: "
+				  "%s\n", acpi_device_bid(tz->device), name));
+		/* This is hard coded limited to 16, why? */
+		strncpy(tz->thermal_zone->temp_input.name, name, 16);
+		kfree(name);
+		tz->thermal_zone->temp_input.name[15] = '\n';
+	}
+
 	/* Get temperature [_TMP] (required) */
 	result = acpi_thermal_get_temperature(tz);
 	if (result)
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index fe07462..51a7077 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -371,8 +371,15 @@ thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 	tz->hwmon = hwmon;
 	hwmon->count++;
 
-	snprintf(tz->temp_input.name, THERMAL_NAME_LENGTH,
-		 "temp%d_input", hwmon->count);
+	/*
+	 * What happens if we get duplicate zone names from the BIOS?
+	 * Is that allowed? If not, should we still just add the
+	 * hwmon count at the end?
+	 * tz->temp_input.name is char[16], this is ugly to do...
+	 */
+	if (!tz->temp_input.name)
+		snprintf(tz->temp_input.name, THERMAL_NAME_LENGTH,
+			 "temp%d_input", hwmon->count);
 	tz->temp_input.attr.attr.name = tz->temp_input.name;
 	tz->temp_input.attr.attr.mode = 0444;
 	tz->temp_input.attr.show = temp_input_show;
-- 
1.5.4.5

Re: [PATCH 2/2] Give ACPI hwmon thermal devices a name if BIOS provides one

[Thomas Renninger]

On Monday 25 August 2008 12:11:37 am Thomas Renninger wrote:
> On a HP tx2500z laptop one thermal device provides this function:
>             Name (REGN, "Processor Thermal Zone")

I wonder what we can do to get this added to the ACPI spec or to make
vendors use the same function for this.

What about trying for above "REGN" as it already exists on HPs.
Also try for _NAM (or similar), document that we do this and tell vendors
that it's a good thing to provide a sane string/name for thermal_zones on 
Linux.
On ACPI everything that does not start with "_" means unspecified
and the function should only be used internally and not by the OS...

     Thomas

Re: [PATCH 2/2] Give ACPI hwmon thermal devices a name if BIOS provides one

[Zhang Rui]

On Mon, 2008-08-25 at 06:20 +0800, Thomas Renninger wrote:
> On Monday 25 August 2008 12:11:37 am Thomas Renninger wrote:
> > On a HP tx2500z laptop one thermal device provides this function:
> >             Name (REGN, "Processor Thermal Zone")
> 
> I wonder what we can do to get this added to the ACPI spec or to make
> vendors use the same function for this.

> What about trying for above "REGN" as it already exists on HPs.
> Also try for _NAM (or similar), document that we do this and tell
> vendors that it's a good thing to provide a sane string/name for
> thermal_zones on Linux.

> On ACPI everything that does not start with "_" means unspecified
> and the function should only be used internally and not by the OS...

Sounds good. But I guess it's not easy to push this the vendors, is it?

thanks,
rui

Re: [PATCH 2/2] Give ACPI hwmon thermal devices a name if BIOS provides one

[Thomas Renninger]

On Monday 25 August 2008 04:15:02 Zhang Rui wrote:
> On Mon, 2008-08-25 at 06:20 +0800, Thomas Renninger wrote:
> > On Monday 25 August 2008 12:11:37 am Thomas Renninger wrote:
> > > On a HP tx2500z laptop one thermal device provides this function:
> > >             Name (REGN, "Processor Thermal Zone")
> >
> > I wonder what we can do to get this added to the ACPI spec or to make
> > vendors use the same function for this.
> >
> > What about trying for above "REGN" as it already exists on HPs.
> > Also try for _NAM (or similar), document that we do this and tell
> > vendors that it's a good thing to provide a sane string/name for
> > thermal_zones on Linux.
> >
> > On ACPI everything that does not start with "_" means unspecified
> > and the function should only be used internally and not by the OS...
>
> Sounds good. But I guess it's not easy to push this the vendors, is it?
At least this is a really easy thing for them to add.

I wonder how localization should work here. One does not want to add
the description of the thermal zone in all possibly supported languages
into BIOS ROM.

Pre-defining 10 different areas:
  0 Processor
  1 Memory
  2 Disk
  3 Graphics card
  4 ...
 10 Others

and then returning an integer value sounds more sane.
If Len or Bob say it makes sense to come up with or add something which
we can tell the vendors to use and which may find its way to a later Spec
version...

After first being excited that HP provided a sane description of a thermal
zone, I think it's not worth adding this right now (or is it?).

      Thomas

PS: Forgot to add lm-sensors list, adding them now.

Re: [PATCH 2/2] Give ACPI hwmon thermal devices a name if BIOS provides one

[Zhang Rui]

On Mon, 2008-08-25 at 06:11 +0800, Thomas Renninger wrote:
> On a HP tx2500z laptop one thermal device provides this function:
>             Name (REGN, "Processor Thermal Zone")
> 
> Evaluate it and pass it as a name to hwmon.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> ---
>  drivers/acpi/thermal.c        |   15 ++++++++++++++-
>  drivers/thermal/thermal_sys.c |   11 +++++++++--
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 9127036..8095e9e 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -1600,11 +1600,24 @@ static void acpi_thermal_notify(acpi_handle
> handle, u32 event, void *data)
>  static int acpi_thermal_get_info(struct acpi_thermal *tz)
>  {
>         int result = 0;
> -
> +       char *name = NULL;
> +       acpi_status status;
> 
>         if (!tz)
>                 return -EINVAL;
> 
> +       /* Get optional name of the thermal zone */
> +       status = acpi_evaluate_string(tz->device->handle, "REGN",
> NULL,
> +                                     &name);

As this is not defined in the ACPI spec, I don't know if it's
appropriate to implement this in the generic code.

> +       if (ACPI_SUCCESS(status)){
> +               ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Thermal zone %s has
> name: "
> +                                 "%s\n", acpi_device_bid(tz->device),
> name));
> +               /* This is hard coded limited to 16, why? */
> +               strncpy(tz->thermal_zone->temp_input.name, name, 16);
> +               kfree(name);
> +               tz->thermal_zone->temp_input.name[15] = '\n';
> +       }
> +
>         /* Get temperature [_TMP] (required) */
>         result = acpi_thermal_get_temperature(tz);
>         if (result)
> diff --git a/drivers/thermal/thermal_sys.c
> b/drivers/thermal/thermal_sys.c
> index fe07462..51a7077 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -371,8 +371,15 @@ thermal_add_hwmon_sysfs(struct
> thermal_zone_device *tz)
>         tz->hwmon = hwmon;
>         hwmon->count++;
> 
> -       snprintf(tz->temp_input.name, THERMAL_NAME_LENGTH,
> -                "temp%d_input", hwmon->count);
> +       /*
> +        * What happens if we get duplicate zone names from the BIOS?
> +        * Is that allowed? If not, should we still just add the
> +        * hwmon count at the end?
> +        * tz->temp_input.name is char[16], this is ugly to do...
> +        */
> +       if (!tz->temp_input.name)
> +               snprintf(tz->temp_input.name, THERMAL_NAME_LENGTH,
> +                        "temp%d_input", hwmon->count);
we should create a new attribute here rather than replacing the old one.
the new hwmon sysfs I/F should be like:
temp1_input:	current temperature
temp1_crit:	critical threshold
temp1_label:	string like "Processor Thermal Zone"

thanks,
rui
>         tz->temp_input.attr.attr.name = tz->temp_input.name;
>         tz->temp_input.attr.attr.mode = 0444;
>         tz->temp_input.attr.show = temp_input_show;
> --
> 1.5.4.5
> 
> 
> 

--
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] Give ACPI hwmon thermal devices a name if BIOS provides one

[Jean Delvare]

Hi Thomas,

On Mon, 25 Aug 2008 09:58:16 +0800, Zhang Rui wrote:
> On Mon, 2008-08-25 at 06:11 +0800, Thomas Renninger wrote:
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index fe07462..51a7077 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -371,8 +371,15 @@ thermal_add_hwmon_sysfs(struct
> > thermal_zone_device *tz)
> >         tz->hwmon = hwmon;
> >         hwmon->count++;
> > 
> > -       snprintf(tz->temp_input.name, THERMAL_NAME_LENGTH,
> > -                "temp%d_input", hwmon->count);
> > +       /*
> > +        * What happens if we get duplicate zone names from the BIOS?
> > +        * Is that allowed? If not, should we still just add the
> > +        * hwmon count at the end?
> > +        * tz->temp_input.name is char[16], this is ugly to do...
> > +        */
> > +       if (!tz->temp_input.name)
> > +               snprintf(tz->temp_input.name, THERMAL_NAME_LENGTH,
> > +                        "temp%d_input", hwmon->count);
>
> we should create a new attribute here rather than replacing the old one.
> the new hwmon sysfs I/F should be like:
> temp1_input:	current temperature
> temp1_crit:	critical threshold
> temp1_label:	string like "Processor Thermal Zone"

Rui is correct here, you don't want to use the string as a file name
(what your patch is currently doing) but provide it to libsensors
(amongst others) as the contents of a separate file (tempN_label.)
libsensors needs these standard file names, otherwise it can't guess
which is what.

-- 
Jean Delvare
--
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