| Message ID | 10247847.nUPlyArG6x@kreacher |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thermal: core/ACPI: Fix processor cooling device regression | expand |
On Fri, 2023-03-03 at 20:23 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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. > > Link: > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/thermal/thermal_core.c | 47 ++++++++++++++++++++++++++ > drivers/thermal/thermal_core.h | 1 > drivers/thermal/thermal_sysfs.c | 72 > +++++++++++++++++++++++++++++++++++----- > include/linux/thermal.h | 1 > 4 files changed, 113 insertions(+), 8 deletions(-) > > Index: linux-pm/drivers/thermal/thermal_core.c > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_core.c > +++ linux-pm/drivers/thermal/thermal_core.c > @@ -1057,6 +1057,53 @@ static bool thermal_cooling_device_prese > return false; > } > > +void thermal_cooling_device_update(struct thermal_cooling_device > *cdev) > +{ > + unsigned long state; > + > + if (!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); > + > + if (cdev->ops->get_cur_state(cdev, &state)) > + goto unlock; > + > + if (state <= cdev->max_state) > + goto update_stats; > + how could the .get_cur_state() callback returns a value higher than .get_max_state()? Isn't this a driver problem? > + if (cdev->ops->set_cur_state(cdev, state)) > + goto unlock; even if we don't error out, should we reevaluate .get_max_state() and update cdev->max_state? thanks, rui > + > +update_stats: > + 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 > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c > +++ linux-pm/drivers/thermal/thermal_sysfs.c > @@ -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 d > > spin_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++) { > @@ -775,8 +815,10 @@ static ssize_t trans_table_show(struct d > break; > 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 d > > if (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(s > unsigned 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; > } > @@ -879,6 +929,12 @@ void thermal_cooling_device_destroy_sysf > cooling_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 > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_core.h > +++ linux-pm/drivers/thermal/thermal_core.h > @@ -127,6 +127,7 @@ int thermal_zone_create_device_groups(st > void 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 > *); > Index: linux-pm/include/linux/thermal.h > =================================================================== > --- linux-pm.orig/include/linux/thermal.h > +++ linux-pm/include/linux/thermal.h > @@ -384,6 +384,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); > > >
On Tue, Mar 7, 2023 at 5:44 PM Zhang, Rui <rui.zhang@intel.com> wrote: > > On Fri, 2023-03-03 at 20:23 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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. > > > > Link: > > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/thermal/thermal_core.c | 47 ++++++++++++++++++++++++++ > > drivers/thermal/thermal_core.h | 1 > > drivers/thermal/thermal_sysfs.c | 72 > > +++++++++++++++++++++++++++++++++++----- > > include/linux/thermal.h | 1 > > 4 files changed, 113 insertions(+), 8 deletions(-) > > > > Index: linux-pm/drivers/thermal/thermal_core.c > > =================================================================== > > --- linux-pm.orig/drivers/thermal/thermal_core.c > > +++ linux-pm/drivers/thermal/thermal_core.c > > @@ -1057,6 +1057,53 @@ static bool thermal_cooling_device_prese > > return false; > > } > > > > +void thermal_cooling_device_update(struct thermal_cooling_device > > *cdev) > > +{ > > + unsigned long state; > > + > > + if (!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); > > + > > + if (cdev->ops->get_cur_state(cdev, &state)) > > + goto unlock; > > + > > + if (state <= cdev->max_state) > > + goto update_stats; > > + > how could the .get_cur_state() callback returns a value higher than > .get_max_state()? Isn't this a driver problem? It is a driver problem, but the check needs to be done here in case the driver has this problem, because thermal_cooling_device_stats_update() below might write out of array bounds otherwise. > > + if (cdev->ops->set_cur_state(cdev, state)) > > + goto unlock; > > even if we don't error out, should we reevaluate .get_max_state() and > update cdev->max_state? This has just been done and in fact it is not necessary to call the driver to set the state to the value that has just been returned by it. Actually, this particular piece is broken, because passing a value above max_state to the "set" callback here doesn't make sense. I'll fix this in v2. > > + > > +update_stats: > > + 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 > > =================================================================== > > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c > > +++ linux-pm/drivers/thermal/thermal_sysfs.c > > @@ -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 d > > > > spin_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++) { > > @@ -775,8 +815,10 @@ static ssize_t trans_table_show(struct d > > break; > > 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 d > > > > if (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(s > > unsigned 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; > > } > > @@ -879,6 +929,12 @@ void thermal_cooling_device_destroy_sysf > > cooling_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 > > =================================================================== > > --- linux-pm.orig/drivers/thermal/thermal_core.h > > +++ linux-pm/drivers/thermal/thermal_core.h > > @@ -127,6 +127,7 @@ int thermal_zone_create_device_groups(st > > void 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 > > *); > > Index: linux-pm/include/linux/thermal.h > > =================================================================== > > --- linux-pm.orig/include/linux/thermal.h > > +++ linux-pm/include/linux/thermal.h > > @@ -384,6 +384,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); > > > > > >
Hi, this leads to the stacktrace below triggered by lockdep_assert_held(&cdev->lock) in cooling_device_stats_setup(), and diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 566df4522b885..132175b14814f 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -918,7 +918,9 @@ __thermal_cooling_device_register(struct device_node *np, if (ret) goto out_cdev_type; + mutex_lock(&cdev->lock); thermal_cooling_device_setup_sysfs(cdev); + mutex_unlock(&cdev->lock); ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); if (ret) fixed it up for me, but not sure if it's the correct fix. --Imre [ 4.662358] ------------[ cut here ]------------ [ 4.662361] WARNING: CPU: 3 PID: 1 at drivers/thermal/thermal_sysfs.c:879 cooling_device_stats_setup+0xb4/0xc0 [ 4.662370] Modules linked in: [ 4.662375] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G I 6.3.0-rc4-imre+ #771 [ 4.662379] Hardware name: Intel Corporation Shark Bay Client platform/Flathead Creek Crb, BIOS HSWLPTU1.86C.0109.R03.1301282055 01/28/2013 [ 4.662382] RIP: 0010:cooling_device_stats_setup+0xb4/0xc0 [ 4.662387] Code: 89 1d 58 52 36 01 5b 41 5c 41 5d 5d c3 cc cc cc cc 48 8d bf 18 05 00 00 be ff ff ff ff e8 f4 d2 3e 00 85 c0 0f 85 6f ff ff ff <0f> 0b e9 68 ff ff ff 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 [ 4.662390] RSP: 0000:ffff9f48c0057b30 EFLAGS: 00010246 [ 4.662395] RAX: 0000000000000000 RBX: ffff8fc381ca9800 RCX: 0000000000000000 [ 4.662398] RDX: 0000000000000000 RSI: ffffffff94ad1d28 RDI: ffffffff94b58cc6 [ 4.662401] RBP: ffff9f48c0057b48 R08: 0000000000000004 R09: 0000000000000000 [ 4.662404] R10: ffff8fc381c77cd0 R11: 0000000000000000 R12: 0000000000000002 [ 4.662406] R13: ffff8fc381ca9800 R14: ffff8fc381b0a000 R15: 0000000000000000 [ 4.662409] FS: 0000000000000000(0000) GS:ffff8fc6b5580000(0000) knlGS:0000000000000000 [ 4.662412] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 4.662415] CR2: 0000000000000000 CR3: 0000000283856001 CR4: 00000000001706e0 [ 4.662418] Call Trace: [ 4.662421] <TASK> [ 4.662427] thermal_cooling_device_setup_sysfs+0x12/0x30 [ 4.662433] __thermal_cooling_device_register+0x195/0x410 [ 4.662442] thermal_cooling_device_register+0x19/0x20 [ 4.662446] acpi_fan_probe+0xd7/0x5a0 [ 4.662458] ? acpi_match_device_ids+0x12/0x20 [ 4.662464] ? acpi_dev_pm_attach+0x41/0x110 [ 4.662473] platform_probe+0x48/0xc0 [ 4.662481] really_probe+0x1be/0x420 [ 4.662487] __driver_probe_device+0x8c/0x190 [ 4.662493] driver_probe_device+0x24/0x90 [ 4.662498] __driver_attach+0xf7/0x200 [ 4.662503] ? __pfx___driver_attach+0x10/0x10 [ 4.662507] bus_for_each_dev+0x80/0xd0 [ 4.662516] driver_attach+0x1e/0x30 [ 4.662522] bus_add_driver+0x11f/0x230 [ 4.662530] driver_register+0x5e/0x120 [ 4.662534] ? __pfx_acpi_fan_driver_init+0x10/0x10 [ 4.662540] __platform_driver_register+0x1e/0x30 [ 4.662545] acpi_fan_driver_init+0x17/0x20 [ 4.662549] do_one_initcall+0x61/0x280 [ 4.662559] ? debug_smp_processor_id+0x17/0x20 [ 4.662568] kernel_init_freeable+0x411/0x640 [ 4.662582] ? __pfx_kernel_init+0x10/0x10 [ 4.662589] kernel_init+0x1b/0x1f0 [ 4.662594] ? __pfx_kernel_init+0x10/0x10 [ 4.662599] ret_from_fork+0x2c/0x50 [ 4.662615] </TASK> [ 4.662618] irq event stamp: 506869 [ 4.662620] hardirqs last enabled at (506875): [<ffffffff9338e2d8>] __up_console_sem+0x68/0x80 [ 4.662625] hardirqs last disabled at (506880): [<ffffffff9338e2bd>] __up_console_sem+0x4d/0x80 [ 4.662628] softirqs last enabled at (504698): [<ffffffff932de49f>] __irq_exit_rcu+0xbf/0x140 [ 4.662633] softirqs last disabled at (504689): [<ffffffff932de49f>] __irq_exit_rcu+0xbf/0x140 [ 4.662636] ---[ end trace 0000000000000000 ]--- [ 4.662779] ------------[ cut here ]------------
On Mon, Mar 27, 2023 at 10:58 PM Imre Deak <imre.deak@intel.com> wrote: > > Hi, > > this leads to the stacktrace below triggered by > lockdep_assert_held(&cdev->lock) in cooling_device_stats_setup(), Thanks for the report! > and > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 566df4522b885..132175b14814f 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -918,7 +918,9 @@ __thermal_cooling_device_register(struct device_node *np, > if (ret) > goto out_cdev_type; > > + mutex_lock(&cdev->lock); > thermal_cooling_device_setup_sysfs(cdev); > + mutex_unlock(&cdev->lock); > > ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); > if (ret) > > fixed it up for me, but not sure if it's the correct fix. There are other cases when the lockdep_assert_held() annotations may trigger, so it is better to remove them from cooling_device_stats_setup() and cooling_device_stats_destroy() and to put one into thermal_cooling_device_stats_reinit(). I'll send a patch to do that later today. > [ 4.662358] ------------[ cut here ]------------ > [ 4.662361] WARNING: CPU: 3 PID: 1 at drivers/thermal/thermal_sysfs.c:879 cooling_device_stats_setup+0xb4/0xc0 > [ 4.662370] Modules linked in: > [ 4.662375] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G I 6.3.0-rc4-imre+ #771 > [ 4.662379] Hardware name: Intel Corporation Shark Bay Client platform/Flathead Creek Crb, BIOS HSWLPTU1.86C.0109.R03.1301282055 01/28/2013 > [ 4.662382] RIP: 0010:cooling_device_stats_setup+0xb4/0xc0 > [ 4.662387] Code: 89 1d 58 52 36 01 5b 41 5c 41 5d 5d c3 cc cc cc cc 48 8d bf 18 05 00 00 be ff ff ff ff e8 f4 d2 3e 00 85 c0 0f 85 6f ff ff ff <0f> 0b e9 68 ff ff ff 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 > [ 4.662390] RSP: 0000:ffff9f48c0057b30 EFLAGS: 00010246 > [ 4.662395] RAX: 0000000000000000 RBX: ffff8fc381ca9800 RCX: 0000000000000000 > [ 4.662398] RDX: 0000000000000000 RSI: ffffffff94ad1d28 RDI: ffffffff94b58cc6 > [ 4.662401] RBP: ffff9f48c0057b48 R08: 0000000000000004 R09: 0000000000000000 > [ 4.662404] R10: ffff8fc381c77cd0 R11: 0000000000000000 R12: 0000000000000002 > [ 4.662406] R13: ffff8fc381ca9800 R14: ffff8fc381b0a000 R15: 0000000000000000 > [ 4.662409] FS: 0000000000000000(0000) GS:ffff8fc6b5580000(0000) knlGS:0000000000000000 > [ 4.662412] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 4.662415] CR2: 0000000000000000 CR3: 0000000283856001 CR4: 00000000001706e0 > [ 4.662418] Call Trace: > [ 4.662421] <TASK> > [ 4.662427] thermal_cooling_device_setup_sysfs+0x12/0x30 > [ 4.662433] __thermal_cooling_device_register+0x195/0x410 > [ 4.662442] thermal_cooling_device_register+0x19/0x20 > [ 4.662446] acpi_fan_probe+0xd7/0x5a0 > [ 4.662458] ? acpi_match_device_ids+0x12/0x20 > [ 4.662464] ? acpi_dev_pm_attach+0x41/0x110 > [ 4.662473] platform_probe+0x48/0xc0 > [ 4.662481] really_probe+0x1be/0x420 > [ 4.662487] __driver_probe_device+0x8c/0x190 > [ 4.662493] driver_probe_device+0x24/0x90 > [ 4.662498] __driver_attach+0xf7/0x200 > [ 4.662503] ? __pfx___driver_attach+0x10/0x10 > [ 4.662507] bus_for_each_dev+0x80/0xd0 > [ 4.662516] driver_attach+0x1e/0x30 > [ 4.662522] bus_add_driver+0x11f/0x230 > [ 4.662530] driver_register+0x5e/0x120 > [ 4.662534] ? __pfx_acpi_fan_driver_init+0x10/0x10 > [ 4.662540] __platform_driver_register+0x1e/0x30 > [ 4.662545] acpi_fan_driver_init+0x17/0x20 > [ 4.662549] do_one_initcall+0x61/0x280 > [ 4.662559] ? debug_smp_processor_id+0x17/0x20 > [ 4.662568] kernel_init_freeable+0x411/0x640 > [ 4.662582] ? __pfx_kernel_init+0x10/0x10 > [ 4.662589] kernel_init+0x1b/0x1f0 > [ 4.662594] ? __pfx_kernel_init+0x10/0x10 > [ 4.662599] ret_from_fork+0x2c/0x50 > [ 4.662615] </TASK> > [ 4.662618] irq event stamp: 506869 > [ 4.662620] hardirqs last enabled at (506875): [<ffffffff9338e2d8>] __up_console_sem+0x68/0x80 > [ 4.662625] hardirqs last disabled at (506880): [<ffffffff9338e2bd>] __up_console_sem+0x4d/0x80 > [ 4.662628] softirqs last enabled at (504698): [<ffffffff932de49f>] __irq_exit_rcu+0xbf/0x140 > [ 4.662633] softirqs last disabled at (504689): [<ffffffff932de49f>] __irq_exit_rcu+0xbf/0x140 > [ 4.662636] ---[ end trace 0000000000000000 ]--- > [ 4.662779] ------------[ cut here ]------------
Index: linux-pm/drivers/thermal/thermal_core.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.c +++ linux-pm/drivers/thermal/thermal_core.c @@ -1057,6 +1057,53 @@ static bool thermal_cooling_device_prese return false; } +void thermal_cooling_device_update(struct thermal_cooling_device *cdev) +{ + unsigned long state; + + if (!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); + + if (cdev->ops->get_cur_state(cdev, &state)) + goto unlock; + + if (state <= cdev->max_state) + goto update_stats; + + if (cdev->ops->set_cur_state(cdev, state)) + goto unlock; + +update_stats: + 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 =================================================================== --- linux-pm.orig/drivers/thermal/thermal_sysfs.c +++ linux-pm/drivers/thermal/thermal_sysfs.c @@ -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 d spin_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++) { @@ -775,8 +815,10 @@ static ssize_t trans_table_show(struct d break; 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 d if (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(s unsigned 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; } @@ -879,6 +929,12 @@ void thermal_cooling_device_destroy_sysf cooling_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 =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.h +++ linux-pm/drivers/thermal/thermal_core.h @@ -127,6 +127,7 @@ int thermal_zone_create_device_groups(st void 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 *); Index: linux-pm/include/linux/thermal.h =================================================================== --- linux-pm.orig/include/linux/thermal.h +++ linux-pm/include/linux/thermal.h @@ -384,6 +384,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);