From 7bda1910c4bccd4b8d4726620bb3d6bbfb62286e Mon Sep 17 00:00:00 2001 From: Sumeet Pawnikar Date: Sat, 6 Dec 2025 00:32:16 +0530 Subject: powercap: fix race condition in register_control_type() The device becomes visible to userspace via device_register() even before it fully initialized by idr_init(). If userspace or another thread tries to register a zone immediately after device_register(), the control_type_valid() will fail because the control_type is not yet in the list. The IDR is not yet initialized, so this race condition causes zone registration failure. Move idr_init() and list addition before device_register() fix the race condition. Signed-off-by: Sumeet Pawnikar [ rjw: Subject adjustment, empty line added ] Link: https://patch.msgid.link/20251205190216.5032-1-sumeet4linux@gmail.com Signed-off-by: Rafael J. Wysocki --- drivers/powercap/powercap_sys.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c index 4112a0097338..d14b36b75189 100644 --- a/drivers/powercap/powercap_sys.c +++ b/drivers/powercap/powercap_sys.c @@ -625,17 +625,23 @@ struct powercap_control_type *powercap_register_control_type( INIT_LIST_HEAD(&control_type->node); control_type->dev.class = &powercap_class; dev_set_name(&control_type->dev, "%s", name); - result = device_register(&control_type->dev); - if (result) { - put_device(&control_type->dev); - return ERR_PTR(result); - } idr_init(&control_type->idr); mutex_lock(&powercap_cntrl_list_lock); list_add_tail(&control_type->node, &powercap_cntrl_list); mutex_unlock(&powercap_cntrl_list_lock); + result = device_register(&control_type->dev); + if (result) { + mutex_lock(&powercap_cntrl_list_lock); + list_del(&control_type->node); + mutex_unlock(&powercap_cntrl_list_lock); + + idr_destroy(&control_type->idr); + put_device(&control_type->dev); + return ERR_PTR(result); + } + return control_type; } EXPORT_SYMBOL_GPL(powercap_register_control_type); -- cgit v1.2.3 From efc4c35b741af973de90f6826bf35d3b3ac36bf1 Mon Sep 17 00:00:00 2001 From: Sumeet Pawnikar Date: Sun, 7 Dec 2025 20:45:48 +0530 Subject: powercap: fix sscanf() error return value handling Fix inconsistent error handling for sscanf() return value check. Implicit boolean conversion is used instead of explicit return value checks. The code checks if (!sscanf(...)) which is incorrect because: 1. sscanf returns the number of successfully parsed items 2. On success, it returns 1 (one item passed) 3. On failure, it returns 0 or EOF 4. The check 'if (!sscanf(...))' is wrong because it treats success (1) as failure All occurrences of sscanf() now uses explicit return value check. With this behavior it returns '-EINVAL' when parsing fails (returns 0 or EOF), and continues when parsing succeeds (returns 1). Signed-off-by: Sumeet Pawnikar [ rjw: Subject and changelog edits ] Link: https://patch.msgid.link/20251207151549.202452-1-sumeet4linux@gmail.com Signed-off-by: Rafael J. Wysocki --- drivers/powercap/powercap_sys.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c index d14b36b75189..1ff369880beb 100644 --- a/drivers/powercap/powercap_sys.c +++ b/drivers/powercap/powercap_sys.c @@ -68,7 +68,7 @@ static ssize_t show_constraint_##_attr(struct device *dev, \ int id; \ struct powercap_zone_constraint *pconst;\ \ - if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id)) \ + if (sscanf(dev_attr->attr.name, "constraint_%d_", &id) != 1) \ return -EINVAL; \ if (id >= power_zone->const_id_cnt) \ return -EINVAL; \ @@ -93,7 +93,7 @@ static ssize_t store_constraint_##_attr(struct device *dev,\ int id; \ struct powercap_zone_constraint *pconst;\ \ - if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id)) \ + if (sscanf(dev_attr->attr.name, "constraint_%d_", &id) != 1) \ return -EINVAL; \ if (id >= power_zone->const_id_cnt) \ return -EINVAL; \ @@ -162,7 +162,7 @@ static ssize_t show_constraint_name(struct device *dev, ssize_t len = -ENODATA; struct powercap_zone_constraint *pconst; - if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id)) + if (sscanf(dev_attr->attr.name, "constraint_%d_", &id) != 1) return -EINVAL; if (id >= power_zone->const_id_cnt) return -EINVAL; -- cgit v1.2.3 From dcd0b625fe440d68bb4b97c71d18ca48ecd6e594 Mon Sep 17 00:00:00 2001 From: Srinivas Pandruvada Date: Wed, 17 Dec 2025 07:34:55 -0800 Subject: powercap: intel_rapl: Fix possible recursive lock warning With the RAPL PMU addition, there is a recursive locking when CPU online callback function calls rapl_package_add_pmu(). Here cpu_hotplug_lock is already acquired by cpuhp_thread_fun() and rapl_package_add_pmu() tries to acquire again. <4>[ 8.197433] ============================================ <4>[ 8.197437] WARNING: possible recursive locking detected <4>[ 8.197440] 6.19.0-rc1-lgci-xe-xe-4242-05b7c58b3367dca84+ #1 Not tainted <4>[ 8.197444] -------------------------------------------- <4>[ 8.197447] cpuhp/0/20 is trying to acquire lock: <4>[ 8.197450] ffffffff83487870 (cpu_hotplug_lock){++++}-{0:0}, at: rapl_package_add_pmu+0x37/0x370 [intel_rapl_common] <4>[ 8.197463] but task is already holding lock: <4>[ 8.197466] ffffffff83487870 (cpu_hotplug_lock){++++}-{0:0}, at: cpuhp_thread_fun+0x6d/0x290 <4>[ 8.197477] other info that might help us debug this: <4>[ 8.197480] Possible unsafe locking scenario: <4>[ 8.197483] CPU0 <4>[ 8.197485] ---- <4>[ 8.197487] lock(cpu_hotplug_lock); <4>[ 8.197490] lock(cpu_hotplug_lock); <4>[ 8.197493] *** DEADLOCK *** .. .. <4>[ 8.197542] __lock_acquire+0x146e/0x2790 <4>[ 8.197548] lock_acquire+0xc4/0x2c0 <4>[ 8.197550] ? rapl_package_add_pmu+0x37/0x370 [intel_rapl_common] <4>[ 8.197556] cpus_read_lock+0x41/0x110 <4>[ 8.197558] ? rapl_package_add_pmu+0x37/0x370 [intel_rapl_common] <4>[ 8.197561] rapl_package_add_pmu+0x37/0x370 [intel_rapl_common] <4>[ 8.197565] rapl_cpu_online+0x85/0x87 [intel_rapl_msr] <4>[ 8.197568] ? __pfx_rapl_cpu_online+0x10/0x10 [intel_rapl_msr] <4>[ 8.197570] cpuhp_invoke_callback+0x41f/0x6c0 <4>[ 8.197573] ? cpuhp_thread_fun+0x6d/0x290 <4>[ 8.197575] cpuhp_thread_fun+0x1e2/0x290 <4>[ 8.197578] ? smpboot_thread_fn+0x26/0x290 <4>[ 8.197581] smpboot_thread_fn+0x12f/0x290 <4>[ 8.197584] ? __pfx_smpboot_thread_fn+0x10/0x10 <4>[ 8.197586] kthread+0x11f/0x250 <4>[ 8.197589] ? __pfx_kthread+0x10/0x10 <4>[ 8.197592] ret_from_fork+0x344/0x3a0 <4>[ 8.197595] ? __pfx_kthread+0x10/0x10 <4>[ 8.197597] ret_from_fork_asm+0x1a/0x30 <4>[ 8.197604] Fix this issue in the same way as rapl powercap package domain is added from the same CPU online callback by introducing another interface which doesn't call cpus_read_lock(). Add rapl_package_add_pmu_locked() and rapl_package_remove_pmu_locked() which don't call cpus_read_lock(). Fixes: 748d6ba43afd ("powercap: intel_rapl: Enable MSR-based RAPL PMU support") Reported-by: Borah, Chaitanya Kumar Closes: https://lore.kernel.org/linux-pm/5427ede1-57a0-43d1-99f3-8ca4b0643e82@intel.com/T/#u Tested-by: Kuppuswamy Sathyanarayanan Tested-by: RavitejaX Veesam Signed-off-by: Srinivas Pandruvada Link: https://patch.msgid.link/20251217153455.3560176-1-srinivas.pandruvada@linux.intel.com Signed-off-by: Rafael J. Wysocki --- drivers/powercap/intel_rapl_common.c | 24 ++++++++++++++++++------ drivers/powercap/intel_rapl_msr.c | 4 ++-- include/linux/intel_rapl.h | 4 ++++ 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c index b9d87e56cbbc..3ff6da3bf4e6 100644 --- a/drivers/powercap/intel_rapl_common.c +++ b/drivers/powercap/intel_rapl_common.c @@ -2032,7 +2032,7 @@ end: return ret; } -int rapl_package_add_pmu(struct rapl_package *rp) +int rapl_package_add_pmu_locked(struct rapl_package *rp) { struct rapl_package_pmu_data *data = &rp->pmu_data; int idx; @@ -2040,8 +2040,6 @@ int rapl_package_add_pmu(struct rapl_package *rp) if (rp->has_pmu) return -EEXIST; - guard(cpus_read_lock)(); - for (idx = 0; idx < rp->nr_domains; idx++) { struct rapl_domain *rd = &rp->domains[idx]; int domain = rd->id; @@ -2091,17 +2089,23 @@ int rapl_package_add_pmu(struct rapl_package *rp) return rapl_pmu_update(rp); } +EXPORT_SYMBOL_GPL(rapl_package_add_pmu_locked); + +int rapl_package_add_pmu(struct rapl_package *rp) +{ + guard(cpus_read_lock)(); + + return rapl_package_add_pmu_locked(rp); +} EXPORT_SYMBOL_GPL(rapl_package_add_pmu); -void rapl_package_remove_pmu(struct rapl_package *rp) +void rapl_package_remove_pmu_locked(struct rapl_package *rp) { struct rapl_package *pos; if (!rp->has_pmu) return; - guard(cpus_read_lock)(); - list_for_each_entry(pos, &rapl_packages, plist) { /* PMU is still needed */ if (pos->has_pmu && pos != rp) @@ -2111,6 +2115,14 @@ void rapl_package_remove_pmu(struct rapl_package *rp) perf_pmu_unregister(&rapl_pmu.pmu); memset(&rapl_pmu, 0, sizeof(struct rapl_pmu)); } +EXPORT_SYMBOL_GPL(rapl_package_remove_pmu_locked); + +void rapl_package_remove_pmu(struct rapl_package *rp) +{ + guard(cpus_read_lock)(); + + rapl_package_remove_pmu_locked(rp); +} EXPORT_SYMBOL_GPL(rapl_package_remove_pmu); #endif diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c index 0ce1096b6314..9a7e150b3536 100644 --- a/drivers/powercap/intel_rapl_msr.c +++ b/drivers/powercap/intel_rapl_msr.c @@ -82,7 +82,7 @@ static int rapl_cpu_online(unsigned int cpu) if (IS_ERR(rp)) return PTR_ERR(rp); if (rapl_msr_pmu) - rapl_package_add_pmu(rp); + rapl_package_add_pmu_locked(rp); } cpumask_set_cpu(cpu, &rp->cpumask); return 0; @@ -101,7 +101,7 @@ static int rapl_cpu_down_prep(unsigned int cpu) lead_cpu = cpumask_first(&rp->cpumask); if (lead_cpu >= nr_cpu_ids) { if (rapl_msr_pmu) - rapl_package_remove_pmu(rp); + rapl_package_remove_pmu_locked(rp); rapl_remove_package_cpuslocked(rp); } else if (rp->lead_cpu == cpu) { rp->lead_cpu = lead_cpu; diff --git a/include/linux/intel_rapl.h b/include/linux/intel_rapl.h index e9ade2ff4af6..f479ef5b3341 100644 --- a/include/linux/intel_rapl.h +++ b/include/linux/intel_rapl.h @@ -214,10 +214,14 @@ void rapl_remove_package(struct rapl_package *rp); #ifdef CONFIG_PERF_EVENTS int rapl_package_add_pmu(struct rapl_package *rp); +int rapl_package_add_pmu_locked(struct rapl_package *rp); void rapl_package_remove_pmu(struct rapl_package *rp); +void rapl_package_remove_pmu_locked(struct rapl_package *rp); #else static inline int rapl_package_add_pmu(struct rapl_package *rp) { return 0; } +static inline int rapl_package_add_pmu_locked(struct rapl_package *rp) { return 0; } static inline void rapl_package_remove_pmu(struct rapl_package *rp) { } +static inline void rapl_package_remove_pmu_locked(struct rapl_package *rp) { } #endif #endif /* __INTEL_RAPL_H__ */ -- cgit v1.2.3