The code was reorganized in 2012 with the commit 0c01ebbfd3caf1.
The main change is a loop on the trip points array and a unconditional
call to the throttle() ops of the governors for each of them even if
the trip temperature is not reached yet.
With this change, the 'forced_passive' is no longer checked in the
thermal_zone_device_update() function but in the step wise governor's
throttle() callback.
As the force_passive does no belong to the trip point array, the
thermal_zone_device_update() can not compare with the specified
passive temperature, thus does not detect the passive limit has been
crossed. Consequently, throttle() is never called and the
'forced_passive' branch is unreached.
In addition, the default processor cooling device is not automatically
bound to the thermal zone if there is not passive trip point, thus the
'forced_passive' can not operate.
If there is an active trip point, then the throttle function will be
called to mitigate at this temperature and the 'forced_passive' will
override the mitigation of the active trip point in this case but with
the default cooling device bound to the thermal zone, so usually a
fan, and that is not a passive cooling effect.
Given the regression exists since more than 8 years, nobody complained
and at the best of my knowledge there is no bug open in
https://bugzilla.kernel.org, it is reasonable to say it is unused.
Remove the 'forced_passive' related code.
Signed-off-by: Daniel Lezcano <[email protected]>
---
.../driver-api/thermal/sysfs-api.rst | 13 ---
drivers/thermal/gov_step_wise.c | 14 +---
drivers/thermal/thermal_sysfs.c | 80 -------------------
include/linux/thermal.h | 4 -
4 files changed, 3 insertions(+), 108 deletions(-)
diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
index e7520cb439ac..a4969c474cc3 100644
--- a/Documentation/driver-api/thermal/sysfs-api.rst
+++ b/Documentation/driver-api/thermal/sysfs-api.rst
@@ -520,19 +520,6 @@ available_policies
RW, Optional
-passive
- Attribute is only present for zones in which the passive cooling
- policy is not supported by native thermal driver. Default is zero
- and can be set to a temperature (in millidegrees) to enable a
- passive trip point for the zone. Activation is done by polling with
- an interval of 1 second.
-
- Unit: millidegrees Celsius
-
- Valid values: 0 (disabled) or greater than 1000
-
- RW, Optional
-
emul_temp
Interface to set the emulated temperature method in thermal zone
(sensor). After setting this temperature, the thermal zone may pass
diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c
index 2ae7198d3067..12acb12aac50 100644
--- a/drivers/thermal/gov_step_wise.c
+++ b/drivers/thermal/gov_step_wise.c
@@ -109,7 +109,7 @@ static void update_passive_instance(struct thermal_zone_device *tz,
* If value is +1, activate a passive instance.
* If value is -1, deactivate a passive instance.
*/
- if (type == THERMAL_TRIP_PASSIVE || type == THERMAL_TRIPS_NONE)
+ if (type == THERMAL_TRIP_PASSIVE)
tz->passive += value;
}
@@ -122,13 +122,8 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
bool throttle = false;
int old_target;
- if (trip == THERMAL_TRIPS_NONE) {
- trip_temp = tz->forced_passive;
- trip_type = THERMAL_TRIPS_NONE;
- } else {
- tz->ops->get_trip_temp(tz, trip, &trip_temp);
- tz->ops->get_trip_type(tz, trip, &trip_type);
- }
+ tz->ops->get_trip_temp(tz, trip, &trip_temp);
+ tz->ops->get_trip_type(tz, trip, &trip_type);
trend = get_tz_trend(tz, trip);
@@ -189,9 +184,6 @@ static int step_wise_throttle(struct thermal_zone_device *tz, int trip)
thermal_zone_trip_update(tz, trip);
- if (tz->forced_passive)
- thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
-
mutex_lock(&tz->lock);
list_for_each_entry(instance, &tz->thermal_instances, tz_node)
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 0866e949339b..4e7f9e880d76 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -216,49 +216,6 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr,
return ret ? ret : sprintf(buf, "%d\n", temperature);
}
-static ssize_t
-passive_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct thermal_zone_device *tz = to_thermal_zone(dev);
- int state;
-
- if (sscanf(buf, "%d\n", &state) != 1)
- return -EINVAL;
-
- /* sanity check: values below 1000 millicelcius don't make sense
- * and can cause the system to go into a thermal heart attack
- */
- if (state && state < 1000)
- return -EINVAL;
-
- if (state && !tz->forced_passive) {
- if (!tz->passive_delay)
- tz->passive_delay = 1000;
- thermal_zone_device_rebind_exception(tz, "Processor",
- sizeof("Processor"));
- } else if (!state && tz->forced_passive) {
- tz->passive_delay = 0;
- thermal_zone_device_unbind_exception(tz, "Processor",
- sizeof("Processor"));
- }
-
- tz->forced_passive = state;
-
- thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
-
- return count;
-}
-
-static ssize_t
-passive_show(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- struct thermal_zone_device *tz = to_thermal_zone(dev);
-
- return sprintf(buf, "%d\n", tz->forced_passive);
-}
-
static ssize_t
policy_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
@@ -403,7 +360,6 @@ static DEVICE_ATTR_RW(sustainable_power);
/* These thermal zone device attributes are created based on conditions */
static DEVICE_ATTR_RW(mode);
-static DEVICE_ATTR_RW(passive);
/* These attributes are unconditionally added to a thermal zone */
static struct attribute *thermal_zone_dev_attrs[] = {
@@ -438,45 +394,9 @@ static const struct attribute_group thermal_zone_mode_attribute_group = {
.attrs = thermal_zone_mode_attrs,
};
-/* We expose passive only if passive trips are present */
-static struct attribute *thermal_zone_passive_attrs[] = {
- &dev_attr_passive.attr,
- NULL,
-};
-
-static umode_t thermal_zone_passive_is_visible(struct kobject *kobj,
- struct attribute *attr,
- int attrno)
-{
- struct device *dev = kobj_to_dev(kobj);
- struct thermal_zone_device *tz;
- enum thermal_trip_type trip_type;
- int count, passive = 0;
-
- tz = container_of(dev, struct thermal_zone_device, device);
-
- for (count = 0; count < tz->trips && !passive; count++) {
- tz->ops->get_trip_type(tz, count, &trip_type);
-
- if (trip_type == THERMAL_TRIP_PASSIVE)
- passive = 1;
- }
-
- if (!passive)
- return attr->mode;
-
- return 0;
-}
-
-static const struct attribute_group thermal_zone_passive_attribute_group = {
- .attrs = thermal_zone_passive_attrs,
- .is_visible = thermal_zone_passive_is_visible,
-};
-
static const struct attribute_group *thermal_zone_attribute_groups[] = {
&thermal_zone_attribute_group,
&thermal_zone_mode_attribute_group,
- &thermal_zone_passive_attribute_group,
/* This is not NULL terminated as we create the group dynamically */
};
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index c80032322158..a57232a9a6f9 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -131,9 +131,6 @@ struct thermal_cooling_device {
trip point.
* @prev_high_trip: the above current temperature if you've crossed a
passive trip point.
- * @forced_passive: If > 0, temperature at which to switch on all ACPI
- * processor cooling devices. Currently only used by the
- * step-wise governor.
* @need_update: if equals 1, thermal_zone_device_update needs to be invoked.
* @ops: operations this &thermal_zone_device supports
* @tzp: thermal zone parameters
@@ -167,7 +164,6 @@ struct thermal_zone_device {
int passive;
int prev_low_trip;
int prev_high_trip;
- unsigned int forced_passive;
atomic_t need_update;
struct thermal_zone_device_ops *ops;
struct thermal_zone_params *tzp;
--
2.25.1
The loop is here to create default cooling device binding on the
THERMAL_TRIPS_NONE number which is used to be the 'forced_passive'
feature. However, we removed all code dealing with that in the thermal
core, thus this binding does no longer make sense.
Remove it.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/acpi/thermal.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index b5e4bc9e3282..26a89ff80a0e 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -764,25 +764,6 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
}
}
- for (i = 0; i < tz->devices.count; i++) {
- handle = tz->devices.handles[i];
- status = acpi_bus_get_device(handle, &dev);
- if (ACPI_SUCCESS(status) && (dev == device)) {
- if (bind)
- result = thermal_zone_bind_cooling_device
- (thermal, THERMAL_TRIPS_NONE,
- cdev, THERMAL_NO_LIMIT,
- THERMAL_NO_LIMIT,
- THERMAL_WEIGHT_DEFAULT);
- else
- result = thermal_zone_unbind_cooling_device
- (thermal, THERMAL_TRIPS_NONE,
- cdev);
- if (result)
- goto failed;
- }
- }
-
failed:
return result;
}
--
2.25.1
The functions thermal_zone_device_rebind_exception and
thermal_zone_device_unbind_exception are not used from anywhere.
Remove that code.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_core.c | 37 ----------------------------------
drivers/thermal/thermal_core.h | 4 ----
2 files changed, 41 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 567bc6f254c0..a0f0c33c8d9c 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -598,26 +598,6 @@ static void thermal_zone_device_check(struct work_struct *work)
thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
}
-void thermal_zone_device_rebind_exception(struct thermal_zone_device *tz,
- const char *cdev_type, size_t size)
-{
- struct thermal_cooling_device *cdev = NULL;
-
- mutex_lock(&thermal_list_lock);
- list_for_each_entry(cdev, &thermal_cdev_list, node) {
- /* skip non matching cdevs */
- if (strncmp(cdev_type, cdev->type, size))
- continue;
-
- /* re binding the exception matching the type pattern */
- thermal_zone_bind_cooling_device(tz, THERMAL_TRIPS_NONE, cdev,
- THERMAL_NO_LIMIT,
- THERMAL_NO_LIMIT,
- THERMAL_WEIGHT_DEFAULT);
- }
- mutex_unlock(&thermal_list_lock);
-}
-
int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
void *data)
{
@@ -685,23 +665,6 @@ struct thermal_zone_device *thermal_zone_get_by_id(int id)
return match;
}
-void thermal_zone_device_unbind_exception(struct thermal_zone_device *tz,
- const char *cdev_type, size_t size)
-{
- struct thermal_cooling_device *cdev = NULL;
-
- mutex_lock(&thermal_list_lock);
- list_for_each_entry(cdev, &thermal_cdev_list, node) {
- /* skip non matching cdevs */
- if (strncmp(cdev_type, cdev->type, size))
- continue;
- /* unbinding the exception matching the type pattern */
- thermal_zone_unbind_cooling_device(tz, THERMAL_TRIPS_NONE,
- cdev);
- }
- mutex_unlock(&thermal_list_lock);
-}
-
/*
* Device management section: cooling devices, zones devices, and binding
*
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 8df600fa7b79..e50c6b2909fe 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -118,10 +118,6 @@ struct thermal_instance {
int thermal_register_governor(struct thermal_governor *);
void thermal_unregister_governor(struct thermal_governor *);
-void thermal_zone_device_rebind_exception(struct thermal_zone_device *,
- const char *, size_t);
-void thermal_zone_device_unbind_exception(struct thermal_zone_device *,
- const char *, size_t);
int thermal_zone_device_set_policy(struct thermal_zone_device *, char *);
int thermal_build_list_of_policies(char *buf);
--
2.25.1
The last site calling the thermal_zone_bind_cooling_device() function
with the THERMAL_TRIPS_NONE parameter was removed.
We can get rid of this test as no user of this function is calling
this function with this parameter.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index a0f0c33c8d9c..bcc2ea4f5482 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -710,7 +710,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
unsigned long max_state;
int result, ret;
- if (trip >= tz->trips || (trip < 0 && trip != THERMAL_TRIPS_NONE))
+ if (trip >= tz->trips || trip < 0)
return -EINVAL;
list_for_each_entry(pos1, &thermal_tz_list, node) {
--
2.25.1
The THERMAL_TRIPS_NONE is equal to -1, it is pointless to do a
conversion in this function.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_sysfs.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 4e7f9e880d76..345917a58f2f 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -875,10 +875,7 @@ trip_point_show(struct device *dev, struct device_attribute *attr, char *buf)
instance =
container_of(attr, struct thermal_instance, attr);
- if (instance->trip == THERMAL_TRIPS_NONE)
- return sprintf(buf, "-1\n");
- else
- return sprintf(buf, "%d\n", instance->trip);
+ return sprintf(buf, "%d\n", instance->trip);
}
ssize_t
--
2.25.1
The macro THERMAL_TRIPS_NONE is no longer used, remove it.
Signed-off-by: Daniel Lezcano <[email protected]>
---
include/linux/thermal.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index a57232a9a6f9..060a2160add4 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -17,7 +17,6 @@
#include <linux/workqueue.h>
#include <uapi/linux/thermal.h>
-#define THERMAL_TRIPS_NONE -1
#define THERMAL_MAX_TRIPS 12
/* invalid cooling state */
--
2.25.1
On 15/12/2020 00:38, Daniel Lezcano wrote:
> The code was reorganized in 2012 with the commit 0c01ebbfd3caf1.
>
> The main change is a loop on the trip points array and a unconditional
> call to the throttle() ops of the governors for each of them even if
> the trip temperature is not reached yet.
>
> With this change, the 'forced_passive' is no longer checked in the
> thermal_zone_device_update() function but in the step wise governor's
> throttle() callback.
>
> As the force_passive does no belong to the trip point array, the
> thermal_zone_device_update() can not compare with the specified
> passive temperature, thus does not detect the passive limit has been
> crossed. Consequently, throttle() is never called and the
> 'forced_passive' branch is unreached.
>
> In addition, the default processor cooling device is not automatically
> bound to the thermal zone if there is not passive trip point, thus the
> 'forced_passive' can not operate.
>
> If there is an active trip point, then the throttle function will be
> called to mitigate at this temperature and the 'forced_passive' will
> override the mitigation of the active trip point in this case but with
> the default cooling device bound to the thermal zone, so usually a
> fan, and that is not a passive cooling effect.
>
> Given the regression exists since more than 8 years, nobody complained
> and at the best of my knowledge there is no bug open in
> https://bugzilla.kernel.org, it is reasonable to say it is unused.
>
> Remove the 'forced_passive' related code.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
Is there any concern with this series ?
> ---
> .../driver-api/thermal/sysfs-api.rst | 13 ---
> drivers/thermal/gov_step_wise.c | 14 +---
> drivers/thermal/thermal_sysfs.c | 80 -------------------
> include/linux/thermal.h | 4 -
> 4 files changed, 3 insertions(+), 108 deletions(-)
>
> diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> index e7520cb439ac..a4969c474cc3 100644
> --- a/Documentation/driver-api/thermal/sysfs-api.rst
> +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> @@ -520,19 +520,6 @@ available_policies
>
> RW, Optional
>
> -passive
> - Attribute is only present for zones in which the passive cooling
> - policy is not supported by native thermal driver. Default is zero
> - and can be set to a temperature (in millidegrees) to enable a
> - passive trip point for the zone. Activation is done by polling with
> - an interval of 1 second.
> -
> - Unit: millidegrees Celsius
> -
> - Valid values: 0 (disabled) or greater than 1000
> -
> - RW, Optional
> -
> emul_temp
> Interface to set the emulated temperature method in thermal zone
> (sensor). After setting this temperature, the thermal zone may pass
> diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c
> index 2ae7198d3067..12acb12aac50 100644
> --- a/drivers/thermal/gov_step_wise.c
> +++ b/drivers/thermal/gov_step_wise.c
> @@ -109,7 +109,7 @@ static void update_passive_instance(struct thermal_zone_device *tz,
> * If value is +1, activate a passive instance.
> * If value is -1, deactivate a passive instance.
> */
> - if (type == THERMAL_TRIP_PASSIVE || type == THERMAL_TRIPS_NONE)
> + if (type == THERMAL_TRIP_PASSIVE)
> tz->passive += value;
> }
>
> @@ -122,13 +122,8 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
> bool throttle = false;
> int old_target;
>
> - if (trip == THERMAL_TRIPS_NONE) {
> - trip_temp = tz->forced_passive;
> - trip_type = THERMAL_TRIPS_NONE;
> - } else {
> - tz->ops->get_trip_temp(tz, trip, &trip_temp);
> - tz->ops->get_trip_type(tz, trip, &trip_type);
> - }
> + tz->ops->get_trip_temp(tz, trip, &trip_temp);
> + tz->ops->get_trip_type(tz, trip, &trip_type);
>
> trend = get_tz_trend(tz, trip);
>
> @@ -189,9 +184,6 @@ static int step_wise_throttle(struct thermal_zone_device *tz, int trip)
>
> thermal_zone_trip_update(tz, trip);
>
> - if (tz->forced_passive)
> - thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
> -
> mutex_lock(&tz->lock);
>
> list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 0866e949339b..4e7f9e880d76 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -216,49 +216,6 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr,
> return ret ? ret : sprintf(buf, "%d\n", temperature);
> }
>
> -static ssize_t
> -passive_store(struct device *dev, struct device_attribute *attr,
> - const char *buf, size_t count)
> -{
> - struct thermal_zone_device *tz = to_thermal_zone(dev);
> - int state;
> -
> - if (sscanf(buf, "%d\n", &state) != 1)
> - return -EINVAL;
> -
> - /* sanity check: values below 1000 millicelcius don't make sense
> - * and can cause the system to go into a thermal heart attack
> - */
> - if (state && state < 1000)
> - return -EINVAL;
> -
> - if (state && !tz->forced_passive) {
> - if (!tz->passive_delay)
> - tz->passive_delay = 1000;
> - thermal_zone_device_rebind_exception(tz, "Processor",
> - sizeof("Processor"));
> - } else if (!state && tz->forced_passive) {
> - tz->passive_delay = 0;
> - thermal_zone_device_unbind_exception(tz, "Processor",
> - sizeof("Processor"));
> - }
> -
> - tz->forced_passive = state;
> -
> - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> -
> - return count;
> -}
> -
> -static ssize_t
> -passive_show(struct device *dev, struct device_attribute *attr,
> - char *buf)
> -{
> - struct thermal_zone_device *tz = to_thermal_zone(dev);
> -
> - return sprintf(buf, "%d\n", tz->forced_passive);
> -}
> -
> static ssize_t
> policy_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -403,7 +360,6 @@ static DEVICE_ATTR_RW(sustainable_power);
>
> /* These thermal zone device attributes are created based on conditions */
> static DEVICE_ATTR_RW(mode);
> -static DEVICE_ATTR_RW(passive);
>
> /* These attributes are unconditionally added to a thermal zone */
> static struct attribute *thermal_zone_dev_attrs[] = {
> @@ -438,45 +394,9 @@ static const struct attribute_group thermal_zone_mode_attribute_group = {
> .attrs = thermal_zone_mode_attrs,
> };
>
> -/* We expose passive only if passive trips are present */
> -static struct attribute *thermal_zone_passive_attrs[] = {
> - &dev_attr_passive.attr,
> - NULL,
> -};
> -
> -static umode_t thermal_zone_passive_is_visible(struct kobject *kobj,
> - struct attribute *attr,
> - int attrno)
> -{
> - struct device *dev = kobj_to_dev(kobj);
> - struct thermal_zone_device *tz;
> - enum thermal_trip_type trip_type;
> - int count, passive = 0;
> -
> - tz = container_of(dev, struct thermal_zone_device, device);
> -
> - for (count = 0; count < tz->trips && !passive; count++) {
> - tz->ops->get_trip_type(tz, count, &trip_type);
> -
> - if (trip_type == THERMAL_TRIP_PASSIVE)
> - passive = 1;
> - }
> -
> - if (!passive)
> - return attr->mode;
> -
> - return 0;
> -}
> -
> -static const struct attribute_group thermal_zone_passive_attribute_group = {
> - .attrs = thermal_zone_passive_attrs,
> - .is_visible = thermal_zone_passive_is_visible,
> -};
> -
> static const struct attribute_group *thermal_zone_attribute_groups[] = {
> &thermal_zone_attribute_group,
> &thermal_zone_mode_attribute_group,
> - &thermal_zone_passive_attribute_group,
> /* This is not NULL terminated as we create the group dynamically */
> };
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index c80032322158..a57232a9a6f9 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -131,9 +131,6 @@ struct thermal_cooling_device {
> trip point.
> * @prev_high_trip: the above current temperature if you've crossed a
> passive trip point.
> - * @forced_passive: If > 0, temperature at which to switch on all ACPI
> - * processor cooling devices. Currently only used by the
> - * step-wise governor.
> * @need_update: if equals 1, thermal_zone_device_update needs to be invoked.
> * @ops: operations this &thermal_zone_device supports
> * @tzp: thermal zone parameters
> @@ -167,7 +164,6 @@ struct thermal_zone_device {
> int passive;
> int prev_low_trip;
> int prev_high_trip;
> - unsigned int forced_passive;
> atomic_t need_update;
> struct thermal_zone_device_ops *ops;
> struct thermal_zone_params *tzp;
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Hi Rui,
On 15/12/2020 00:38, Daniel Lezcano wrote:
> The loop is here to create default cooling device binding on the
> THERMAL_TRIPS_NONE number which is used to be the 'forced_passive'
> feature. However, we removed all code dealing with that in the thermal
> core, thus this binding does no longer make sense.
>
> Remove it.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
Are you fine with this change?
Thanks
-- Daniel
> ---
> drivers/acpi/thermal.c | 19 -------------------
> 1 file changed, 19 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index b5e4bc9e3282..26a89ff80a0e 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -764,25 +764,6 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
> }
> }
>
> - for (i = 0; i < tz->devices.count; i++) {
> - handle = tz->devices.handles[i];
> - status = acpi_bus_get_device(handle, &dev);
> - if (ACPI_SUCCESS(status) && (dev == device)) {
> - if (bind)
> - result = thermal_zone_bind_cooling_device
> - (thermal, THERMAL_TRIPS_NONE,
> - cdev, THERMAL_NO_LIMIT,
> - THERMAL_NO_LIMIT,
> - THERMAL_WEIGHT_DEFAULT);
> - else
> - result = thermal_zone_unbind_cooling_device
> - (thermal, THERMAL_TRIPS_NONE,
> - cdev);
> - if (result)
> - goto failed;
> - }
> - }
> -
> failed:
> return result;
> }
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
ACPI thermal driver binds the devices listed in _TZD method with THERMAL_TRIPS_NONE.
Now given that
1. THERMAL_TRIPS_NONE is removed from thermal framework
2. _TZP is rarely supported. I searched ~500 acpidumps from different platforms reported by end users in kernel Bugzilla, there is only one platform with _TZP implemented, and it was almost 10 years ago.
So, I think it is safe to remove this piece of code.
> -----Original Message-----
> From: Daniel Lezcano <[email protected]>
> Sent: Tuesday, January 05, 2021 11:44 PM
> To: Zhang, Rui <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> Rafael J. Wysocki <[email protected]>; Len Brown <[email protected]>; open
> list:ACPI THERMAL DRIVER <[email protected]>
> Subject: Re: [PATCH 4/6] acpi/drivers/thermal: Remove TRIPS_NONE cooling
> device binding
> Importance: High
>
> Hi Rui,
>
>
> On 15/12/2020 00:38, Daniel Lezcano wrote:
> > The loop is here to create default cooling device binding on the
> > THERMAL_TRIPS_NONE number which is used to be the 'forced_passive'
> > feature. However, we removed all code dealing with that in the thermal
> > core, thus this binding does no longer make sense.
> >
> > Remove it.
> >
> > Signed-off-by: Daniel Lezcano <[email protected]>
Acked-by: Zhang Rui <[email protected]>
Thanks,
rui
>
> Are you fine with this change?
>
> Thanks
>
> -- Daniel
>
> > ---
> > drivers/acpi/thermal.c | 19 -------------------
> > 1 file changed, 19 deletions(-)
> >
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c index
> > b5e4bc9e3282..26a89ff80a0e 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -764,25 +764,6 @@ static int acpi_thermal_cooling_device_cb(struct
> thermal_zone_device *thermal,
> > }
> > }
> >
> > - for (i = 0; i < tz->devices.count; i++) {
> > - handle = tz->devices.handles[i];
> > - status = acpi_bus_get_device(handle, &dev);
> > - if (ACPI_SUCCESS(status) && (dev == device)) {
> > - if (bind)
> > - result = thermal_zone_bind_cooling_device
> > - (thermal,
> THERMAL_TRIPS_NONE,
> > - cdev, THERMAL_NO_LIMIT,
> > - THERMAL_NO_LIMIT,
> > -
> THERMAL_WEIGHT_DEFAULT);
> > - else
> > - result =
> thermal_zone_unbind_cooling_device
> > - (thermal,
> THERMAL_TRIPS_NONE,
> > - cdev);
> > - if (result)
> > - goto failed;
> > - }
> > - }
> > -
> > failed:
> > return result;
> > }
> >
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-
> blog/> Blog
On 07/01/2021 06:10, Zhang, Rui wrote:
> ACPI thermal driver binds the devices listed in _TZD method with THERMAL_TRIPS_NONE.
> Now given that
> 1. THERMAL_TRIPS_NONE is removed from thermal framework
> 2. _TZP is rarely supported. I searched ~500 acpidumps from different platforms reported by end users in kernel Bugzilla, there is only one platform with _TZP implemented, and it was almost 10 years ago.
>
> So, I think it is safe to remove this piece of code.
Thanks Rui for digging into
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 12/14/20 6:38 PM, Daniel Lezcano wrote:
> The functions thermal_zone_device_rebind_exception and
> thermal_zone_device_unbind_exception are not used from anywhere.
>
> Remove that code.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
Reviewed-by: Thara Gopinath <[email protected]>
> ---
> drivers/thermal/thermal_core.c | 37 ----------------------------------
> drivers/thermal/thermal_core.h | 4 ----
> 2 files changed, 41 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 567bc6f254c0..a0f0c33c8d9c 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -598,26 +598,6 @@ static void thermal_zone_device_check(struct work_struct *work)
> thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> }
>
> -void thermal_zone_device_rebind_exception(struct thermal_zone_device *tz,
> - const char *cdev_type, size_t size)
> -{
> - struct thermal_cooling_device *cdev = NULL;
> -
> - mutex_lock(&thermal_list_lock);
> - list_for_each_entry(cdev, &thermal_cdev_list, node) {
> - /* skip non matching cdevs */
> - if (strncmp(cdev_type, cdev->type, size))
> - continue;
> -
> - /* re binding the exception matching the type pattern */
> - thermal_zone_bind_cooling_device(tz, THERMAL_TRIPS_NONE, cdev,
> - THERMAL_NO_LIMIT,
> - THERMAL_NO_LIMIT,
> - THERMAL_WEIGHT_DEFAULT);
> - }
> - mutex_unlock(&thermal_list_lock);
> -}
> -
> int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
> void *data)
> {
> @@ -685,23 +665,6 @@ struct thermal_zone_device *thermal_zone_get_by_id(int id)
> return match;
> }
>
> -void thermal_zone_device_unbind_exception(struct thermal_zone_device *tz,
> - const char *cdev_type, size_t size)
> -{
> - struct thermal_cooling_device *cdev = NULL;
> -
> - mutex_lock(&thermal_list_lock);
> - list_for_each_entry(cdev, &thermal_cdev_list, node) {
> - /* skip non matching cdevs */
> - if (strncmp(cdev_type, cdev->type, size))
> - continue;
> - /* unbinding the exception matching the type pattern */
> - thermal_zone_unbind_cooling_device(tz, THERMAL_TRIPS_NONE,
> - cdev);
> - }
> - mutex_unlock(&thermal_list_lock);
> -}
> -
> /*
> * Device management section: cooling devices, zones devices, and binding
> *
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 8df600fa7b79..e50c6b2909fe 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -118,10 +118,6 @@ struct thermal_instance {
>
> int thermal_register_governor(struct thermal_governor *);
> void thermal_unregister_governor(struct thermal_governor *);
> -void thermal_zone_device_rebind_exception(struct thermal_zone_device *,
> - const char *, size_t);
> -void thermal_zone_device_unbind_exception(struct thermal_zone_device *,
> - const char *, size_t);
> int thermal_zone_device_set_policy(struct thermal_zone_device *, char *);
> int thermal_build_list_of_policies(char *buf);
>
>
--
Warm Regards
Thara
On 12/14/20 6:38 PM, Daniel Lezcano wrote:
> The macro THERMAL_TRIPS_NONE is no longer used, remove it.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
Reviewed-by: Thara Gopinath <[email protected]>
> include/linux/thermal.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index a57232a9a6f9..060a2160add4 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -17,7 +17,6 @@
> #include <linux/workqueue.h>
> #include <uapi/linux/thermal.h>
>
> -#define THERMAL_TRIPS_NONE -1
> #define THERMAL_MAX_TRIPS 12
>
> /* invalid cooling state */
>
--
Warm Regards
Thara
On 12/14/20 6:38 PM, Daniel Lezcano wrote:
> The last site calling the thermal_zone_bind_cooling_device() function
> with the THERMAL_TRIPS_NONE parameter was removed.
>
> We can get rid of this test as no user of this function is calling
> this function with this parameter.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
Reviewed-by: Thara Gopinath <[email protected]>
> drivers/thermal/thermal_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index a0f0c33c8d9c..bcc2ea4f5482 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -710,7 +710,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
> unsigned long max_state;
> int result, ret;
>
> - if (trip >= tz->trips || (trip < 0 && trip != THERMAL_TRIPS_NONE))
> + if (trip >= tz->trips || trip < 0)
> return -EINVAL;
>
> list_for_each_entry(pos1, &thermal_tz_list, node) {
>
--
Warm Regards
Thara
On 12/14/20 6:38 PM, Daniel Lezcano wrote:
> The code was reorganized in 2012 with the commit 0c01ebbfd3caf1.
>
> The main change is a loop on the trip points array and a unconditional
> call to the throttle() ops of the governors for each of them even if
> the trip temperature is not reached yet.
>
> With this change, the 'forced_passive' is no longer checked in the
> thermal_zone_device_update() function but in the step wise governor's
> throttle() callback.
>
> As the force_passive does no belong to the trip point array, the
> thermal_zone_device_update() can not compare with the specified
> passive temperature, thus does not detect the passive limit has been
> crossed. Consequently, throttle() is never called and the
> 'forced_passive' branch is unreached.
>
> In addition, the default processor cooling device is not automatically
> bound to the thermal zone if there is not passive trip point, thus the
> 'forced_passive' can not operate.
>
> If there is an active trip point, then the throttle function will be
> called to mitigate at this temperature and the 'forced_passive' will
> override the mitigation of the active trip point in this case but with
> the default cooling device bound to the thermal zone, so usually a
> fan, and that is not a passive cooling effect.
>
> Given the regression exists since more than 8 years, nobody complained
> and at the best of my knowledge there is no bug open in
> https://bugzilla.kernel.org, it is reasonable to say it is unused.
>
> Remove the 'forced_passive' related code.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
Reviewed-by: Thara Gopinath <[email protected]>
> ---
> .../driver-api/thermal/sysfs-api.rst | 13 ---
> drivers/thermal/gov_step_wise.c | 14 +---
> drivers/thermal/thermal_sysfs.c | 80 -------------------
> include/linux/thermal.h | 4 -
> 4 files changed, 3 insertions(+), 108 deletions(-)
>
> diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> index e7520cb439ac..a4969c474cc3 100644
> --- a/Documentation/driver-api/thermal/sysfs-api.rst
> +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> @@ -520,19 +520,6 @@ available_policies
>
> RW, Optional
>
> -passive
> - Attribute is only present for zones in which the passive cooling
> - policy is not supported by native thermal driver. Default is zero
> - and can be set to a temperature (in millidegrees) to enable a
> - passive trip point for the zone. Activation is done by polling with
> - an interval of 1 second.
> -
> - Unit: millidegrees Celsius
> -
> - Valid values: 0 (disabled) or greater than 1000
> -
> - RW, Optional
> -
> emul_temp
> Interface to set the emulated temperature method in thermal zone
> (sensor). After setting this temperature, the thermal zone may pass
> diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c
> index 2ae7198d3067..12acb12aac50 100644
> --- a/drivers/thermal/gov_step_wise.c
> +++ b/drivers/thermal/gov_step_wise.c
> @@ -109,7 +109,7 @@ static void update_passive_instance(struct thermal_zone_device *tz,
> * If value is +1, activate a passive instance.
> * If value is -1, deactivate a passive instance.
> */
> - if (type == THERMAL_TRIP_PASSIVE || type == THERMAL_TRIPS_NONE)
> + if (type == THERMAL_TRIP_PASSIVE)
> tz->passive += value;
> }
>
> @@ -122,13 +122,8 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
> bool throttle = false;
> int old_target;
>
> - if (trip == THERMAL_TRIPS_NONE) {
> - trip_temp = tz->forced_passive;
> - trip_type = THERMAL_TRIPS_NONE;
> - } else {
> - tz->ops->get_trip_temp(tz, trip, &trip_temp);
> - tz->ops->get_trip_type(tz, trip, &trip_type);
> - }
> + tz->ops->get_trip_temp(tz, trip, &trip_temp);
> + tz->ops->get_trip_type(tz, trip, &trip_type);
>
> trend = get_tz_trend(tz, trip);
>
> @@ -189,9 +184,6 @@ static int step_wise_throttle(struct thermal_zone_device *tz, int trip)
>
> thermal_zone_trip_update(tz, trip);
>
> - if (tz->forced_passive)
> - thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
> -
> mutex_lock(&tz->lock);
>
> list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 0866e949339b..4e7f9e880d76 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -216,49 +216,6 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr,
> return ret ? ret : sprintf(buf, "%d\n", temperature);
> }
>
> -static ssize_t
> -passive_store(struct device *dev, struct device_attribute *attr,
> - const char *buf, size_t count)
> -{
> - struct thermal_zone_device *tz = to_thermal_zone(dev);
> - int state;
> -
> - if (sscanf(buf, "%d\n", &state) != 1)
> - return -EINVAL;
> -
> - /* sanity check: values below 1000 millicelcius don't make sense
> - * and can cause the system to go into a thermal heart attack
> - */
> - if (state && state < 1000)
> - return -EINVAL;
> -
> - if (state && !tz->forced_passive) {
> - if (!tz->passive_delay)
> - tz->passive_delay = 1000;
> - thermal_zone_device_rebind_exception(tz, "Processor",
> - sizeof("Processor"));
> - } else if (!state && tz->forced_passive) {
> - tz->passive_delay = 0;
> - thermal_zone_device_unbind_exception(tz, "Processor",
> - sizeof("Processor"));
> - }
> -
> - tz->forced_passive = state;
> -
> - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> -
> - return count;
> -}
> -
> -static ssize_t
> -passive_show(struct device *dev, struct device_attribute *attr,
> - char *buf)
> -{
> - struct thermal_zone_device *tz = to_thermal_zone(dev);
> -
> - return sprintf(buf, "%d\n", tz->forced_passive);
> -}
> -
> static ssize_t
> policy_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -403,7 +360,6 @@ static DEVICE_ATTR_RW(sustainable_power);
>
> /* These thermal zone device attributes are created based on conditions */
> static DEVICE_ATTR_RW(mode);
> -static DEVICE_ATTR_RW(passive);
>
> /* These attributes are unconditionally added to a thermal zone */
> static struct attribute *thermal_zone_dev_attrs[] = {
> @@ -438,45 +394,9 @@ static const struct attribute_group thermal_zone_mode_attribute_group = {
> .attrs = thermal_zone_mode_attrs,
> };
>
> -/* We expose passive only if passive trips are present */
> -static struct attribute *thermal_zone_passive_attrs[] = {
> - &dev_attr_passive.attr,
> - NULL,
> -};
> -
> -static umode_t thermal_zone_passive_is_visible(struct kobject *kobj,
> - struct attribute *attr,
> - int attrno)
> -{
> - struct device *dev = kobj_to_dev(kobj);
> - struct thermal_zone_device *tz;
> - enum thermal_trip_type trip_type;
> - int count, passive = 0;
> -
> - tz = container_of(dev, struct thermal_zone_device, device);
> -
> - for (count = 0; count < tz->trips && !passive; count++) {
> - tz->ops->get_trip_type(tz, count, &trip_type);
> -
> - if (trip_type == THERMAL_TRIP_PASSIVE)
> - passive = 1;
> - }
> -
> - if (!passive)
> - return attr->mode;
> -
> - return 0;
> -}
> -
> -static const struct attribute_group thermal_zone_passive_attribute_group = {
> - .attrs = thermal_zone_passive_attrs,
> - .is_visible = thermal_zone_passive_is_visible,
> -};
> -
> static const struct attribute_group *thermal_zone_attribute_groups[] = {
> &thermal_zone_attribute_group,
> &thermal_zone_mode_attribute_group,
> - &thermal_zone_passive_attribute_group,
> /* This is not NULL terminated as we create the group dynamically */
> };
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index c80032322158..a57232a9a6f9 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -131,9 +131,6 @@ struct thermal_cooling_device {
> trip point.
> * @prev_high_trip: the above current temperature if you've crossed a
> passive trip point.
> - * @forced_passive: If > 0, temperature at which to switch on all ACPI
> - * processor cooling devices. Currently only used by the
> - * step-wise governor.
> * @need_update: if equals 1, thermal_zone_device_update needs to be invoked.
> * @ops: operations this &thermal_zone_device supports
> * @tzp: thermal zone parameters
> @@ -167,7 +164,6 @@ struct thermal_zone_device {
> int passive;
> int prev_low_trip;
> int prev_high_trip;
> - unsigned int forced_passive;
> atomic_t need_update;
> struct thermal_zone_device_ops *ops;
> struct thermal_zone_params *tzp;
>
--
Warm Regards
Thara
On 12/14/20 6:38 PM, Daniel Lezcano wrote:
> The THERMAL_TRIPS_NONE is equal to -1, it is pointless to do a
> conversion in this function.
It is pointless because you have removed force_passive and
will be removing THERMAL_TRIPS_NONE in the following patches.
Maybe that should be mentioned in the commit message.
Otherwise,
Reviewed-by: Thara Gopinath <[email protected]>
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/thermal_sysfs.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 4e7f9e880d76..345917a58f2f 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -875,10 +875,7 @@ trip_point_show(struct device *dev, struct device_attribute *attr, char *buf)
> instance =
> container_of(attr, struct thermal_instance, attr);
>
> - if (instance->trip == THERMAL_TRIPS_NONE)
> - return sprintf(buf, "-1\n");
> - else
> - return sprintf(buf, "%d\n", instance->trip);
> + return sprintf(buf, "%d\n", instance->trip);
> }
>
> ssize_t
>
--
Warm Regards
Thara