Re: [PATCH] platform/x86: thinkpad_acpi: Add labels to the first 2 temperature sensors

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/13/21 11:06 AM, Andy Shevchenko wrote:
> 
> 
> On Tuesday, April 13, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> 
>     On EC version 3, the first 2 temperature sensors are always CPU and GPU
>     add labels for these.
> 
>     This changes e.g. the "sensors" command output on a X1C8 from:
> 
>     thinkpad-isa-0000
>     Adapter: ISA adapter
>     fan1:        2694 RPM
>     temp1:        +42.0°C
>     temp2:            N/A
>     temp3:        +33.0°C
>     temp4:         +0.0°C
>     temp5:        +35.0°C
>     temp6:        +42.0°C
>     temp7:        +42.0°C
>     temp8:            N/A
> 
>     into:
> 
>     thinkpad-isa-0000
>     Adapter: ISA adapter
>     fan1:        2694 RPM
>     CPU:          +42.0°C
>     GPU:              N/A
>     temp3:        +33.0°C
>     temp4:         +0.0°C
>     temp5:        +35.0°C
>     temp6:        +42.0°C
>     temp7:        +42.0°C
>     temp8:            N/A
> 
> 
> Is it an ABI change?

I don't think so, it adds 2 new sysfs files, all existing files keep working as is.

The _label attributes are part of the standard hwmon API and any tools
implementing that API will either ignore them (if they are ancient) or
use them to set a better default label then "temp1" and "temp2", while
still honoring any labels from config files with a higher priority then
the kernel provided ones.

> Any updates to the documentation? 
> 
> If the answer is yes, I would rather do one of the following approach: 
> 1) enable labels by user request (sysfs knob)
> 2) just add additional two lines
> 3) add an additional column for labels (or completely another file)
> 
> I have understand that there are cons of each of them. I dunno which one is the best. 

The answer is no :) So no need to worry about this

Regards,

Hans



> 
>  
> 
>     Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>     ---
>      drivers/platform/x86/thinkpad_acpi.c | 72 ++++++++++++++++++----------
>      1 file changed, 47 insertions(+), 25 deletions(-)
> 
>     diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>     index ec98089d98c9..dd60c9397d35 100644
>     --- a/drivers/platform/x86/thinkpad_acpi.c
>     +++ b/drivers/platform/x86/thinkpad_acpi.c
>     @@ -6296,6 +6296,8 @@ struct ibm_thermal_sensors_struct {
>      };
> 
>      static enum thermal_access_mode thermal_read_mode;
>     +static const struct attribute_group *thermal_attr_group;
>     +static bool thermal_use_labels;
> 
>      /* idx is zero-based */
>      static int thermal_get_sensor(int idx, s32 *value)
>     @@ -6478,6 +6480,28 @@ static const struct attribute_group thermal_temp_input8_group = {
>      #undef THERMAL_SENSOR_ATTR_TEMP
>      #undef THERMAL_ATTRS
> 
>     +static ssize_t temp1_label_show(struct device *dev, struct device_attribute *attr, char *buf)
>     +{
>     +       return sysfs_emit(buf, "CPU\n");
>     +}
>     +static DEVICE_ATTR_RO(temp1_label);
>     +
>     +static ssize_t temp2_label_show(struct device *dev, struct device_attribute *attr, char *buf)
>     +{
>     +       return sysfs_emit(buf, "GPU\n");
>     +}
>     +static DEVICE_ATTR_RO(temp2_label);
>     +
>     +static struct attribute *temp_label_attributes[] = {
>     +       &dev_attr_temp1_label.attr,
>     +       &dev_attr_temp2_label.attr,
>     +       NULL
>     +};
>     +
>     +static const struct attribute_group temp_label_attr_group = {
>     +       .attrs = temp_label_attributes,
>     +};
>     +
>      /* --------------------------------------------------------------------- */
> 
>      static int __init thermal_init(struct ibm_init_struct *iibm)
>     @@ -6533,12 +6557,14 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>                                     thermal_read_mode = TPACPI_THERMAL_NONE;
>                             }
>                     } else {
>     -                       if (ver >= 3)
>     +                       if (ver >= 3) {
>                                     thermal_read_mode = TPACPI_THERMAL_TPEC_8;
>     -                       else
>     +                               thermal_use_labels = true;
>     +                       } else {
>                                     thermal_read_mode =
>                                             (ta2 != 0) ?
>                                             TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
>     +                       }
>                     }
>             } else if (acpi_tmp7) {
>                     if (tpacpi_is_ibm() &&
>     @@ -6560,44 +6586,40 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
> 
>             switch (thermal_read_mode) {
>             case TPACPI_THERMAL_TPEC_16:
>     -               res = sysfs_create_group(&tpacpi_hwmon->kobj,
>     -                               &thermal_temp_input16_group);
>     -               if (res)
>     -                       return res;
>     +               thermal_attr_group = &thermal_temp_input16_group;
>                     break;
>             case TPACPI_THERMAL_TPEC_8:
>             case TPACPI_THERMAL_ACPI_TMP07:
>             case TPACPI_THERMAL_ACPI_UPDT:
>     -               res = sysfs_create_group(&tpacpi_hwmon->kobj,
>     -                               &thermal_temp_input8_group);
>     -               if (res)
>     -                       return res;
>     +               thermal_attr_group = &thermal_temp_input8_group;
>                     break;
>             case TPACPI_THERMAL_NONE:
>             default:
>                     return 1;
>             }
> 
>     +       res = sysfs_create_group(&tpacpi_hwmon->kobj, thermal_attr_group);
>     +       if (res)
>     +               return res;
>     +
>     +       if (thermal_use_labels) {
>     +               res = sysfs_create_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
>     +               if (res) {
>     +                       sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
>     +                       return res;
>     +               }
>     +       }
>     +
>             return 0;
>      }
> 
>      static void thermal_exit(void)
>      {
>     -       switch (thermal_read_mode) {
>     -       case TPACPI_THERMAL_TPEC_16:
>     -               sysfs_remove_group(&tpacpi_hwmon->kobj,
>     -                                  &thermal_temp_input16_group);
>     -               break;
>     -       case TPACPI_THERMAL_TPEC_8:
>     -       case TPACPI_THERMAL_ACPI_TMP07:
>     -       case TPACPI_THERMAL_ACPI_UPDT:
>     -               sysfs_remove_group(&tpacpi_hwmon->kobj,
>     -                                  &thermal_temp_input8_group);
>     -               break;
>     -       case TPACPI_THERMAL_NONE:
>     -       default:
>     -               break;
>     -       }
>     +       if (thermal_attr_group)
>     +               sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
>     +
>     +       if (thermal_use_labels)
>     +               sysfs_remove_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
>      }
> 
>      static int thermal_read(struct seq_file *m)
>     -- 
>     2.31.1
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>