thermal/core: Update cooling device during thermal zone unregistration
Subsystems (2)
Zhang Rui
reviewerPATCH v2 1/5 thermal/core: Update cooling device during thermal zone unregistration
When unregistering a thermal zone device, update all cooling devices bound to the thermal zone device.
This fixes a problem that the frequency of ACPI processors are still limited after unloading ACPI thermal driver while ACPI passive cooling is activated.
---
v1 -> v2
Changelog update.
Rearrange the code to elimiate an "iterator used outside loop"
warning.
--- drivers/thermal/thermal_core.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
drivers/thermal/thermal_core.c +26 -1
@@ -1497,9 +1497,24 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)/* Unbind all cdevs associated with 'this' thermal zone */ list_for_each_entry (cdev, &thermal_cdev_list, node) { +struct thermal_instance *ti; + +mutex_lock (&tz->lock); +list_for_each_entry (ti, &tz->thermal_instances, tz_node) { +if (ti->cdev == cdev) { +mutex_unlock (&tz->lock); + goto unbind; + } + } + +/* The cooling device is not bound to current thermal zone */ +mutex_unlock (&tz->lock); +continue ; + +unbind:if (tz->ops->unbind) { tz->ops->unbind (tz, cdev); -continue ; + goto deactivate; }if (!tzp || !tzp->tbp)
@@ -1511,6 +1526,16 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) tzp->tbp[i].cdev =NULL ; } } + +deactivate: +/* +* The thermal instances for current thermal zone has been +* removed. Update the cooling device in case it is activated +* by current thermal zone device. +*/ +mutex_lock (&cdev->lock); +__thermal_cdev_update (cdev); +mutex_unlock (&cdev->lock); }mutex_unlock (&thermal_list_lock);
Signature · 1 line
2.25.1
Zhang Rui
reviewerPATCH 2/5 thermal/core: Reset cooling state during cooling device unregistration
When unregistering a cooling device, it is possible that the cooling device has been activated. And once the cooling device is unregistered, no one will deactivate it anymore.
Reset cooling state during cooling device unregistration.
---
In theory, this problem that this patch fixes can be triggered on a platform with ACPI Active cooling, by
1. overheat the system to trigger ACPI active cooling
2. unload ACPI fan driver
3. check if the fan is still spinning
But I don't have such a system so I didn't trigger then problem and I only did build & boot test.
--- drivers/thermal/thermal_core.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/thermal/thermal_core.c +4 -0
@@ -1192,6 +1192,10 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev) } } +mutex_lock (&cdev->lock); + cdev->ops->set_cur_state (cdev,0 ); +mutex_unlock (&cdev->lock); +mutex_unlock (&thermal_list_lock);device_unregister (&cdev->device);
Signature · 1 line
2.25.1
Rafael J. Wysocki
maintainerRe: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration
Quoted 8 lines · Zhang Rui
When unregistering a cooling device, it is possible that the cooling device has been activated. And once the cooling device is unregistered, no one will deactivate it anymore.
Reset cooling state during cooling device unregistration.
Signed-off-by Zhang Rui <[email protected]>
Quoted 8 lines
---
In theory, this problem that this patch fixes can be triggered on a platform with ACPI Active cooling, by
1. overheat the system to trigger ACPI active cooling
2. unload ACPI fan driver
3. check if the fan is still spinning
But I don't have such a system so I didn't trigger then problem and I only did build & boot test.
So I'm not sure if this change is actually safe.
In the example above, the system will still need the fan to spin after the ACPI fan driver is unloaded in order to cool down, won't it?
Quoted 22 lines
--- drivers/thermal/thermal_core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/thermal/thermal_core.c +4
-1@@ -1192,6 +1192,10 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev) } } +mutex_lock (&cdev->lock); + cdev->ops->set_cur_state (cdev,0 ); +mutex_unlock (&cdev->lock); +mutex_unlock (&thermal_list_lock);device_unregister (&cdev->device); --2.25.1
Zhang, Rui
reviewerRe: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration
Quoted 21 lines · Rafael J. Wysocki
Quoted 9 lines · Zhang Rui
When unregistering a cooling device, it is possible that the cooling
device has been activated. And once the cooling device is unregistered,
no one will deactivate it anymore.Reset cooling state during cooling device unregistration.
Signed-off-by Zhang Rui <[email protected]>Quoted 9 lines
---
In theory, this problem that this patch fixes can be triggered on a platform with ACPI Active cooling, by
1. overheat the system to trigger ACPI active cooling
2. unload ACPI fan driver
3. check if the fan is still spinning
But I don't have such a system so I didn't trigger then problem and I
only did build & boot test.
Quoted 5 lines
So I'm not sure if this change is actually safe.
In the example above, the system will still need the fan to spin after
the ACPI fan driver is unloaded in order to cool down, won't it?
Then we can argue that the ACPI fan driver should not be unloaded in this case.
Actually, this is the same situation as patch 1/5.
Patch 1/5 fixes the problem that cooling state not restored to 0 when unloading the thermal driver, and this fixes the same problem when unloading the cooling device driver.
thanks,
rui
Quoted 26 lines
Quoted 26 lines
--- drivers/thermal/thermal_core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/thermal/thermal_core.c +0
-0@@ -1192,6 +1192,10 @@ void
thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)} }drivers/thermal/thermal_core.c +4
-1+mutex_lock (&cdev->lock); + cdev->ops->set_cur_state (cdev,0 ); +mutex_unlock (&cdev->lock); +mutex_unlock (&thermal_list_lock);device_unregister (&cdev->device); --2.25.1
Rafael J. Wysocki
maintainerRe: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration
Quoted 23 lines · Rui
Quoted 21 lines · Rafael J. Wysocki
Quoted 18 lines · Zhang Rui
When unregistering a cooling device, it is possible that the cooling
device has been activated. And once the cooling device is unregistered,
no one will deactivate it anymore.Reset cooling state during cooling device unregistration.
Signed-off-by Zhang Rui <[email protected]>---
In theory, this problem that this patch fixes can be triggered on a platform with ACPI Active cooling, by
1. overheat the system to trigger ACPI active cooling
2. unload ACPI fan driver
3. check if the fan is still spinning
But I don't have such a system so I didn't trigger then problem and I
only did build & boot test.
Quoted 8 lines
Quoted 5 lines
So I'm not sure if this change is actually safe.
In the example above, the system will still need the fan to spin after
the ACPI fan driver is unloaded in order to cool down, won't it?Then we can argue that the ACPI fan driver should not be unloaded in this case.
I don't think that whether or not the driver is expected to be unloaded at a given time has any bearing on how it should behave when actually unloaded.
Leaving the cooling device in its current state is "safe" from the thermal control perspective, but it may affect the general user experience (which may include performance too) going forward, so there is a tradeoff.
You can argue that even if the cooling device is reset on the driver removal, there should be another thermal control mechanism in place that will take care of the overheat condition instead of it, but that mechanism may be an emergency system shutdown.
What do the other cooling device drivers do in general when they get removed?
Quoted 4 lines
Actually, this is the same situation as patch 1/5.
Patch 1/5 fixes the problem that cooling state not restored to 0 when unloading the thermal driver, and this fixes the same problem when unloading the cooling device driver.
Right, it is analogous.
Zhang, Rui
reviewerRe: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration
Quoted 40 lines · Rafael J. Wysocki
Quoted 26 lines · Rui
Quoted 25 lines · Rafael J. Wysocki
Quoted 9 lines · Zhang Rui
When unregistering a cooling device, it is possible that the cooling
device has been activated. And once the cooling device is unregistered,
no one will deactivate it anymore.Reset cooling state during cooling device unregistration.
Signed-off-by Zhang Rui <[email protected]>Quoted 11 lines
---
In theory, this problem that this patch fixes can be triggered on a
platform with ACPI Active cooling, by
1. overheat the system to trigger ACPI active cooling
2. unload ACPI fan driver
3. check if the fan is still spinning
But I don't have such a system so I didn't trigger then problem and
I
only did build & boot test.So I'm not sure if this change is actually safe.
Quoted 7 lines
Quoted 3 lines
In the example above, the system will still need the fan to spin after
the ACPI fan driver is unloaded in order to cool down, won't it?Then we can argue that the ACPI fan driver should not be unloaded in
this case.I don't think that whether or not the driver is expected to be unloaded at a given time has any bearing on how it should behave when actually unloaded.
Quoted 5 lines
Leaving the cooling device in its current state is "safe" from the thermal control perspective, but it may affect the general user experience (which may include performance too) going forward, so there
is a tradeoff.
Right.
If we don't have a third choice, then the question is simple. "thermal safety" vs. "user experience"?
I'd vote for "thermal safety" and drop this patch series.
Quoted 3 lines
What do the other cooling device drivers do in general when they get removed?
No cooling device driver has extra handling after cdev unregistration.
thanks,
rui
Rafael J. Wysocki
maintainerRe: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration
Quoted 48 lines · Rui
Quoted 40 lines · Rafael J. Wysocki
Quoted 26 lines · Rui
Quoted 25 lines · Rafael J. Wysocki
Quoted 9 lines · Zhang Rui
When unregistering a cooling device, it is possible that the cooling
device has been activated. And once the cooling device is unregistered,
no one will deactivate it anymore.Reset cooling state during cooling device unregistration.
Signed-off-by Zhang Rui <[email protected]>Quoted 11 lines
---
In theory, this problem that this patch fixes can be triggered on a
platform with ACPI Active cooling, by
1. overheat the system to trigger ACPI active cooling
2. unload ACPI fan driver
3. check if the fan is still spinning
But I don't have such a system so I didn't trigger then problem and
I
only did build & boot test.So I'm not sure if this change is actually safe.
Quoted 7 lines
Quoted 3 lines
In the example above, the system will still need the fan to spin after
the ACPI fan driver is unloaded in order to cool down, won't it?Then we can argue that the ACPI fan driver should not be unloaded in
this case.I don't think that whether or not the driver is expected to be unloaded at a given time has any bearing on how it should behave when actually unloaded.
Quoted 5 lines
Leaving the cooling device in its current state is "safe" from the thermal control perspective, but it may affect the general user experience (which may include performance too) going forward, so there
is a tradeoff.
Quoted 5 lines
Right.
If we don't have a third choice, then the question is simple. "thermal safety" vs. "user experience"?I'd vote for "thermal safety" and drop this patch series.
Works for me.
Quoted 4 lines
Quoted 2 lines
What do the other cooling device drivers do in general when they get removed?
No cooling device driver has extra handling after cdev unregistration.
However, the question regarding what to do when the driver of a cooling device in use is being removed is a valid one.
One possible approach that comes to mind could be to defer the driver removal until the overheat condition goes away, but anyway it would be better to do that in the core IMV.
Zhang, Rui
reviewerRe: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration
Quoted 7 lines · Rafael J. Wysocki
Quoted 6 lines
Quoted 3 lines
What do the other cooling device drivers do in general when they get
removed?No cooling device driver has extra handling after cdev unregistration.
Quoted 7 lines
However, the question regarding what to do when the driver of a cooling device in use is being removed is a valid one.
One possible approach that comes to mind could be to defer the driver removal until the overheat condition goes away, but anyway it would be
better to do that in the core IMV.
In this case, we should guarantee that the thermal zone driver is still functional. i.e. it still can get temperature change notifications and update the thermal zone. I doubt if current thermal zone drivers can guarantee this.
Given that this is a rare case, and the current behavior is not perfect but still acceptable, maybe we can leave this low priority for now.
thanks,
rui
Zhang Rui
reviewerPATCH 3/5 thermal/core: Remove thermal_bind_params structure
In reply to Zhang RuiRemove struct thermal_bind_params because no one is using it for thermal binding now.
--- .../driver-api/thermal/sysfs-api.rst | 40 ------ drivers/thermal/thermal_core.c | 134 ++---------------- include/linux/thermal.h | 38 ----- 3 files changed, 14 insertions(+), 198 deletions(-)
Documentation/driver-api/thermal/sysfs-api.rst +0 -40
@@ -304,42 +304,6 @@ temperature) and throttle appropriate devices. 1.4 Thermal Zone Parameters --------------------------- - :: - - struct thermal_bind_params - - This structure defines the following parameters that are used to bind - a zone with a cooling device for a particular trip point. - - .cdev: - The cooling device pointer - .weight: - The 'influence' of a particular cooling device on this - zone. This is relative to the rest of the cooling - devices. For example, if all cooling devices have a - weight of 1, then they all contribute the same. You can - use percentages if you want, but it's not mandatory. A - weight of 0 means that this cooling device doesn't - contribute to the cooling of this zone unless all cooling - devices have a weight of 0. If all weights are 0, then - they all contribute the same. - .trip_mask: - This is a bit mask that gives the binding relation between - this thermal zone and cdev, for a particular trip point. - If nth bit is set, then the cdev and thermal zone are bound - for trip point n. - .binding_limits: - This is an array of cooling state limits. Must have - exactly 2 * thermal_zone.number_of_trip_points. It is an - array consisting of tuples <lower-state upper-state> of - state limits. Each trip will be associated with one state - limit tuple when binding. A NULL pointer means - <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> on all trips. - These limits are used when binding a cdev to a trip point. - .match: - This call back returns success(0) if the 'tz and cdev' need to - be bound, as per platform data. - :: struct thermal_zone_params
@@ -357,10 +321,6 @@ temperature) and throttle appropriate devices. will be created. when no_hwmon == true, nothing will be done. In case the thermal_zone_params is NULL, the hwmon interface will be created (for backward compatibility). - .num_tbps: - Number of thermal_bind_params entries for this zone - .tbp: - thermal_bind_params entries 2. sysfs attributes structure =============================
drivers/thermal/thermal_core.c +14 -120
202 lines not on this page · raw message
include/linux/thermal.h +0 -38
@@ -207,41 +207,6 @@ struct thermal_governor {struct list_head governor_list; }; -/* Structure that holds binding parameters for a zone */ -struct thermal_bind_params { -struct thermal_cooling_device *cdev; - -/* -* This is a measure of 'how effectively these devices can -* cool 'this' thermal zone. It shall be determined by -* platform characterization. This value is relative to the -* rest of the weights so a cooling device whose weight is -* double that of another cooling device is twice as -* effective. See Documentation/driver-api/thermal/sysfs-api.rst for more -* information. -*/ -int weight; - -/* -* This is a bit mask that gives the binding relation between this -* thermal zone and cdev, for a particular trip point. -* See Documentation/driver-api/thermal/sysfs-api.rst for more information. -*/ -int trip_mask; - -/* -* This is an array of cooling state limits. Must have exactly -* 2 * thermal_zone.number_of_trip_points. It is an array consisting -* of tuples <lower-state upper-state> of state limits. Each trip -* will be associated with one state limit tuple when binding. -* A NULL pointer means <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> -* on all trips. -*/ -unsigned long *binding_limits; -int (*match) (struct thermal_zone_device *tz, -struct thermal_cooling_device *cdev); -}; -/* Structure to define Thermal Zone parameters */ struct thermal_zone_params {char governor_name[THERMAL_NAME_LENGTH ];
@@ -253,9 +218,6 @@ struct thermal_zone_params {*/ bool no_hwmon; -int num_tbps;/* Number of tbp entries */ -struct thermal_bind_params *tbp; -/* * Sustainable power (heat) that this thermal zone can dissipate in * mW
Signature · 1 line
2.25.1
Rafael J. Wysocki
maintainerRe: [PATCH 3/5] thermal/core: Remove thermal_bind_params structure
Quoted 5 lines · Zhang Rui
Remove struct
thermal_bind_paramsbecause no one is using it for thermal binding now.Signed-off-by Zhang Rui <[email protected]>
I think that this patch is useful by itself. Does it depend on the rest of the series?
Quoted 336 lines
--- .../driver-api/thermal/sysfs-api.rst | 40 ------ drivers/thermal/thermal_core.c | 134 ++---------------- include/linux/thermal.h | 38 ----- 3 files changed, 14 insertions(+), 198 deletions(-)Documentation/driver-api/thermal/sysfs-api.rst +0
-40@@ -304,42 +304,6 @@ temperature) and throttle appropriate devices. 1.4 Thermal Zone Parameters --------------------------- - :: - - struct thermal_bind_params - - This structure defines the following parameters that are used to bind - a zone with a cooling device for a particular trip point. - - .cdev: - The cooling device pointer - .weight: - The 'influence' of a particular cooling device on this - zone. This is relative to the rest of the cooling - devices. For example, if all cooling devices have a - weight of 1, then they all contribute the same. You can - use percentages if you want, but it's not mandatory. A - weight of 0 means that this cooling device doesn't - contribute to the cooling of this zone unless all cooling - devices have a weight of 0. If all weights are 0, then - they all contribute the same. - .trip_mask: - This is a bit mask that gives the binding relation between - this thermal zone and cdev, for a particular trip point. - If nth bit is set, then the cdev and thermal zone are bound - for trip point n. - .binding_limits: - This is an array of cooling state limits. Must have - exactly 2 * thermal_zone.number_of_trip_points. It is an - array consisting of tuples <lower-state upper-state> of - state limits. Each trip will be associated with one state - limit tuple when binding. A NULL pointer means - <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> on all trips. - These limits are used when binding a cdev to a trip point. - .match: - This call back returns success(0) if the 'tz and cdev' need to - be bound, as per platform data. - :: struct thermal_zone_params@@ -357,10 +321,6 @@ temperature) and throttle appropriate devices. will be created. when no_hwmon == true, nothing will be done. In case the thermal_zone_params is NULL, the hwmon interface will be created (for backward compatibility). - .num_tbps: - Number of thermal_bind_params entries for this zone - .tbp: - thermal_bind_params entries 2. sysfs attributes structure =============================drivers/thermal/thermal_core.c +14
-120@@ -794,65 +794,20 @@ void print_bind_err_msg(struct thermal_zone_device *tz, tz->type, cdev->type, ret); } -static void __bind (struct thermal_zone_device *tz,int mask, -struct thermal_cooling_device *cdev, -unsigned long *limits, -unsigned int weight) -{ -int i, ret; - -for (i =0 ; i < tz->num_trips; i++) { -if (mask & (1 << i)) { -unsigned long upper, lower; - - upper =THERMAL_NO_LIMIT ; - lower =THERMAL_NO_LIMIT ; -if (limits) { - lower = limits[i *2 ]; - upper = limits[i *2 +1 ]; - } - ret =thermal_zone_bind_cooling_device (tz, i, cdev, - upper, lower, - weight); -if (ret) -print_bind_err_msg (tz, cdev, ret); - } - } -} -static void bind_cdev (struct thermal_cooling_device *cdev) { -int i, ret; -const struct thermal_zone_params *tzp; +int ret;struct thermal_zone_device *pos =NULL ;mutex_lock (&thermal_list_lock);list_for_each_entry (pos, &thermal_tz_list, node) { -if (!pos->tzp && !pos->ops->bind) +if (!pos->ops->bind)continue ; -if (pos->ops->bind) { - ret = pos->ops->bind (pos, cdev); -if (ret) -print_bind_err_msg (pos, cdev, ret); -continue ; - } - - tzp = pos->tzp; -if (!tzp || !tzp->tbp) -continue ; - -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev || !tzp->tbp[i].match) -continue ; -if (tzp->tbp[i].match (pos, cdev)) -continue ; - tzp->tbp[i].cdev = cdev; -__bind (pos, tzp->tbp[i].trip_mask, cdev, - tzp->tbp[i].binding_limits, - tzp->tbp[i].weight); - } + ret = pos->ops->bind (pos, cdev); +if (ret) +print_bind_err_msg (pos, cdev, ret); }mutex_unlock (&thermal_list_lock);@@ -1138,16 +1093,6 @@ void thermal_cooling_device_update(struct thermal_cooling_device *cdev) }EXPORT_SYMBOL_GPL (thermal_cooling_device_update); -static void __unbind (struct thermal_zone_device *tz,int mask, -struct thermal_cooling_device *cdev) -{ -int i; - -for (i =0 ; i < tz->num_trips; i++) -if (mask & (1 << i)) -thermal_zone_unbind_cooling_device (tz, i, cdev); -} -/** * thermal_cooling_device_unregister - removes a thermal cooling device * @cdev: the thermal cooling device to remove. @@ -1157,8 +1102,6 @@ static void __unbind(struct thermal_zone_device *tz, int mask,*/ void thermal_cooling_device_unregister (struct thermal_cooling_device *cdev) { -int i; -const struct thermal_zone_params *tzp;struct thermal_zone_device *tz;if (!cdev)@@ -1175,21 +1118,8 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)/* Unbind all thermal zones associated with 'this' cdev */ list_for_each_entry (tz, &thermal_tz_list, node) { -if (tz->ops->unbind) { +if (tz->ops->unbind) tz->ops->unbind (tz, cdev); -continue ; - } - -if (!tz->tzp || !tz->tzp->tbp) -continue ; - - tzp = tz->tzp; -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev == cdev) { -__unbind (tz, tzp->tbp[i].trip_mask, cdev); - tzp->tbp[i].cdev =NULL ; - } - } }mutex_lock (&cdev->lock);@@ -1204,41 +1134,20 @@ EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);static void bind_tz (struct thermal_zone_device *tz) { -int i, ret; +int ret;struct thermal_cooling_device *pos =NULL ; -const struct thermal_zone_params *tzp = tz->tzp; -if (!tzp && !tz->ops->bind) +if (!tz->ops->bind)return ;mutex_lock (&thermal_list_lock); -/* If there is ops->bind, try to use ops->bind */ -if (tz->ops->bind) { -list_for_each_entry (pos, &thermal_cdev_list, node) { - ret = tz->ops->bind (tz, pos); -if (ret) -print_bind_err_msg (tz, pos, ret); - } - goto exit; - } - -if (!tzp || !tzp->tbp) - goto exit; -list_for_each_entry (pos, &thermal_cdev_list, node) { -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev || !tzp->tbp[i].match) -continue ; -if (tzp->tbp[i].match (tz, pos)) -continue ; - tzp->tbp[i].cdev = pos; -__bind (tz, tzp->tbp[i].trip_mask, pos, - tzp->tbp[i].binding_limits, - tzp->tbp[i].weight); - } + ret = tz->ops->bind (tz, pos); +if (ret) +print_bind_err_msg (tz, pos, ret); } -exit: +mutex_unlock (&thermal_list_lock); }@@ -1477,15 +1386,13 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_register);*/ void thermal_zone_device_unregister (struct thermal_zone_device *tz) { -int i, tz_id; -const struct thermal_zone_params *tzp; +int tz_id;struct thermal_cooling_device *cdev;struct thermal_zone_device *pos =NULL ;if (!tz)return ; - tzp = tz->tzp; tz_id = tz->id;mutex_lock (&thermal_list_lock);@@ -1516,22 +1423,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)continue ; unbind: -if (tz->ops->unbind) { +if (tz->ops->unbind) tz->ops->unbind (tz, cdev); - goto deactivate; - } - -if (!tzp || !tzp->tbp) -break ; - -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev == cdev) { -__unbind (tz, tzp->tbp[i].trip_mask, cdev); - tzp->tbp[i].cdev =NULL ; - } - } -deactivate:/* * The thermal instances for current thermal zone has been * removed. Update the cooling device in case it is activated include/linux/thermal.h +0
-39@@ -207,41 +207,6 @@ struct thermal_governor {struct list_head governor_list; }; -/* Structure that holds binding parameters for a zone */ -struct thermal_bind_params { -struct thermal_cooling_device *cdev; - -/* -* This is a measure of 'how effectively these devices can -* cool 'this' thermal zone. It shall be determined by -* platform characterization. This value is relative to the -* rest of the weights so a cooling device whose weight is -* double that of another cooling device is twice as -* effective. See Documentation/driver-api/thermal/sysfs-api.rst for more -* information. -*/ -int weight; - -/* -* This is a bit mask that gives the binding relation between this -* thermal zone and cdev, for a particular trip point. -* See Documentation/driver-api/thermal/sysfs-api.rst for more information. -*/ -int trip_mask; - -/* -* This is an array of cooling state limits. Must have exactly -* 2 * thermal_zone.number_of_trip_points. It is an array consisting -* of tuples <lower-state upper-state> of state limits. Each trip -* will be associated with one state limit tuple when binding. -* A NULL pointer means <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> -* on all trips. -*/ -unsigned long *binding_limits; -int (*match) (struct thermal_zone_device *tz, -struct thermal_cooling_device *cdev); -}; -/* Structure to define Thermal Zone parameters */ struct thermal_zone_params {char governor_name[THERMAL_NAME_LENGTH ];@@ -253,9 +218,6 @@ struct thermal_zone_params {*/ bool no_hwmon; -int num_tbps;/* Number of tbp entries */ -struct thermal_bind_params *tbp; -/* * Sustainable power (heat) that this thermal zone can dissipate in * mW -- 2.25.1
Zhang, Rui
reviewerRe: [PATCH 3/5] thermal/core: Remove thermal_bind_params structure
Quoted 10 lines · Rafael J. Wysocki
Quoted 5 lines · Zhang Rui
Remove struct
thermal_bind_paramsbecause no one is using it for thermal
binding now.Signed-off-by Zhang Rui <[email protected]>I think that this patch is useful by itself. Does it depend on the rest of the series?
Yes. But I have refreshed it and will submit soon.
thanks,
rui
Quoted 384 lines
Quoted 384 lines
--- .../driver-api/thermal/sysfs-api.rst | 40 ------ drivers/thermal/thermal_core.c | 134 ++---------- ------ include/linux/thermal.h | 38 ----- 3 files changed, 14 insertions(+), 198 deletions(-)Documentation/driver-api/thermal/sysfs-api.rst +0
-5@@ -304,42 +304,6 @@ temperature) and throttle appropriate devices. 1.4 Thermal Zone Parameters --------------------------- - :: - - struct thermal_bind_params - - This structure defines the following parameters that are usedto bind
Documentation/driver-api/thermal/sysfs-api.rst +0
-9- a zone with a cooling device for a particular trip point. - - .cdev: - The cooling device pointer - .weight: - The 'influence' of a particular cooling device on this - zone. This is relative to the rest of the cooling - devices. For example, if all cooling devices have a - weight of 1, then they all contribute the same. Youcan
Documentation/driver-api/thermal/sysfs-api.rst +0
-3- use percentages if you want, but it's not mandatory. A - weight of 0 means that this cooling device doesn't - contribute to the cooling of this zone unless allcooling
Documentation/driver-api/thermal/sysfs-api.rst +0
-4- devices have a weight of 0. If all weights are 0, then - they all contribute the same. - .trip_mask: - This is a bit mask that gives the binding relationbetween
- this thermal zone and cdev, for a particular trip
point.
- If nth bit is set, then the cdev and thermal zone
are boundDocumentation/driver-api/thermal/sysfs-api.rst +0
-3- for trip point n. - .binding_limits: - This is an array of cooling state limits. Musthave
- exactly 2 *
thermal_zone.number_of_trip_points. It is an
- array consisting of tuples <lower-state upper-
state> of
- state limits. Each trip will be associated
with one stateDocumentation/driver-api/thermal/sysfs-api.rst +0
-2- limit tuple when binding. A NULL pointer means - <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> on alltrips.
- These limits are used when binding a cdev to a
trip point.Documentation/driver-api/thermal/sysfs-api.rst +0
-2- .match: - This call back returns success(0) if the 'tz and cdev'need to
Documentation/driver-api/thermal/sysfs-api.rst +0
-2- be bound, as per platform data. - :: struct thermal_zone_params@@ -357,10 +321,6 @@ temperature) and throttle appropriate devices. will be created. when no_hwmon == true, nothing willbe done.
In case thethermal_zone_paramsis NULL, the hwmon
interface
will be created (for backward compatibility).Documentation/driver-api/thermal/sysfs-api.rst +0
-4- .num_tbps: - Number of thermal_bind_params entries for this zone - .tbp: - thermal_bind_params entries 2. sysfs attributes structure =============================drivers/thermal/thermal_core.c +5
-60@@ -794,65 +794,20 @@ void print_bind_err_msg(struct thermal_zone_device *tz, tz->type, cdev->type, ret); } -static void __bind (struct thermal_zone_device *tz,int mask, -struct thermal_cooling_device *cdev, -unsigned long *limits, -unsigned int weight) -{ -int i, ret; - -for (i =0 ; i < tz->num_trips; i++) { -if (mask & (1 << i)) { -unsigned long upper, lower; - - upper =THERMAL_NO_LIMIT ; - lower =THERMAL_NO_LIMIT ; -if (limits) { - lower = limits[i *2 ]; - upper = limits[i *2 +1 ]; - } - ret =thermal_zone_bind_cooling_device (tz, i, cdev, - uppe r, lower, - weig ht); -if (ret) -print_bind_err_msg (tz, cdev, ret); - } - } -} -static void bind_cdev (struct thermal_cooling_device *cdev) { -int i, ret; -const struct thermal_zone_params *tzp; +int ret;struct thermal_zone_device *pos =NULL ;mutex_lock (&thermal_list_lock);list_for_each_entry (pos, &thermal_tz_list, node) { -if (!pos->tzp && !pos->ops->bind) +if (!pos->ops->bind)continue ; -if (pos->ops->bind) { - ret = pos->ops->bind (pos, cdev); -if (ret) -print_bind_err_msg (pos, cdev, ret); -continue ; - } - - tzp = pos->tzp; -if (!tzp || !tzp->tbp) -continue ; - -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev || !tzp->tbp[i].match) -continue ; -if (tzp->tbp[i].match (pos, cdev)) -continue ; - tzp->tbp[i].cdev = cdev; -__bind (pos, tzp->tbp[i].trip_mask, cdev, - tzp->tbp[i].binding_limits, - tzp->tbp[i].weight); - } + ret = pos->ops->bind (pos, cdev); +if (ret) +print_bind_err_msg (pos, cdev, ret); }mutex_unlock (&thermal_list_lock);@@ -1138,16 +1093,6 @@ void thermal_cooling_device_update(struct thermal_cooling_device *cdev) }EXPORT_SYMBOL_GPL (thermal_cooling_device_update); -static void __unbind (struct thermal_zone_device *tz,int mask, -struct thermal_cooling_device *cdev) -{ -int i; - -for (i =0 ; i < tz->num_trips; i++) -if (mask & (1 << i)) -thermal_zone_unbind_cooling_device (tz, i, cdev); -} -/** *
thermal_cooling_device_unregister- removes a thermal cooling
device
* @cdev: the thermal cooling device to remove.drivers/thermal/thermal_core.c +0
-2@@ -1157,8 +1102,6 @@ static void __unbind(structthermal_zone_device *tz, int mask, */ void thermal_cooling_device_unregister (struct thermal_cooling_device *cdev) { -int i; -const struct thermal_zone_params *tzp;struct thermal_zone_device *tz;if (!cdev)drivers/thermal/thermal_core.c +0
-0@@ -1175,21 +1118,8 @@ void
thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)/* Unbind all thermal zones associated with 'this' cdev */ list_for_each_entry(tz, &thermal_tz_list, node) {drivers/thermal/thermal_core.c +1
-11-if (tz->ops->unbind) { +if (tz->ops->unbind) tz->ops->unbind (tz, cdev); -continue ; - } - -if (!tz->tzp || !tz->tzp->tbp) -continue ; - - tzp = tz->tzp; -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev == cdev) { - __unbind(tz, tzp->tbp[i].trip_mask,cdev);
drivers/thermal/thermal_core.c +6
-30- tzp->tbp[i].cdev =NULL ; - } - } }mutex_lock (&cdev->lock);@@ -1204,41 +1134,20 @@EXPORT_SYMBOL_GPL (thermal_cooling_device_unregister);static void bind_tz (struct thermal_zone_device *tz) { -int i, ret; +int ret;struct thermal_cooling_device *pos =NULL ; -const struct thermal_zone_params *tzp = tz->tzp; -if (!tzp && !tz->ops->bind) +if (!tz->ops->bind)return ;mutex_lock (&thermal_list_lock); -/* If there is ops->bind, try to use ops->bind */ -if (tz->ops->bind) { -list_for_each_entry (pos, &thermal_cdev_list, node) { - ret = tz->ops->bind (tz, pos); -if (ret) -print_bind_err_msg (tz, pos, ret); - } - goto exit; - } - -if (!tzp || !tzp->tbp) - goto exit; -list_for_each_entry (pos, &thermal_cdev_list, node) { -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev || !tzp->tbp[i].match) -continue ; -if (tzp->tbp[i].match (tz, pos)) -continue ; - tzp->tbp[i].cdev = pos; -__bind (tz, tzp->tbp[i].trip_mask, pos, - tzp->tbp[i].binding_limits, - tzp->tbp[i].weight); - } + ret = tz->ops->bind (tz, pos); +if (ret) +print_bind_err_msg (tz, pos, ret); } -exit: +mutex_unlock (&thermal_list_lock);}
drivers/thermal/thermal_core.c +1
-3@@ -1477,15 +1386,13 @@EXPORT_SYMBOL_GPL(thermal_zone_device_register); */ void thermal_zone_device_unregister (struct thermal_zone_device *tz) { -int i, tz_id; -const struct thermal_zone_params *tzp; +int tz_id;struct thermal_cooling_device *cdev;struct thermal_zone_device *pos =NULL ;if (!tz)return ; - tzp = tz->tzp; tz_id = tz->id;mutex_lock(&thermal_list_lock);drivers/thermal/thermal_core.c +1
-14@@ -1516,22 +1423,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) continue; unbind: -if (tz->ops->unbind) { +if (tz->ops->unbind) tz->ops->unbind (tz, cdev); - goto deactivate; - } - -if (!tzp || !tzp->tbp) -break ; - -for (i =0 ; i < tzp->num_tbps; i++) { -if (tzp->tbp[i].cdev == cdev) { -__unbind (tz, tzp->tbp[i].trip_mask, cdev); - tzp->tbp[i].cdev =NULL ; - } - } -deactivate:/* * The thermal instances for current thermal zone
has been
* removed. Update the cooling device in case it is
activatedinclude/linux/thermal.h +0
-11@@ -207,41 +207,6 @@ struct thermal_governor {struct list_head governor_list; }; -/* Structure that holds binding parameters for a zone */ -struct thermal_bind_params { -struct thermal_cooling_device *cdev; - -/* -* This is a measure of 'how effectively these devices can -* cool 'this' thermal zone. It shall be determined by -* platform characterization. This value is relative to the -* rest of the weights so a cooling device whose weight is -* double that of another cooling device is twice as -* effective. See Documentation/driver-api/thermal/sysfs-
api.rstfor moreinclude/linux/thermal.h +0
-6-* information. -*/ -int weight; - -/* -* This is a bit mask that gives the binding relation between this
include/linux/thermal.h +0
-2-* thermal zone and cdev, for a particular trip point. -* See Documentation/driver-api/thermal/sysfs-api.rst for more information.
include/linux/thermal.h +0
-5-*/ -int trip_mask; - -/* -* This is an array of cooling state limits. Must have exactly
- * 2 * thermal_zone.number_of_trip_points. It is an array
consisting
- * of tuples <lower-state upper-state> of state limits.
Each trip
- * will be associated with one state limit tuple when
binding.
- * A NULL pointer means <THERMAL_NO_LIMITSTHERMAL_NO_LIMITS>include/linux/thermal.h +0
-10-* on all trips. -*/ -unsigned long *binding_limits; -int (*match) (struct thermal_zone_device *tz, -struct thermal_cooling_device *cdev); -}; -/* Structure to define Thermal Zone parameters */ struct thermal_zone_params {char governor_name[THERMAL_NAME_LENGTH ];@@ -253,9 +218,6 @@ struct thermal_zone_params {*/ bool no_hwmon; -int num_tbps;/* Number of tbp entries */ -struct thermal_bind_params *tbp; -/* * Sustainable power (heat) that this thermal zone can dissipate in
* mW
--
2.25.1
Zhang Rui
reviewerPATCH 4/5 thermal/core: Enforce paired .bind/.unbind callbacks
In reply to Zhang RuiThe .bind/.unbind callbacks are designed to allow the thermal zone device to bind to/unbind from a matched cooling device, with thermal instances created/deleted.
In this sense, .bind/.unbind callbacks must exist in pairs.
--- drivers/thermal/thermal_core.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/thermal/thermal_core.c +5 -0
@@ -1258,6 +1258,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *tif (num_trips >0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)return ERR_PTR (-EINVAL ); +if ((ops->bind && !ops->unbind) || (!ops->bind && ops->unbind)) { +pr_err ("Thermal zone device .bind/.unbind not paired\n" ); +return ERR_PTR (-EINVAL ); + } +if (!thermal_class)return ERR_PTR (-ENODEV );
Signature · 1 line
2.25.1
Rafael J. Wysocki
maintainerRe: [PATCH 4/5] thermal/core: Enforce paired .bind/.unbind callbacks
Quoted 17 lines · Zhang Rui
The .bind/.unbind callbacks are designed to allow the thermal zone device to bind to/unbind from a matched cooling device, with thermal instances created/deleted.
In this sense, .bind/.unbind callbacks must exist in pairs.
Signed-off-by Zhang Rui <[email protected]>--- drivers/thermal/thermal_core.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/thermal/thermal_core.c +0
-0
drivers/thermal/thermal_core.c thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t · line 1261
Quoted 4 lines
drivers/thermal/thermal_core.c +1
-0@@ -1258,6 +1258,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *tif (num_trips >0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)return ERR_PTR (-EINVAL ); +if ((ops->bind && !ops->unbind) || (!ops->bind && ops->unbind)) {
This can be written as
if (!!ops->bind != !!ops->unbind) {drivers/thermal/thermal_core.c thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t · line 1262
Quoted 1 line
drivers/thermal/thermal_core.c +1
-0+pr_err ("Thermal zone device .bind/.unbind not paired\n" );
And surely none of the existing drivers do that? Because it would be a functional regression if they did.
Quoted 7 lines
drivers/thermal/thermal_core.c +3
-1+return ERR_PTR (-EINVAL ); + } +if (!thermal_class)return ERR_PTR (-ENODEV ); --
Rafael J. Wysocki
maintainerRe: [PATCH 4/5] thermal/core: Enforce paired .bind/.unbind callbacks
Quoted 22 lines · Rafael J. Wysocki
Quoted 20 lines · Zhang Rui
The .bind/.unbind callbacks are designed to allow the thermal zone device to bind to/unbind from a matched cooling device, with thermal instances created/deleted.
In this sense, .bind/.unbind callbacks must exist in pairs.
Signed-off-by Zhang Rui <[email protected]>--- drivers/thermal/thermal_core.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/thermal/thermal_core.c +0
-0@@ -1258,6 +1258,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *tif (num_trips >0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)return ERR_PTR (-EINVAL );
Quoted 5 lines
drivers/thermal/thermal_core.c thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t · line 1261
Quoted 1 line
drivers/thermal/thermal_core.c +1
-0+if ((ops->bind && !ops->unbind) || (!ops->bind && ops->unbind)) {This can be written as
if (!!ops->bind != !!ops->unbind) {
Or even
if (!ops->bind != !ops->unbind) {for that matter.
Quoted 13 lines
drivers/thermal/thermal_core.c thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t · line 1262
Quoted 2 lines
drivers/thermal/thermal_core.c +1
-0+pr_err ("Thermal zone device .bind/.unbind not paired\n" );And surely none of the existing drivers do that? Because it would be a functional regression if they did.
Quoted 7 lines
drivers/thermal/thermal_core.c +3
-1+return ERR_PTR (-EINVAL ); + } +if (!thermal_class)return ERR_PTR (-ENODEV ); --
Zhang, Rui
reviewerRe: [PATCH 4/5] thermal/core: Enforce paired .bind/.unbind callbacks
In reply to Rafael J. WysockiQuoted 31 lines · Rafael J. Wysocki
Quoted 20 lines · Zhang Rui
The .bind/.unbind callbacks are designed to allow the thermal zone device to bind to/unbind from a matched cooling device, with thermal
instances created/deleted.In this sense, .bind/.unbind callbacks must exist in pairs.
Signed-off-by Zhang Rui <[email protected]>--- drivers/thermal/thermal_core.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/thermal/thermal_core.c +0
-0@@ -1258,6 +1258,11 @@thermal_zone_device_register_with_trips(const char *type, struct
thermal_trip*tQuoted 6 lines
if (num_trips > 0 && (!ops->get_trip_type || !ops-Quoted 1 line
get_trip_temp) && !trips)return ERR_PTR(-EINVAL);+ if ((
ops->bind&& !ops->unbind) || (!ops->bind && ops-Quoted 1 line
unbind)) {
This can be written as
Quoted 7 lines
if (!!ops->bind != !!ops->unbind) {Quoted 2 lines
+ pr_err("Thermal zone device .bind/.unbind not
paired\n");And surely none of the existing drivers do that? Because it would be a functional regression if they did.
Yeah, I did a check and all drivers provide .bind/.unbind callbacks in pairs.
Hi, Daniel,
I know you're dealing with various of thermal drivers recently, are you aware of any exceptions?
thanks,
rui
Zhang Rui
reviewerPATCH 5/5 thermal/core: Avoid unbind for thermal zones without .unbind()
In reply to Zhang RuiThe ops->bind()/unbind() callback is the only way to bind/unbind a cooling device to/from a thermal zone.
Optimize the code to avoid trying unbind when .unbind() callback doesn't exist.
--- drivers/thermal/thermal_core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/thermal/thermal_core.c +5 -2
@@ -1411,6 +1411,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) }list_del (&tz->node); +if (!tz->ops->unbind) + goto unbind_done; +/* Unbind all cdevs associated with 'this' thermal zone */ list_for_each_entry (cdev, &thermal_cdev_list, node) {struct thermal_instance *ti;
@@ -1428,8 +1431,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)continue ; unbind: -if (tz->ops->unbind) - tz->ops->unbind (tz, cdev); + tz->ops->unbind (tz, cdev);/* * The thermal instances for current thermal zone has been
@@ -1441,6 +1443,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)mutex_unlock (&cdev->lock); } +unbind_done:mutex_unlock (&thermal_list_lock);cancel_delayed_work_sync (&tz->poll_queue);
Signature · 1 line
2.25.1