thermal: core/ACPI: Fix processor cooling device regression
Subsystems (3)
v3 cover thermal: core/ACPI: Fix processor cooling device regression
Hi All,
The two previous revisions of this patch series was posted as
https://lore.kernel.org/linux-pm/2148907.irdbgypaU6@kreacher/
https://lore.kernel.org/linux-acpi/2692681.mvXUDI8C0e@kreacher/
As reported by Rui in this thread:
some recent changes in the thermal core cause the CPU cooling devices registered by the ACPI processor driver to become unusable in some cases and somewhat crippled in general.
The problem is that the ACPI processor driver changes its ->get_max_state() callback return value depending on whether or not cpufreq is available and there is a cpufreq policy for a given CPU. However, the thermal core has always assumed that the return value of that callback will not change, which in fact is relied on by the cooling device statistics code. In particular, when the ->get_max_state() grows, the memory buffer allocated for storing the statistics will be too small and corruption may ensue as a result.
For this reason, the issue needs to be addressed in the ACPI processor driver and not in the thermal core, but the core needs to help somewhat too. Namely, it needs to provide a helper allowing an interested driver to update the max_state value for an already registered cooling device in certain situations which will also cause the statistics to be rebuilt.
This series implements the above and for details please refer to the individual patch chagelogs.
Thanks!
PATCH v3 1/4 ACPI: processor: Reorder acpi_processor_driver_init()
From: Rafael J. Wysocki <[email protected]>
The cpufreq policy notifier in the ACPI processor driver may as well be registered before the driver itself, which causes acpi_processor_cpufreq_init to be true (unless the notifier registration fails, which is unlikely at that point) when the ACPI CPU thermal cooling devices are registered, so the processor_get_max_state() result does not change while acpi_processor_driver_init() is running.
Change the ordering in acpi_processor_driver_init() accordingly to prevent the max_state value from remaining 0 permanently for all ACPI CPU cooling devices due to setting acpi_processor_cpufreq_init too late. [Note that processor_get_max_state() may still return different values at different times after this change, depending on the cpufreq driver registration time, but that issue needs to be addressed separately.]
---
v2 -> v3: Add tags from Rui.
v1 -> v2: Expand changelog to explain that this particular patch addresses
part of the issue.
--- drivers/acpi/processor_driver.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/acpi/processor_driver.c +6 -6
@@ -263,6 +263,12 @@ static int __init acpi_processor_driver_if (acpi_disabled)return 0 ; +if (!cpufreq_register_notifier (&acpi_processor_notifier_block, +CPUFREQ_POLICY_NOTIFIER )) { + acpi_processor_cpufreq_init = true; +acpi_processor_ignore_ppc_init (); + } + result =driver_register (&acpi_processor_driver);if (result <0 )return result;
@@ -276,12 +282,6 @@ static int __init acpi_processor_driver_cpuhp_setup_state_nocalls (CPUHP_ACPI_CPUDRV_DEAD ,"acpi/cpu-drv:dead" ,NULL , acpi_soft_cpu_dead); -if (!cpufreq_register_notifier (&acpi_processor_notifier_block, -CPUFREQ_POLICY_NOTIFIER )) { - acpi_processor_cpufreq_init = true; -acpi_processor_ignore_ppc_init (); - } -acpi_processor_throttling_init ();return 0 ; err:
PATCH v3 2/4 thermal: core: Introduce thermal_cooling_device_present()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Introduce a helper function, thermal_cooling_device_present(), for checking if the given cooling device is in the list of registered cooling devices to avoid some code duplication in a subsequent patch.
No expected functional impact.
---
v2 -> v3: Add tags from Rui.
v1 -> v2: No changes
--- drivers/thermal/thermal_core.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
drivers/thermal/thermal_core.c +15 -6
@@ -1045,6 +1045,18 @@ devm_thermal_of_cooling_device_register( }EXPORT_SYMBOL_GPL (devm_thermal_of_cooling_device_register); +static bool thermal_cooling_device_present (struct thermal_cooling_device *cdev) +{ +struct thermal_cooling_device *pos =NULL ; + +list_for_each_entry (pos, &thermal_cdev_list, node) { +if (pos == cdev) +return true; + } + +return false; +} +static void __unbind (struct thermal_zone_device *tz,int mask,struct thermal_cooling_device *cdev) {
@@ -1067,20 +1079,17 @@ void thermal_cooling_device_unregister(sint i;const struct thermal_zone_params *tzp;struct thermal_zone_device *tz; -struct thermal_cooling_device *pos =NULL ;if (!cdev)return ;mutex_lock (&thermal_list_lock); -list_for_each_entry (pos, &thermal_cdev_list, node) -if (pos == cdev) -break ; -if (pos != cdev) { -/* thermal cooling device not found */ + +if (!thermal_cooling_device_present (cdev)) {mutex_unlock (&thermal_list_lock);return ; } +list_del (&cdev->node);/* Unbind all thermal zones associated with 'this' cdev */
PATCH v3 3/4 thermal: core: Introduce thermal_cooling_device_update()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Introduce a core thermal API function, thermal_cooling_device_update(), for updating the max_state value for a cooling device and rearranging its statistics in sysfs after a possible change of its ->get_max_state() callback return value.
That callback is now invoked only once, during cooling device registration, to populate the max_state field in the cooling device object, so if its return value changes, it needs to be invoked again and the new return value needs to be stored as max_state. Moreover, the statistics presented in sysfs need to be rearranged in general, because there may not be enough room in them to store data for all of the possible states (in the case when max_state grows).
The new function takes care of that (and some other minor things related to it), but some extra locking and lockdep annotations are added in several places too to protect against crashes in the cases when the statistics are not present or when a stale max_state value might be used by sysfs attributes.
Note that the actual user of the new function will be added separately.
---
Rui, please have a look at this again and give it a go if possible. It is the only patch in the series changing in v3.
v2 -> v3:
Make thermal_cooling_device_update() check thermal instances involving the given thermal device and update the "upper" value for them if necessary (the "lower" and "target" values may need to be adjusted too in some cases) to avoid leaving thermal instances with "upper" above the new max_state of the corresponding cooling device.
v1 -> v2:
* Make thermal_cooling_device_update() do the full IS_ERR_OR_NULL() check on cdev, so that its callers don't need to do it.
* Remove ->set_cur_state() callback invocation from thermal_cooling_device_update(), because it is redundant (and it doesn't make sense to pass a state value exceeding max_state to it anyway).
--- drivers/thermal/thermal_core.c | 83 +++++++++++++++++++++++++++++++++++++++- drivers/thermal/thermal_core.h | 2 drivers/thermal/thermal_sysfs.c | 72 ++++++++++++++++++++++++++++++---- include/linux/thermal.h | 1 4 files changed, 149 insertions(+), 9 deletions(-)
drivers/thermal/thermal_core.c +82 -1
@@ -613,6 +613,7 @@ int thermal_zone_bind_cooling_device(strstruct thermal_instance *pos;struct thermal_zone_device *pos1;struct thermal_cooling_device *pos2; +bool upper_no_limit;int result;if (trip >= tz->num_trips || trip <0 )
@@ -632,7 +633,13 @@ int thermal_zone_bind_cooling_device(str/* lower default 0, upper default max_state */ lower = lower ==THERMAL_NO_LIMIT ?0 : lower; - upper = upper ==THERMAL_NO_LIMIT ? cdev->max_state : upper; + +if (upper ==THERMAL_NO_LIMIT ) { + upper = cdev->max_state; + upper_no_limit = true; + }else { + upper_no_limit = false; + }if (lower > upper || upper > cdev->max_state)return -EINVAL ;
@@ -644,6 +651,7 @@ int thermal_zone_bind_cooling_device(str dev->cdev = cdev; dev->trip = trip; dev->upper = upper; + dev->upper_no_limit = upper_no_limit; dev->lower = lower; dev->target =THERMAL_NO_TARGET ; dev->weight = weight;
@@ -1057,6 +1065,79 @@ static bool thermal_cooling_device_presereturn false; } +/** +* thermal_cooling_device_update - Update a cooling device object +* @cdev: Target cooling device. +* +* Update @cdev to reflect a change of the underlying hardware or platform. +* +* Must be called when the maximum cooling state of @cdev becomes invalid and so +* its .get_max_state() callback needs to be run to produce the new maximum +* cooling state value. +*/ +void thermal_cooling_device_update (struct thermal_cooling_device *cdev) +{ +struct thermal_instance *ti; +unsigned long state; + +if (IS_ERR_OR_NULL (cdev)) +return ; + +/* +* Hold thermal_list_lock throughout the update to prevent the device +* from going away while being updated. +*/ +mutex_lock (&thermal_list_lock); + +if (!thermal_cooling_device_present (cdev)) + goto unlock_list; + +/* +* Update under the cdev lock to prevent the state from being set beyond +* the new limit concurrently. +*/ +mutex_lock (&cdev->lock); + +if (cdev->ops->get_max_state (cdev, &cdev->max_state)) + goto unlock; + +thermal_cooling_device_stats_reinit (cdev); + +list_for_each_entry (ti, &cdev->thermal_instances, cdev_node) { +if (ti->upper == cdev->max_state) +continue ; + +if (ti->upper < cdev->max_state) { +if (ti->upper_no_limit) + ti->upper = cdev->max_state; + +continue ; + } + + ti->upper = cdev->max_state; +if (ti->lower > ti->upper) + ti->lower = ti->upper; + +if (ti->target ==THERMAL_NO_TARGET ) +continue ; + +if (ti->target > ti->upper) + ti->target = ti->upper; + } + +if (cdev->ops->get_cur_state (cdev, &state) || state > cdev->max_state) + goto unlock; + +thermal_cooling_device_stats_update (cdev, state); + +unlock: +mutex_unlock (&cdev->lock); + +unlock_list: +mutex_unlock (&thermal_list_lock); +} +EXPORT_SYMBOL_GPL (thermal_cooling_device_update); +static void __unbind (struct thermal_zone_device *tz,int mask,struct thermal_cooling_device *cdev) {
drivers/thermal/thermal_sysfs.c +64 -8
158 lines not on this page · raw message
drivers/thermal/thermal_core.h +2 -0
@@ -101,6 +101,7 @@ struct thermal_instance {struct list_head tz_node;/* node in tz->thermal_instances */ struct list_head cdev_node;/* node in cdev->thermal_instances */ unsigned int weight;/* The weight of the cooling device */ +bool upper_no_limit; };#define to_thermal_zone (_dev) \
@@ -127,6 +128,7 @@ int thermal_zone_create_device_groups(stvoid thermal_zone_destroy_device_groups (struct thermal_zone_device *);void thermal_cooling_device_setup_sysfs (struct thermal_cooling_device *);void thermal_cooling_device_destroy_sysfs (struct thermal_cooling_device *cdev); +void thermal_cooling_device_stats_reinit (struct thermal_cooling_device *cdev);/* used only at binding time */ ssize_t trip_point_show (struct device *,struct device_attribute *,char *);ssize_t weight_show (struct device *,struct device_attribute *,char *);
include/linux/thermal.h +1 -0
@@ -388,6 +388,7 @@ devm_thermal_of_cooling_device_register(struct device_node *np, char *type, void *devdata, const struct thermal_cooling_device_ops *ops); +void thermal_cooling_device_update (struct thermal_cooling_device *);void thermal_cooling_device_unregister (struct thermal_cooling_device *);struct thermal_zone_device *thermal_zone_get_zone_by_name (const char *name);int thermal_zone_get_temp (struct thermal_zone_device *tz,int *temp);
Re: [PATCH v3 3/4] thermal: core: Introduce thermal_cooling_device_update()
Quoted 26 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Introduce a core thermal API function,
thermal_cooling_device_update(),
for updating themax_statevalue for a cooling device and rearranging its statistics in sysfs after a possible change of its ->get_max_state()
callback return value.That callback is now invoked only once, during cooling device registration, to populate the
max_statefield in the cooling device object, so if its return value changes, it needs to be invoked again and the new return value needs to be stored asmax_state. Moreover, the statistics presented in sysfs need to be rearranged in general, because there may not be enough room in them to store data for all of the possible states (in the case whenmax_stategrows).The new function takes care of that (and some other minor things related to it), but some extra locking and lockdep annotations are added in several places too to protect against crashes in the cases when the statistics are not present or when a stale
max_statevalue might be used by sysfs attributes.Note that the actual user of the new function will be added separately.
Quoted 8 lines
Link:
https://lore.kernel.org/linux-pm/[email protected]/Signed-off-by Rafael J. Wysocki <[email protected]>---
Rui, please have a look at this again and give it a go if possible. It is the
only patch in the series changing in v3.
Hi, Rafael,
Thanks for the patch, I can confirm that it works, with ACPI passive cooling activated on my side.
thanks,
rui
Quoted 381 lines
v2 -> v3:
Makethermal_cooling_device_update()check thermal instances
involving the
given thermal device and update the "upper" value for them if
necessary (the
"lower" and "target" values may need to be adjusted too in some
cases) to avoid
leaving thermal instances with "upper" above the newmax_stateof
the
corresponding cooling device.v1 -> v2:
* Makethermal_cooling_device_update()do the fullIS_ERR_OR_NULL()check
on cdev, so that its callers don't need to do it.
* Remove ->set_cur_state() callback invocation fromthermal_cooling_device_update(), because it is redundant (and
it doesn't
make sense to pass a state value exceedingmax_stateto it
anyway).--- drivers/thermal/thermal_core.c | 83 +++++++++++++++++++++++++++++++++++++++- drivers/thermal/thermal_core.h | 2 drivers/thermal/thermal_sysfs.c | 72 ++++++++++++++++++++++++++++++---- include/linux/thermal.h | 1 4 files changed, 149 insertions(+), 9 deletions(-)Index:
linux-pm/drivers/thermal/thermal_core.c
===================================================================drivers/thermal/thermal_core.c +14
-1@@ -613,6 +613,7 @@ int thermal_zone_bind_cooling_device(strstruct thermal_instance *pos;struct thermal_zone_device *pos1;struct thermal_cooling_device *pos2; +bool upper_no_limit;int result;if (trip >= tz->num_trips || trip <0 )@@ -632,7 +633,13 @@ int thermal_zone_bind_cooling_device(str/* lower default 0, upper default max_state */ lower = lower ==THERMAL_NO_LIMIT ?0 : lower; - upper = upper ==THERMAL_NO_LIMIT ? cdev->max_state : upper; + +if (upper ==THERMAL_NO_LIMIT ) { + upper = cdev->max_state; + upper_no_limit = true; + }else { + upper_no_limit = false; + }if (lower > upper || upper > cdev->max_state)return -EINVAL ;@@ -644,6 +651,7 @@ int thermal_zone_bind_cooling_device(str dev->cdev = cdev; dev->trip = trip; dev->upper = upper; + dev->upper_no_limit = upper_no_limit; dev->lower = lower; dev->target =THERMAL_NO_TARGET ; dev->weight = weight;@@ -1057,6 +1065,79 @@ static bool thermal_cooling_device_presereturn false; } +/** +* thermal_cooling_device_update - Update a cooling device object +* @cdev: Target cooling device. +* +* Update @cdev to reflect a change of the underlying hardware or platform.
+ *
+ * Must be called when the maximum cooling state of @cdev becomes invalid and so
+ * its .get_max_state() callback needs to be run to produce the new maximum
+ * cooling state value.
+ */
+voidthermal_cooling_device_update(struct thermal_cooling_device *cdev)drivers/thermal/thermal_core.c +9
-0+{ +struct thermal_instance *ti; +unsigned long state; + +if (IS_ERR_OR_NULL (cdev)) +return ; + +/* +* Hold thermal_list_lock throughout the update to prevent the device
drivers/thermal/thermal_core.c +9
-0+* from going away while being updated. +*/ +mutex_lock (&thermal_list_lock); + +if (!thermal_cooling_device_present (cdev)) + goto unlock_list; + +/* +* Update under the cdev lock to prevent the state from being set beyond
drivers/thermal/thermal_core.c +32
-0+* the new limit concurrently. +*/ +mutex_lock (&cdev->lock); + +if (cdev->ops->get_max_state (cdev, &cdev->max_state)) + goto unlock; + +thermal_cooling_device_stats_reinit (cdev); + +list_for_each_entry (ti, &cdev->thermal_instances, cdev_node) { +if (ti->upper == cdev->max_state) +continue ; + +if (ti->upper < cdev->max_state) { +if (ti->upper_no_limit) + ti->upper = cdev->max_state; + +continue ; + } + + ti->upper = cdev->max_state; +if (ti->lower > ti->upper) + ti->lower = ti->upper; + +if (ti->target ==THERMAL_NO_TARGET ) +continue ; + +if (ti->target > ti->upper) + ti->target = ti->upper; + } + + if (cdev->ops->get_cur_state(cdev, &state) || state > cdev-Quoted 1 line
max_state)drivers/thermal/thermal_core.c +12
-0+ goto unlock; + +thermal_cooling_device_stats_update (cdev, state); + +unlock: +mutex_unlock (&cdev->lock); + +unlock_list: +mutex_unlock (&thermal_list_lock); +} +EXPORT_SYMBOL_GPL (thermal_cooling_device_update); +static void __unbind (struct thermal_zone_device *tz,int mask,struct thermal_cooling_device *cdev) {Index:
linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================drivers/thermal/thermal_sysfs.c +45
-5@@ -685,6 +685,8 @@ void thermal_cooling_device_stats_update {struct cooling_dev_stats *stats = cdev->stats; +lockdep_assert_held (&cdev->lock); +if (!stats)return ;@@ -706,13 +708,22 @@ static ssize_t total_trans_show(struct d struct device_attribute *attr, char *buf) { struct thermal_cooling_device *cdev = to_cooling_device(dev); -struct cooling_dev_stats *stats = cdev->stats; +struct cooling_dev_stats *stats;int ret; +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) + goto unlock; +spin_lock (&stats->lock); ret =sprintf (buf,"%u\n" , stats->total_trans);spin_unlock (&stats->lock); +unlock: +mutex_unlock (&cdev->lock); +return ret; }@@ -721,11 +732,18 @@ time_in_state_ms_show(struct device *dev char *buf) {struct thermal_cooling_device *cdev =to_cooling_device (dev); -struct cooling_dev_stats *stats = cdev->stats; +struct cooling_dev_stats *stats;ssize_t len =0 ;int i; +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) + goto unlock; +spin_lock (&stats->lock); +update_time_in_state (stats);for (i =0 ; i <= cdev->max_state; i++) {@@ -734,6 +752,9 @@ time_in_state_ms_show(struct device *dev }spin_unlock (&stats->lock); +unlock: +mutex_unlock (&cdev->lock); +return len; }@@ -742,8 +763,16 @@ reset_store(struct device *dev, struct d size_t count) {struct thermal_cooling_device *cdev =to_cooling_device (dev); -struct cooling_dev_stats *stats = cdev->stats; -int i, states = cdev->max_state +1 ; +struct cooling_dev_stats *stats; +int i, states; + +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) + goto unlock; + + states = cdev->max_state +1 ;spin_lock (&stats->lock);@@ -757,6 +786,9 @@ reset_store(struct device *dev, struct dspin_unlock (&stats->lock); +unlock: +mutex_unlock (&cdev->lock); +return count; }@@ -764,10 +796,18 @@ static ssize_t trans_table_show(struct d struct device_attribute *attr, char *buf) { struct thermal_cooling_device *cdev = to_cooling_device(dev); -struct cooling_dev_stats *stats = cdev->stats; +struct cooling_dev_stats *stats;ssize_t len =0 ;int i, j; +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) { + len = -ENODATA ; + goto unlock; + } + len +=snprintf (buf + len,PAGE_SIZE - len," From : To\n");drivers/thermal/thermal_sysfs.c +13
-3len +=snprintf (buf + len,PAGE_SIZE - len," : " );for (i =0 ; i <= cdev->max_state; i++) {@@ -775,8 +815,10 @@ static ssize_t trans_table_show(struct dbreak ; len +=snprintf (buf + len,PAGE_SIZE - len,"state%2u " , i); } -if (len >=PAGE_SIZE ) -return PAGE_SIZE ; +if (len >=PAGE_SIZE ) { + len =PAGE_SIZE ; + goto unlock; + } len +=snprintf (buf + len,PAGE_SIZE - len,"\n" );@@ -799,8 +841,12 @@ static ssize_t trans_table_show(struct dif (len >=PAGE_SIZE ) { pr_warn_once("Thermal transition table exceeds PAGE_SIZE. Disabling\n"); - return -EFBIG; + len = -EFBIG; } + +unlock: +mutex_unlock (&cdev->lock); +return len; }@@ -830,6 +876,8 @@ static void cooling_device_stats_setup(sunsigned long states = cdev->max_state +1 ;int var; +lockdep_assert_held (&cdev->lock); + var =sizeof (*stats); var +=sizeof (*stats->time_in_state) * states; var +=sizeof (*stats->trans_table) * states * states;@@ -855,6 +903,8 @@ out:static void cooling_device_stats_destroy (struct thermal_cooling_device *cdev) { +lockdep_assert_held (&cdev->lock); +kfree (cdev->stats); cdev->stats =NULL ;}
drivers/thermal/thermal_sysfs.c +6
-0@@ -879,6 +929,12 @@ void thermal_cooling_device_destroy_sysfcooling_device_stats_destroy (cdev); } +void thermal_cooling_device_stats_reinit (struct thermal_cooling_device *cdev) +{ +cooling_device_stats_destroy (cdev); +cooling_device_stats_setup (cdev); +} +/* these helper will be used only at the time of bindig */ ssize_t trip_point_show(struct device *dev, struct
device_attribute*attr,
char *buf)
Index:linux-pm/drivers/thermal/thermal_core.h
===================================================================drivers/thermal/thermal_core.h +1
-0@@ -101,6 +101,7 @@ struct thermal_instance {struct list_head tz_node;/* node in tz->thermal_instances */ struct list_head cdev_node;/* node in cdev->thermal_instances */ unsigned int weight;/* The weight of the cooling device */ +bool upper_no_limit; };#define
to_thermal_zone(_dev)\drivers/thermal/thermal_core.h +1
-0@@ -127,6 +128,7 @@ int thermal_zone_create_device_groups(stvoid thermal_zone_destroy_device_groups (struct thermal_zone_device *);void thermal_cooling_device_setup_sysfs (struct thermal_cooling_device *);void thermal_cooling_device_destroy_sysfs (struct thermal_cooling_device *cdev); +void thermal_cooling_device_stats_reinit(struct
thermal_cooling_device*cdev);
/* used only at binding time */
ssize_ttrip_point_show(struct device *, structdevice_attribute*,
char *);
ssize_tweight_show(struct device *, structdevice_attribute*, char
*);
Index:linux-pm/include/linux/thermal.h
===================================================================include/linux/thermal.h +1
-0@@ -388,6 +388,7 @@ devm_thermal_of_cooling_device_register(struct device_node *np, char *type, void *devdata, const struct thermal_cooling_device_ops *ops); +void thermal_cooling_device_update (struct thermal_cooling_device *);void thermal_cooling_device_unregister (struct thermal_cooling_device *);struct
thermal_zone_device*thermal_zone_get_zone_by_name(const char
*name);
int thermal_zone_get_temp(structthermal_zone_device*tz, int
*temp);
Rafael J. Wysocki
maintainerRe: [PATCH v3 3/4] thermal: core: Introduce thermal_cooling_device_update()
Quoted 37 lines · Rui
Quoted 26 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Introduce a core thermal API function,
thermal_cooling_device_update(),
for updating themax_statevalue for a cooling device and rearranging its statistics in sysfs after a possible change of its ->get_max_state()
callback return value.That callback is now invoked only once, during cooling device registration, to populate the
max_statefield in the cooling device object, so if its return value changes, it needs to be invoked again and the new return value needs to be stored asmax_state. Moreover, the statistics presented in sysfs need to be rearranged in general, because there may not be enough room in them to store data for all of the possible states (in the case whenmax_stategrows).The new function takes care of that (and some other minor things related to it), but some extra locking and lockdep annotations are added in several places too to protect against crashes in the cases when the statistics are not present or when a stale
max_statevalue might be used by sysfs attributes.Note that the actual user of the new function will be added separately.
Quoted 8 lines
Link:
https://lore.kernel.org/linux-pm/[email protected]/Signed-off-by Rafael J. Wysocki <[email protected]>---
Rui, please have a look at this again and give it a go if possible. It is the
only patch in the series changing in v3.
Quoted 7 lines
Hi, Rafael,
Thanks for the patch, I can confirm that it works, with ACPI passive cooling activated on my side.
Tested-by Zhang Rui <[email protected]>Reviewed-by Zhang Rui <[email protected]>
Thank you!
I'll queue up this series for 6.3-rc4.
Quoted 382 lines
Quoted 380 lines
v2 -> v3:
Makethermal_cooling_device_update()check thermal instances
involving the
given thermal device and update the "upper" value for them if
necessary (the
"lower" and "target" values may need to be adjusted too in some
cases) to avoid
leaving thermal instances with "upper" above the newmax_stateof
the
corresponding cooling device.v1 -> v2:
* Makethermal_cooling_device_update()do the fullIS_ERR_OR_NULL()check
on cdev, so that its callers don't need to do it.
* Remove ->set_cur_state() callback invocation fromthermal_cooling_device_update(), because it is redundant (and
it doesn't
make sense to pass a state value exceedingmax_stateto it
anyway).--- drivers/thermal/thermal_core.c | 83 +++++++++++++++++++++++++++++++++++++++- drivers/thermal/thermal_core.h | 2 drivers/thermal/thermal_sysfs.c | 72 ++++++++++++++++++++++++++++++---- include/linux/thermal.h | 1 4 files changed, 149 insertions(+), 9 deletions(-)Index:
linux-pm/drivers/thermal/thermal_core.c
===================================================================drivers/thermal/thermal_core.c +14
-1@@ -613,6 +613,7 @@ int thermal_zone_bind_cooling_device(strstruct thermal_instance *pos;struct thermal_zone_device *pos1;struct thermal_cooling_device *pos2; +bool upper_no_limit;int result;if (trip >= tz->num_trips || trip <0 )@@ -632,7 +633,13 @@ int thermal_zone_bind_cooling_device(str/* lower default 0, upper default max_state */ lower = lower ==THERMAL_NO_LIMIT ?0 : lower; - upper = upper ==THERMAL_NO_LIMIT ? cdev->max_state : upper; + +if (upper ==THERMAL_NO_LIMIT ) { + upper = cdev->max_state; + upper_no_limit = true; + }else { + upper_no_limit = false; + }if (lower > upper || upper > cdev->max_state)return -EINVAL ;@@ -644,6 +651,7 @@ int thermal_zone_bind_cooling_device(str dev->cdev = cdev; dev->trip = trip; dev->upper = upper; + dev->upper_no_limit = upper_no_limit; dev->lower = lower; dev->target =THERMAL_NO_TARGET ; dev->weight = weight;@@ -1057,6 +1065,79 @@ static bool thermal_cooling_device_presereturn false; } +/** +* thermal_cooling_device_update - Update a cooling device object +* @cdev: Target cooling device. +* +* Update @cdev to reflect a change of the underlying hardware or platform.
+ *
+ * Must be called when the maximum cooling state of @cdev becomes invalid and so
+ * its .get_max_state() callback needs to be run to produce the new maximum
+ * cooling state value.
+ */
+voidthermal_cooling_device_update(struct thermal_cooling_device *cdev)drivers/thermal/thermal_core.c +9
-0+{ +struct thermal_instance *ti; +unsigned long state; + +if (IS_ERR_OR_NULL (cdev)) +return ; + +/* +* Hold thermal_list_lock throughout the update to prevent the device
drivers/thermal/thermal_core.c +9
-0+* from going away while being updated. +*/ +mutex_lock (&thermal_list_lock); + +if (!thermal_cooling_device_present (cdev)) + goto unlock_list; + +/* +* Update under the cdev lock to prevent the state from being set beyond
drivers/thermal/thermal_core.c +32
-0+* the new limit concurrently. +*/ +mutex_lock (&cdev->lock); + +if (cdev->ops->get_max_state (cdev, &cdev->max_state)) + goto unlock; + +thermal_cooling_device_stats_reinit (cdev); + +list_for_each_entry (ti, &cdev->thermal_instances, cdev_node) { +if (ti->upper == cdev->max_state) +continue ; + +if (ti->upper < cdev->max_state) { +if (ti->upper_no_limit) + ti->upper = cdev->max_state; + +continue ; + } + + ti->upper = cdev->max_state; +if (ti->lower > ti->upper) + ti->lower = ti->upper; + +if (ti->target ==THERMAL_NO_TARGET ) +continue ; + +if (ti->target > ti->upper) + ti->target = ti->upper; + } + + if (cdev->ops->get_cur_state(cdev, &state) || state > cdev-Quoted 1 line
max_state)drivers/thermal/thermal_core.c +12
-0+ goto unlock; + +thermal_cooling_device_stats_update (cdev, state); + +unlock: +mutex_unlock (&cdev->lock); + +unlock_list: +mutex_unlock (&thermal_list_lock); +} +EXPORT_SYMBOL_GPL (thermal_cooling_device_update); +static void __unbind (struct thermal_zone_device *tz,int mask,struct thermal_cooling_device *cdev) {Index:
linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================drivers/thermal/thermal_sysfs.c +45
-5@@ -685,6 +685,8 @@ void thermal_cooling_device_stats_update {struct cooling_dev_stats *stats = cdev->stats; +lockdep_assert_held (&cdev->lock); +if (!stats)return ;@@ -706,13 +708,22 @@ static ssize_t total_trans_show(struct d struct device_attribute *attr, char *buf) { struct thermal_cooling_device *cdev = to_cooling_device(dev); -struct cooling_dev_stats *stats = cdev->stats; +struct cooling_dev_stats *stats;int ret; +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) + goto unlock; +spin_lock (&stats->lock); ret =sprintf (buf,"%u\n" , stats->total_trans);spin_unlock (&stats->lock); +unlock: +mutex_unlock (&cdev->lock); +return ret; }@@ -721,11 +732,18 @@ time_in_state_ms_show(struct device *dev char *buf) {struct thermal_cooling_device *cdev =to_cooling_device (dev); -struct cooling_dev_stats *stats = cdev->stats; +struct cooling_dev_stats *stats;ssize_t len =0 ;int i; +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) + goto unlock; +spin_lock (&stats->lock); +update_time_in_state (stats);for (i =0 ; i <= cdev->max_state; i++) {@@ -734,6 +752,9 @@ time_in_state_ms_show(struct device *dev }spin_unlock (&stats->lock); +unlock: +mutex_unlock (&cdev->lock); +return len; }@@ -742,8 +763,16 @@ reset_store(struct device *dev, struct d size_t count) {struct thermal_cooling_device *cdev =to_cooling_device (dev); -struct cooling_dev_stats *stats = cdev->stats; -int i, states = cdev->max_state +1 ; +struct cooling_dev_stats *stats; +int i, states; + +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) + goto unlock; + + states = cdev->max_state +1 ;spin_lock (&stats->lock);@@ -757,6 +786,9 @@ reset_store(struct device *dev, struct dspin_unlock (&stats->lock); +unlock: +mutex_unlock (&cdev->lock); +return count; }@@ -764,10 +796,18 @@ static ssize_t trans_table_show(struct d struct device_attribute *attr, char *buf) { struct thermal_cooling_device *cdev = to_cooling_device(dev); -struct cooling_dev_stats *stats = cdev->stats; +struct cooling_dev_stats *stats;ssize_t len =0 ;int i, j; +mutex_lock (&cdev->lock); + + stats = cdev->stats; +if (!stats) { + len = -ENODATA ; + goto unlock; + } + len +=snprintf (buf + len,PAGE_SIZE - len," From : To\n");len += snprintf(buf + len, PAGE_SIZE - len, " : "); for (i = 0; i <= cdev->max_state; i++) {drivers/thermal/thermal_sysfs.c +13
-3@@ -775,8 +815,10 @@ static ssize_t trans_table_show(struct dbreak ; len +=snprintf (buf + len,PAGE_SIZE - len,"state%2u " , i); } -if (len >=PAGE_SIZE ) -return PAGE_SIZE ; +if (len >=PAGE_SIZE ) { + len =PAGE_SIZE ; + goto unlock; + } len +=snprintf (buf + len,PAGE_SIZE - len,"\n" );@@ -799,8 +841,12 @@ static ssize_t trans_table_show(struct dif (len >=PAGE_SIZE ) { pr_warn_once("Thermal transition table exceeds PAGE_SIZE. Disabling\n"); - return -EFBIG; + len = -EFBIG; } + +unlock: +mutex_unlock (&cdev->lock); +return len; }@@ -830,6 +876,8 @@ static void cooling_device_stats_setup(sunsigned long states = cdev->max_state +1 ;int var; +lockdep_assert_held (&cdev->lock); + var =sizeof (*stats); var +=sizeof (*stats->time_in_state) * states; var +=sizeof (*stats->trans_table) * states * states;@@ -855,6 +903,8 @@ out:static void cooling_device_stats_destroy (struct thermal_cooling_device *cdev) { +lockdep_assert_held (&cdev->lock); +kfree (cdev->stats); cdev->stats =NULL ;}
drivers/thermal/thermal_sysfs.c +6
-0@@ -879,6 +929,12 @@ void thermal_cooling_device_destroy_sysfcooling_device_stats_destroy (cdev); } +void thermal_cooling_device_stats_reinit (struct thermal_cooling_device *cdev) +{ +cooling_device_stats_destroy (cdev); +cooling_device_stats_setup (cdev); +} +/* these helper will be used only at the time of bindig */ ssize_t trip_point_show(struct device *dev, struct
device_attribute*attr,
char *buf)
Index:linux-pm/drivers/thermal/thermal_core.h
===================================================================drivers/thermal/thermal_core.h +1
-0@@ -101,6 +101,7 @@ struct thermal_instance {struct list_head tz_node;/* node in tz->thermal_instances */ struct list_head cdev_node;/* node in cdev->thermal_instances */ unsigned int weight;/* The weight of the cooling device */ +bool upper_no_limit; };#define
to_thermal_zone(_dev)\drivers/thermal/thermal_core.h +1
-0@@ -127,6 +128,7 @@ int thermal_zone_create_device_groups(stvoid thermal_zone_destroy_device_groups (struct thermal_zone_device *);void thermal_cooling_device_setup_sysfs (struct thermal_cooling_device *);void thermal_cooling_device_destroy_sysfs (struct thermal_cooling_device *cdev); +void thermal_cooling_device_stats_reinit(struct
thermal_cooling_device*cdev);
/* used only at binding time */
ssize_ttrip_point_show(struct device *, structdevice_attribute*,
char *);
ssize_tweight_show(struct device *, structdevice_attribute*, char
*);
Index:linux-pm/include/linux/thermal.h
===================================================================include/linux/thermal.h +1
-0@@ -388,6 +388,7 @@ devm_thermal_of_cooling_device_register(struct device_node *np, char *type, void *devdata, const struct thermal_cooling_device_ops *ops); +void thermal_cooling_device_update (struct thermal_cooling_device *);void thermal_cooling_device_unregister (struct thermal_cooling_device *);struct
thermal_zone_device*thermal_zone_get_zone_by_name(const char
*name);
int thermal_zone_get_temp(structthermal_zone_device*tz, int
*temp);
PATCH v3 4/4 ACPI: processor: thermal: Update CPU cooling devices on cpufreq policy changes
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
When a cpufreq policy appears or goes away, the CPU cooling devices for the CPUs covered by that policy need to be updated so that the new processor_get_max_state() value is stored as max_state and the statistics in sysfs are rearranged for each of them.
Do that accordingly in acpi_thermal_cpufreq_init() and acpi_thermal_cpufreq_exit().
---
v2 -> v3: Add tags from Rui.
v1 -> v2: Remove the now redundant IS_ERR() checks on cdev before calling
thermal_cooling_device_update().
--- drivers/acpi/processor_thermal.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/acpi/processor_thermal.c +11 -3
@@ -140,9 +140,13 @@ void acpi_thermal_cpufreq_init(struct cp ret =freq_qos_add_request (&policy->constraints, &pr->thermal_req,FREQ_QOS_MAX ,INT_MAX ); -if (ret <0 ) +if (ret <0 ) {pr_err ("Failed to add freq constraint for CPU%d (%d)\n" , cpu, ret); +continue ; + } + +thermal_cooling_device_update (pr->cdev); } }
@@ -153,8 +157,12 @@ void acpi_thermal_cpufreq_exit(struct cpfor_each_cpu (cpu, policy->related_cpus) {struct acpi_processor *pr =per_cpu (processors, cpu); -if (pr) -freq_qos_remove_request (&pr->thermal_req); +if (!pr) +continue ; + +freq_qos_remove_request (&pr->thermal_req); + +thermal_cooling_device_update (pr->cdev); } }#else /* ! CONFIG_CPU_FREQ */