Re: [RFC][PATCH 3/5] thermal: Add support for setting notification thresholds
From: Srinivas Pandruvada
Date: Mon May 18 2020 - 19:40:35 EST
On Mon, 2020-05-18 at 18:37 +0200, Daniel Lezcano wrote:
> On 04/05/2020 20:16, Srinivas Pandruvada wrote:
> > Add new attributes in thermal syfs when a thermal drivers provides
> > callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE is
> > defined.
> >
> > These attribute allow user space to stop polling for temperature.
> >
> > These attributes are:
> > - temp_thres_low: Specify a notification temperature for a low
> > temperature threshold event.
> > temp_thres_high: Specify a notification temperature for a high
> > temperature threshold event.
> > temp_thres_hyst: Specify a change in temperature to send
> > notification
> > again.
> >
> > This is implemented by adding additional sysfs attribute group. The
> > changes in this patch are trivial to add new attributes in thermal
> > sysfs as done for other attributes.
>
> Isn't it duplicate with the trip point?
A trip point is where an in-kernel governor takes some action. This is
not same as a notification temperature. For example at trip point
configured by ACPI at 85C, the thermal governor may start aggressive
throttling.
But a user space can set a notification threshold at 80C and start some
active controls like activate some fan to reduce the impact of passive
control on performance.
We need a way to distinguish between temperature notification threshold
and actual trip point. Changing a trip point means that user wants
kernel to throttle at temperature.
Thanks,
Srinivas
>
>
>
>
> > Signed-off-by: Srinivas Pandruvada <
> > srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > ---
> > drivers/thermal/thermal_sysfs.c | 136
> > +++++++++++++++++++++++++++++++-
> > include/linux/thermal.h | 10 ++-
> > 2 files changed, 143 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sysfs.c
> > b/drivers/thermal/thermal_sysfs.c
> > index aa99edb4dff7..aa85424c3ac4 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -215,6 +215,125 @@ trip_point_hyst_show(struct device *dev,
> > struct device_attribute *attr,
> > return ret ? ret : sprintf(buf, "%d\n", temperature);
> > }
> >
> > +#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
> > +
> > +#define create_thres_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); \
> > + int temperature, ret;
> > \
> > +
> > \
> > + ret = tz->ops->get_##name(tz, &temperature);
> > \
> > +
> > \
> > + return ret ? ret : sprintf(buf, "%d\n", temperature); \
> > + } \
> > +
> > \
> > + 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);
> > \
> > + int temperature, ret;
> > \
> > +
> > \
> > + if (kstrtoint(buf, 10, &temperature))
> > \
> > + return -EINVAL; \
> > +
> > \
> > + ret = tz->ops->set_##name(tz, temperature); \
> > + return ret ? ret : count; \
> > + }
> > +
> > +create_thres_attr(temp_thres_low);
> > +create_thres_attr(temp_thres_high);
> > +create_thres_attr(temp_thres_hyst);
> > +
> > +static int create_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > + struct attribute **attrs;
> > + int index = 0;
> > +
> > + if (tz->ops->get_temp_thres_low)
> > + ++index;
> > + if (tz->ops->get_temp_thres_high)
> > + ++index;
> > + if (tz->ops->get_temp_thres_high)
> > + ++index;
> > +
> > + /* One additional space for NULL */
> > + attrs = kcalloc(index + 1, sizeof(*attrs), GFP_KERNEL);
> > + if (!attrs)
> > + return -ENOMEM;
> > +
> > + tz->threshold_attrs = kcalloc(index, sizeof(*tz-
> > >threshold_attrs), GFP_KERNEL);
> > + if (!tz->threshold_attrs) {
> > + kfree(attrs);
> > + return -ENOMEM;
> > + }
> > +
> > + index = 0;
> > +
> > + if (tz->ops->get_temp_thres_low) {
> > + snprintf(tz->threshold_attrs[index].name,
> > THERMAL_NAME_LENGTH,
> > + "temp_thres_low");
> > +
> > + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> > + tz->threshold_attrs[index].attr.attr.name =
> > + tz-
> > >threshold_attrs[index].name;
> > + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR |
> > S_IRUGO;
> > + tz->threshold_attrs[index].attr.show =
> > temp_thres_low_show;
> > + tz->threshold_attrs[index].attr.store =
> > temp_thres_low_store;
> > + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> > + ++index;
> > + }
> > + if (tz->ops->get_temp_thres_high) {
> > + snprintf(tz->threshold_attrs[index].name,
> > THERMAL_NAME_LENGTH,
> > + "temp_thres_high");
> > +
> > + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> > + tz->threshold_attrs[index].attr.attr.name =
> > + tz-
> > >threshold_attrs[index].name;
> > + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR |
> > S_IRUGO;
> > + tz->threshold_attrs[index].attr.show =
> > temp_thres_high_show;
> > + tz->threshold_attrs[index].attr.store =
> > temp_thres_high_store;
> > + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> > + ++index;
> > + }
> > + if (tz->ops->get_temp_thres_hyst) {
> > + snprintf(tz->threshold_attrs[index].name,
> > THERMAL_NAME_LENGTH,
> > + "temp_thres_hyst");
> > +
> > + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> > + tz->threshold_attrs[index].attr.attr.name =
> > + tz-
> > >threshold_attrs[index].name;
> > + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR |
> > S_IRUGO;
> > + tz->threshold_attrs[index].attr.show =
> > temp_thres_hyst_show;
> > + tz->threshold_attrs[index].attr.store =
> > temp_thres_hyst_store;
> > + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> > + ++index;
> > + }
> > + attrs[index] = NULL;
> > + tz->threshold_attribute_group.attrs = attrs;
> > +
> > + return 0;
> > +}
> > +
> > +static void delete_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > + kfree(tz->threshold_attrs);
> > + kfree(tz->threshold_attribute_group.attrs);
> > +}
> > +#else
> > +static int create_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static void delete_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > +}
> > +#endif
> > +
> > static ssize_t
> > passive_store(struct device *dev, struct device_attribute *attr,
> > const char *buf, size_t count)
> > @@ -625,16 +744,27 @@ int thermal_zone_create_device_groups(struct
> > thermal_zone_device *tz,
> > {
> > const struct attribute_group **groups;
> > int i, size, result;
> > + int start = 0;
> >
> > /* we need one extra for trips and the NULL to terminate the
> > array */
> > size = ARRAY_SIZE(thermal_zone_attribute_groups) + 2;
> > +
> > + result = create_user_events_attrs(tz);
> > + if (!result) {
> > + ++size;
> > + ++start;
> > + }
> > +
> > /* This also takes care of API requirement to be NULL
> > terminated */
> > groups = kcalloc(size, sizeof(*groups), GFP_KERNEL);
> > if (!groups)
> > return -ENOMEM;
> >
> > - for (i = 0; i < size - 2; i++)
> > - groups[i] = thermal_zone_attribute_groups[i];
> > + if (start)
> > + groups[0] = &tz->threshold_attribute_group;
> > +
> > + for (i = 0; i < size - 2 - start; i++)
> > + groups[i + start] = thermal_zone_attribute_groups[i];
> >
> > if (tz->trips) {
> > result = create_trip_attrs(tz, mask);
> > @@ -660,6 +790,8 @@ void thermal_zone_destroy_device_groups(struct
> > thermal_zone_device *tz)
> > if (tz->trips)
> > destroy_trip_attrs(tz);
> >
> > + delete_user_events_attrs(tz);
> > +
> > kfree(tz->device.groups);
> > }
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index f5e1e7c6a9a2..ee9d79ace7ce 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -102,6 +102,12 @@ struct thermal_zone_device_ops {
> > enum thermal_trend *);
> > int (*notify) (struct thermal_zone_device *, int,
> > enum thermal_trip_type);
> > + int (*set_temp_thres_low)(struct thermal_zone_device *, int);
> > + int (*set_temp_thres_high)(struct thermal_zone_device *, int);
> > + int (*set_temp_thres_hyst)(struct thermal_zone_device *, int);
> > + int (*get_temp_thres_low)(struct thermal_zone_device *, int *);
> > + int (*get_temp_thres_high)(struct thermal_zone_device *, int
> > *);
> > + int (*get_temp_thres_hyst)(struct thermal_zone_device *, int
> > *);
> > };
> >
> > struct thermal_cooling_device_ops {
> > @@ -208,6 +214,8 @@ struct thermal_zone_device {
> > struct list_head node;
> > struct delayed_work poll_queue;
> > enum thermal_notify_event notify_event;
> > + struct attribute_group threshold_attribute_group;
> > + struct thermal_attr *threshold_attrs;
> > };
> >
> > /**
> > @@ -558,7 +566,7 @@ enum thermal_device_events {
> > THERMAL_PERF_CHANGED,
> > };
> >
> > -#ifdef CONFIG_THERMAL_USER_EVENT_INTERFACE
> > +#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
> > int thermal_dev_send_event(int zone_id, enum thermal_device_events
> > event, u64 event_data);
> > #else
> > int thermal_dev_send_event(int zone_id, enum thermal_device_events
> > event, u64 event_data)
> >
>
>