From c408b3d1d9bbc7de5fb0304fea424ef2539da616 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Mon, 17 Oct 2022 15:33:01 +0530 Subject: [PATCH 01/20] thermal: Validate new state in cur_state_store() In cur_state_store(), the new state of the cooling device is received from user-space and is not validated by the thermal core but the same is left for the individual drivers to take care of. Apart from duplicating the code it leaves possibility for introducing bugs where a driver may not do it right. Lets make the thermal core check the new state itself and store the max value in the cooling device structure. Link: https://lore.kernel.org/all/Y0ltRJRjO7AkawvE@kili/ Reported-by: Dan Carpenter Signed-off-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/thermal/gov_fair_share.c | 6 +----- drivers/thermal/thermal_core.c | 15 +++++++-------- drivers/thermal/thermal_sysfs.c | 11 +++++------ include/linux/thermal.h | 1 + 4 files changed, 14 insertions(+), 19 deletions(-) diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c index a4ee4661e9cc..1cfeac16e7ac 100644 --- a/drivers/thermal/gov_fair_share.c +++ b/drivers/thermal/gov_fair_share.c @@ -49,11 +49,7 @@ static int get_trip_level(struct thermal_zone_device *tz) static long get_target_state(struct thermal_zone_device *tz, struct thermal_cooling_device *cdev, int percentage, int level) { - unsigned long max_state; - - cdev->ops->get_max_state(cdev, &max_state); - - return (long)(percentage * level * max_state) / (100 * tz->num_trips); + return (long)(percentage * level * cdev->max_state) / (100 * tz->num_trips); } /** diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 117eeaf7dd24..08de59369e94 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -603,8 +603,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz, struct thermal_instance *pos; struct thermal_zone_device *pos1; struct thermal_cooling_device *pos2; - unsigned long max_state; - int result, ret; + int result; if (trip >= tz->num_trips || trip < 0) return -EINVAL; @@ -621,15 +620,11 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz, if (tz != pos1 || cdev != pos2) return -EINVAL; - ret = cdev->ops->get_max_state(cdev, &max_state); - if (ret) - return ret; - /* lower default 0, upper default max_state */ lower = lower == THERMAL_NO_LIMIT ? 0 : lower; - upper = upper == THERMAL_NO_LIMIT ? max_state : upper; + upper = upper == THERMAL_NO_LIMIT ? cdev->max_state : upper; - if (lower > upper || upper > max_state) + if (lower > upper || upper > cdev->max_state) return -EINVAL; dev = kzalloc(sizeof(*dev), GFP_KERNEL); @@ -900,6 +895,10 @@ __thermal_cooling_device_register(struct device_node *np, cdev->updated = false; cdev->device.class = &thermal_class; cdev->devdata = devdata; + + if (cdev->ops->get_max_state(cdev, &cdev->max_state)) + goto out_kfree_type; + thermal_cooling_device_setup_sysfs(cdev); ret = device_register(&cdev->device); if (ret) diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c index ec495c7dff03..bd7596125461 100644 --- a/drivers/thermal/thermal_sysfs.c +++ b/drivers/thermal/thermal_sysfs.c @@ -589,13 +589,8 @@ static ssize_t max_state_show(struct device *dev, struct device_attribute *attr, char *buf) { struct thermal_cooling_device *cdev = to_cooling_device(dev); - unsigned long state; - int ret; - ret = cdev->ops->get_max_state(cdev, &state); - if (ret) - return ret; - return sprintf(buf, "%ld\n", state); + return sprintf(buf, "%ld\n", cdev->max_state); } static ssize_t cur_state_show(struct device *dev, struct device_attribute *attr, @@ -625,6 +620,10 @@ cur_state_store(struct device *dev, struct device_attribute *attr, if ((long)state < 0) return -EINVAL; + /* Requested state should be less than max_state + 1 */ + if (state > cdev->max_state) + return -EINVAL; + mutex_lock(&cdev->lock); result = cdev->ops->set_cur_state(cdev, state); diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 9ecc128944a1..5e093602e8fc 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -100,6 +100,7 @@ struct thermal_cooling_device_ops { struct thermal_cooling_device { int id; char *type; + unsigned long max_state; struct device device; struct device_node *np; void *devdata; From a365105c685cad63e3c185c294373a7b81d3ea63 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Mon, 17 Oct 2022 15:33:02 +0530 Subject: [PATCH 02/20] thermal: sysfs: Reuse cdev->max_state Now that the cooling device structure stores the max_state value, reuse it and drop max_states from struct cooling_dev_stats. Signed-off-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_sysfs.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c index bd7596125461..febf9e76c440 100644 --- a/drivers/thermal/thermal_sysfs.c +++ b/drivers/thermal/thermal_sysfs.c @@ -661,7 +661,6 @@ struct cooling_dev_stats { spinlock_t lock; unsigned int total_trans; unsigned long state; - unsigned long max_states; ktime_t last_time; ktime_t *time_in_state; unsigned int *trans_table; @@ -691,7 +690,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, goto unlock; update_time_in_state(stats); - stats->trans_table[stats->state * stats->max_states + new_state]++; + stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++; stats->state = new_state; stats->total_trans++; @@ -725,7 +724,7 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr, spin_lock(&stats->lock); update_time_in_state(stats); - for (i = 0; i < stats->max_states; i++) { + for (i = 0; i <= cdev->max_state; i++) { len += sprintf(buf + len, "state%u\t%llu\n", i, ktime_to_ms(stats->time_in_state[i])); } @@ -740,7 +739,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf, { struct thermal_cooling_device *cdev = to_cooling_device(dev); struct cooling_dev_stats *stats = cdev->stats; - int i, states = stats->max_states; + int i, states = cdev->max_state + 1; spin_lock(&stats->lock); @@ -749,7 +748,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf, memset(stats->trans_table, 0, states * states * sizeof(*stats->trans_table)); - for (i = 0; i < stats->max_states; i++) + for (i = 0; i < states; i++) stats->time_in_state[i] = ktime_set(0, 0); spin_unlock(&stats->lock); @@ -767,7 +766,7 @@ static ssize_t trans_table_show(struct device *dev, len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n"); len += snprintf(buf + len, PAGE_SIZE - len, " : "); - for (i = 0; i < stats->max_states; i++) { + for (i = 0; i <= cdev->max_state; i++) { if (len >= PAGE_SIZE) break; len += snprintf(buf + len, PAGE_SIZE - len, "state%2u ", i); @@ -777,17 +776,17 @@ static ssize_t trans_table_show(struct device *dev, len += snprintf(buf + len, PAGE_SIZE - len, "\n"); - for (i = 0; i < stats->max_states; i++) { + for (i = 0; i <= cdev->max_state; i++) { if (len >= PAGE_SIZE) break; len += snprintf(buf + len, PAGE_SIZE - len, "state%2u:", i); - for (j = 0; j < stats->max_states; j++) { + for (j = 0; j <= cdev->max_state; j++) { if (len >= PAGE_SIZE) break; len += snprintf(buf + len, PAGE_SIZE - len, "%8u ", - stats->trans_table[i * stats->max_states + j]); + stats->trans_table[i * (cdev->max_state + 1) + j]); } if (len >= PAGE_SIZE) break; @@ -823,14 +822,10 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) { const struct attribute_group *stats_attr_group = NULL; struct cooling_dev_stats *stats; - unsigned long states; + /* Total number of states is highest state + 1 */ + unsigned long states = cdev->max_state + 1; int var; - if (cdev->ops->get_max_state(cdev, &states)) - goto out; - - states++; /* Total number of states is highest state + 1 */ - var = sizeof(*stats); var += sizeof(*stats->time_in_state) * states; var += sizeof(*stats->trans_table) * states * states; @@ -843,7 +838,6 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) stats->trans_table = (unsigned int *)(stats->time_in_state + states); cdev->stats = stats; stats->last_time = ktime_get(); - stats->max_states = states; spin_lock_init(&stats->lock); From e49a1e1ee078aee21006192076a8d93335e0daa9 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Fri, 28 Oct 2022 18:02:34 +0300 Subject: [PATCH 03/20] thermal/core: fix error code in __thermal_cooling_device_register() Return an error pointer if ->get_max_state() fails. The current code returns NULL which will cause an oops in the callers. Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") Signed-off-by: Dan Carpenter Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 08de59369e94..e0ca631ac4c8 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -896,7 +896,8 @@ __thermal_cooling_device_register(struct device_node *np, cdev->device.class = &thermal_class; cdev->devdata = devdata; - if (cdev->ops->get_max_state(cdev, &cdev->max_state)) + ret = cdev->ops->get_max_state(cdev, &cdev->max_state); + if (ret) goto out_kfree_type; thermal_cooling_device_setup_sysfs(cdev); From 54d9135cf223f221546bd51b0f5e4a73e99891f4 Mon Sep 17 00:00:00 2001 From: Ricardo Neri Date: Tue, 18 Oct 2022 04:22:40 -0700 Subject: [PATCH 04/20] thermal: intel: hfi: Improve the type of hfi_features::nr_table_pages A Coverity static code scan raised a potential overflow_before_widen warning when hfi_features::nr_table_pages is used as an argument to memcpy in intel_hfi_process_event(). Even though the overflow can never happen (the maximum number of pages of the HFI table is 0x10 and 0x10 << PAGE_SHIFT = 0x10000), using size_t as the data type of hfi_features::nr_table_pages makes Coverity happy and matches the data type of the argument 'size' of memcpy(). Signed-off-by: Ricardo Neri Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_hfi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c index a0640f762dc5..239afe02e518 100644 --- a/drivers/thermal/intel/intel_hfi.c +++ b/drivers/thermal/intel/intel_hfi.c @@ -137,7 +137,7 @@ struct hfi_instance { * Parameters and supported features that are common to all HFI instances */ struct hfi_features { - unsigned int nr_table_pages; + size_t nr_table_pages; unsigned int cpu_stride; unsigned int hdr_size; }; From be6abd3ed65678f8c7bd212808a9841785c2d5ca Mon Sep 17 00:00:00 2001 From: Zhang Rui Date: Tue, 8 Nov 2022 16:12:19 +0800 Subject: [PATCH 05/20] thermal: intel: intel_tcc_cooling: Detect TCC lock bit When MSR_IA32_TEMPERATURE_TARGET is locked, TCC Offset can not be updated even if the PROGRAMMABE Bit is set. Yield the driver on platforms with MSR_IA32_TEMPERATURE_TARGET locked. Signed-off-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_tcc_cooling.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/thermal/intel/intel_tcc_cooling.c b/drivers/thermal/intel/intel_tcc_cooling.c index 95adac427b6f..c9e84e615fce 100644 --- a/drivers/thermal/intel/intel_tcc_cooling.c +++ b/drivers/thermal/intel/intel_tcc_cooling.c @@ -14,6 +14,7 @@ #define TCC_SHIFT 24 #define TCC_MASK (0x3fULL<<24) #define TCC_PROGRAMMABLE BIT(30) +#define TCC_LOCKED BIT(31) static struct thermal_cooling_device *tcc_cdev; @@ -108,6 +109,15 @@ static int __init tcc_cooling_init(void) if (!(val & TCC_PROGRAMMABLE)) return -ENODEV; + err = rdmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, &val); + if (err) + return err; + + if (val & TCC_LOCKED) { + pr_info("TCC Offset locked\n"); + return -ENODEV; + } + pr_info("Programmable TCC Offset detected\n"); tcc_cdev = From e77f069fd6cea822efd15ea79aa61aa6422d4f67 Mon Sep 17 00:00:00 2001 From: Zhang Rui Date: Tue, 8 Nov 2022 16:12:20 +0800 Subject: [PATCH 06/20] thermal: intel: intel_tcc_cooling: Add TCC cooling support for RaptorLake-S Add RaptorLake to the list of processor models supported by the Intel TCC cooling driver. Signed-off-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_tcc_cooling.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/thermal/intel/intel_tcc_cooling.c b/drivers/thermal/intel/intel_tcc_cooling.c index c9e84e615fce..a89e7e1890e4 100644 --- a/drivers/thermal/intel/intel_tcc_cooling.c +++ b/drivers/thermal/intel/intel_tcc_cooling.c @@ -85,6 +85,7 @@ static const struct x86_cpu_id tcc_ids[] __initconst = { X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE_N, NULL), X86_MATCH_INTEL_FAM6_MODEL(RAPTORLAKE, NULL), X86_MATCH_INTEL_FAM6_MODEL(RAPTORLAKE_P, NULL), + X86_MATCH_INTEL_FAM6_MODEL(RAPTORLAKE_S, NULL), {} }; From d35f29ed9d11ccc4f9b957871d14726f4451a4ad Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:52 -0800 Subject: [PATCH 07/20] thermal/core: Destroy thermal zone device mutex in release function Accesses to thermal zones, and with it the thermal zone device mutex, are still possible after the thermal zone device has been unregistered. For example, thermal_zone_get_temp() can be called from temp_show() in thermal_sysfs.c if the sysfs attribute was opened before the thermal device was unregistered. Move the call to mutex_destroy from thermal_zone_device_unregister() to thermal_release() to ensure that it is only destroyed after it is guaranteed to be no longer accessed. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- 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 e0ca631ac4c8..b31d32476672 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -754,6 +754,7 @@ static void thermal_release(struct device *dev) sizeof("thermal_zone") - 1)) { tz = to_thermal_zone(dev); thermal_zone_destroy_device_groups(tz); + mutex_destroy(&tz->lock); kfree(tz); } else if (!strncmp(dev_name(dev), "cooling_device", sizeof("cooling_device") - 1)) { @@ -1390,7 +1391,6 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) thermal_remove_hwmon_sysfs(tz); ida_free(&thermal_tz_ida, tz->id); ida_destroy(&tz->ida); - mutex_destroy(&tz->lock); device_unregister(&tz->device); thermal_notify_tz_delete(tz_id); From 30b2ae07d3d60a4f9763b08a1f696b789e777337 Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:53 -0800 Subject: [PATCH 08/20] thermal/core: Delete device under thermal device zone lock Thermal device attributes may still be opened after unregistering the thermal zone and deleting the thermal device. Currently there is no protection against accessing thermal device operations after unregistering a thermal zone. To enable adding such protection, protect the device delete operation with the thermal zone device mutex. This requires splitting the call to device_unregister() into its components, device_del() and put_device(). Only the first call can be executed under mutex protection, since put_device() may result in releasing the thermal zone device memory. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_core.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index b31d32476672..5d19dc6a82b4 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -1391,7 +1391,12 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) thermal_remove_hwmon_sysfs(tz); ida_free(&thermal_tz_ida, tz->id); ida_destroy(&tz->ida); - device_unregister(&tz->device); + + mutex_lock(&tz->lock); + device_del(&tz->device); + mutex_unlock(&tz->lock); + + put_device(&tz->device); thermal_notify_tz_delete(tz_id); } From 1c6b30060777352e7881383bab726046d8c3c610 Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:54 -0800 Subject: [PATCH 09/20] thermal/core: Ensure that thermal device is registered in thermal_zone_get_temp Calls to thermal_zone_get_temp() are not protected against thermal zone device removal. As result, it is possible that the thermal zone operations callbacks are no longer valid when thermal_zone_get_temp() is called. This may result in crashes such as BUG: unable to handle page fault for address: ffffffffc04ef420 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 5d60e067 P4D 5d60e067 PUD 5d610067 PMD 110197067 PTE 0 Oops: 0000 [#1] PREEMPT SMP NOPTI CPU: 1 PID: 3209 Comm: cat Tainted: G W 5.10.136-19389-g615abc6eb807 #1 02df41ac0b12f3a64f4b34245188d8875bb3bce1 Hardware name: Google Coral/Coral, BIOS Google_Coral.10068.92.0 11/27/2018 RIP: 0010:thermal_zone_get_temp+0x26/0x73 Code: 89 c3 eb d3 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 53 48 85 ff 74 50 48 89 fb 48 81 ff 00 f0 ff ff 77 44 48 8b 83 98 03 00 00 <48> 83 78 10 00 74 36 49 89 f6 4c 8d bb d8 03 00 00 4c 89 ff e8 9f RSP: 0018:ffffb3758138fd38 EFLAGS: 00010287 RAX: ffffffffc04ef410 RBX: ffff98f14d7fb000 RCX: 0000000000000000 RDX: ffff98f17cf90000 RSI: ffffb3758138fd64 RDI: ffff98f14d7fb000 RBP: ffffb3758138fd50 R08: 0000000000001000 R09: ffff98f17cf90000 R10: 0000000000000000 R11: ffffffff8dacad28 R12: 0000000000001000 R13: ffff98f1793a7d80 R14: ffff98f143231708 R15: ffff98f14d7fb018 FS: 00007ec166097800(0000) GS:ffff98f1bbd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffffc04ef420 CR3: 000000010ee9a000 CR4: 00000000003506e0 Call Trace: temp_show+0x31/0x68 dev_attr_show+0x1d/0x4f sysfs_kf_seq_show+0x92/0x107 seq_read_iter+0xf5/0x3f2 vfs_read+0x205/0x379 __x64_sys_read+0x7c/0xe2 do_syscall_64+0x43/0x55 entry_SYSCALL_64_after_hwframe+0x61/0xc6 if a thermal device is removed while accesses to its device attributes are ongoing. The problem is exposed by code in iwl_op_mode_mvm_start(), which registers a thermal zone device only to unregister it shortly afterwards if an unrelated failure is encountered while accessing the hardware. Check if the thermal zone device is registered after acquiring the thermal zone device mutex to ensure this does not happen. The code was tested by triggering the failure in iwl_op_mode_mvm_start() on purpose. Without this patch, the kernel crashes reliably. The crash is no longer observed after applying this and the preceding patches. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_helpers.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c index c65cdce8f856..fca0b23570f9 100644 --- a/drivers/thermal/thermal_helpers.c +++ b/drivers/thermal/thermal_helpers.c @@ -115,7 +115,12 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp) int ret; mutex_lock(&tz->lock); - ret = __thermal_zone_get_temp(tz, temp); + + if (device_is_registered(&tz->device)) + ret = __thermal_zone_get_temp(tz, temp); + else + ret = -ENODEV; + mutex_unlock(&tz->lock); return ret; From ed97d10a8b2c78d1664b01658c5f581e6791ff7d Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:55 -0800 Subject: [PATCH 10/20] thermal/core: Move parameter validation from __thermal_zone_get_temp to thermal_zone_get_temp All callers of __thermal_zone_get_temp() already validated the thermal zone parameters. Move validation to thermal_zone_get_temp() where it is actually needed. Also add kernel documentation for __thermal_zone_get_temp(), listing the requirement that the function must be called with validated parameters and with thermal device mutex held. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_helpers.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c index fca0b23570f9..321f8020082d 100644 --- a/drivers/thermal/thermal_helpers.c +++ b/drivers/thermal/thermal_helpers.c @@ -64,6 +64,20 @@ get_thermal_instance(struct thermal_zone_device *tz, } EXPORT_SYMBOL(get_thermal_instance); +/** + * __thermal_zone_get_temp() - returns the temperature of a thermal zone + * @tz: a valid pointer to a struct thermal_zone_device + * @temp: a valid pointer to where to store the resulting temperature. + * + * When a valid thermal zone reference is passed, it will fetch its + * temperature and fill @temp. + * + * Both tz and tz->ops must be valid pointers when calling this function, + * and the tz->ops->get_temp callback must be provided. + * The function must be called under tz->lock. + * + * Return: On success returns 0, an error code otherwise + */ int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp) { int ret = -EINVAL; @@ -73,9 +87,6 @@ int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp) lockdep_assert_held(&tz->lock); - if (!tz || IS_ERR(tz) || !tz->ops->get_temp) - return -EINVAL; - ret = tz->ops->get_temp(tz, temp); if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) { @@ -114,13 +125,22 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp) { int ret; + if (IS_ERR_OR_NULL(tz)) + return -EINVAL; + mutex_lock(&tz->lock); + if (!tz->ops->get_temp) { + ret = -EINVAL; + goto unlock; + } + if (device_is_registered(&tz->device)) ret = __thermal_zone_get_temp(tz, temp); else ret = -ENODEV; +unlock: mutex_unlock(&tz->lock); return ret; From 1c439dec359caa225c7752334c7a14ef9e3344c7 Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:56 -0800 Subject: [PATCH 11/20] thermal/core: Introduce locked version of thermal_zone_device_update In thermal_zone_device_set_mode(), the thermal zone mutex is released only to be reacquired in the subsequent call to thermal_zone_device_update(). Introduce __thermal_zone_device_update(), which is similar to thermal_zone_device_update() but has to be called with the thermal device mutex held. Call the new function from thermal_zone_device_set_mode() to avoid the extra thermal device mutex release/acquire sequence in that function. With the new function in place, re-implement thermal_zone_device_update() as wrapper around __thermal_zone_device_update() to acquire and release the thermal device mutex. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_core.c | 57 ++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 5d19dc6a82b4..0657e4bd4791 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -403,6 +403,34 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz) pos->initialized = false; } +static void __thermal_zone_device_update(struct thermal_zone_device *tz, + enum thermal_notify_event event) +{ + int count; + + if (atomic_read(&in_suspend)) + return; + + if (WARN_ONCE(!tz->ops->get_temp, + "'%s' must not be called without 'get_temp' ops set\n", + __func__)) + return; + + if (!thermal_zone_device_is_enabled(tz)) + return; + + update_temperature(tz); + + __thermal_zone_set_trips(tz); + + tz->notify_event = event; + + for (count = 0; count < tz->num_trips; count++) + handle_thermal_trip(tz, count); + + monitor_thermal_zone(tz); +} + static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, enum thermal_device_mode mode) { @@ -423,9 +451,9 @@ static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, if (!ret) tz->mode = mode; - mutex_unlock(&tz->lock); + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + mutex_unlock(&tz->lock); if (mode == THERMAL_DEVICE_ENABLED) thermal_notify_tz_enable(tz->id); @@ -457,31 +485,8 @@ int thermal_zone_device_is_enabled(struct thermal_zone_device *tz) void thermal_zone_device_update(struct thermal_zone_device *tz, enum thermal_notify_event event) { - int count; - - if (atomic_read(&in_suspend)) - return; - - if (WARN_ONCE(!tz->ops->get_temp, "'%s' must not be called without " - "'get_temp' ops set\n", __func__)) - return; - mutex_lock(&tz->lock); - - if (!thermal_zone_device_is_enabled(tz)) - goto out; - - update_temperature(tz); - - __thermal_zone_set_trips(tz); - - tz->notify_event = event; - - for (count = 0; count < tz->num_trips; count++) - handle_thermal_trip(tz, count); - - monitor_thermal_zone(tz); -out: + __thermal_zone_device_update(tz, event); mutex_unlock(&tz->lock); } EXPORT_SYMBOL_GPL(thermal_zone_device_update); From ea37bec51ff442546e4a57d5cca2de9cc64a9df3 Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:57 -0800 Subject: [PATCH 12/20] thermal/core: Protect hwmon accesses to thermal operations with thermal zone mutex In preparation to protecting access to thermal operations against thermal zone device removal, protect hwmon accesses to thermal zone operations with the thermal zone mutex. After acquiring the mutex, ensure that the thermal zone device is registered before proceeding. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_hwmon.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c index f53f4ceb6a5d..c594c42bea6d 100644 --- a/drivers/thermal/thermal_hwmon.c +++ b/drivers/thermal/thermal_hwmon.c @@ -77,7 +77,15 @@ temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf) int temperature; int ret; - ret = tz->ops->get_crit_temp(tz, &temperature); + mutex_lock(&tz->lock); + + if (device_is_registered(&tz->device)) + ret = tz->ops->get_crit_temp(tz, &temperature); + else + ret = -ENODEV; + + mutex_unlock(&tz->lock); + if (ret) return ret; From 05eeee2b51b44c7ba5f1b8e5c41362020513f163 Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:58 -0800 Subject: [PATCH 13/20] thermal/core: Protect sysfs accesses to thermal operations with thermal zone mutex Protect access to thermal operations against thermal zone removal by acquiring the thermal zone device mutex. After acquiring the mutex, check if the thermal zone device is registered and abort the operation if not. With this change, we can call __thermal_zone_device_update() instead of thermal_zone_device_update() from trip_point_temp_store() and from emul_temp_store(). Similar, we can call __thermal_zone_set_trips() instead of thermal_zone_set_trips() from trip_point_hyst_store(). Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_core.c | 4 +- drivers/thermal/thermal_core.h | 2 + drivers/thermal/thermal_sysfs.c | 81 +++++++++++++++++++++++++++------ 3 files changed, 70 insertions(+), 17 deletions(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 0657e4bd4791..173b049dca41 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -403,8 +403,8 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz) pos->initialized = false; } -static void __thermal_zone_device_update(struct thermal_zone_device *tz, - enum thermal_notify_event event) +void __thermal_zone_device_update(struct thermal_zone_device *tz, + enum thermal_notify_event event) { int count; diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h index 1571917bd3c8..7b51b9a22e7e 100644 --- a/drivers/thermal/thermal_core.h +++ b/drivers/thermal/thermal_core.h @@ -109,6 +109,8 @@ int thermal_register_governor(struct thermal_governor *); void thermal_unregister_governor(struct thermal_governor *); int thermal_zone_device_set_policy(struct thermal_zone_device *, char *); int thermal_build_list_of_policies(char *buf); +void __thermal_zone_device_update(struct thermal_zone_device *tz, + enum thermal_notify_event event); /* Helpers */ void thermal_zone_set_trips(struct thermal_zone_device *tz); diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c index febf9e76c440..d97f0bc0a26b 100644 --- a/drivers/thermal/thermal_sysfs.c +++ b/drivers/thermal/thermal_sysfs.c @@ -92,7 +92,14 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr, if (sscanf(attr->attr.name, "trip_point_%d_type", &trip) != 1) return -EINVAL; - result = tz->ops->get_trip_type(tz, trip, &type); + mutex_lock(&tz->lock); + + if (device_is_registered(dev)) + result = tz->ops->get_trip_type(tz, trip, &type); + else + result = -ENODEV; + + mutex_unlock(&tz->lock); if (result) return result; @@ -128,10 +135,17 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, if (kstrtoint(buf, 10, &temperature)) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; + } + if (tz->ops->set_trip_temp) { ret = tz->ops->set_trip_temp(tz, trip, temperature); if (ret) - return ret; + goto unlock; } if (tz->trips) @@ -140,16 +154,22 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, if (tz->ops->get_trip_hyst) { ret = tz->ops->get_trip_hyst(tz, trip, &hyst); if (ret) - return ret; + goto unlock; } ret = tz->ops->get_trip_type(tz, trip, &type); if (ret) - return ret; + goto unlock; thermal_notify_tz_trip_change(tz->id, trip, type, temperature, hyst); - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + +unlock: + mutex_unlock(&tz->lock); + + if (ret) + return ret; return count; } @@ -168,7 +188,14 @@ trip_point_temp_show(struct device *dev, struct device_attribute *attr, if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip) != 1) return -EINVAL; - ret = tz->ops->get_trip_temp(tz, trip, &temperature); + mutex_lock(&tz->lock); + + if (device_is_registered(dev)) + ret = tz->ops->get_trip_temp(tz, trip, &temperature); + else + ret = -ENODEV; + + mutex_unlock(&tz->lock); if (ret) return ret; @@ -193,6 +220,13 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr, if (kstrtoint(buf, 10, &temperature)) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; + } + /* * We are not doing any check on the 'temperature' value * here. The driver implementing 'set_trip_hyst' has to @@ -201,7 +235,10 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr, ret = tz->ops->set_trip_hyst(tz, trip, temperature); if (!ret) - thermal_zone_set_trips(tz); + __thermal_zone_set_trips(tz); + +unlock: + mutex_unlock(&tz->lock); return ret ? ret : count; } @@ -220,7 +257,14 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr, if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip) != 1) return -EINVAL; - ret = tz->ops->get_trip_hyst(tz, trip, &temperature); + mutex_lock(&tz->lock); + + if (device_is_registered(dev)) + ret = tz->ops->get_trip_hyst(tz, trip, &temperature); + else + ret = -ENODEV; + + mutex_unlock(&tz->lock); return ret ? ret : sprintf(buf, "%d\n", temperature); } @@ -269,16 +313,23 @@ emul_temp_store(struct device *dev, struct device_attribute *attr, if (kstrtoint(buf, 10, &temperature)) return -EINVAL; - if (!tz->ops->set_emul_temp) { - mutex_lock(&tz->lock); - tz->emul_temperature = temperature; - mutex_unlock(&tz->lock); - } else { - ret = tz->ops->set_emul_temp(tz, temperature); + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; } + if (!tz->ops->set_emul_temp) + tz->emul_temperature = temperature; + else + ret = tz->ops->set_emul_temp(tz, temperature); + if (!ret) - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + +unlock: + mutex_unlock(&tz->lock); return ret ? ret : count; } From 91b3aafc2238a91051dff79a8d072a18c805ee50 Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:24:59 -0800 Subject: [PATCH 14/20] thermal/core: Remove thermal_zone_set_trips() Since no callers of thermal_zone_set_trips() are left, remove the function. Document __thermal_zone_set_trips() instead. Explicitly state that the thermal zone lock must be held when calling the function, and that the pointer to the thermal zone must be valid. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_core.h | 1 - drivers/thermal/thermal_helpers.c | 34 ++++++++++++++----------------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h index 7b51b9a22e7e..b834cb273429 100644 --- a/drivers/thermal/thermal_core.h +++ b/drivers/thermal/thermal_core.h @@ -113,7 +113,6 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz, enum thermal_notify_event event); /* Helpers */ -void thermal_zone_set_trips(struct thermal_zone_device *tz); void __thermal_zone_set_trips(struct thermal_zone_device *tz); int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp); diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c index 321f8020082d..56aa2e88f34f 100644 --- a/drivers/thermal/thermal_helpers.c +++ b/drivers/thermal/thermal_helpers.c @@ -147,6 +147,21 @@ unlock: } EXPORT_SYMBOL_GPL(thermal_zone_get_temp); +/** + * __thermal_zone_set_trips - Computes the next trip points for the driver + * @tz: a pointer to a thermal zone device structure + * + * The function computes the next temperature boundaries by browsing + * the trip points. The result is the closer low and high trip points + * to the current temperature. These values are passed to the backend + * driver to let it set its own notification mechanism (usually an + * interrupt). + * + * This function must be called with tz->lock held. Both tz and tz->ops + * must be valid pointers. + * + * It does not return a value + */ void __thermal_zone_set_trips(struct thermal_zone_device *tz) { int low = -INT_MAX; @@ -193,25 +208,6 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz) dev_err(&tz->device, "Failed to set trips: %d\n", ret); } -/** - * thermal_zone_set_trips - Computes the next trip points for the driver - * @tz: a pointer to a thermal zone device structure - * - * The function computes the next temperature boundaries by browsing - * the trip points. The result is the closer low and high trip points - * to the current temperature. These values are passed to the backend - * driver to let it set its own notification mechanism (usually an - * interrupt). - * - * It does not return a value - */ -void thermal_zone_set_trips(struct thermal_zone_device *tz) -{ - mutex_lock(&tz->lock); - __thermal_zone_set_trips(tz); - mutex_unlock(&tz->lock); -} - static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev, int target) { From b778b4d782d48b9a8751b21deb8eb2f054a3c772 Mon Sep 17 00:00:00 2001 From: Guenter Roeck Date: Thu, 10 Nov 2022 07:25:00 -0800 Subject: [PATCH 15/20] thermal/core: Protect thermal device operations against thermal device removal Thermal device operations may be called after thermal zone device removal. After thermal zone device removal, thermal zone device operations must no longer be called. To prevent such calls from happening, ensure that the thermal device is registered before executing any thermal device operations. Signed-off-by: Guenter Roeck Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_core.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 173b049dca41..cc7dbcb2c739 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -203,6 +203,9 @@ int thermal_zone_device_set_policy(struct thermal_zone_device *tz, mutex_lock(&thermal_governor_lock); mutex_lock(&tz->lock); + if (!device_is_registered(&tz->device)) + goto exit; + gov = __find_governor(strim(policy)); if (!gov) goto exit; @@ -445,6 +448,12 @@ static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, return ret; } + if (!device_is_registered(&tz->device)) { + mutex_unlock(&tz->lock); + + return -ENODEV; + } + if (tz->ops->change_mode) ret = tz->ops->change_mode(tz, mode); @@ -486,7 +495,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz, enum thermal_notify_event event) { mutex_lock(&tz->lock); - __thermal_zone_device_update(tz, event); + if (device_is_registered(&tz->device)) + __thermal_zone_device_update(tz, event); mutex_unlock(&tz->lock); } EXPORT_SYMBOL_GPL(thermal_zone_device_update); From 6fe1e64b60269aa58fa00568807738025ae3bd05 Mon Sep 17 00:00:00 2001 From: Srinivas Pandruvada Date: Tue, 15 Nov 2022 18:54:16 -0800 Subject: [PATCH 16/20] thermal: intel: Prevent accidental clearing of HFI status When there is a package thermal interrupt with PROCHOT log, it will be processed and cleared. It is possible that there is an active HFI event status, which is about to get processed or getting processed. While clearing PROCHOT log bit, it will also clear HFI status bit. This means that hardware is free to update HFI memory. When clearing a package thermal interrupt, some processors will generate a "general protection fault" when any of the read only bit is set to 1. The driver maintains a mask of all read-write bits which can be set. This mask doesn't include HFI status bit. This bit will also be cleared, as it will be assumed read-only bit. So, add HFI status bit 26 to the mask. Signed-off-by: Srinivas Pandruvada Reviewed-by: Ricardo Neri Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/therm_throt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/thermal/intel/therm_throt.c b/drivers/thermal/intel/therm_throt.c index 8352083b87c7..9e8ab31d756e 100644 --- a/drivers/thermal/intel/therm_throt.c +++ b/drivers/thermal/intel/therm_throt.c @@ -197,7 +197,7 @@ static const struct attribute_group thermal_attr_group = { #define THERM_STATUS_PROCHOT_LOG BIT(1) #define THERM_STATUS_CLEAR_CORE_MASK (BIT(1) | BIT(3) | BIT(5) | BIT(7) | BIT(9) | BIT(11) | BIT(13) | BIT(15)) -#define THERM_STATUS_CLEAR_PKG_MASK (BIT(1) | BIT(3) | BIT(5) | BIT(7) | BIT(9) | BIT(11)) +#define THERM_STATUS_CLEAR_PKG_MASK (BIT(1) | BIT(3) | BIT(5) | BIT(7) | BIT(9) | BIT(11) | BIT(26)) static void clear_therm_status_log(int level) { From 930d06bf071aa746db11d68d2d75660b449deff3 Mon Sep 17 00:00:00 2001 From: Srinivas Pandruvada Date: Tue, 15 Nov 2022 18:54:17 -0800 Subject: [PATCH 17/20] thermal: intel: Protect clearing of thermal status bits The clearing of the package thermal status is done by Read-Modify-Write operation. This may result in clearing of some new status bits which are being or about to be processed. For example, while clearing of HFI status, after read of thermal status register, a new thermal status bit is set by the hardware. But during write back, the newly generated status bit will be set to 0 or cleared. So, it is not safe to do read-modify-write. Since thermal status Read-Write bits can be set to only 0 not 1, it is safe to set all other bits to 1 which are not getting cleared. Create a common interface for clearing package thermal status bits. Use this interface to replace existing code to clear thermal package status bits. It is safe to call from different CPUs without protection as there is no read-modify-write. Also wrmsrl results in just single instruction. For example while CPU 0 and CPU 3 are clearing bit 1 and 3 respectively. If CPU 3 wins the race, it will write 0x4000aa2, then CPU 1 will write 0x4000aa8. The bits which are not part of clear are set to 1. The default mask for bits, which can be written here is 0x4000aaa. Signed-off-by: Srinivas Pandruvada Reviewed-by: Ricardo Neri Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_hfi.c | 8 ++----- drivers/thermal/intel/therm_throt.c | 23 ++++++++++---------- drivers/thermal/intel/thermal_interrupt.h | 6 +++++ drivers/thermal/intel/x86_pkg_temp_thermal.c | 9 ++------ 4 files changed, 22 insertions(+), 24 deletions(-) diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c index 239afe02e518..65b902939e1f 100644 --- a/drivers/thermal/intel/intel_hfi.c +++ b/drivers/thermal/intel/intel_hfi.c @@ -42,9 +42,7 @@ #include "../thermal_core.h" #include "intel_hfi.h" - -#define THERM_STATUS_CLEAR_PKG_MASK (BIT(1) | BIT(3) | BIT(5) | BIT(7) | \ - BIT(9) | BIT(11) | BIT(26)) +#include "thermal_interrupt.h" /* Hardware Feedback Interface MSR configuration bits */ #define HW_FEEDBACK_PTR_VALID_BIT BIT(0) @@ -304,9 +302,7 @@ void intel_hfi_process_event(__u64 pkg_therm_status_msr_val) * Let hardware know that we are done reading the HFI table and it is * free to update it again. */ - pkg_therm_status_msr_val &= THERM_STATUS_CLEAR_PKG_MASK & - ~PACKAGE_THERM_STATUS_HFI_UPDATED; - wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS, pkg_therm_status_msr_val); + thermal_clear_package_intr_status(PACKAGE_LEVEL, PACKAGE_THERM_STATUS_HFI_UPDATED); queue_delayed_work(hfi_updates_wq, &hfi_instance->update_work, HFI_UPDATE_INTERVAL); diff --git a/drivers/thermal/intel/therm_throt.c b/drivers/thermal/intel/therm_throt.c index 9e8ab31d756e..4bb7fddaa143 100644 --- a/drivers/thermal/intel/therm_throt.c +++ b/drivers/thermal/intel/therm_throt.c @@ -190,32 +190,33 @@ static const struct attribute_group thermal_attr_group = { }; #endif /* CONFIG_SYSFS */ -#define CORE_LEVEL 0 -#define PACKAGE_LEVEL 1 - #define THERM_THROT_POLL_INTERVAL HZ #define THERM_STATUS_PROCHOT_LOG BIT(1) #define THERM_STATUS_CLEAR_CORE_MASK (BIT(1) | BIT(3) | BIT(5) | BIT(7) | BIT(9) | BIT(11) | BIT(13) | BIT(15)) #define THERM_STATUS_CLEAR_PKG_MASK (BIT(1) | BIT(3) | BIT(5) | BIT(7) | BIT(9) | BIT(11) | BIT(26)) -static void clear_therm_status_log(int level) +/* + * Clear the bits in package thermal status register for bit = 1 + * in bitmask + */ +void thermal_clear_package_intr_status(int level, u64 bit_mask) { + u64 msr_val; int msr; - u64 mask, msr_val; if (level == CORE_LEVEL) { msr = MSR_IA32_THERM_STATUS; - mask = THERM_STATUS_CLEAR_CORE_MASK; + msr_val = THERM_STATUS_CLEAR_CORE_MASK; } else { msr = MSR_IA32_PACKAGE_THERM_STATUS; - mask = THERM_STATUS_CLEAR_PKG_MASK; + msr_val = THERM_STATUS_CLEAR_PKG_MASK; } - rdmsrl(msr, msr_val); - msr_val &= mask; - wrmsrl(msr, msr_val & ~THERM_STATUS_PROCHOT_LOG); + msr_val &= ~bit_mask; + wrmsrl(msr, msr_val); } +EXPORT_SYMBOL_GPL(thermal_clear_package_intr_status); static void get_therm_status(int level, bool *proc_hot, u8 *temp) { @@ -295,7 +296,7 @@ static void __maybe_unused throttle_active_work(struct work_struct *work) state->average = avg; re_arm: - clear_therm_status_log(state->level); + thermal_clear_package_intr_status(state->level, THERM_STATUS_PROCHOT_LOG); schedule_delayed_work_on(this_cpu, &state->therm_work, THERM_THROT_POLL_INTERVAL); } diff --git a/drivers/thermal/intel/thermal_interrupt.h b/drivers/thermal/intel/thermal_interrupt.h index 01e7bed2ffc7..01dfd4cdb5df 100644 --- a/drivers/thermal/intel/thermal_interrupt.h +++ b/drivers/thermal/intel/thermal_interrupt.h @@ -2,6 +2,9 @@ #ifndef _INTEL_THERMAL_INTERRUPT_H #define _INTEL_THERMAL_INTERRUPT_H +#define CORE_LEVEL 0 +#define PACKAGE_LEVEL 1 + /* Interrupt Handler for package thermal thresholds */ extern int (*platform_thermal_package_notify)(__u64 msr_val); @@ -15,4 +18,7 @@ extern bool (*platform_thermal_package_rate_control)(void); /* Handle HWP interrupt */ extern void notify_hwp_interrupt(void); +/* Common function to clear Package thermal status register */ +extern void thermal_clear_package_intr_status(int level, u64 bit_mask); + #endif /* _INTEL_THERMAL_INTERRUPT_H */ diff --git a/drivers/thermal/intel/x86_pkg_temp_thermal.c b/drivers/thermal/intel/x86_pkg_temp_thermal.c index a0e234fce71a..84c3a116ed04 100644 --- a/drivers/thermal/intel/x86_pkg_temp_thermal.c +++ b/drivers/thermal/intel/x86_pkg_temp_thermal.c @@ -265,7 +265,6 @@ static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work) struct thermal_zone_device *tzone = NULL; int cpu = smp_processor_id(); struct zone_device *zonedev; - u64 msr_val, wr_val; mutex_lock(&thermal_zone_mutex); raw_spin_lock_irq(&pkg_temp_lock); @@ -279,12 +278,8 @@ static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work) } zonedev->work_scheduled = false; - rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val); - wr_val = msr_val & ~(THERM_LOG_THRESHOLD0 | THERM_LOG_THRESHOLD1); - if (wr_val != msr_val) { - wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS, wr_val); - tzone = zonedev->tzone; - } + thermal_clear_package_intr_status(PACKAGE_LEVEL, THERM_LOG_THRESHOLD0 | THERM_LOG_THRESHOLD1); + tzone = zonedev->tzone; enable_pkg_thres_interrupt(); raw_spin_unlock_irq(&pkg_temp_lock); From c0e3acdcdeb14099765de38224dfe0ad019c8482 Mon Sep 17 00:00:00 2001 From: Srinivas Pandruvada Date: Wed, 16 Nov 2022 15:14:59 -0800 Subject: [PATCH 18/20] thermal: intel: hfi: ACK HFI for the same timestamp Some processors issue more than one HFI interrupt with the same timestamp. Each interrupt must be acknowledged to let the hardware issue new HFI interrupts. But this can't be done without some additional flow modification in the existing interrupt handling. For background, the HFI interrupt is a package level thermal interrupt delivered via a LVT. This LVT is common for both the CPU and package level interrupts. Hence, all CPUs receive the HFI interrupts. But only one CPU should process interrupt and others simply exit by issuing EOI to LAPIC. The current HFI interrupt processing flow: 1. Receive Thermal interrupt 2. Check if there is an active HFI status in MSR_IA32_THERM_STATUS 3. Try and get spinlock, one CPU will enter spinlock and others will simply return from here to issue EOI. (Let's assume CPU 4 is processing interrupt) 4. Check the stored time-stamp from the HFI memory time-stamp 5. if same 6. ignore interrupt, unlock and return 7. Copy the HFI message to local buffer 8. unlock spinlock 9. ACK HFI interrupt 10. Queue the message for processing in a work-queue It is tempting to simply acknowledge all the interrupts even if they have the same timestamp. This may cause some interrupts to not be processed. Let's say CPU5 is slightly late and reaches step 4 while CPU4 is between steps 8 and 9. Currently we simply ignore interrupts with the same timestamp. No issue here for CPU5. When CPU4 acknowledges the interrupt, the next HFI interrupt can be delivered. If we acknowledge interrupts with the same timestamp (at step 6), there is a race condition. Under the same scenario, CPU 5 will acknowledge the HFI interrupt. This lets hardware generate another HFI interrupt, before CPU 4 start executing step 9. Once CPU 4 complete step 9, it will acknowledge the newly arrived HFI interrupt, without actually processing it. Acknowledge the interrupt when holding the spinlock. This avoids contention of the interrupt acknowledgment. Updated flow: 1. Receive HFI Thermal interrupt 2. Check if there is an active HFI status in MSR_IA32_THERM_STATUS 3. Try and get spin-lock Let's assume CPU 4 is processing interrupt 4.1 Read MSR_IA32_PACKAGE_THERM_STATUS and check HFI status bit 4.2 If hfi status is 0 4.3 unlock spinlock 4.4 return 4.5 Check the stored time-stamp from the HFI memory time-stamp 5. if same 6.1 ACK HFI Interrupt, 6.2 unlock spinlock 6.3 return 7. Copy the HFI message to local buffer 8. ACK HFI interrupt 9. unlock spinlock 10. Queue the message for processing in a work-queue To avoid taking the lock unnecessarily, intel_hfi_process_event() checks the status of the HFI interrupt before taking the lock. If CPU5 is late, when it starts processing the interrupt there are two scenarios: a) CPU4 acknowledged the HFI interrupt before CPU5 read MSR_IA32_THERM_STATUS. CPU5 exits. b) CPU5 reads MSR_IA32_THERM_STATUS before CPU4 has acknowledged the interrupt. CPU5 will take the lock if CPU4 has released it. It then re-reads MSR_IA32_THERM_STATUS. If there is not a new interrupt, the HFI status bit is clear and CPU5 exits. If a new HFI interrupt was generated it will find that the status bit is set and it will continue to process the interrupt. In this case even if timestamp is not changed, the ACK can be issued as this is a new interrupt. Signed-off-by: Srinivas Pandruvada Reviewed-by: Ricardo Neri Tested-by: Arshad, Adeel Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_hfi.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c index 65b902939e1f..90a11eef44f7 100644 --- a/drivers/thermal/intel/intel_hfi.c +++ b/drivers/thermal/intel/intel_hfi.c @@ -250,7 +250,7 @@ void intel_hfi_process_event(__u64 pkg_therm_status_msr_val) struct hfi_instance *hfi_instance; int cpu = smp_processor_id(); struct hfi_cpu_info *info; - u64 new_timestamp; + u64 new_timestamp, msr, hfi; if (!pkg_therm_status_msr_val) return; @@ -279,9 +279,21 @@ void intel_hfi_process_event(__u64 pkg_therm_status_msr_val) if (!raw_spin_trylock(&hfi_instance->event_lock)) return; - /* Skip duplicated updates. */ + rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr); + hfi = msr & PACKAGE_THERM_STATUS_HFI_UPDATED; + if (!hfi) { + raw_spin_unlock(&hfi_instance->event_lock); + return; + } + + /* + * Ack duplicate update. Since there is an active HFI + * status from HW, it must be a new event, not a case + * where a lagging CPU entered the locked region. + */ new_timestamp = *(u64 *)hfi_instance->hw_table; if (*hfi_instance->timestamp == new_timestamp) { + thermal_clear_package_intr_status(PACKAGE_LEVEL, PACKAGE_THERM_STATUS_HFI_UPDATED); raw_spin_unlock(&hfi_instance->event_lock); return; } @@ -295,15 +307,15 @@ void intel_hfi_process_event(__u64 pkg_therm_status_msr_val) memcpy(hfi_instance->local_table, hfi_instance->hw_table, hfi_features.nr_table_pages << PAGE_SHIFT); - raw_spin_unlock(&hfi_instance->table_lock); - raw_spin_unlock(&hfi_instance->event_lock); - /* * Let hardware know that we are done reading the HFI table and it is * free to update it again. */ thermal_clear_package_intr_status(PACKAGE_LEVEL, PACKAGE_THERM_STATUS_HFI_UPDATED); + raw_spin_unlock(&hfi_instance->table_lock); + raw_spin_unlock(&hfi_instance->event_lock); + queue_delayed_work(hfi_updates_wq, &hfi_instance->update_work, HFI_UPDATE_INTERVAL); } From 4748f9687caaeefab8578285b97b2f30789fc4b4 Mon Sep 17 00:00:00 2001 From: Yang Yingliang Date: Tue, 15 Nov 2022 17:19:45 +0800 Subject: [PATCH 19/20] thermal: core: fix some possible name leaks in error paths In some error paths before device_register(), the names allocated by dev_set_name() are not freed. Move dev_set_name() front to device_register(), so the name can be freed while calling put_device(). Fixes: 1dd7128b839f ("thermal/core: Fix null pointer dereference in thermal_release()") Signed-off-by: Yang Yingliang Signed-off-by: Rafael J. Wysocki --- drivers/thermal/thermal_core.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index cc7dbcb2c739..f17ab2316dbd 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -894,10 +894,6 @@ __thermal_cooling_device_register(struct device_node *np, cdev->id = ret; id = ret; - ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); - if (ret) - goto out_ida_remove; - cdev->type = kstrdup(type ? type : "", GFP_KERNEL); if (!cdev->type) { ret = -ENOMEM; @@ -917,6 +913,11 @@ __thermal_cooling_device_register(struct device_node *np, goto out_kfree_type; thermal_cooling_device_setup_sysfs(cdev); + ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); + if (ret) { + thermal_cooling_device_destroy_sysfs(cdev); + goto out_kfree_type; + } ret = device_register(&cdev->device); if (ret) goto out_kfree_type; @@ -1250,10 +1251,6 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t tz->id = id; strscpy(tz->type, type, sizeof(tz->type)); - result = dev_set_name(&tz->device, "thermal_zone%d", tz->id); - if (result) - goto remove_id; - if (!ops->critical) ops->critical = thermal_zone_device_critical; @@ -1276,6 +1273,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t /* A new thermal zone needs to be updated anyway. */ atomic_set(&tz->need_update, 1); + result = dev_set_name(&tz->device, "thermal_zone%d", tz->id); + if (result) { + thermal_zone_destroy_device_groups(tz); + goto remove_id; + } result = device_register(&tz->device); if (result) goto release_device; From 3a3073b69c76a8909374c5f9d610ea2f02ba3402 Mon Sep 17 00:00:00 2001 From: Ricardo Neri Date: Mon, 28 Nov 2022 08:20:01 -0800 Subject: [PATCH 20/20] thermal: intel: hfi: Remove a pointless die_id check die_id is an u16 quantity. On single-die systems the default value of die_id is 0. No need to check for negative values. Plus, removing this check makes Coverity happy. Signed-off-by: Ricardo Neri Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_hfi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c index 90a11eef44f7..6e604bda2b93 100644 --- a/drivers/thermal/intel/intel_hfi.c +++ b/drivers/thermal/intel/intel_hfi.c @@ -379,7 +379,7 @@ void intel_hfi_online(unsigned int cpu) die_id = topology_logical_die_id(cpu); hfi_instance = info->hfi_instance; if (!hfi_instance) { - if (die_id < 0 || die_id >= max_hfi_instances) + if (die_id >= max_hfi_instances) return; hfi_instance = &hfi_instances[die_id];