| Message ID | 20200504181616.175477-4-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | thermal: Add new mechanism to get thermal notification | expand |
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? > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > 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) >
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@linux.intel.com> > > --- > > 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) > > > >
On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > 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. Then what is the use of thermal trip type "ACTIVE" ? > 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.
On Wed, 2020-05-20 at 09:58 +0530, Amit Kucheria wrote: > On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > 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. > > Then what is the use of thermal trip type "ACTIVE" ? This is an example. The defaults are set by the OEMs via ACPI. User can't modify that if they want to optimize for their usage on Linux. There are fan control daemon's which user use on top. Thanks, Srinivas > > > 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.
Hi Srinivas, On Wed, May 20, 2020 at 11:46 PM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > On Wed, 2020-05-20 at 09:58 +0530, Amit Kucheria wrote: > > On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada > > <srinivas.pandruvada@linux.intel.com> wrote: > > > 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. > > > > Then what is the use of thermal trip type "ACTIVE" ? > This is an example. > The defaults are set by the OEMs via ACPI. User can't modify that if > they want to optimize for their usage on Linux. There are fan control > daemon's which user use on top. -ENOPARSE. Are you saying users "can" modify these? In any case, how is what you described earlier not possible with an ACTIVE trip point directly wired to the fan as a cooling device or with a HOT trip point that causes the platform driver to send notification to userspace where a fan control daemon can do what it needs to? Basically, I think the issue of polling is orthogonal to the introduction of the new attributes introduced in this patch and I don't understand the reason for these attributes from your commit description. > > > 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. >
Hi Amit, On Thu, 2020-05-21 at 10:41 +0530, Amit Kucheria wrote: > Hi Srinivas, > > On Wed, May 20, 2020 at 11:46 PM Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > On Wed, 2020-05-20 at 09:58 +0530, Amit Kucheria wrote: > > > On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada > > > <srinivas.pandruvada@linux.intel.com> wrote: > > > > 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. > > > > > > Then what is the use of thermal trip type "ACTIVE" ? > > This is an example. > > The defaults are set by the OEMs via ACPI. User can't modify that > > if > > they want to optimize for their usage on Linux. There are fan > > control > > daemon's which user use on top. > > -ENOPARSE. Are you saying users "can" modify these? Most of the x86 laptops will not have an active trip as the fan control is done by embedded controller. This is a safety and regulatory issue. Even when you have an active trip it will be read only and also ACPI fan cooling device will have few fix states to control. There are fine grain controls on top are available outside of thermal drivers via hwmon or others. https://wiki.archlinux.org/index.php/Fan_speed_control#ThinkPad_laptops Like in thermald we have XML config, which can be used to set different speed levels at different temperatures. Instead of polling of temperature, these attributes allow notification of temperature threshold. We currently mimic this behavior via adding a RW passive trip (The RW passive trips has a well defined usage different than what we are using for). There can be already existing RO passive/active trips in that zone already bound to some cooling device. So from user space we search for some RW passive trip and hope this is will give notifications. This I believe is a hack to use a fake trip point for notifications for temperature thresholds. Thanks, Srinivas > > In any case, how is what you described earlier not possible with an > ACTIVE trip point directly wired to the fan as a cooling device or > with a HOT trip point that causes the platform driver to send > notification to userspace where a fan control daemon can do what it > needs to? > > Basically, I think the issue of polling is orthogonal to the > introduction of the new attributes introduced in this patch and I > don't understand the reason for these attributes from your commit > description. > > > > > 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.
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)
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. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/thermal/thermal_sysfs.c | 136 +++++++++++++++++++++++++++++++- include/linux/thermal.h | 10 ++- 2 files changed, 143 insertions(+), 3 deletions(-)