Re: [PATCH v2 7/7] thermal: export thermal_zone_parameters to sysfs

Eduardo Valentin
2015-02-26T23:45:32Z

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

On Thu, Feb 26, 2015 at 07:00:33PM +0000, Javi Merino wrote:
> It's useful for tuning to be able to edit thermal_zone_parameters from
> userspace. Export them to the thermal_zone sysfs so that they can be
> easily changed.
>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> Documentation/thermal/sysfs-api.txt | 52 +++++++++++++++++
> drivers/thermal/thermal_core.c | 110 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 162 insertions(+)
>
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index fc7dfe10778b..7d44d7f1a71b 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -184,6 +184,12 @@ Thermal zone device sys I/F, created once it's registered:
> |---trip_point_[0-*]_type: Trip point type
> |---trip_point_[0-*]_hyst: Hysteresis value for this trip point
> |---emul_temp: Emulated temperature set node
> + |---sustainable_power: Sustainable dissipatable power
> + |---k_po: Proportional term during temperature overshoot
> + |---k_pu: Proportional term during temperature undershoot
> + |---k_i: PID's integral term in the power allocator gov
> + |---k_d: PID's derivative term in the power allocator
> + |---integral_cutoff: Offset above which errors are accumulated

Can this be under a specific directory?

I thought of something like
/sys/class/thermal/thermal_zoneX/governor_params/<gov spec files>

The above node can be handled by the governor code.

>
> Thermal cooling device sys I/F, created once it's registered:
> /sys/class/thermal/cooling_device[0-*]:
> @@ -307,6 +313,52 @@ emul_temp
> because userland can easily disable the thermal policy by simply
> flooding this sysfs node with low temperature values.
>
> +sustainable_power
> + An estimate of the sustained power that can be dissipated by
> + the thermal zone. Used by the power allocator governor. For
> + more information see Documentation/thermal/power_allocator.txt
> + Unit: milliwatts
> + RW, Optional
> +
> +k_po
> + The proportional term of the power allocator governor's PID
> + controller during temperature overshoot. Temperature overshoot
> + is when the current temperature is above the "desired
> + temperature" trip point. For more information see
> + Documentation/thermal/power_allocator.txt
> + RW, Optional
> +
> +k_pu
> + The proportional term of the power allocator governor's PID
> + controller during temperature undershoot. Temperature undershoot
> + is when the current temperature is below the "desired
> + temperature" trip point. For more information see
> + Documentation/thermal/power_allocator.txt
> + RW, Optional
> +
> +k_i
> + The integral term of the power allocator governor's PID
> + controller. This term allows the PID controller to compensate
> + for long term drift. For more information see
> + Documentation/thermal/power_allocator.txt
> + RW, Optional
> +
> +k_d
> + The derivative term of the power allocator governor's PID
> + controller. For more information see
> + Documentation/thermal/power_allocator.txt
> + RW, Optional
> +
> +integral_cutoff
> + Temperature offset from the desired temperature trip point
> + above which the integral term of the power allocator
> + governor's PID controller starts accumulating errors. For
> + example, if integral_cutoff is 0, then the integral term only
> + accumulates error when temperature is above the desired
> + temperature trip point. For more information see
> + Documentation/thermal/power_allocator.txt
> + RW, Optional
> +
> *****************************
> * Cooling device attributes *
> *****************************
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index df9ba3bf55dc..228e93f8a146 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -873,6 +873,111 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
> static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
> #endif/*CONFIG_THERMAL_EMULATION*/
>
> +#ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
> +
> +static ssize_t
> +sustainable_power_show(struct device *dev, struct device_attribute *devattr,
> + char *buf)
> +{
> + struct thermal_zone_device *tz = to_thermal_zone(dev);
> +
> + if (tz->tzp)
> + return sprintf(buf, "%u\n", tz->tzp->sustainable_power);
> + else
> + return -EIO;
> +}
> +
> +static ssize_t
> +sustainable_power_store(struct device *dev, struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct thermal_zone_device *tz = to_thermal_zone(dev);
> + u32 sustainable_power;
> +
> + if (!tz->tzp)
> + return -EIO;
> +
> + if (kstrtou32(buf, 10, &sustainable_power))
> + return -EINVAL;
> +
> + tz->tzp->sustainable_power = sustainable_power;
> +
> + return count;
> +}
> +static DEVICE_ATTR(sustainable_power, S_IWUSR | S_IRUGO, sustainable_power_show,
> + sustainable_power_store);
> +
> +#define create_s32_tzp_attr(name) \
> + static ssize_t \
> + name##_show(struct device *dev, struct device_attribute *devattr, \
> + char *buf) \
> + { \
> + struct thermal_zone_device *tz = to_thermal_zone(dev); \
> + \
> + if (tz->tzp) \
> + return sprintf(buf, "%u\n", tz->tzp->name); \
> + else \
> + return -EIO; \
> + } \
> + \
> + static ssize_t \
> + name##_store(struct device *dev, struct device_attribute *devattr, \
> + const char *buf, size_t count) \
> + { \
> + struct thermal_zone_device *tz = to_thermal_zone(dev); \
> + s32 value; \
> + \
> + if (!tz->tzp) \
> + return -EIO; \
> + \
> + if (kstrtos32(buf, 10, &value)) \
> + return -EINVAL; \
> + \
> + tz->tzp->name = value; \
> + \
> + return count; \
> + } \
> + static DEVICE_ATTR(name, S_IWUSR | S_IRUGO, name##_show, name##_store)
> +
> +create_s32_tzp_attr(k_po);
> +create_s32_tzp_attr(k_pu);
> +create_s32_tzp_attr(k_i);
> +create_s32_tzp_attr(k_d);
> +create_s32_tzp_attr(integral_cutoff);
> +#undef create_s32_tzp_attr
> +
> +static struct device_attribute *dev_tzp_attrs[] = {
> + &dev_attr_sustainable_power,
> + &dev_attr_k_po,
> + &dev_attr_k_pu,
> + &dev_attr_k_i,
> + &dev_attr_k_d,
> + &dev_attr_integral_cutoff,
> +};
> +
> +static int create_power_allocator_tzp_attrs(struct device *dev)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(dev_tzp_attrs); i++) {
> + int ret;
> + struct device_attribute *dev_attr = dev_tzp_attrs[i];
> +
> + ret = device_create_file(dev, dev_attr);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +#else /* !CONFIG_THERMAL_GOV_POWER_ALLOCATOR */
> +static int create_power_allocator_tzp_attrs(struct device *dev)
> +{
> + return 0;
> +}
> +#endif
> +

It is better if this code is not part of thermal_core. The governor
specific code must be in the governor code. Durga has done a pretty good
job splitting governor code out of thermal_core.c. I don't think we want
to get them back.

> /**
> * power_actor_get_max_power() - get the maximum power that a cdev can consume
> * @cdev: pointer to &thermal_cooling_device
> @@ -1712,6 +1817,11 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> if (result)
> goto unregister;
>
> + /* Add power_allocator specific thermal zone params */
> + result = create_power_allocator_tzp_attrs(&tz->device);
> + if (result)
> + goto unregister;

Here you could create the governor_params and have a callback from
governor to populate it.

> +
> /* Update 'this' zone's governor information */
> mutex_lock(&thermal_governor_lock);
>
> --
> 1.9.1
>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]