* [PATCH v2 00/11] thermal: core: Reimplement locking through guards
@ 2024-10-10 21:59 Rafael J. Wysocki
2024-10-10 22:05 ` [PATCH v2 01/11] thermal: core: Add and use thermal zone guard Rafael J. Wysocki
` (12 more replies)
0 siblings, 13 replies; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 21:59 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
Hi Everyone,
This is a continuation of
https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
and (quite obviously) it is based on that series.
The majority of the patches in it are new iterations of patches included in
https://lore.kernel.org/linux-pm/6100907.lOV4Wx5bFT@rjwysocki.net/
and there is one new patch ([02/11]).
All of these patches are related to locking, but some of them are preparatory.
The series as a whole introduces guards for thermal zones and cooling devices
and uses them to re-implement locking in the thermal core. It also uses mutex
guards for thermal_list_lock and thermal_governor_lock locking.
As usual, the details are described by the individual patch changelogs.
Thanks!
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
@ 2024-10-10 22:05 ` Rafael J. Wysocki
2024-10-21 11:40 ` Markus Elfring
2024-10-22 21:02 ` [PATCH v2 " Lukasz Luba
2024-10-10 22:07 ` [PATCH v2 02/11] thermal: core: Add and use a reverse " Rafael J. Wysocki
` (11 subsequent siblings)
12 siblings, 2 replies; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:05 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Add and use a guard for thermal zone locking.
This allows quite a few error code paths to be simplified among
other things and brings in a noticeable code size reduction for
a good measure.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a new iteration of
https://lore.kernel.org/linux-pm/3241904.5fSG56mABF@rjwysocki.net/
that has been combined with
https://lore.kernel.org/linux-pm/4613601.LvFx2qVVIh@rjwysocki.net/
and rebased on top of
https://lore.kernel.org/linux-pm/12549318.O9o76ZdvQC@rjwysocki.net/
and
https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 61 +++++++---------------------
drivers/thermal/thermal_core.h | 4 +
drivers/thermal/thermal_debugfs.c | 25 +++++++----
drivers/thermal/thermal_helpers.c | 17 ++-----
drivers/thermal/thermal_hwmon.c | 5 --
drivers/thermal/thermal_netlink.c | 21 ++-------
drivers/thermal/thermal_sysfs.c | 81 ++++++++++++++++----------------------
drivers/thermal/thermal_trip.c | 8 ---
8 files changed, 86 insertions(+), 136 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -9,6 +9,7 @@
#ifndef __THERMAL_CORE_H__
#define __THERMAL_CORE_H__
+#include <linux/cleanup.h>
#include <linux/device.h>
#include <linux/thermal.h>
@@ -144,6 +145,9 @@ struct thermal_zone_device {
struct thermal_trip_desc trips[] __counted_by(num_trips);
};
+DEFINE_GUARD(thermal_zone, struct thermal_zone_device *, mutex_lock(&_T->lock),
+ mutex_unlock(&_T->lock))
+
/* Initial thermal zone temperature. */
#define THERMAL_TEMP_INIT INT_MIN
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -50,13 +50,13 @@ static ssize_t
mode_show(struct device *dev, struct device_attribute *attr, char *buf)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- int enabled;
- mutex_lock(&tz->lock);
- enabled = tz->mode == THERMAL_DEVICE_ENABLED;
- mutex_unlock(&tz->lock);
+ guard(thermal_zone)(tz);
- return sprintf(buf, "%s\n", enabled ? "enabled" : "disabled");
+ if (tz->mode == THERMAL_DEVICE_ENABLED)
+ return sprintf(buf, "enabled\n");
+
+ return sprintf(buf, "disabled\n");
}
static ssize_t
@@ -103,38 +103,34 @@ trip_point_temp_store(struct device *dev
{
struct thermal_trip *trip = thermal_trip_of_attr(attr, temp);
struct thermal_zone_device *tz = to_thermal_zone(dev);
- int ret, temp;
+ int temp;
- ret = kstrtoint(buf, 10, &temp);
- if (ret)
+ if (kstrtoint(buf, 10, &temp))
return -EINVAL;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
if (temp == trip->temperature)
- goto unlock;
+ return count;
/* Arrange the condition to avoid integer overflows. */
if (temp != THERMAL_TEMP_INVALID &&
- temp <= trip->hysteresis + THERMAL_TEMP_INVALID) {
- ret = -EINVAL;
- goto unlock;
- }
+ temp <= trip->hysteresis + THERMAL_TEMP_INVALID)
+ return -EINVAL;
if (tz->ops.set_trip_temp) {
+ int ret;
+
ret = tz->ops.set_trip_temp(tz, trip, temp);
if (ret)
- goto unlock;
+ return ret;
}
thermal_zone_set_trip_temp(tz, trip, temp);
__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
-unlock:
- mutex_unlock(&tz->lock);
-
- return ret ? ret : count;
+ return count;
}
static ssize_t
@@ -152,16 +148,15 @@ trip_point_hyst_store(struct device *dev
{
struct thermal_trip *trip = thermal_trip_of_attr(attr, hyst);
struct thermal_zone_device *tz = to_thermal_zone(dev);
- int ret, hyst;
+ int hyst;
- ret = kstrtoint(buf, 10, &hyst);
- if (ret || hyst < 0)
+ if (kstrtoint(buf, 10, &hyst) || hyst < 0)
return -EINVAL;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
if (hyst == trip->hysteresis)
- goto unlock;
+ return count;
/*
* Allow the hysteresis to be updated when the temperature is invalid
@@ -171,22 +166,17 @@ trip_point_hyst_store(struct device *dev
*/
if (trip->temperature == THERMAL_TEMP_INVALID) {
WRITE_ONCE(trip->hysteresis, hyst);
- goto unlock;
+ return count;
}
- if (trip->temperature - hyst <= THERMAL_TEMP_INVALID) {
- ret = -EINVAL;
- goto unlock;
- }
+ if (trip->temperature - hyst <= THERMAL_TEMP_INVALID)
+ return -EINVAL;
thermal_zone_set_trip_hyst(tz, trip, hyst);
__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
-unlock:
- mutex_unlock(&tz->lock);
-
- return ret ? ret : count;
+ return count;
}
static ssize_t
@@ -236,25 +226,26 @@ emul_temp_store(struct device *dev, stru
const char *buf, size_t count)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- int ret = 0;
int temperature;
if (kstrtoint(buf, 10, &temperature))
return -EINVAL;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
- if (!tz->ops.set_emul_temp)
- tz->emul_temperature = temperature;
- else
- ret = tz->ops.set_emul_temp(tz, temperature);
+ if (tz->ops.set_emul_temp) {
+ int ret;
- if (!ret)
- __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
+ ret = tz->ops.set_emul_temp(tz, temperature);
+ if (ret)
+ return ret;
+ } else {
+ tz->emul_temperature = temperature;
+ }
- mutex_unlock(&tz->lock);
+ __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
- return ret ? ret : count;
+ return count;
}
static DEVICE_ATTR_WO(emul_temp);
#endif
@@ -894,13 +885,11 @@ ssize_t weight_store(struct device *dev,
instance = container_of(attr, struct thermal_instance, weight_attr);
/* Don't race with governors using the 'weight' value */
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
instance->weight = weight;
thermal_governor_update_tz(tz, THERMAL_INSTANCE_WEIGHT_CHANGED);
- mutex_unlock(&tz->lock);
-
return count;
}
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -45,13 +45,9 @@ int thermal_zone_for_each_trip(struct th
int (*cb)(struct thermal_trip *, void *),
void *data)
{
- int ret;
+ guard(thermal_zone)(tz);
- mutex_lock(&tz->lock);
- ret = for_each_thermal_trip(tz, cb, data);
- mutex_unlock(&tz->lock);
-
- return ret;
+ return for_each_thermal_trip(tz, cb, data);
}
EXPORT_SYMBOL_GPL(thermal_zone_for_each_trip);
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -60,13 +60,13 @@ bool thermal_trip_is_bound_to_cdev(struc
{
bool ret;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
+
mutex_lock(&cdev->lock);
ret = thermal_instance_present(tz, cdev, trip);
mutex_unlock(&cdev->lock);
- mutex_unlock(&tz->lock);
return ret;
}
@@ -138,19 +138,14 @@ int thermal_zone_get_temp(struct thermal
if (IS_ERR_OR_NULL(tz))
return -EINVAL;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
- if (!tz->ops.get_temp) {
- ret = -EINVAL;
- goto unlock;
- }
+ if (!tz->ops.get_temp)
+ return -EINVAL;
ret = __thermal_zone_get_temp(tz, temp);
if (!ret && *temp <= THERMAL_TEMP_INVALID)
- ret = -ENODATA;
-
-unlock:
- mutex_unlock(&tz->lock);
+ return -ENODATA;
return ret;
}
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -202,16 +202,13 @@ int thermal_zone_device_set_policy(struc
int ret = -EINVAL;
mutex_lock(&thermal_governor_lock);
- mutex_lock(&tz->lock);
- gov = __find_governor(strim(policy));
- if (!gov)
- goto exit;
+ guard(thermal_zone)(tz);
- ret = thermal_set_governor(tz, gov);
+ gov = __find_governor(strim(policy));
+ if (gov)
+ ret = thermal_set_governor(tz, gov);
-exit:
- mutex_unlock(&tz->lock);
mutex_unlock(&thermal_governor_lock);
thermal_notify_tz_gov_change(tz, policy);
@@ -615,26 +612,18 @@ static int thermal_zone_device_set_mode(
{
int ret;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
/* do nothing if mode isn't changing */
- if (mode == tz->mode) {
- mutex_unlock(&tz->lock);
-
+ if (mode == tz->mode)
return 0;
- }
ret = __thermal_zone_device_set_mode(tz, mode);
- if (ret) {
- mutex_unlock(&tz->lock);
-
+ if (ret)
return ret;
- }
__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
- mutex_unlock(&tz->lock);
-
if (mode == THERMAL_DEVICE_ENABLED)
thermal_notify_tz_enable(tz);
else
@@ -663,10 +652,10 @@ static bool thermal_zone_is_present(stru
void thermal_zone_device_update(struct thermal_zone_device *tz,
enum thermal_notify_event event)
{
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
+
if (thermal_zone_is_present(tz))
__thermal_zone_device_update(tz, event);
- mutex_unlock(&tz->lock);
}
EXPORT_SYMBOL_GPL(thermal_zone_device_update);
@@ -970,12 +959,10 @@ static bool __thermal_zone_cdev_bind(str
static void thermal_zone_cdev_bind(struct thermal_zone_device *tz,
struct thermal_cooling_device *cdev)
{
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
if (__thermal_zone_cdev_bind(tz, cdev))
__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
-
- mutex_unlock(&tz->lock);
}
/**
@@ -1282,11 +1269,9 @@ static void __thermal_zone_cdev_unbind(s
static void thermal_zone_cdev_unbind(struct thermal_zone_device *tz,
struct thermal_cooling_device *cdev)
{
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
__thermal_zone_cdev_unbind(tz, cdev);
-
- mutex_unlock(&tz->lock);
}
/**
@@ -1332,7 +1317,7 @@ int thermal_zone_get_crit_temp(struct th
if (tz->ops.get_crit_temp)
return tz->ops.get_crit_temp(tz, temp);
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
for_each_trip_desc(tz, td) {
const struct thermal_trip *trip = &td->trip;
@@ -1344,8 +1329,6 @@ int thermal_zone_get_crit_temp(struct th
}
}
- mutex_unlock(&tz->lock);
-
return ret;
}
EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
@@ -1358,7 +1341,7 @@ static void thermal_zone_init_complete(s
list_add_tail(&tz->node, &thermal_tz_list);
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
/* Bind cooling devices for this zone. */
list_for_each_entry(cdev, &thermal_cdev_list, node)
@@ -1375,8 +1358,6 @@ static void thermal_zone_init_complete(s
__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
- mutex_unlock(&tz->lock);
-
mutex_unlock(&thermal_list_lock);
}
@@ -1607,7 +1588,7 @@ static bool thermal_zone_exit(struct the
goto unlock;
}
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
tz->state |= TZ_STATE_FLAG_EXIT;
list_del_init(&tz->node);
@@ -1616,8 +1597,6 @@ static bool thermal_zone_exit(struct the
list_for_each_entry(cdev, &thermal_cdev_list, node)
__thermal_zone_cdev_unbind(tz, cdev);
- mutex_unlock(&tz->lock);
-
unlock:
mutex_unlock(&thermal_list_lock);
@@ -1701,7 +1680,7 @@ static void thermal_zone_device_resume(s
tz = container_of(work, struct thermal_zone_device, poll_queue.work);
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
tz->state &= ~(TZ_STATE_FLAG_SUSPENDED | TZ_STATE_FLAG_RESUMING);
@@ -1711,13 +1690,11 @@ static void thermal_zone_device_resume(s
__thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
complete(&tz->resume);
-
- mutex_unlock(&tz->lock);
}
static void thermal_zone_pm_prepare(struct thermal_zone_device *tz)
{
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
if (tz->state & TZ_STATE_FLAG_RESUMING) {
/*
@@ -1733,13 +1710,11 @@ static void thermal_zone_pm_prepare(stru
}
tz->state |= TZ_STATE_FLAG_SUSPENDED;
-
- mutex_unlock(&tz->lock);
}
static void thermal_zone_pm_complete(struct thermal_zone_device *tz)
{
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
cancel_delayed_work(&tz->poll_queue);
@@ -1753,8 +1728,6 @@ static void thermal_zone_pm_complete(str
INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_resume);
/* Queue up the work without a delay. */
mod_delayed_work(system_freezable_power_efficient_wq, &tz->poll_queue, 0);
-
- mutex_unlock(&tz->lock);
}
static int thermal_pm_notify(struct notifier_block *nb,
Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -885,6 +885,19 @@ void thermal_debug_tz_add(struct thermal
tz->debugfs = thermal_dbg;
}
+static struct thermal_debugfs *thermal_debug_tz_clear(struct thermal_zone_device *tz)
+{
+ struct thermal_debugfs *thermal_dbg;
+
+ guard(thermal_zone)(tz);
+
+ thermal_dbg = tz->debugfs;
+ if (thermal_dbg)
+ tz->debugfs = NULL;
+
+ return thermal_dbg;
+}
+
void thermal_debug_tz_remove(struct thermal_zone_device *tz)
{
struct thermal_debugfs *thermal_dbg;
@@ -892,17 +905,9 @@ void thermal_debug_tz_remove(struct ther
struct tz_debugfs *tz_dbg;
int *trips_crossed;
- mutex_lock(&tz->lock);
-
- thermal_dbg = tz->debugfs;
- if (!thermal_dbg) {
- mutex_unlock(&tz->lock);
+ thermal_dbg = thermal_debug_tz_clear(tz);
+ if (!thermal_dbg)
return;
- }
-
- tz->debugfs = NULL;
-
- mutex_unlock(&tz->lock);
tz_dbg = &thermal_dbg->tz_dbg;
Index: linux-pm/drivers/thermal/thermal_hwmon.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_hwmon.c
+++ linux-pm/drivers/thermal/thermal_hwmon.c
@@ -78,12 +78,9 @@ temp_crit_show(struct device *dev, struc
int temperature;
int ret;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
ret = tz->ops.get_crit_temp(tz, &temperature);
-
- mutex_unlock(&tz->lock);
-
if (ret)
return ret;
Index: linux-pm/drivers/thermal/thermal_netlink.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_netlink.c
+++ linux-pm/drivers/thermal/thermal_netlink.c
@@ -459,7 +459,7 @@ static int thermal_genl_cmd_tz_get_trip(
if (!start_trip)
return -EMSGSIZE;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
for_each_trip_desc(tz, td) {
const struct thermal_trip *trip = &td->trip;
@@ -469,19 +469,12 @@ static int thermal_genl_cmd_tz_get_trip(
nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, trip->type) ||
nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TEMP, trip->temperature) ||
nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_HYST, trip->hysteresis))
- goto out_cancel_nest;
+ return -EMSGSIZE;
}
- mutex_unlock(&tz->lock);
-
nla_nest_end(msg, start_trip);
return 0;
-
-out_cancel_nest:
- mutex_unlock(&tz->lock);
-
- return -EMSGSIZE;
}
static int thermal_genl_cmd_tz_get_temp(struct param *p)
@@ -512,7 +505,7 @@ static int thermal_genl_cmd_tz_get_temp(
static int thermal_genl_cmd_tz_get_gov(struct param *p)
{
struct sk_buff *msg = p->msg;
- int id, ret = 0;
+ int id;
if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
return -EINVAL;
@@ -523,16 +516,14 @@ static int thermal_genl_cmd_tz_get_gov(s
if (!tz)
return -EINVAL;
- mutex_lock(&tz->lock);
+ guard(thermal_zone)(tz);
if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id) ||
nla_put_string(msg, THERMAL_GENL_ATTR_TZ_GOV_NAME,
tz->governor->name))
- ret = -EMSGSIZE;
-
- mutex_unlock(&tz->lock);
+ return -EMSGSIZE;
- return ret;
+ return 0;
}
static int __thermal_genl_cmd_cdev_get(struct thermal_cooling_device *cdev,
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 02/11] thermal: core: Add and use a reverse thermal zone guard
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
2024-10-10 22:05 ` [PATCH v2 01/11] thermal: core: Add and use thermal zone guard Rafael J. Wysocki
@ 2024-10-10 22:07 ` Rafael J. Wysocki
2024-10-22 21:03 ` Lukasz Luba
2024-10-10 22:09 ` [PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock Rafael J. Wysocki
` (10 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:07 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Add a guard for unlocking a locked thermal zone temporarily and use it
in thermal_zone_pm_prepare().
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a new patch
---
drivers/thermal/thermal_core.c | 8 +++-----
drivers/thermal/thermal_core.h | 3 +++
2 files changed, 6 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1702,11 +1702,9 @@ static void thermal_zone_pm_prepare(stru
* acquired the lock yet, so release it to let the function run
* and wait util it has done the work.
*/
- mutex_unlock(&tz->lock);
-
- wait_for_completion(&tz->resume);
-
- mutex_lock(&tz->lock);
+ scoped_guard(thermal_zone_reverse, tz) {
+ wait_for_completion(&tz->resume);
+ }
}
tz->state |= TZ_STATE_FLAG_SUSPENDED;
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -148,6 +148,9 @@ struct thermal_zone_device {
DEFINE_GUARD(thermal_zone, struct thermal_zone_device *, mutex_lock(&_T->lock),
mutex_unlock(&_T->lock))
+DEFINE_GUARD(thermal_zone_reverse, struct thermal_zone_device *,
+ mutex_unlock(&_T->lock), mutex_lock(&_T->lock))
+
/* Initial thermal zone temperature. */
#define THERMAL_TEMP_INIT INT_MIN
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
2024-10-10 22:05 ` [PATCH v2 01/11] thermal: core: Add and use thermal zone guard Rafael J. Wysocki
2024-10-10 22:07 ` [PATCH v2 02/11] thermal: core: Add and use a reverse " Rafael J. Wysocki
@ 2024-10-10 22:09 ` Rafael J. Wysocki
2024-10-22 21:09 ` Lukasz Luba
2024-10-10 22:10 ` [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard Rafael J. Wysocki
` (9 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:09 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
To prepare for a subsequent change that will switch over the thermal
core to using a mutex guard for thermal_list_lock management, move the
code running under thermal_list_lock during the initialization and
unregistration of cooling devices into separate functions.
While at it, drop some comments that do not add value.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/1822468.VLH7GnMWUR@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 64 +++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 28 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -965,6 +965,20 @@ static void thermal_zone_cdev_bind(struc
__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
}
+static void thermal_cooling_device_init_complete(struct thermal_cooling_device *cdev)
+{
+ struct thermal_zone_device *tz;
+
+ mutex_lock(&thermal_list_lock);
+
+ list_add(&cdev->node, &thermal_cdev_list);
+
+ list_for_each_entry(tz, &thermal_tz_list, node)
+ thermal_zone_cdev_bind(tz, cdev);
+
+ mutex_unlock(&thermal_list_lock);
+}
+
/**
* __thermal_cooling_device_register() - register a new thermal cooling device
* @np: a pointer to a device tree node.
@@ -987,7 +1001,6 @@ __thermal_cooling_device_register(struct
const struct thermal_cooling_device_ops *ops)
{
struct thermal_cooling_device *cdev;
- struct thermal_zone_device *pos;
unsigned long current_state;
int id, ret;
@@ -1054,16 +1067,7 @@ __thermal_cooling_device_register(struct
if (current_state <= cdev->max_state)
thermal_debug_cdev_add(cdev, current_state);
- /* Add 'this' new cdev to the global cdev list */
- mutex_lock(&thermal_list_lock);
-
- list_add(&cdev->node, &thermal_cdev_list);
-
- /* Update binding information for 'this' new cdev */
- list_for_each_entry(pos, &thermal_tz_list, node)
- thermal_zone_cdev_bind(pos, cdev);
-
- mutex_unlock(&thermal_list_lock);
+ thermal_cooling_device_init_complete(cdev);
return cdev;
@@ -1274,38 +1278,42 @@ static void thermal_zone_cdev_unbind(str
__thermal_zone_cdev_unbind(tz, cdev);
}
-/**
- * thermal_cooling_device_unregister - removes a thermal cooling device
- * @cdev: the thermal cooling device to remove.
- *
- * thermal_cooling_device_unregister() must be called when a registered
- * thermal cooling device is no longer needed.
- */
-void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
+static bool thermal_cooling_device_exit(struct thermal_cooling_device *cdev)
{
struct thermal_zone_device *tz;
-
- if (!cdev)
- return;
-
- thermal_debug_cdev_remove(cdev);
+ bool ret = true;
mutex_lock(&thermal_list_lock);
if (!thermal_cooling_device_present(cdev)) {
- mutex_unlock(&thermal_list_lock);
- return;
+ ret = false;
+ goto unlock;
}
list_del(&cdev->node);
- /* Unbind all thermal zones associated with 'this' cdev */
list_for_each_entry(tz, &thermal_tz_list, node)
thermal_zone_cdev_unbind(tz, cdev);
+unlock:
mutex_unlock(&thermal_list_lock);
- device_unregister(&cdev->device);
+ return ret;
+}
+
+/**
+ * thermal_cooling_device_unregister() - removes a thermal cooling device
+ * @cdev: Thermal cooling device to remove.
+ */
+void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
+{
+ if (!cdev)
+ return;
+
+ thermal_debug_cdev_remove(cdev);
+
+ if (thermal_cooling_device_exit(cdev))
+ device_unregister(&cdev->device);
}
EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (2 preceding siblings ...)
2024-10-10 22:09 ` [PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock Rafael J. Wysocki
@ 2024-10-10 22:10 ` Rafael J. Wysocki
2024-10-22 22:50 ` Lukasz Luba
2024-10-10 22:12 ` [PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock Rafael J. Wysocki
` (8 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:10 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Switch over the thermal core to using a mutex guard for
thermal_list_lock management.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/2018087.usQuhbGJ8B@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 148 ++++++++++++++++++-----------------------
1 file changed, 68 insertions(+), 80 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -140,7 +140,7 @@ int thermal_register_governor(struct the
def_governor = governor;
}
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
list_for_each_entry(pos, &thermal_tz_list, node) {
/*
@@ -163,7 +163,6 @@ int thermal_register_governor(struct the
}
}
- mutex_unlock(&thermal_list_lock);
mutex_unlock(&thermal_governor_lock);
return err;
@@ -181,7 +180,9 @@ void thermal_unregister_governor(struct
if (!__find_governor(governor->name))
goto exit;
- mutex_lock(&thermal_list_lock);
+ list_del(&governor->governor_list);
+
+ guard(mutex)(&thermal_list_lock);
list_for_each_entry(pos, &thermal_tz_list, node) {
if (!strncasecmp(pos->governor->name, governor->name,
@@ -189,8 +190,6 @@ void thermal_unregister_governor(struct
thermal_set_governor(pos, NULL);
}
- mutex_unlock(&thermal_list_lock);
- list_del(&governor->governor_list);
exit:
mutex_unlock(&thermal_governor_lock);
}
@@ -686,51 +685,52 @@ int for_each_thermal_cooling_device(int
void *), void *data)
{
struct thermal_cooling_device *cdev;
- int ret = 0;
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
+
list_for_each_entry(cdev, &thermal_cdev_list, node) {
+ int ret;
+
ret = cb(cdev, data);
if (ret)
- break;
+ return ret;
}
- mutex_unlock(&thermal_list_lock);
- return ret;
+ return 0;
}
int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
void *data)
{
struct thermal_zone_device *tz;
- int ret = 0;
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
+
list_for_each_entry(tz, &thermal_tz_list, node) {
+ int ret;
+
ret = cb(tz, data);
if (ret)
- break;
+ return ret;
}
- mutex_unlock(&thermal_list_lock);
- return ret;
+ return 0;
}
struct thermal_zone_device *thermal_zone_get_by_id(int id)
{
- struct thermal_zone_device *tz, *match = NULL;
+ struct thermal_zone_device *tz;
+
+ guard(mutex)(&thermal_list_lock);
- mutex_lock(&thermal_list_lock);
list_for_each_entry(tz, &thermal_tz_list, node) {
if (tz->id == id) {
get_device(&tz->device);
- match = tz;
- break;
+ return tz;
}
}
- mutex_unlock(&thermal_list_lock);
- return match;
+ return NULL;
}
/*
@@ -969,14 +969,12 @@ static void thermal_cooling_device_init_
{
struct thermal_zone_device *tz;
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
list_add(&cdev->node, &thermal_cdev_list);
list_for_each_entry(tz, &thermal_tz_list, node)
thermal_zone_cdev_bind(tz, cdev);
-
- mutex_unlock(&thermal_list_lock);
}
/**
@@ -1210,10 +1208,10 @@ void thermal_cooling_device_update(struc
* Hold thermal_list_lock throughout the update to prevent the device
* from going away while being updated.
*/
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
if (!thermal_cooling_device_present(cdev))
- goto unlock_list;
+ return;
/*
* Update under the cdev lock to prevent the state from being set beyond
@@ -1255,9 +1253,6 @@ void thermal_cooling_device_update(struc
unlock:
mutex_unlock(&cdev->lock);
-
-unlock_list:
- mutex_unlock(&thermal_list_lock);
}
EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
@@ -1281,24 +1276,18 @@ static void thermal_zone_cdev_unbind(str
static bool thermal_cooling_device_exit(struct thermal_cooling_device *cdev)
{
struct thermal_zone_device *tz;
- bool ret = true;
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
- if (!thermal_cooling_device_present(cdev)) {
- ret = false;
- goto unlock;
- }
+ if (!thermal_cooling_device_present(cdev))
+ return false;
list_del(&cdev->node);
list_for_each_entry(tz, &thermal_tz_list, node)
thermal_zone_cdev_unbind(tz, cdev);
-unlock:
- mutex_unlock(&thermal_list_lock);
-
- return ret;
+ return true;
}
/**
@@ -1345,7 +1334,7 @@ static void thermal_zone_init_complete(s
{
struct thermal_cooling_device *cdev;
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
list_add_tail(&tz->node, &thermal_tz_list);
@@ -1365,8 +1354,6 @@ static void thermal_zone_init_complete(s
tz->state |= TZ_STATE_FLAG_SUSPENDED;
__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
-
- mutex_unlock(&thermal_list_lock);
}
/**
@@ -1587,14 +1574,11 @@ EXPORT_SYMBOL_GPL(thermal_zone_device);
static bool thermal_zone_exit(struct thermal_zone_device *tz)
{
struct thermal_cooling_device *cdev;
- bool ret = true;
- mutex_lock(&thermal_list_lock);
+ guard(mutex)(&thermal_list_lock);
- if (list_empty(&tz->node)) {
- ret = false;
- goto unlock;
- }
+ if (list_empty(&tz->node))
+ return false;
guard(thermal_zone)(tz);
@@ -1605,10 +1589,7 @@ static bool thermal_zone_exit(struct the
list_for_each_entry(cdev, &thermal_cdev_list, node)
__thermal_zone_cdev_unbind(tz, cdev);
-unlock:
- mutex_unlock(&thermal_list_lock);
-
- return ret;
+ return true;
}
/**
@@ -1660,24 +1641,23 @@ struct thermal_zone_device *thermal_zone
unsigned int found = 0;
if (!name)
- goto exit;
+ return ERR_PTR(-EINVAL);
+
+ guard(mutex)(&thermal_list_lock);
- mutex_lock(&thermal_list_lock);
list_for_each_entry(pos, &thermal_tz_list, node)
if (!strncasecmp(name, pos->type, THERMAL_NAME_LENGTH)) {
found++;
ref = pos;
}
- mutex_unlock(&thermal_list_lock);
- /* nothing has been found, thus an error code for it */
- if (found == 0)
- ref = ERR_PTR(-ENODEV);
- else if (found > 1)
- /* Success only when an unique zone is found */
- ref = ERR_PTR(-EEXIST);
+ if (!found)
+ return ERR_PTR(-ENODEV);
+
+ /* Success only when one zone is found. */
+ if (found > 1)
+ return ERR_PTR(-EEXIST);
-exit:
return ref;
}
EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
@@ -1718,6 +1698,18 @@ static void thermal_zone_pm_prepare(stru
tz->state |= TZ_STATE_FLAG_SUSPENDED;
}
+static void thermal_pm_notify_prepare(void)
+{
+ struct thermal_zone_device *tz;
+
+ guard(mutex)(&thermal_list_lock);
+
+ thermal_pm_suspended = true;
+
+ list_for_each_entry(tz, &thermal_tz_list, node)
+ thermal_zone_pm_prepare(tz);
+}
+
static void thermal_zone_pm_complete(struct thermal_zone_device *tz)
{
guard(thermal_zone)(tz);
@@ -1736,35 +1728,31 @@ static void thermal_zone_pm_complete(str
mod_delayed_work(system_freezable_power_efficient_wq, &tz->poll_queue, 0);
}
-static int thermal_pm_notify(struct notifier_block *nb,
- unsigned long mode, void *_unused)
+static void thermal_pm_notify_complete(void)
{
struct thermal_zone_device *tz;
+ guard(mutex)(&thermal_list_lock);
+
+ thermal_pm_suspended = false;
+
+ list_for_each_entry(tz, &thermal_tz_list, node)
+ thermal_zone_pm_complete(tz);
+}
+
+static int thermal_pm_notify(struct notifier_block *nb,
+ unsigned long mode, void *_unused)
+{
switch (mode) {
case PM_HIBERNATION_PREPARE:
case PM_RESTORE_PREPARE:
case PM_SUSPEND_PREPARE:
- mutex_lock(&thermal_list_lock);
-
- thermal_pm_suspended = true;
-
- list_for_each_entry(tz, &thermal_tz_list, node)
- thermal_zone_pm_prepare(tz);
-
- mutex_unlock(&thermal_list_lock);
+ thermal_pm_notify_prepare();
break;
case PM_POST_HIBERNATION:
case PM_POST_RESTORE:
case PM_POST_SUSPEND:
- mutex_lock(&thermal_list_lock);
-
- thermal_pm_suspended = false;
-
- list_for_each_entry(tz, &thermal_tz_list, node)
- thermal_zone_pm_complete(tz);
-
- mutex_unlock(&thermal_list_lock);
+ thermal_pm_notify_complete();
break;
default:
break;
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (3 preceding siblings ...)
2024-10-10 22:10 ` [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard Rafael J. Wysocki
@ 2024-10-10 22:12 ` Rafael J. Wysocki
2024-10-22 21:10 ` Lukasz Luba
2024-10-10 22:13 ` [PATCH v2 06/11] thermal: core: Introduce thermal_instance_add() Rafael J. Wysocki
` (7 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:12 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Holding a cooling device lock under thermal_governor_update_tz() is not
necessary and it may cause lockdep to complain if any governor's
.update_tz() callback attempts to lock a cdev.
For this reason, move the thermal_governor_update_tz() calls in
thermal_bind_cdev_to_trip() and thermal_unbind_cdev_from_trip() from
under the cdev lock.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/1921484.CQOukoFCf9@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -832,13 +832,13 @@ static int thermal_bind_cdev_to_trip(str
if (!result) {
list_add_tail(&dev->trip_node, &td->thermal_instances);
list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
-
- thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
}
mutex_unlock(&cdev->lock);
- if (!result)
+ if (!result) {
+ thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
return 0;
+ }
device_remove_file(&tz->device, &dev->weight_attr);
remove_trip_file:
@@ -873,9 +873,6 @@ static void thermal_unbind_cdev_from_tri
if (pos->cdev == cdev) {
list_del(&pos->trip_node);
list_del(&pos->cdev_node);
-
- thermal_governor_update_tz(tz, THERMAL_TZ_UNBIND_CDEV);
-
mutex_unlock(&cdev->lock);
goto unbind;
}
@@ -885,6 +882,8 @@ static void thermal_unbind_cdev_from_tri
return;
unbind:
+ thermal_governor_update_tz(tz, THERMAL_TZ_UNBIND_CDEV);
+
device_remove_file(&tz->device, &pos->weight_attr);
device_remove_file(&tz->device, &pos->attr);
sysfs_remove_link(&tz->device.kobj, pos->name);
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 06/11] thermal: core: Introduce thermal_instance_add()
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (4 preceding siblings ...)
2024-10-10 22:12 ` [PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock Rafael J. Wysocki
@ 2024-10-10 22:13 ` Rafael J. Wysocki
2024-10-22 21:45 ` Lukasz Luba
2024-10-10 22:15 ` [PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete() Rafael J. Wysocki
` (6 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:13 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
To reduce the number of redundant result checks in
thermal_bind_cdev_to_trip() and make the code in it easier to
follow, move some of it to a new function called thermal_instance_add()
and make thermal_bind_cdev_to_trip() invoke that function.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/2641944.Lt9SDvczpP@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 46 ++++++++++++++++++++++++++---------------
1 file changed, 30 insertions(+), 16 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -743,6 +743,28 @@ struct thermal_zone_device *thermal_zone
* binding, and unbinding.
*/
+static int thermal_instance_add(struct thermal_instance *new_instance,
+ struct thermal_cooling_device *cdev,
+ struct thermal_trip_desc *td)
+{
+ struct thermal_instance *instance;
+
+ list_for_each_entry(instance, &td->thermal_instances, trip_node) {
+ if (instance->cdev == cdev)
+ return -EEXIST;
+ }
+
+ list_add_tail(&new_instance->trip_node, &td->thermal_instances);
+
+ mutex_lock(&cdev->lock);
+
+ list_add_tail(&new_instance->cdev_node, &cdev->thermal_instances);
+
+ mutex_unlock(&cdev->lock);
+
+ return 0;
+}
+
/**
* thermal_bind_cdev_to_trip - bind a cooling device to a thermal zone
* @tz: pointer to struct thermal_zone_device
@@ -761,7 +783,7 @@ static int thermal_bind_cdev_to_trip(str
struct thermal_cooling_device *cdev,
struct cooling_spec *cool_spec)
{
- struct thermal_instance *dev, *instance;
+ struct thermal_instance *dev;
bool upper_no_limit;
int result;
@@ -823,23 +845,15 @@ static int thermal_bind_cdev_to_trip(str
if (result)
goto remove_trip_file;
- mutex_lock(&cdev->lock);
- list_for_each_entry(instance, &td->thermal_instances, trip_node)
- if (instance->cdev == cdev) {
- result = -EEXIST;
- break;
- }
- if (!result) {
- list_add_tail(&dev->trip_node, &td->thermal_instances);
- list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
- }
- mutex_unlock(&cdev->lock);
+ result = thermal_instance_add(dev, cdev, td);
+ if (result)
+ goto remove_weight_file;
- if (!result) {
- thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
- return 0;
- }
+ thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
+
+ return 0;
+remove_weight_file:
device_remove_file(&tz->device, &dev->weight_attr);
remove_trip_file:
device_remove_file(&tz->device, &dev->attr);
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete()
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (5 preceding siblings ...)
2024-10-10 22:13 ` [PATCH v2 06/11] thermal: core: Introduce thermal_instance_add() Rafael J. Wysocki
@ 2024-10-10 22:15 ` Rafael J. Wysocki
2024-10-22 21:48 ` Lukasz Luba
2024-10-10 22:16 ` [PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck() Rafael J. Wysocki
` (5 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:15 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
It is not necessary to walk the thermal_instances list in a trip
descriptor under a cooling device lock, so acquire that lock only
for deleting the given thermal instance from the list of thermal
instances in the given cdev.
Moreover, in analogy with the previous change that introduced
thermal_instance_add(), put the code deleting the given thermal
instance from the lists it is on into a separate new function
called thermal_instance_delete().
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/2224279.Mh6RI2rZIc@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -866,6 +866,17 @@ free_mem:
return result;
}
+static void thermal_instance_delete(struct thermal_instance *instance)
+{
+ list_del(&instance->trip_node);
+
+ mutex_lock(&instance->cdev->lock);
+
+ list_del(&instance->cdev_node);
+
+ mutex_unlock(&instance->cdev->lock);
+}
+
/**
* thermal_unbind_cdev_from_trip - unbind a cooling device from a thermal zone.
* @tz: pointer to a struct thermal_zone_device.
@@ -882,16 +893,12 @@ static void thermal_unbind_cdev_from_tri
{
struct thermal_instance *pos, *next;
- mutex_lock(&cdev->lock);
list_for_each_entry_safe(pos, next, &td->thermal_instances, trip_node) {
if (pos->cdev == cdev) {
- list_del(&pos->trip_node);
- list_del(&pos->cdev_node);
- mutex_unlock(&cdev->lock);
+ thermal_instance_delete(pos);
goto unbind;
}
}
- mutex_unlock(&cdev->lock);
return;
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck()
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (6 preceding siblings ...)
2024-10-10 22:15 ` [PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete() Rafael J. Wysocki
@ 2024-10-10 22:16 ` Rafael J. Wysocki
2024-10-22 22:23 ` Lukasz Luba
2024-10-10 22:19 ` [PATCH v2 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
` (4 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:16 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Three thermal governors call __thermal_cdev_update() under the
cdev lock without doing any checks, so in order to reduce the
related code duplication, introduce a new helper function called
thermal_cdev_update_nocheck() for them and make them use it.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/3791590.MHq7AAxBmi@rjwysocki.net/
---
drivers/thermal/gov_bang_bang.c | 4 +---
drivers/thermal/gov_fair_share.c | 4 +---
drivers/thermal/gov_power_allocator.c | 5 ++---
drivers/thermal/thermal_core.h | 1 +
drivers/thermal/thermal_helpers.c | 13 +++++++++++++
5 files changed, 18 insertions(+), 9 deletions(-)
Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -30,9 +30,7 @@ static void bang_bang_set_instance_targe
dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
- mutex_lock(&instance->cdev->lock);
- __thermal_cdev_update(instance->cdev);
- mutex_unlock(&instance->cdev->lock);
+ thermal_cdev_update_nocheck(instance->cdev);
}
/**
Index: linux-pm/drivers/thermal/gov_fair_share.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_fair_share.c
+++ linux-pm/drivers/thermal/gov_fair_share.c
@@ -89,9 +89,7 @@ static void fair_share_throttle(struct t
}
instance->target = div_u64(dividend, divisor);
- mutex_lock(&cdev->lock);
- __thermal_cdev_update(cdev);
- mutex_unlock(&cdev->lock);
+ thermal_cdev_update_nocheck(cdev);
}
}
Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -322,9 +322,8 @@ power_actor_set_power(struct thermal_coo
return ret;
instance->target = clamp_val(state, instance->lower, instance->upper);
- mutex_lock(&cdev->lock);
- __thermal_cdev_update(cdev);
- mutex_unlock(&cdev->lock);
+
+ thermal_cdev_update_nocheck(cdev);
return 0;
}
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -213,6 +213,7 @@ static inline bool cdev_is_power_actor(s
}
void thermal_cdev_update(struct thermal_cooling_device *);
+void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev);
void __thermal_cdev_update(struct thermal_cooling_device *cdev);
int get_tz_trend(struct thermal_zone_device *tz, const struct thermal_trip *trip);
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -206,6 +206,19 @@ void thermal_cdev_update(struct thermal_
}
/**
+ * thermal_cdev_update_nocheck() - Unconditionally update cooling device state
+ * @cdev: Target cooling device.
+ */
+void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev)
+{
+ mutex_lock(&cdev->lock);
+
+ __thermal_cdev_update(cdev);
+
+ mutex_unlock(&cdev->lock);
+}
+
+/**
* thermal_zone_get_slope - return the slope attribute of the thermal zone
* @tz: thermal zone device with the slope attribute
*
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 09/11] thermal: core: Add and use cooling device guard
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (7 preceding siblings ...)
2024-10-10 22:16 ` [PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck() Rafael J. Wysocki
@ 2024-10-10 22:19 ` Rafael J. Wysocki
2024-10-14 12:26 ` Rafael J. Wysocki
2024-10-10 22:20 ` [PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization Rafael J. Wysocki
` (3 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:19 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Add and use a special guard for cooling devices.
This allows quite a few error code paths to be simplified among
other things and brings in code size reduction for a good measure.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a new iteration of
https://lore.kernel.org/linux-pm/1890654.atdPhlSkOF@rjwysocki.net/
v1 -> v2: Rearrange cur_state_store()
---
drivers/thermal/gov_power_allocator.c | 21 +++++++--------
drivers/thermal/gov_step_wise.c | 6 ++--
drivers/thermal/thermal_core.c | 17 +++---------
drivers/thermal/thermal_debugfs.c | 25 +++++++++++-------
drivers/thermal/thermal_helpers.c | 19 +++-----------
drivers/thermal/thermal_sysfs.c | 45 ++++++++++++----------------------
include/linux/thermal.h | 3 ++
7 files changed, 57 insertions(+), 79 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -756,12 +756,10 @@ static int thermal_instance_add(struct t
list_add_tail(&new_instance->trip_node, &td->thermal_instances);
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
list_add_tail(&new_instance->cdev_node, &cdev->thermal_instances);
- mutex_unlock(&cdev->lock);
-
return 0;
}
@@ -870,11 +868,9 @@ static void thermal_instance_delete(stru
{
list_del(&instance->trip_node);
- mutex_lock(&instance->cdev->lock);
+ guard(cooling_dev)(instance->cdev);
list_del(&instance->cdev_node);
-
- mutex_unlock(&instance->cdev->lock);
}
/**
@@ -1237,10 +1233,10 @@ void thermal_cooling_device_update(struc
* Update under the cdev lock to prevent the state from being set beyond
* the new limit concurrently.
*/
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
if (cdev->ops->get_max_state(cdev, &cdev->max_state))
- goto unlock;
+ return;
thermal_cooling_device_stats_reinit(cdev);
@@ -1267,12 +1263,9 @@ void thermal_cooling_device_update(struc
}
if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
- goto unlock;
+ return;
thermal_cooling_device_stats_update(cdev, state);
-
-unlock:
- mutex_unlock(&cdev->lock);
}
EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -137,6 +137,9 @@ struct thermal_cooling_device {
#endif
};
+DEFINE_GUARD(cooling_dev, struct thermal_cooling_device *, mutex_lock(&_T->lock),
+ mutex_unlock(&_T->lock))
+
/* Structure to define Thermal Zone parameters */
struct thermal_zone_params {
const char *governor_name;
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -544,14 +544,15 @@ cur_state_store(struct device *dev, stru
if (state > cdev->max_state)
return -EINVAL;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
result = cdev->ops->set_cur_state(cdev, state);
- if (!result)
- thermal_cooling_device_stats_update(cdev, state);
+ if (result)
+ return result;
- mutex_unlock(&cdev->lock);
- return result ? result : count;
+ thermal_cooling_device_stats_update(cdev, state);
+
+ return count;
}
static struct device_attribute
@@ -625,21 +626,18 @@ static ssize_t total_trans_show(struct d
{
struct thermal_cooling_device *cdev = to_cooling_device(dev);
struct cooling_dev_stats *stats;
- int ret = 0;
+ int ret;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
if (!stats)
- goto unlock;
+ return 0;
spin_lock(&stats->lock);
ret = sprintf(buf, "%u\n", stats->total_trans);
spin_unlock(&stats->lock);
-unlock:
- mutex_unlock(&cdev->lock);
-
return ret;
}
@@ -652,11 +650,11 @@ time_in_state_ms_show(struct device *dev
ssize_t len = 0;
int i;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
if (!stats)
- goto unlock;
+ return 0;
spin_lock(&stats->lock);
@@ -668,9 +666,6 @@ time_in_state_ms_show(struct device *dev
}
spin_unlock(&stats->lock);
-unlock:
- mutex_unlock(&cdev->lock);
-
return len;
}
@@ -682,11 +677,11 @@ reset_store(struct device *dev, struct d
struct cooling_dev_stats *stats;
int i, states;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
if (!stats)
- goto unlock;
+ return count;
states = cdev->max_state + 1;
@@ -702,9 +697,6 @@ reset_store(struct device *dev, struct d
spin_unlock(&stats->lock);
-unlock:
- mutex_unlock(&cdev->lock);
-
return count;
}
@@ -716,13 +708,11 @@ static ssize_t trans_table_show(struct d
ssize_t len = 0;
int i, j;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
- if (!stats) {
- len = -ENODATA;
- goto unlock;
- }
+ if (!stats)
+ return -ENODATA;
len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n");
len += snprintf(buf + len, PAGE_SIZE - len, " : ");
@@ -760,9 +750,6 @@ static ssize_t trans_table_show(struct d
len = -EFBIG;
}
-unlock:
- mutex_unlock(&cdev->lock);
-
return len;
}
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -58,17 +58,10 @@ bool thermal_trip_is_bound_to_cdev(struc
const struct thermal_trip *trip,
struct thermal_cooling_device *cdev)
{
- bool ret;
-
guard(thermal_zone)(tz);
+ guard(cooling_dev)(cdev);
- mutex_lock(&cdev->lock);
-
- ret = thermal_instance_present(tz, cdev, trip);
-
- mutex_unlock(&cdev->lock);
-
- return ret;
+ return thermal_instance_present(tz, cdev, trip);
}
EXPORT_SYMBOL_GPL(thermal_trip_is_bound_to_cdev);
@@ -197,12 +190,12 @@ void __thermal_cdev_update(struct therma
*/
void thermal_cdev_update(struct thermal_cooling_device *cdev)
{
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
+
if (!cdev->updated) {
__thermal_cdev_update(cdev);
cdev->updated = true;
}
- mutex_unlock(&cdev->lock);
}
/**
@@ -211,11 +204,9 @@ void thermal_cdev_update(struct thermal_
*/
void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev)
{
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
__thermal_cdev_update(cdev);
-
- mutex_unlock(&cdev->lock);
}
/**
Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -516,6 +516,19 @@ void thermal_debug_cdev_add(struct therm
cdev->debugfs = thermal_dbg;
}
+static struct thermal_debugfs *thermal_debug_cdev_clear(struct thermal_cooling_device *cdev)
+{
+ struct thermal_debugfs *thermal_dbg;
+
+ guard(cooling_dev)(cdev);
+
+ thermal_dbg = cdev->debugfs;
+ if (thermal_dbg)
+ cdev->debugfs = NULL;
+
+ return thermal_dbg;
+}
+
/**
* thermal_debug_cdev_remove - Remove a cooling device debugfs entry
*
@@ -527,17 +540,9 @@ void thermal_debug_cdev_remove(struct th
{
struct thermal_debugfs *thermal_dbg;
- mutex_lock(&cdev->lock);
-
- thermal_dbg = cdev->debugfs;
- if (!thermal_dbg) {
- mutex_unlock(&cdev->lock);
+ thermal_dbg = thermal_debug_cdev_clear(cdev);
+ if (!thermal_dbg)
return;
- }
-
- cdev->debugfs = NULL;
-
- mutex_unlock(&cdev->lock);
mutex_lock(&thermal_dbg->lock);
Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -549,18 +549,17 @@ static void allow_maximum_power(struct t
cdev = instance->cdev;
instance->target = 0;
- mutex_lock(&cdev->lock);
- /*
- * Call for updating the cooling devices local stats and avoid
- * periods of dozen of seconds when those have not been
- * maintained.
- */
- cdev->ops->get_requested_power(cdev, &req_power);
+ scoped_guard(cooling_dev, cdev) {
+ /*
+ * Call for updating the cooling devices local stats and
+ * avoid periods of dozen of seconds when those have not
+ * been maintained.
+ */
+ cdev->ops->get_requested_power(cdev, &req_power);
- if (params->update_cdevs)
- __thermal_cdev_update(cdev);
-
- mutex_unlock(&cdev->lock);
+ if (params->update_cdevs)
+ __thermal_cdev_update(cdev);
+ }
}
}
Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -97,9 +97,9 @@ static void thermal_zone_trip_update(str
instance->initialized = true;
- mutex_lock(&instance->cdev->lock);
- instance->cdev->updated = false; /* cdev needs update */
- mutex_unlock(&instance->cdev->lock);
+ scoped_guard(cooling_dev, instance->cdev) {
+ instance->cdev->updated = false; /* cdev needs update */
+ }
}
}
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (8 preceding siblings ...)
2024-10-10 22:19 ` [PATCH v2 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
@ 2024-10-10 22:20 ` Rafael J. Wysocki
2024-10-22 22:32 ` Lukasz Luba
2024-10-10 22:22 ` [PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard Rafael J. Wysocki
` (2 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:20 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
In preparation for a subsequent change that will switch over the thermal
core to using a mutex guard for managing thermal_governor_lock, move
the code running in thermal_zone_device_register_with_trips() under that
lock into a separate function called thermal_zone_init_governor().
While at it, drop a useless comment.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/2495577.jE0xQCEvom@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1343,6 +1343,25 @@ int thermal_zone_get_crit_temp(struct th
}
EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
+static int thermal_zone_init_governor(struct thermal_zone_device *tz)
+{
+ struct thermal_governor *governor;
+ int ret;
+
+ mutex_lock(&thermal_governor_lock);
+
+ if (tz->tzp)
+ governor = __find_governor(tz->tzp->governor_name);
+ else
+ governor = def_governor;
+
+ ret = thermal_set_governor(tz, governor);
+
+ mutex_unlock(&thermal_governor_lock);
+
+ return ret;
+}
+
static void thermal_zone_init_complete(struct thermal_zone_device *tz)
{
struct thermal_cooling_device *cdev;
@@ -1407,7 +1426,6 @@ thermal_zone_device_register_with_trips(
struct thermal_trip_desc *td;
int id;
int result;
- struct thermal_governor *governor;
if (!type || strlen(type) == 0) {
pr_err("No thermal zone type defined\n");
@@ -1505,21 +1523,9 @@ thermal_zone_device_register_with_trips(
if (result)
goto release_device;
- /* Update 'this' zone's governor information */
- mutex_lock(&thermal_governor_lock);
-
- if (tz->tzp)
- governor = __find_governor(tz->tzp->governor_name);
- else
- governor = def_governor;
-
- result = thermal_set_governor(tz, governor);
- if (result) {
- mutex_unlock(&thermal_governor_lock);
+ result = thermal_zone_init_governor(tz);
+ if (result)
goto unregister;
- }
-
- mutex_unlock(&thermal_governor_lock);
if (!tz->tzp || !tz->tzp->no_hwmon) {
result = thermal_add_hwmon_sysfs(tz);
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (9 preceding siblings ...)
2024-10-10 22:20 ` [PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization Rafael J. Wysocki
@ 2024-10-10 22:22 ` Rafael J. Wysocki
2024-10-22 22:35 ` Lukasz Luba
2024-10-11 18:51 ` [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
2024-10-14 14:59 ` [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-10 22:22 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Switch over the thermal core to using a mutex guard for
thermal_governor_lock management.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a resend of
https://lore.kernel.org/linux-pm/863177860.0ifERbkFSE@rjwysocki.net/
---
drivers/thermal/thermal_core.c | 40 +++++++++++++---------------------------
1 file changed, 13 insertions(+), 27 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -124,7 +124,7 @@ int thermal_register_governor(struct the
if (!governor)
return -EINVAL;
- mutex_lock(&thermal_governor_lock);
+ guard(mutex)(&thermal_governor_lock);
err = -EBUSY;
if (!__find_governor(governor->name)) {
@@ -163,8 +163,6 @@ int thermal_register_governor(struct the
}
}
- mutex_unlock(&thermal_governor_lock);
-
return err;
}
@@ -175,10 +173,10 @@ void thermal_unregister_governor(struct
if (!governor)
return;
- mutex_lock(&thermal_governor_lock);
+ guard(mutex)(&thermal_governor_lock);
if (!__find_governor(governor->name))
- goto exit;
+ return;
list_del(&governor->governor_list);
@@ -189,9 +187,6 @@ void thermal_unregister_governor(struct
THERMAL_NAME_LENGTH))
thermal_set_governor(pos, NULL);
}
-
-exit:
- mutex_unlock(&thermal_governor_lock);
}
int thermal_zone_device_set_policy(struct thermal_zone_device *tz,
@@ -200,16 +195,13 @@ int thermal_zone_device_set_policy(struc
struct thermal_governor *gov;
int ret = -EINVAL;
- mutex_lock(&thermal_governor_lock);
-
+ guard(mutex)(&thermal_governor_lock);
guard(thermal_zone)(tz);
gov = __find_governor(strim(policy));
if (gov)
ret = thermal_set_governor(tz, gov);
- mutex_unlock(&thermal_governor_lock);
-
thermal_notify_tz_gov_change(tz, policy);
return ret;
@@ -220,15 +212,13 @@ int thermal_build_list_of_policies(char
struct thermal_governor *pos;
ssize_t count = 0;
- mutex_lock(&thermal_governor_lock);
+ guard(mutex)(&thermal_governor_lock);
list_for_each_entry(pos, &thermal_governor_list, governor_list) {
count += sysfs_emit_at(buf, count, "%s ", pos->name);
}
count += sysfs_emit_at(buf, count, "\n");
- mutex_unlock(&thermal_governor_lock);
-
return count;
}
@@ -668,17 +658,18 @@ int for_each_thermal_governor(int (*cb)(
void *data)
{
struct thermal_governor *gov;
- int ret = 0;
- mutex_lock(&thermal_governor_lock);
+ guard(mutex)(&thermal_governor_lock);
+
list_for_each_entry(gov, &thermal_governor_list, governor_list) {
+ int ret;
+
ret = cb(gov, data);
if (ret)
- break;
+ return ret;
}
- mutex_unlock(&thermal_governor_lock);
- return ret;
+ return 0;
}
int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
@@ -1346,20 +1337,15 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_
static int thermal_zone_init_governor(struct thermal_zone_device *tz)
{
struct thermal_governor *governor;
- int ret;
- mutex_lock(&thermal_governor_lock);
+ guard(mutex)(&thermal_governor_lock);
if (tz->tzp)
governor = __find_governor(tz->tzp->governor_name);
else
governor = def_governor;
- ret = thermal_set_governor(tz, governor);
-
- mutex_unlock(&thermal_governor_lock);
-
- return ret;
+ return thermal_set_governor(tz, governor);
}
static void thermal_zone_init_complete(struct thermal_zone_device *tz)
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (10 preceding siblings ...)
2024-10-10 22:22 ` [PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard Rafael J. Wysocki
@ 2024-10-11 18:51 ` Rafael J. Wysocki
2024-10-21 11:08 ` Rafael J. Wysocki
2024-10-14 14:59 ` [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-11 18:51 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada,
Rafael J. Wysocki
On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> This is a continuation of
>
> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
>
> and (quite obviously) it is based on that series.
>
> The majority of the patches in it are new iterations of patches included in
>
> https://lore.kernel.org/linux-pm/6100907.lOV4Wx5bFT@rjwysocki.net/
>
> and there is one new patch ([02/11]).
>
> All of these patches are related to locking, but some of them are preparatory.
>
> The series as a whole introduces guards for thermal zones and cooling devices
> and uses them to re-implement locking in the thermal core. It also uses mutex
> guards for thermal_list_lock and thermal_governor_lock locking.
>
> As usual, the details are described by the individual patch changelogs.
This material is now present in the thermal-core-experimental branch
in linux-pm.git.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/11] thermal: core: Add and use cooling device guard
2024-10-10 22:19 ` [PATCH v2 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
@ 2024-10-14 12:26 ` Rafael J. Wysocki
2024-10-22 22:25 ` Lukasz Luba
0 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-14 12:26 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui,
Srinivas Pandruvada
On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Add and use a special guard for cooling devices.
>
> This allows quite a few error code paths to be simplified among
> other things and brings in code size reduction for a good measure.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a new iteration of
>
> https://lore.kernel.org/linux-pm/1890654.atdPhlSkOF@rjwysocki.net/
>
> v1 -> v2: Rearrange cur_state_store()
>
> ---
> drivers/thermal/gov_power_allocator.c | 21 +++++++--------
> drivers/thermal/gov_step_wise.c | 6 ++--
> drivers/thermal/thermal_core.c | 17 +++---------
> drivers/thermal/thermal_debugfs.c | 25 +++++++++++-------
> drivers/thermal/thermal_helpers.c | 19 +++-----------
> drivers/thermal/thermal_sysfs.c | 45 ++++++++++++----------------------
> include/linux/thermal.h | 3 ++
> 7 files changed, 57 insertions(+), 79 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -756,12 +756,10 @@ static int thermal_instance_add(struct t
>
> list_add_tail(&new_instance->trip_node, &td->thermal_instances);
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> list_add_tail(&new_instance->cdev_node, &cdev->thermal_instances);
>
> - mutex_unlock(&cdev->lock);
> -
> return 0;
> }
>
> @@ -870,11 +868,9 @@ static void thermal_instance_delete(stru
> {
> list_del(&instance->trip_node);
>
> - mutex_lock(&instance->cdev->lock);
> + guard(cooling_dev)(instance->cdev);
>
> list_del(&instance->cdev_node);
> -
> - mutex_unlock(&instance->cdev->lock);
> }
>
> /**
> @@ -1237,10 +1233,10 @@ void thermal_cooling_device_update(struc
> * Update under the cdev lock to prevent the state from being set beyond
> * the new limit concurrently.
> */
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> if (cdev->ops->get_max_state(cdev, &cdev->max_state))
> - goto unlock;
> + return;
>
> thermal_cooling_device_stats_reinit(cdev);
>
> @@ -1267,12 +1263,9 @@ void thermal_cooling_device_update(struc
> }
>
> if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
> - goto unlock;
> + return;
>
> thermal_cooling_device_stats_update(cdev, state);
> -
> -unlock:
> - mutex_unlock(&cdev->lock);
> }
> EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
>
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -137,6 +137,9 @@ struct thermal_cooling_device {
> #endif
> };
>
> +DEFINE_GUARD(cooling_dev, struct thermal_cooling_device *, mutex_lock(&_T->lock),
> + mutex_unlock(&_T->lock))
> +
> /* Structure to define Thermal Zone parameters */
> struct thermal_zone_params {
> const char *governor_name;
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -544,14 +544,15 @@ cur_state_store(struct device *dev, stru
> if (state > cdev->max_state)
> return -EINVAL;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> result = cdev->ops->set_cur_state(cdev, state);
> - if (!result)
> - thermal_cooling_device_stats_update(cdev, state);
> + if (result)
> + return result;
>
> - mutex_unlock(&cdev->lock);
> - return result ? result : count;
> + thermal_cooling_device_stats_update(cdev, state);
> +
> + return count;
> }
>
> static struct device_attribute
> @@ -625,21 +626,18 @@ static ssize_t total_trans_show(struct d
> {
> struct thermal_cooling_device *cdev = to_cooling_device(dev);
> struct cooling_dev_stats *stats;
> - int ret = 0;
> + int ret;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> if (!stats)
> - goto unlock;
> + return 0;
>
> spin_lock(&stats->lock);
> ret = sprintf(buf, "%u\n", stats->total_trans);
> spin_unlock(&stats->lock);
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return ret;
> }
>
> @@ -652,11 +650,11 @@ time_in_state_ms_show(struct device *dev
> ssize_t len = 0;
> int i;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> if (!stats)
> - goto unlock;
> + return 0;
>
> spin_lock(&stats->lock);
>
> @@ -668,9 +666,6 @@ time_in_state_ms_show(struct device *dev
> }
> spin_unlock(&stats->lock);
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return len;
> }
>
> @@ -682,11 +677,11 @@ reset_store(struct device *dev, struct d
> struct cooling_dev_stats *stats;
> int i, states;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> if (!stats)
> - goto unlock;
> + return count;
>
> states = cdev->max_state + 1;
>
> @@ -702,9 +697,6 @@ reset_store(struct device *dev, struct d
>
> spin_unlock(&stats->lock);
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return count;
> }
>
> @@ -716,13 +708,11 @@ static ssize_t trans_table_show(struct d
> ssize_t len = 0;
> int i, j;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> - if (!stats) {
> - len = -ENODATA;
> - goto unlock;
> - }
> + if (!stats)
> + return -ENODATA;
>
> len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n");
> len += snprintf(buf + len, PAGE_SIZE - len, " : ");
There is one more "goto unlock" statement in this function that needs
to be replaced with "return".
I'll send an update of it shortly.
> @@ -760,9 +750,6 @@ static ssize_t trans_table_show(struct d
> len = -EFBIG;
> }
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return len;
> }
>
> Index: linux-pm/drivers/thermal/thermal_helpers.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_helpers.c
> +++ linux-pm/drivers/thermal/thermal_helpers.c
> @@ -58,17 +58,10 @@ bool thermal_trip_is_bound_to_cdev(struc
> const struct thermal_trip *trip,
> struct thermal_cooling_device *cdev)
> {
> - bool ret;
> -
> guard(thermal_zone)(tz);
> + guard(cooling_dev)(cdev);
>
> - mutex_lock(&cdev->lock);
> -
> - ret = thermal_instance_present(tz, cdev, trip);
> -
> - mutex_unlock(&cdev->lock);
> -
> - return ret;
> + return thermal_instance_present(tz, cdev, trip);
> }
> EXPORT_SYMBOL_GPL(thermal_trip_is_bound_to_cdev);
>
> @@ -197,12 +190,12 @@ void __thermal_cdev_update(struct therma
> */
> void thermal_cdev_update(struct thermal_cooling_device *cdev)
> {
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
> +
> if (!cdev->updated) {
> __thermal_cdev_update(cdev);
> cdev->updated = true;
> }
> - mutex_unlock(&cdev->lock);
> }
>
> /**
> @@ -211,11 +204,9 @@ void thermal_cdev_update(struct thermal_
> */
> void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev)
> {
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> __thermal_cdev_update(cdev);
> -
> - mutex_unlock(&cdev->lock);
> }
>
> /**
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -516,6 +516,19 @@ void thermal_debug_cdev_add(struct therm
> cdev->debugfs = thermal_dbg;
> }
>
> +static struct thermal_debugfs *thermal_debug_cdev_clear(struct thermal_cooling_device *cdev)
> +{
> + struct thermal_debugfs *thermal_dbg;
> +
> + guard(cooling_dev)(cdev);
> +
> + thermal_dbg = cdev->debugfs;
> + if (thermal_dbg)
> + cdev->debugfs = NULL;
> +
> + return thermal_dbg;
> +}
> +
> /**
> * thermal_debug_cdev_remove - Remove a cooling device debugfs entry
> *
> @@ -527,17 +540,9 @@ void thermal_debug_cdev_remove(struct th
> {
> struct thermal_debugfs *thermal_dbg;
>
> - mutex_lock(&cdev->lock);
> -
> - thermal_dbg = cdev->debugfs;
> - if (!thermal_dbg) {
> - mutex_unlock(&cdev->lock);
> + thermal_dbg = thermal_debug_cdev_clear(cdev);
> + if (!thermal_dbg)
> return;
> - }
> -
> - cdev->debugfs = NULL;
> -
> - mutex_unlock(&cdev->lock);
>
> mutex_lock(&thermal_dbg->lock);
>
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -549,18 +549,17 @@ static void allow_maximum_power(struct t
> cdev = instance->cdev;
>
> instance->target = 0;
> - mutex_lock(&cdev->lock);
> - /*
> - * Call for updating the cooling devices local stats and avoid
> - * periods of dozen of seconds when those have not been
> - * maintained.
> - */
> - cdev->ops->get_requested_power(cdev, &req_power);
> + scoped_guard(cooling_dev, cdev) {
> + /*
> + * Call for updating the cooling devices local stats and
> + * avoid periods of dozen of seconds when those have not
> + * been maintained.
> + */
> + cdev->ops->get_requested_power(cdev, &req_power);
>
> - if (params->update_cdevs)
> - __thermal_cdev_update(cdev);
> -
> - mutex_unlock(&cdev->lock);
> + if (params->update_cdevs)
> + __thermal_cdev_update(cdev);
> + }
> }
> }
>
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -97,9 +97,9 @@ static void thermal_zone_trip_update(str
>
> instance->initialized = true;
>
> - mutex_lock(&instance->cdev->lock);
> - instance->cdev->updated = false; /* cdev needs update */
> - mutex_unlock(&instance->cdev->lock);
> + scoped_guard(cooling_dev, instance->cdev) {
> + instance->cdev->updated = false; /* cdev needs update */
> + }
> }
> }
>
>
>
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
` (11 preceding siblings ...)
2024-10-11 18:51 ` [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
@ 2024-10-14 14:59 ` Rafael J. Wysocki
2024-10-24 8:13 ` Lukasz Luba
12 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-14 14:59 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Add and use a special guard for cooling devices.
This allows quite a few error code paths to be simplified among
other things and brings in code size reduction for a good measure.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
This is a new iteration of
https://lore.kernel.org/linux-pm/1890654.atdPhlSkOF@rjwysocki.net/
v2 -> v2.1: Add missing hunk in trans_table_show()
v1 -> v2: Rearrange cur_state_store()
---
drivers/thermal/gov_power_allocator.c | 21 ++++++--------
drivers/thermal/gov_step_wise.c | 6 ++--
drivers/thermal/thermal_core.c | 17 +++--------
drivers/thermal/thermal_debugfs.c | 25 ++++++++++------
drivers/thermal/thermal_helpers.c | 19 +++---------
drivers/thermal/thermal_sysfs.c | 51 ++++++++++++----------------------
include/linux/thermal.h | 3 ++
7 files changed, 59 insertions(+), 83 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -758,12 +758,10 @@ static int thermal_instance_add(struct t
list_add_tail(&new_instance->trip_node, &td->thermal_instances);
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
list_add_tail(&new_instance->cdev_node, &cdev->thermal_instances);
- mutex_unlock(&cdev->lock);
-
return 0;
}
@@ -872,11 +870,9 @@ static void thermal_instance_delete(stru
{
list_del(&instance->trip_node);
- mutex_lock(&instance->cdev->lock);
+ guard(cooling_dev)(instance->cdev);
list_del(&instance->cdev_node);
-
- mutex_unlock(&instance->cdev->lock);
}
/**
@@ -1239,10 +1235,10 @@ void thermal_cooling_device_update(struc
* Update under the cdev lock to prevent the state from being set beyond
* the new limit concurrently.
*/
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
if (cdev->ops->get_max_state(cdev, &cdev->max_state))
- goto unlock;
+ return;
thermal_cooling_device_stats_reinit(cdev);
@@ -1269,12 +1265,9 @@ void thermal_cooling_device_update(struc
}
if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
- goto unlock;
+ return;
thermal_cooling_device_stats_update(cdev, state);
-
-unlock:
- mutex_unlock(&cdev->lock);
}
EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -140,6 +140,9 @@ struct thermal_cooling_device {
#endif
};
+DEFINE_GUARD(cooling_dev, struct thermal_cooling_device *, mutex_lock(&_T->lock),
+ mutex_unlock(&_T->lock))
+
/* Structure to define Thermal Zone parameters */
struct thermal_zone_params {
const char *governor_name;
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -544,14 +544,15 @@ cur_state_store(struct device *dev, stru
if (state > cdev->max_state)
return -EINVAL;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
result = cdev->ops->set_cur_state(cdev, state);
- if (!result)
- thermal_cooling_device_stats_update(cdev, state);
+ if (result)
+ return result;
- mutex_unlock(&cdev->lock);
- return result ? result : count;
+ thermal_cooling_device_stats_update(cdev, state);
+
+ return count;
}
static struct device_attribute
@@ -625,21 +626,18 @@ static ssize_t total_trans_show(struct d
{
struct thermal_cooling_device *cdev = to_cooling_device(dev);
struct cooling_dev_stats *stats;
- int ret = 0;
+ int ret;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
if (!stats)
- goto unlock;
+ return 0;
spin_lock(&stats->lock);
ret = sprintf(buf, "%u\n", stats->total_trans);
spin_unlock(&stats->lock);
-unlock:
- mutex_unlock(&cdev->lock);
-
return ret;
}
@@ -652,11 +650,11 @@ time_in_state_ms_show(struct device *dev
ssize_t len = 0;
int i;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
if (!stats)
- goto unlock;
+ return 0;
spin_lock(&stats->lock);
@@ -668,9 +666,6 @@ time_in_state_ms_show(struct device *dev
}
spin_unlock(&stats->lock);
-unlock:
- mutex_unlock(&cdev->lock);
-
return len;
}
@@ -682,11 +677,11 @@ reset_store(struct device *dev, struct d
struct cooling_dev_stats *stats;
int i, states;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
if (!stats)
- goto unlock;
+ return count;
states = cdev->max_state + 1;
@@ -702,9 +697,6 @@ reset_store(struct device *dev, struct d
spin_unlock(&stats->lock);
-unlock:
- mutex_unlock(&cdev->lock);
-
return count;
}
@@ -716,13 +708,11 @@ static ssize_t trans_table_show(struct d
ssize_t len = 0;
int i, j;
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
stats = cdev->stats;
- if (!stats) {
- len = -ENODATA;
- goto unlock;
- }
+ if (!stats)
+ return -ENODATA;
len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n");
len += snprintf(buf + len, PAGE_SIZE - len, " : ");
@@ -731,10 +721,8 @@ static ssize_t trans_table_show(struct d
break;
len += snprintf(buf + len, PAGE_SIZE - len, "state%2u ", i);
}
- if (len >= PAGE_SIZE) {
- len = PAGE_SIZE;
- goto unlock;
- }
+ if (len >= PAGE_SIZE)
+ return PAGE_SIZE;
len += snprintf(buf + len, PAGE_SIZE - len, "\n");
@@ -760,9 +748,6 @@ static ssize_t trans_table_show(struct d
len = -EFBIG;
}
-unlock:
- mutex_unlock(&cdev->lock);
-
return len;
}
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -58,17 +58,10 @@ bool thermal_trip_is_bound_to_cdev(struc
const struct thermal_trip *trip,
struct thermal_cooling_device *cdev)
{
- bool ret;
-
guard(thermal_zone)(tz);
+ guard(cooling_dev)(cdev);
- mutex_lock(&cdev->lock);
-
- ret = thermal_instance_present(tz, cdev, trip);
-
- mutex_unlock(&cdev->lock);
-
- return ret;
+ return thermal_instance_present(tz, cdev, trip);
}
EXPORT_SYMBOL_GPL(thermal_trip_is_bound_to_cdev);
@@ -197,12 +190,12 @@ void __thermal_cdev_update(struct therma
*/
void thermal_cdev_update(struct thermal_cooling_device *cdev)
{
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
+
if (!cdev->updated) {
__thermal_cdev_update(cdev);
cdev->updated = true;
}
- mutex_unlock(&cdev->lock);
}
/**
@@ -211,11 +204,9 @@ void thermal_cdev_update(struct thermal_
*/
void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev)
{
- mutex_lock(&cdev->lock);
+ guard(cooling_dev)(cdev);
__thermal_cdev_update(cdev);
-
- mutex_unlock(&cdev->lock);
}
/**
Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -516,6 +516,19 @@ void thermal_debug_cdev_add(struct therm
cdev->debugfs = thermal_dbg;
}
+static struct thermal_debugfs *thermal_debug_cdev_clear(struct thermal_cooling_device *cdev)
+{
+ struct thermal_debugfs *thermal_dbg;
+
+ guard(cooling_dev)(cdev);
+
+ thermal_dbg = cdev->debugfs;
+ if (thermal_dbg)
+ cdev->debugfs = NULL;
+
+ return thermal_dbg;
+}
+
/**
* thermal_debug_cdev_remove - Remove a cooling device debugfs entry
*
@@ -527,17 +540,9 @@ void thermal_debug_cdev_remove(struct th
{
struct thermal_debugfs *thermal_dbg;
- mutex_lock(&cdev->lock);
-
- thermal_dbg = cdev->debugfs;
- if (!thermal_dbg) {
- mutex_unlock(&cdev->lock);
+ thermal_dbg = thermal_debug_cdev_clear(cdev);
+ if (!thermal_dbg)
return;
- }
-
- cdev->debugfs = NULL;
-
- mutex_unlock(&cdev->lock);
mutex_lock(&thermal_dbg->lock);
Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -549,18 +549,17 @@ static void allow_maximum_power(struct t
cdev = instance->cdev;
instance->target = 0;
- mutex_lock(&cdev->lock);
- /*
- * Call for updating the cooling devices local stats and avoid
- * periods of dozen of seconds when those have not been
- * maintained.
- */
- cdev->ops->get_requested_power(cdev, &req_power);
+ scoped_guard(cooling_dev, cdev) {
+ /*
+ * Call for updating the cooling devices local stats and
+ * avoid periods of dozen of seconds when those have not
+ * been maintained.
+ */
+ cdev->ops->get_requested_power(cdev, &req_power);
- if (params->update_cdevs)
- __thermal_cdev_update(cdev);
-
- mutex_unlock(&cdev->lock);
+ if (params->update_cdevs)
+ __thermal_cdev_update(cdev);
+ }
}
}
Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -97,9 +97,9 @@ static void thermal_zone_trip_update(str
instance->initialized = true;
- mutex_lock(&instance->cdev->lock);
- instance->cdev->updated = false; /* cdev needs update */
- mutex_unlock(&instance->cdev->lock);
+ scoped_guard(cooling_dev, instance->cdev) {
+ instance->cdev->updated = false; /* cdev needs update */
+ }
}
}
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards
2024-10-11 18:51 ` [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
@ 2024-10-21 11:08 ` Rafael J. Wysocki
2024-10-21 22:03 ` Lukasz Luba
0 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-21 11:08 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada,
Rafael J. Wysocki
On Fri, Oct 11, 2024 at 8:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi Everyone,
> >
> > This is a continuation of
> >
> > https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
> >
> > and (quite obviously) it is based on that series.
> >
> > The majority of the patches in it are new iterations of patches included in
> >
> > https://lore.kernel.org/linux-pm/6100907.lOV4Wx5bFT@rjwysocki.net/
> >
> > and there is one new patch ([02/11]).
> >
> > All of these patches are related to locking, but some of them are preparatory.
> >
> > The series as a whole introduces guards for thermal zones and cooling devices
> > and uses them to re-implement locking in the thermal core. It also uses mutex
> > guards for thermal_list_lock and thermal_governor_lock locking.
> >
> > As usual, the details are described by the individual patch changelogs.
>
> This material is now present in the thermal-core-experimental branch
> in linux-pm.git.
I gather that it is not controversial as it was covered in the PM+TC
session at the LPC and it has been around for quite a while, so I've
just queued it up for 6.13.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-10 22:05 ` [PATCH v2 01/11] thermal: core: Add and use thermal zone guard Rafael J. Wysocki
@ 2024-10-21 11:40 ` Markus Elfring
2024-10-21 11:49 ` Rafael J. Wysocki
2024-10-22 21:02 ` [PATCH v2 " Lukasz Luba
1 sibling, 1 reply; 39+ messages in thread
From: Markus Elfring @ 2024-10-21 11:40 UTC (permalink / raw)
To: Rafael J. Wysocki, linux-pm
Cc: Rafael J. Wysocki, LKML, Daniel Lezcano, Lukasz Luba,
Srinivas Pandruvada, Zhang Rui
…
> +++ linux-pm/drivers/thermal/thermal_helpers.c
> @@ -60,13 +60,13 @@ bool thermal_trip_is_bound_to_cdev(struc
> {
> bool ret;
>
> - mutex_lock(&tz->lock);
> + guard(thermal_zone)(tz);
> +
> mutex_lock(&cdev->lock);
>
> ret = thermal_instance_present(tz, cdev, trip);
>
> mutex_unlock(&cdev->lock);
> - mutex_unlock(&tz->lock);
>
> return ret;
> }
…
Would you become interested to apply a statement
like “guard(mutex)(&cdev->lock);”?
Regards,
Markus
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-21 11:40 ` Markus Elfring
@ 2024-10-21 11:49 ` Rafael J. Wysocki
2024-10-21 12:02 ` [v2 " Markus Elfring
0 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-21 11:49 UTC (permalink / raw)
To: Markus Elfring
Cc: Rafael J. Wysocki, linux-pm, Rafael J. Wysocki, LKML,
Daniel Lezcano, Lukasz Luba, Srinivas Pandruvada, Zhang Rui
On Mon, Oct 21, 2024 at 1:41 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> …
> > +++ linux-pm/drivers/thermal/thermal_helpers.c
> > @@ -60,13 +60,13 @@ bool thermal_trip_is_bound_to_cdev(struc
> > {
> > bool ret;
> >
> > - mutex_lock(&tz->lock);
> > + guard(thermal_zone)(tz);
> > +
> > mutex_lock(&cdev->lock);
> >
> > ret = thermal_instance_present(tz, cdev, trip);
> >
> > mutex_unlock(&cdev->lock);
> > - mutex_unlock(&tz->lock);
> >
> > return ret;
> > }
> …
>
> Would you become interested to apply a statement
> like “guard(mutex)(&cdev->lock);”?
Well, please see
https://lore.kernel.org/linux-pm/5837621.DvuYhMxLoT@rjwysocki.net/
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-21 11:49 ` Rafael J. Wysocki
@ 2024-10-21 12:02 ` Markus Elfring
2024-10-21 12:03 ` Rafael J. Wysocki
0 siblings, 1 reply; 39+ messages in thread
From: Markus Elfring @ 2024-10-21 12:02 UTC (permalink / raw)
To: Rafael J. Wysocki, linux-pm
Cc: Rafael J. Wysocki, Rafael J. Wysocki, LKML, Daniel Lezcano,
Lukasz Luba, Srinivas Pandruvada, Zhang Rui
>> …
>>> +++ linux-pm/drivers/thermal/thermal_helpers.c
>>> @@ -60,13 +60,13 @@ bool thermal_trip_is_bound_to_cdev(struc
…
>> …
>>
>> Would you become interested to apply a statement
>> like “guard(mutex)(&cdev->lock);”?
>
> Well, please see
> https://lore.kernel.org/linux-pm/5837621.DvuYhMxLoT@rjwysocki.net/
Will the separation of source code adjustments according to cooling devices
and thermal zones trigger any related clarifications?
Regards,
Markus
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-21 12:02 ` [v2 " Markus Elfring
@ 2024-10-21 12:03 ` Rafael J. Wysocki
0 siblings, 0 replies; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-21 12:03 UTC (permalink / raw)
To: Markus Elfring
Cc: Rafael J. Wysocki, linux-pm, Rafael J. Wysocki, Rafael J. Wysocki,
LKML, Daniel Lezcano, Lukasz Luba, Srinivas Pandruvada, Zhang Rui
On Mon, Oct 21, 2024 at 2:02 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> …
> >>> +++ linux-pm/drivers/thermal/thermal_helpers.c
> >>> @@ -60,13 +60,13 @@ bool thermal_trip_is_bound_to_cdev(struc
> …
> >> …
> >>
> >> Would you become interested to apply a statement
> >> like “guard(mutex)(&cdev->lock);”?
> >
> > Well, please see
> > https://lore.kernel.org/linux-pm/5837621.DvuYhMxLoT@rjwysocki.net/
>
> Will the separation of source code adjustments according to cooling devices
> and thermal zones trigger any related clarifications?
I'm not aware of any.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards
2024-10-21 11:08 ` Rafael J. Wysocki
@ 2024-10-21 22:03 ` Lukasz Luba
2024-10-22 9:57 ` Rafael J. Wysocki
0 siblings, 1 reply; 39+ messages in thread
From: Lukasz Luba @ 2024-10-21 22:03 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada,
Rafael J. Wysocki
Hi Rafael,
On 10/21/24 12:08, Rafael J. Wysocki wrote:
> On Fri, Oct 11, 2024 at 8:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>
>>> Hi Everyone,
>>>
>>> This is a continuation of
>>>
>>> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
>>>
>>> and (quite obviously) it is based on that series.
>>>
>>> The majority of the patches in it are new iterations of patches included in
>>>
>>> https://lore.kernel.org/linux-pm/6100907.lOV4Wx5bFT@rjwysocki.net/
>>>
>>> and there is one new patch ([02/11]).
>>>
>>> All of these patches are related to locking, but some of them are preparatory.
>>>
>>> The series as a whole introduces guards for thermal zones and cooling devices
>>> and uses them to re-implement locking in the thermal core. It also uses mutex
>>> guards for thermal_list_lock and thermal_governor_lock locking.
>>>
>>> As usual, the details are described by the individual patch changelogs.
>>
>> This material is now present in the thermal-core-experimental branch
>> in linux-pm.git.
>
> I gather that it is not controversial as it was covered in the PM+TC
> session at the LPC and it has been around for quite a while, so I've
> just queued it up for 6.13.
If it's not too late, I will do the review tomorrow.
Regards,
Lukasz
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards
2024-10-21 22:03 ` Lukasz Luba
@ 2024-10-22 9:57 ` Rafael J. Wysocki
2024-10-22 10:21 ` Lukasz Luba
0 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-22 9:57 UTC (permalink / raw)
To: Lukasz Luba
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada, Rafael J. Wysocki
Hi Lukasz,
On Tue, Oct 22, 2024 at 12:02 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> On 10/21/24 12:08, Rafael J. Wysocki wrote:
> > On Fri, Oct 11, 2024 at 8:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>>
> >>> Hi Everyone,
> >>>
> >>> This is a continuation of
> >>>
> >>> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
> >>>
> >>> and (quite obviously) it is based on that series.
> >>>
> >>> The majority of the patches in it are new iterations of patches included in
> >>>
> >>> https://lore.kernel.org/linux-pm/6100907.lOV4Wx5bFT@rjwysocki.net/
> >>>
> >>> and there is one new patch ([02/11]).
> >>>
> >>> All of these patches are related to locking, but some of them are preparatory.
> >>>
> >>> The series as a whole introduces guards for thermal zones and cooling devices
> >>> and uses them to re-implement locking in the thermal core. It also uses mutex
> >>> guards for thermal_list_lock and thermal_governor_lock locking.
> >>>
> >>> As usual, the details are described by the individual patch changelogs.
> >>
> >> This material is now present in the thermal-core-experimental branch
> >> in linux-pm.git.
> >
> > I gather that it is not controversial as it was covered in the PM+TC
> > session at the LPC and it has been around for quite a while, so I've
> > just queued it up for 6.13.
>
> If it's not too late, I will do the review tomorrow.
No, it isn't, please do!
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards
2024-10-22 9:57 ` Rafael J. Wysocki
@ 2024-10-22 10:21 ` Lukasz Luba
2024-10-22 15:29 ` Rafael J. Wysocki
0 siblings, 1 reply; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 10:21 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada,
Rafael J. Wysocki
On 10/22/24 10:57, Rafael J. Wysocki wrote:
> Hi Lukasz,
>
> On Tue, Oct 22, 2024 at 12:02 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Rafael,
>>
>> On 10/21/24 12:08, Rafael J. Wysocki wrote:
>>> On Fri, Oct 11, 2024 at 8:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>>
>>>> On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>>
>>>>> Hi Everyone,
>>>>>
>>>>> This is a continuation of
>>>>>
>>>>> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
>>>>>
>>>>> and (quite obviously) it is based on that series.
>>>>>
>>>>> The majority of the patches in it are new iterations of patches included in
>>>>>
>>>>> https://lore.kernel.org/linux-pm/6100907.lOV4Wx5bFT@rjwysocki.net/
>>>>>
>>>>> and there is one new patch ([02/11]).
>>>>>
>>>>> All of these patches are related to locking, but some of them are preparatory.
>>>>>
>>>>> The series as a whole introduces guards for thermal zones and cooling devices
>>>>> and uses them to re-implement locking in the thermal core. It also uses mutex
>>>>> guards for thermal_list_lock and thermal_governor_lock locking.
>>>>>
>>>>> As usual, the details are described by the individual patch changelogs.
>>>>
>>>> This material is now present in the thermal-core-experimental branch
>>>> in linux-pm.git.
>>>
>>> I gather that it is not controversial as it was covered in the PM+TC
>>> session at the LPC and it has been around for quite a while, so I've
>>> just queued it up for 6.13.
>>
>> If it's not too late, I will do the review tomorrow.
>
> No, it isn't, please do!
OK, I will do that today.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards
2024-10-22 10:21 ` Lukasz Luba
@ 2024-10-22 15:29 ` Rafael J. Wysocki
0 siblings, 0 replies; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-22 15:29 UTC (permalink / raw)
To: Lukasz Luba
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada, Rafael J. Wysocki
On Tue, Oct 22, 2024 at 12:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/22/24 10:57, Rafael J. Wysocki wrote:
> > Hi Lukasz,
> >
> > On Tue, Oct 22, 2024 at 12:02 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> On 10/21/24 12:08, Rafael J. Wysocki wrote:
> >>> On Fri, Oct 11, 2024 at 8:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>>>
> >>>> On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>>>>
> >>>>> Hi Everyone,
> >>>>>
> >>>>> This is a continuation of
> >>>>>
> >>>>> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
> >>>>>
> >>>>> and (quite obviously) it is based on that series.
> >>>>>
> >>>>> The majority of the patches in it are new iterations of patches included in
> >>>>>
> >>>>> https://lore.kernel.org/linux-pm/6100907.lOV4Wx5bFT@rjwysocki.net/
> >>>>>
> >>>>> and there is one new patch ([02/11]).
> >>>>>
> >>>>> All of these patches are related to locking, but some of them are preparatory.
> >>>>>
> >>>>> The series as a whole introduces guards for thermal zones and cooling devices
> >>>>> and uses them to re-implement locking in the thermal core. It also uses mutex
> >>>>> guards for thermal_list_lock and thermal_governor_lock locking.
> >>>>>
> >>>>> As usual, the details are described by the individual patch changelogs.
> >>>>
> >>>> This material is now present in the thermal-core-experimental branch
> >>>> in linux-pm.git.
> >>>
> >>> I gather that it is not controversial as it was covered in the PM+TC
> >>> session at the LPC and it has been around for quite a while, so I've
> >>> just queued it up for 6.13.
> >>
> >> If it's not too late, I will do the review tomorrow.
> >
> > No, it isn't, please do!
>
> OK, I will do that today.
Thank you!
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-10 22:05 ` [PATCH v2 01/11] thermal: core: Add and use thermal zone guard Rafael J. Wysocki
2024-10-21 11:40 ` Markus Elfring
@ 2024-10-22 21:02 ` Lukasz Luba
2024-10-23 9:50 ` Rafael J. Wysocki
1 sibling, 1 reply; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 21:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
Hi Rafael,
On 10/10/24 23:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Add and use a guard for thermal zone locking.
>
> This allows quite a few error code paths to be simplified among
> other things and brings in a noticeable code size reduction for
> a good measure.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a new iteration of
>
> https://lore.kernel.org/linux-pm/3241904.5fSG56mABF@rjwysocki.net/
>
> that has been combined with
>
> https://lore.kernel.org/linux-pm/4613601.LvFx2qVVIh@rjwysocki.net/
>
> and rebased on top of
>
> https://lore.kernel.org/linux-pm/12549318.O9o76ZdvQC@rjwysocki.net/
>
> and
>
> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 61 +++++++---------------------
> drivers/thermal/thermal_core.h | 4 +
> drivers/thermal/thermal_debugfs.c | 25 +++++++----
> drivers/thermal/thermal_helpers.c | 17 ++-----
> drivers/thermal/thermal_hwmon.c | 5 --
> drivers/thermal/thermal_netlink.c | 21 ++-------
> drivers/thermal/thermal_sysfs.c | 81 ++++++++++++++++----------------------
> drivers/thermal/thermal_trip.c | 8 ---
> 8 files changed, 86 insertions(+), 136 deletions(-)
>
[snip]
Surprise, how the code can look smaller using that
style with 'guard'.
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 02/11] thermal: core: Add and use a reverse thermal zone guard
2024-10-10 22:07 ` [PATCH v2 02/11] thermal: core: Add and use a reverse " Rafael J. Wysocki
@ 2024-10-22 21:03 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 21:03 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:07, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Add a guard for unlocking a locked thermal zone temporarily and use it
> in thermal_zone_pm_prepare().
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a new patch
>
> ---
> drivers/thermal/thermal_core.c | 8 +++-----
> drivers/thermal/thermal_core.h | 3 +++
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1702,11 +1702,9 @@ static void thermal_zone_pm_prepare(stru
> * acquired the lock yet, so release it to let the function run
> * and wait util it has done the work.
> */
> - mutex_unlock(&tz->lock);
> -
> - wait_for_completion(&tz->resume);
> -
> - mutex_lock(&tz->lock);
> + scoped_guard(thermal_zone_reverse, tz) {
> + wait_for_completion(&tz->resume);
> + }
> }
>
> tz->state |= TZ_STATE_FLAG_SUSPENDED;
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -148,6 +148,9 @@ struct thermal_zone_device {
> DEFINE_GUARD(thermal_zone, struct thermal_zone_device *, mutex_lock(&_T->lock),
> mutex_unlock(&_T->lock))
>
> +DEFINE_GUARD(thermal_zone_reverse, struct thermal_zone_device *,
> + mutex_unlock(&_T->lock), mutex_lock(&_T->lock))
> +
> /* Initial thermal zone temperature. */
> #define THERMAL_TEMP_INIT INT_MIN
>
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock
2024-10-10 22:09 ` [PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock Rafael J. Wysocki
@ 2024-10-22 21:09 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 21:09 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:09, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> To prepare for a subsequent change that will switch over the thermal
> core to using a mutex guard for thermal_list_lock management, move the
> code running under thermal_list_lock during the initialization and
> unregistration of cooling devices into separate functions.
>
> While at it, drop some comments that do not add value.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/1822468.VLH7GnMWUR@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 64 +++++++++++++++++++++++------------------
> 1 file changed, 36 insertions(+), 28 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -965,6 +965,20 @@ static void thermal_zone_cdev_bind(struc
> __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> }
>
> +static void thermal_cooling_device_init_complete(struct thermal_cooling_device *cdev)
> +{
> + struct thermal_zone_device *tz;
> +
> + mutex_lock(&thermal_list_lock);
> +
> + list_add(&cdev->node, &thermal_cdev_list);
> +
> + list_for_each_entry(tz, &thermal_tz_list, node)
> + thermal_zone_cdev_bind(tz, cdev);
> +
> + mutex_unlock(&thermal_list_lock);
> +}
> +
> /**
> * __thermal_cooling_device_register() - register a new thermal cooling device
> * @np: a pointer to a device tree node.
> @@ -987,7 +1001,6 @@ __thermal_cooling_device_register(struct
> const struct thermal_cooling_device_ops *ops)
> {
> struct thermal_cooling_device *cdev;
> - struct thermal_zone_device *pos;
> unsigned long current_state;
> int id, ret;
>
> @@ -1054,16 +1067,7 @@ __thermal_cooling_device_register(struct
> if (current_state <= cdev->max_state)
> thermal_debug_cdev_add(cdev, current_state);
>
> - /* Add 'this' new cdev to the global cdev list */
> - mutex_lock(&thermal_list_lock);
> -
> - list_add(&cdev->node, &thermal_cdev_list);
> -
> - /* Update binding information for 'this' new cdev */
> - list_for_each_entry(pos, &thermal_tz_list, node)
> - thermal_zone_cdev_bind(pos, cdev);
> -
> - mutex_unlock(&thermal_list_lock);
> + thermal_cooling_device_init_complete(cdev);
>
> return cdev;
>
> @@ -1274,38 +1278,42 @@ static void thermal_zone_cdev_unbind(str
> __thermal_zone_cdev_unbind(tz, cdev);
> }
>
> -/**
> - * thermal_cooling_device_unregister - removes a thermal cooling device
> - * @cdev: the thermal cooling device to remove.
> - *
> - * thermal_cooling_device_unregister() must be called when a registered
> - * thermal cooling device is no longer needed.
> - */
> -void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
> +static bool thermal_cooling_device_exit(struct thermal_cooling_device *cdev)
> {
> struct thermal_zone_device *tz;
> -
> - if (!cdev)
> - return;
> -
> - thermal_debug_cdev_remove(cdev);
> + bool ret = true;
>
> mutex_lock(&thermal_list_lock);
>
> if (!thermal_cooling_device_present(cdev)) {
> - mutex_unlock(&thermal_list_lock);
> - return;
> + ret = false;
> + goto unlock;
> }
>
> list_del(&cdev->node);
>
> - /* Unbind all thermal zones associated with 'this' cdev */
> list_for_each_entry(tz, &thermal_tz_list, node)
> thermal_zone_cdev_unbind(tz, cdev);
>
> +unlock:
> mutex_unlock(&thermal_list_lock);
>
> - device_unregister(&cdev->device);
> + return ret;
> +}
> +
> +/**
> + * thermal_cooling_device_unregister() - removes a thermal cooling device
> + * @cdev: Thermal cooling device to remove.
> + */
> +void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
> +{
> + if (!cdev)
> + return;
> +
> + thermal_debug_cdev_remove(cdev);
> +
> + if (thermal_cooling_device_exit(cdev))
> + device_unregister(&cdev->device);
> }
> EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
>
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock
2024-10-10 22:12 ` [PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock Rafael J. Wysocki
@ 2024-10-22 21:10 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 21:10 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Holding a cooling device lock under thermal_governor_update_tz() is not
> necessary and it may cause lockdep to complain if any governor's
> .update_tz() callback attempts to lock a cdev.
>
> For this reason, move the thermal_governor_update_tz() calls in
> thermal_bind_cdev_to_trip() and thermal_unbind_cdev_from_trip() from
> under the cdev lock.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/1921484.CQOukoFCf9@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -832,13 +832,13 @@ static int thermal_bind_cdev_to_trip(str
> if (!result) {
> list_add_tail(&dev->trip_node, &td->thermal_instances);
> list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
> -
> - thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
> }
> mutex_unlock(&cdev->lock);
>
> - if (!result)
> + if (!result) {
> + thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
> return 0;
> + }
>
> device_remove_file(&tz->device, &dev->weight_attr);
> remove_trip_file:
> @@ -873,9 +873,6 @@ static void thermal_unbind_cdev_from_tri
> if (pos->cdev == cdev) {
> list_del(&pos->trip_node);
> list_del(&pos->cdev_node);
> -
> - thermal_governor_update_tz(tz, THERMAL_TZ_UNBIND_CDEV);
> -
> mutex_unlock(&cdev->lock);
> goto unbind;
> }
> @@ -885,6 +882,8 @@ static void thermal_unbind_cdev_from_tri
> return;
>
> unbind:
> + thermal_governor_update_tz(tz, THERMAL_TZ_UNBIND_CDEV);
> +
> device_remove_file(&tz->device, &pos->weight_attr);
> device_remove_file(&tz->device, &pos->attr);
> sysfs_remove_link(&tz->device.kobj, pos->name);
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 06/11] thermal: core: Introduce thermal_instance_add()
2024-10-10 22:13 ` [PATCH v2 06/11] thermal: core: Introduce thermal_instance_add() Rafael J. Wysocki
@ 2024-10-22 21:45 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 21:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:13, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> To reduce the number of redundant result checks in
> thermal_bind_cdev_to_trip() and make the code in it easier to
> follow, move some of it to a new function called thermal_instance_add()
> and make thermal_bind_cdev_to_trip() invoke that function.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/2641944.Lt9SDvczpP@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 46 ++++++++++++++++++++++++++---------------
> 1 file changed, 30 insertions(+), 16 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -743,6 +743,28 @@ struct thermal_zone_device *thermal_zone
> * binding, and unbinding.
> */
>
> +static int thermal_instance_add(struct thermal_instance *new_instance,
> + struct thermal_cooling_device *cdev,
> + struct thermal_trip_desc *td)
> +{
> + struct thermal_instance *instance;
> +
> + list_for_each_entry(instance, &td->thermal_instances, trip_node) {
> + if (instance->cdev == cdev)
> + return -EEXIST;
> + }
> +
> + list_add_tail(&new_instance->trip_node, &td->thermal_instances);
> +
> + mutex_lock(&cdev->lock);
> +
> + list_add_tail(&new_instance->cdev_node, &cdev->thermal_instances);
> +
> + mutex_unlock(&cdev->lock);
> +
> + return 0;
> +}
> +
> /**
> * thermal_bind_cdev_to_trip - bind a cooling device to a thermal zone
> * @tz: pointer to struct thermal_zone_device
> @@ -761,7 +783,7 @@ static int thermal_bind_cdev_to_trip(str
> struct thermal_cooling_device *cdev,
> struct cooling_spec *cool_spec)
> {
> - struct thermal_instance *dev, *instance;
> + struct thermal_instance *dev;
> bool upper_no_limit;
> int result;
>
> @@ -823,23 +845,15 @@ static int thermal_bind_cdev_to_trip(str
> if (result)
> goto remove_trip_file;
>
> - mutex_lock(&cdev->lock);
> - list_for_each_entry(instance, &td->thermal_instances, trip_node)
> - if (instance->cdev == cdev) {
> - result = -EEXIST;
> - break;
> - }
> - if (!result) {
> - list_add_tail(&dev->trip_node, &td->thermal_instances);
> - list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
> - }
> - mutex_unlock(&cdev->lock);
> + result = thermal_instance_add(dev, cdev, td);
> + if (result)
> + goto remove_weight_file;
>
> - if (!result) {
> - thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
> - return 0;
> - }
> + thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
> +
> + return 0;
>
> +remove_weight_file:
> device_remove_file(&tz->device, &dev->weight_attr);
> remove_trip_file:
> device_remove_file(&tz->device, &dev->attr);
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete()
2024-10-10 22:15 ` [PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete() Rafael J. Wysocki
@ 2024-10-22 21:48 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 21:48 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:15, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is not necessary to walk the thermal_instances list in a trip
> descriptor under a cooling device lock, so acquire that lock only
> for deleting the given thermal instance from the list of thermal
> instances in the given cdev.
>
> Moreover, in analogy with the previous change that introduced
> thermal_instance_add(), put the code deleting the given thermal
> instance from the lists it is on into a separate new function
> called thermal_instance_delete().
make sense
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/2224279.Mh6RI2rZIc@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -866,6 +866,17 @@ free_mem:
> return result;
> }
>
> +static void thermal_instance_delete(struct thermal_instance *instance)
> +{
> + list_del(&instance->trip_node);
> +
> + mutex_lock(&instance->cdev->lock);
> +
> + list_del(&instance->cdev_node);
> +
> + mutex_unlock(&instance->cdev->lock);
> +}
> +
> /**
> * thermal_unbind_cdev_from_trip - unbind a cooling device from a thermal zone.
> * @tz: pointer to a struct thermal_zone_device.
> @@ -882,16 +893,12 @@ static void thermal_unbind_cdev_from_tri
> {
> struct thermal_instance *pos, *next;
>
> - mutex_lock(&cdev->lock);
> list_for_each_entry_safe(pos, next, &td->thermal_instances, trip_node) {
> if (pos->cdev == cdev) {
> - list_del(&pos->trip_node);
> - list_del(&pos->cdev_node);
> - mutex_unlock(&cdev->lock);
> + thermal_instance_delete(pos);
> goto unbind;
> }
> }
> - mutex_unlock(&cdev->lock);
>
> return;
>
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck()
2024-10-10 22:16 ` [PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck() Rafael J. Wysocki
@ 2024-10-22 22:23 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 22:23 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:16, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Three thermal governors call __thermal_cdev_update() under the
> cdev lock without doing any checks, so in order to reduce the
> related code duplication, introduce a new helper function called
> thermal_cdev_update_nocheck() for them and make them use it.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/3791590.MHq7AAxBmi@rjwysocki.net/
>
> ---
> drivers/thermal/gov_bang_bang.c | 4 +---
> drivers/thermal/gov_fair_share.c | 4 +---
> drivers/thermal/gov_power_allocator.c | 5 ++---
> drivers/thermal/thermal_core.h | 1 +
> drivers/thermal/thermal_helpers.c | 13 +++++++++++++
> 5 files changed, 18 insertions(+), 9 deletions(-)
>
> Index: linux-pm/drivers/thermal/gov_bang_bang.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_bang_bang.c
> +++ linux-pm/drivers/thermal/gov_bang_bang.c
> @@ -30,9 +30,7 @@ static void bang_bang_set_instance_targe
>
> dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
>
> - mutex_lock(&instance->cdev->lock);
> - __thermal_cdev_update(instance->cdev);
> - mutex_unlock(&instance->cdev->lock);
> + thermal_cdev_update_nocheck(instance->cdev);
> }
>
> /**
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -89,9 +89,7 @@ static void fair_share_throttle(struct t
> }
> instance->target = div_u64(dividend, divisor);
>
> - mutex_lock(&cdev->lock);
> - __thermal_cdev_update(cdev);
> - mutex_unlock(&cdev->lock);
> + thermal_cdev_update_nocheck(cdev);
> }
> }
>
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -322,9 +322,8 @@ power_actor_set_power(struct thermal_coo
> return ret;
>
> instance->target = clamp_val(state, instance->lower, instance->upper);
> - mutex_lock(&cdev->lock);
> - __thermal_cdev_update(cdev);
> - mutex_unlock(&cdev->lock);
> +
> + thermal_cdev_update_nocheck(cdev);
>
> return 0;
> }
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -213,6 +213,7 @@ static inline bool cdev_is_power_actor(s
> }
>
> void thermal_cdev_update(struct thermal_cooling_device *);
> +void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev);
> void __thermal_cdev_update(struct thermal_cooling_device *cdev);
>
> int get_tz_trend(struct thermal_zone_device *tz, const struct thermal_trip *trip);
> Index: linux-pm/drivers/thermal/thermal_helpers.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_helpers.c
> +++ linux-pm/drivers/thermal/thermal_helpers.c
> @@ -206,6 +206,19 @@ void thermal_cdev_update(struct thermal_
> }
>
> /**
> + * thermal_cdev_update_nocheck() - Unconditionally update cooling device state
> + * @cdev: Target cooling device.
> + */
> +void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev)
> +{
> + mutex_lock(&cdev->lock);
> +
> + __thermal_cdev_update(cdev);
> +
> + mutex_unlock(&cdev->lock);
> +}
> +
> +/**
> * thermal_zone_get_slope - return the slope attribute of the thermal zone
> * @tz: thermal zone device with the slope attribute
> *
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/11] thermal: core: Add and use cooling device guard
2024-10-14 12:26 ` Rafael J. Wysocki
@ 2024-10-22 22:25 ` Lukasz Luba
2024-10-23 8:52 ` Rafael J. Wysocki
0 siblings, 1 reply; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 22:25 UTC (permalink / raw)
To: Rafael J. Wysocki, Rafael J. Wysocki
Cc: Linux PM, LKML, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/14/24 13:26, Rafael J. Wysocki wrote:
> On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Add and use a special guard for cooling devices.
>>
>> This allows quite a few error code paths to be simplified among
>> other things and brings in code size reduction for a good measure.
>>
>> No intentional functional impact.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>
>> This is a new iteration of
>>
>> https://lore.kernel.org/linux-pm/1890654.atdPhlSkOF@rjwysocki.net/
>>
>> v1 -> v2: Rearrange cur_state_store()
>>
>> ---
>> drivers/thermal/gov_power_allocator.c | 21 +++++++--------
>> drivers/thermal/gov_step_wise.c | 6 ++--
>> drivers/thermal/thermal_core.c | 17 +++---------
>> drivers/thermal/thermal_debugfs.c | 25 +++++++++++-------
>> drivers/thermal/thermal_helpers.c | 19 +++-----------
>> drivers/thermal/thermal_sysfs.c | 45 ++++++++++++----------------------
>> include/linux/thermal.h | 3 ++
>> 7 files changed, 57 insertions(+), 79 deletions(-)
>>
[snip]
>>
>> stats = cdev->stats;
>> - if (!stats) {
>> - len = -ENODATA;
>> - goto unlock;
>> - }
>> + if (!stats)
>> + return -ENODATA;
>>
>> len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n");
>> len += snprintf(buf + len, PAGE_SIZE - len, " : ");
>
> There is one more "goto unlock" statement in this function that needs
> to be replaced with "return".
>
> I'll send an update of it shortly.
>
OK, I will review that when it's ready.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization
2024-10-10 22:20 ` [PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization Rafael J. Wysocki
@ 2024-10-22 22:32 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 22:32 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:20, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In preparation for a subsequent change that will switch over the thermal
> core to using a mutex guard for managing thermal_governor_lock, move
> the code running in thermal_zone_device_register_with_trips() under that
> lock into a separate function called thermal_zone_init_governor().
>
> While at it, drop a useless comment.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/2495577.jE0xQCEvom@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 36 +++++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1343,6 +1343,25 @@ int thermal_zone_get_crit_temp(struct th
> }
> EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>
> +static int thermal_zone_init_governor(struct thermal_zone_device *tz)
> +{
> + struct thermal_governor *governor;
> + int ret;
> +
> + mutex_lock(&thermal_governor_lock);
> +
> + if (tz->tzp)
> + governor = __find_governor(tz->tzp->governor_name);
> + else
> + governor = def_governor;
> +
> + ret = thermal_set_governor(tz, governor);
> +
> + mutex_unlock(&thermal_governor_lock);
> +
> + return ret;
> +}
> +
> static void thermal_zone_init_complete(struct thermal_zone_device *tz)
> {
> struct thermal_cooling_device *cdev;
> @@ -1407,7 +1426,6 @@ thermal_zone_device_register_with_trips(
> struct thermal_trip_desc *td;
> int id;
> int result;
> - struct thermal_governor *governor;
>
> if (!type || strlen(type) == 0) {
> pr_err("No thermal zone type defined\n");
> @@ -1505,21 +1523,9 @@ thermal_zone_device_register_with_trips(
> if (result)
> goto release_device;
>
> - /* Update 'this' zone's governor information */
> - mutex_lock(&thermal_governor_lock);
> -
> - if (tz->tzp)
> - governor = __find_governor(tz->tzp->governor_name);
> - else
> - governor = def_governor;
> -
> - result = thermal_set_governor(tz, governor);
> - if (result) {
> - mutex_unlock(&thermal_governor_lock);
> + result = thermal_zone_init_governor(tz);
> + if (result)
> goto unregister;
> - }
> -
> - mutex_unlock(&thermal_governor_lock);
>
> if (!tz->tzp || !tz->tzp->no_hwmon) {
> result = thermal_add_hwmon_sysfs(tz);
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard
2024-10-10 22:22 ` [PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard Rafael J. Wysocki
@ 2024-10-22 22:35 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 22:35 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:22, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Switch over the thermal core to using a mutex guard for
> thermal_governor_lock management.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/863177860.0ifERbkFSE@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 40 +++++++++++++---------------------------
> 1 file changed, 13 insertions(+), 27 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -124,7 +124,7 @@ int thermal_register_governor(struct the
> if (!governor)
> return -EINVAL;
>
> - mutex_lock(&thermal_governor_lock);
> + guard(mutex)(&thermal_governor_lock);
>
> err = -EBUSY;
> if (!__find_governor(governor->name)) {
> @@ -163,8 +163,6 @@ int thermal_register_governor(struct the
> }
> }
>
> - mutex_unlock(&thermal_governor_lock);
> -
> return err;
> }
>
> @@ -175,10 +173,10 @@ void thermal_unregister_governor(struct
> if (!governor)
> return;
>
> - mutex_lock(&thermal_governor_lock);
> + guard(mutex)(&thermal_governor_lock);
>
> if (!__find_governor(governor->name))
> - goto exit;
> + return;
>
> list_del(&governor->governor_list);
>
> @@ -189,9 +187,6 @@ void thermal_unregister_governor(struct
> THERMAL_NAME_LENGTH))
> thermal_set_governor(pos, NULL);
> }
> -
> -exit:
> - mutex_unlock(&thermal_governor_lock);
> }
>
> int thermal_zone_device_set_policy(struct thermal_zone_device *tz,
> @@ -200,16 +195,13 @@ int thermal_zone_device_set_policy(struc
> struct thermal_governor *gov;
> int ret = -EINVAL;
>
> - mutex_lock(&thermal_governor_lock);
> -
> + guard(mutex)(&thermal_governor_lock);
> guard(thermal_zone)(tz);
>
> gov = __find_governor(strim(policy));
> if (gov)
> ret = thermal_set_governor(tz, gov);
>
> - mutex_unlock(&thermal_governor_lock);
> -
> thermal_notify_tz_gov_change(tz, policy);
>
> return ret;
> @@ -220,15 +212,13 @@ int thermal_build_list_of_policies(char
> struct thermal_governor *pos;
> ssize_t count = 0;
>
> - mutex_lock(&thermal_governor_lock);
> + guard(mutex)(&thermal_governor_lock);
>
> list_for_each_entry(pos, &thermal_governor_list, governor_list) {
> count += sysfs_emit_at(buf, count, "%s ", pos->name);
> }
> count += sysfs_emit_at(buf, count, "\n");
>
> - mutex_unlock(&thermal_governor_lock);
> -
> return count;
> }
>
> @@ -668,17 +658,18 @@ int for_each_thermal_governor(int (*cb)(
> void *data)
> {
> struct thermal_governor *gov;
> - int ret = 0;
>
> - mutex_lock(&thermal_governor_lock);
> + guard(mutex)(&thermal_governor_lock);
> +
> list_for_each_entry(gov, &thermal_governor_list, governor_list) {
> + int ret;
> +
> ret = cb(gov, data);
> if (ret)
> - break;
> + return ret;
> }
> - mutex_unlock(&thermal_governor_lock);
>
> - return ret;
> + return 0;
> }
>
> int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
> @@ -1346,20 +1337,15 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_
> static int thermal_zone_init_governor(struct thermal_zone_device *tz)
> {
> struct thermal_governor *governor;
> - int ret;
>
> - mutex_lock(&thermal_governor_lock);
> + guard(mutex)(&thermal_governor_lock);
>
> if (tz->tzp)
> governor = __find_governor(tz->tzp->governor_name);
> else
> governor = def_governor;
>
> - ret = thermal_set_governor(tz, governor);
> -
> - mutex_unlock(&thermal_governor_lock);
> -
> - return ret;
> + return thermal_set_governor(tz, governor);
> }
>
> static void thermal_zone_init_complete(struct thermal_zone_device *tz)
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard
2024-10-10 22:10 ` [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard Rafael J. Wysocki
@ 2024-10-22 22:50 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-22 22:50 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/10/24 23:10, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Switch over the thermal core to using a mutex guard for
> thermal_list_lock management.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a resend of
>
> https://lore.kernel.org/linux-pm/2018087.usQuhbGJ8B@rjwysocki.net/
>
> ---
> drivers/thermal/thermal_core.c | 148 ++++++++++++++++++-----------------------
> 1 file changed, 68 insertions(+), 80 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -140,7 +140,7 @@ int thermal_register_governor(struct the
> def_governor = governor;
> }
>
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
>
> list_for_each_entry(pos, &thermal_tz_list, node) {
> /*
> @@ -163,7 +163,6 @@ int thermal_register_governor(struct the
> }
> }
>
> - mutex_unlock(&thermal_list_lock);
> mutex_unlock(&thermal_governor_lock);
>
> return err;
> @@ -181,7 +180,9 @@ void thermal_unregister_governor(struct
> if (!__find_governor(governor->name))
> goto exit;
>
> - mutex_lock(&thermal_list_lock);
> + list_del(&governor->governor_list);
> +
> + guard(mutex)(&thermal_list_lock);
>
> list_for_each_entry(pos, &thermal_tz_list, node) {
> if (!strncasecmp(pos->governor->name, governor->name,
> @@ -189,8 +190,6 @@ void thermal_unregister_governor(struct
> thermal_set_governor(pos, NULL);
> }
>
> - mutex_unlock(&thermal_list_lock);
> - list_del(&governor->governor_list);
> exit:
> mutex_unlock(&thermal_governor_lock);
> }
> @@ -686,51 +685,52 @@ int for_each_thermal_cooling_device(int
> void *), void *data)
> {
> struct thermal_cooling_device *cdev;
> - int ret = 0;
>
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
> +
> list_for_each_entry(cdev, &thermal_cdev_list, node) {
> + int ret;
> +
> ret = cb(cdev, data);
> if (ret)
> - break;
> + return ret;
> }
> - mutex_unlock(&thermal_list_lock);
>
> - return ret;
> + return 0;
> }
>
> int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
> void *data)
> {
> struct thermal_zone_device *tz;
> - int ret = 0;
>
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
> +
> list_for_each_entry(tz, &thermal_tz_list, node) {
> + int ret;
> +
> ret = cb(tz, data);
> if (ret)
> - break;
> + return ret;
> }
> - mutex_unlock(&thermal_list_lock);
>
> - return ret;
> + return 0;
> }
>
> struct thermal_zone_device *thermal_zone_get_by_id(int id)
> {
> - struct thermal_zone_device *tz, *match = NULL;
> + struct thermal_zone_device *tz;
> +
> + guard(mutex)(&thermal_list_lock);
>
> - mutex_lock(&thermal_list_lock);
> list_for_each_entry(tz, &thermal_tz_list, node) {
> if (tz->id == id) {
> get_device(&tz->device);
> - match = tz;
> - break;
> + return tz;
> }
> }
> - mutex_unlock(&thermal_list_lock);
>
> - return match;
> + return NULL;
> }
>
> /*
> @@ -969,14 +969,12 @@ static void thermal_cooling_device_init_
> {
> struct thermal_zone_device *tz;
>
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
>
> list_add(&cdev->node, &thermal_cdev_list);
>
> list_for_each_entry(tz, &thermal_tz_list, node)
> thermal_zone_cdev_bind(tz, cdev);
> -
> - mutex_unlock(&thermal_list_lock);
> }
>
> /**
> @@ -1210,10 +1208,10 @@ void thermal_cooling_device_update(struc
> * Hold thermal_list_lock throughout the update to prevent the device
> * from going away while being updated.
> */
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
>
> if (!thermal_cooling_device_present(cdev))
> - goto unlock_list;
> + return;
>
> /*
> * Update under the cdev lock to prevent the state from being set beyond
> @@ -1255,9 +1253,6 @@ void thermal_cooling_device_update(struc
>
> unlock:
> mutex_unlock(&cdev->lock);
> -
> -unlock_list:
> - mutex_unlock(&thermal_list_lock);
> }
> EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
>
> @@ -1281,24 +1276,18 @@ static void thermal_zone_cdev_unbind(str
> static bool thermal_cooling_device_exit(struct thermal_cooling_device *cdev)
> {
> struct thermal_zone_device *tz;
> - bool ret = true;
>
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
>
> - if (!thermal_cooling_device_present(cdev)) {
> - ret = false;
> - goto unlock;
> - }
> + if (!thermal_cooling_device_present(cdev))
> + return false;
>
> list_del(&cdev->node);
>
> list_for_each_entry(tz, &thermal_tz_list, node)
> thermal_zone_cdev_unbind(tz, cdev);
>
> -unlock:
> - mutex_unlock(&thermal_list_lock);
> -
> - return ret;
> + return true;
> }
>
> /**
> @@ -1345,7 +1334,7 @@ static void thermal_zone_init_complete(s
> {
> struct thermal_cooling_device *cdev;
>
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
>
> list_add_tail(&tz->node, &thermal_tz_list);
>
> @@ -1365,8 +1354,6 @@ static void thermal_zone_init_complete(s
> tz->state |= TZ_STATE_FLAG_SUSPENDED;
>
> __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> -
> - mutex_unlock(&thermal_list_lock);
> }
>
> /**
> @@ -1587,14 +1574,11 @@ EXPORT_SYMBOL_GPL(thermal_zone_device);
> static bool thermal_zone_exit(struct thermal_zone_device *tz)
> {
> struct thermal_cooling_device *cdev;
> - bool ret = true;
>
> - mutex_lock(&thermal_list_lock);
> + guard(mutex)(&thermal_list_lock);
>
> - if (list_empty(&tz->node)) {
> - ret = false;
> - goto unlock;
> - }
> + if (list_empty(&tz->node))
> + return false;
>
> guard(thermal_zone)(tz);
>
> @@ -1605,10 +1589,7 @@ static bool thermal_zone_exit(struct the
> list_for_each_entry(cdev, &thermal_cdev_list, node)
> __thermal_zone_cdev_unbind(tz, cdev);
>
> -unlock:
> - mutex_unlock(&thermal_list_lock);
> -
> - return ret;
> + return true;
> }
>
> /**
> @@ -1660,24 +1641,23 @@ struct thermal_zone_device *thermal_zone
> unsigned int found = 0;
>
> if (!name)
> - goto exit;
> + return ERR_PTR(-EINVAL);
> +
> + guard(mutex)(&thermal_list_lock);
>
> - mutex_lock(&thermal_list_lock);
> list_for_each_entry(pos, &thermal_tz_list, node)
> if (!strncasecmp(name, pos->type, THERMAL_NAME_LENGTH)) {
> found++;
> ref = pos;
> }
> - mutex_unlock(&thermal_list_lock);
>
> - /* nothing has been found, thus an error code for it */
> - if (found == 0)
> - ref = ERR_PTR(-ENODEV);
> - else if (found > 1)
> - /* Success only when an unique zone is found */
> - ref = ERR_PTR(-EEXIST);
> + if (!found)
> + return ERR_PTR(-ENODEV);
> +
> + /* Success only when one zone is found. */
> + if (found > 1)
> + return ERR_PTR(-EEXIST);
>
> -exit:
> return ref;
> }
> EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
> @@ -1718,6 +1698,18 @@ static void thermal_zone_pm_prepare(stru
> tz->state |= TZ_STATE_FLAG_SUSPENDED;
> }
>
> +static void thermal_pm_notify_prepare(void)
> +{
> + struct thermal_zone_device *tz;
> +
> + guard(mutex)(&thermal_list_lock);
> +
> + thermal_pm_suspended = true;
> +
> + list_for_each_entry(tz, &thermal_tz_list, node)
> + thermal_zone_pm_prepare(tz);
> +}
> +
> static void thermal_zone_pm_complete(struct thermal_zone_device *tz)
> {
> guard(thermal_zone)(tz);
> @@ -1736,35 +1728,31 @@ static void thermal_zone_pm_complete(str
> mod_delayed_work(system_freezable_power_efficient_wq, &tz->poll_queue, 0);
> }
>
> -static int thermal_pm_notify(struct notifier_block *nb,
> - unsigned long mode, void *_unused)
> +static void thermal_pm_notify_complete(void)
> {
> struct thermal_zone_device *tz;
>
> + guard(mutex)(&thermal_list_lock);
> +
> + thermal_pm_suspended = false;
> +
> + list_for_each_entry(tz, &thermal_tz_list, node)
> + thermal_zone_pm_complete(tz);
> +}
> +
> +static int thermal_pm_notify(struct notifier_block *nb,
> + unsigned long mode, void *_unused)
> +{
> switch (mode) {
> case PM_HIBERNATION_PREPARE:
> case PM_RESTORE_PREPARE:
> case PM_SUSPEND_PREPARE:
> - mutex_lock(&thermal_list_lock);
> -
> - thermal_pm_suspended = true;
> -
> - list_for_each_entry(tz, &thermal_tz_list, node)
> - thermal_zone_pm_prepare(tz);
> -
> - mutex_unlock(&thermal_list_lock);
> + thermal_pm_notify_prepare();
> break;
> case PM_POST_HIBERNATION:
> case PM_POST_RESTORE:
> case PM_POST_SUSPEND:
> - mutex_lock(&thermal_list_lock);
> -
> - thermal_pm_suspended = false;
> -
> - list_for_each_entry(tz, &thermal_tz_list, node)
> - thermal_zone_pm_complete(tz);
> -
> - mutex_unlock(&thermal_list_lock);
> + thermal_pm_notify_complete();
> break;
> default:
> break;
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/11] thermal: core: Add and use cooling device guard
2024-10-22 22:25 ` Lukasz Luba
@ 2024-10-23 8:52 ` Rafael J. Wysocki
0 siblings, 0 replies; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-23 8:52 UTC (permalink / raw)
To: Lukasz Luba
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PM, LKML,
Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On Wed, Oct 23, 2024 at 12:24 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/14/24 13:26, Rafael J. Wysocki wrote:
> > On Fri, Oct 11, 2024 at 12:22 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Add and use a special guard for cooling devices.
> >>
> >> This allows quite a few error code paths to be simplified among
> >> other things and brings in code size reduction for a good measure.
> >>
> >> No intentional functional impact.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>
> >> This is a new iteration of
> >>
> >> https://lore.kernel.org/linux-pm/1890654.atdPhlSkOF@rjwysocki.net/
> >>
> >> v1 -> v2: Rearrange cur_state_store()
> >>
> >> ---
> >> drivers/thermal/gov_power_allocator.c | 21 +++++++--------
> >> drivers/thermal/gov_step_wise.c | 6 ++--
> >> drivers/thermal/thermal_core.c | 17 +++---------
> >> drivers/thermal/thermal_debugfs.c | 25 +++++++++++-------
> >> drivers/thermal/thermal_helpers.c | 19 +++-----------
> >> drivers/thermal/thermal_sysfs.c | 45 ++++++++++++----------------------
> >> include/linux/thermal.h | 3 ++
> >> 7 files changed, 57 insertions(+), 79 deletions(-)
> >>
>
> [snip]
>
> >>
> >> stats = cdev->stats;
> >> - if (!stats) {
> >> - len = -ENODATA;
> >> - goto unlock;
> >> - }
> >> + if (!stats)
> >> + return -ENODATA;
> >>
> >> len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n");
> >> len += snprintf(buf + len, PAGE_SIZE - len, " : ");
> >
> > There is one more "goto unlock" statement in this function that needs
> > to be replaced with "return".
> >
> > I'll send an update of it shortly.
> >
>
> OK, I will review that when it's ready.
It's been sent already:
https://lore.kernel.org/linux-pm/5837621.DvuYhMxLoT@rjwysocki.net/
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-22 21:02 ` [PATCH v2 " Lukasz Luba
@ 2024-10-23 9:50 ` Rafael J. Wysocki
2024-10-25 13:53 ` Lukasz Luba
0 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2024-10-23 9:50 UTC (permalink / raw)
To: Lukasz Luba
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada
Hi Lukasz,
On Tue, Oct 22, 2024 at 11:01 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> On 10/10/24 23:05, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Add and use a guard for thermal zone locking.
> >
> > This allows quite a few error code paths to be simplified among
> > other things and brings in a noticeable code size reduction for
> > a good measure.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > This is a new iteration of
> >
> > https://lore.kernel.org/linux-pm/3241904.5fSG56mABF@rjwysocki.net/
> >
> > that has been combined with
> >
> > https://lore.kernel.org/linux-pm/4613601.LvFx2qVVIh@rjwysocki.net/
> >
> > and rebased on top of
> >
> > https://lore.kernel.org/linux-pm/12549318.O9o76ZdvQC@rjwysocki.net/
> >
> > and
> >
> > https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
> >
> > ---
> > drivers/thermal/thermal_core.c | 61 +++++++---------------------
> > drivers/thermal/thermal_core.h | 4 +
> > drivers/thermal/thermal_debugfs.c | 25 +++++++----
> > drivers/thermal/thermal_helpers.c | 17 ++-----
> > drivers/thermal/thermal_hwmon.c | 5 --
> > drivers/thermal/thermal_netlink.c | 21 ++-------
> > drivers/thermal/thermal_sysfs.c | 81 ++++++++++++++++----------------------
> > drivers/thermal/thermal_trip.c | 8 ---
> > 8 files changed, 86 insertions(+), 136 deletions(-)
> >
>
>
> [snip]
>
> Surprise, how the code can look smaller using that
> style with 'guard'.
Yes, it gets more concise.
Not only that, though. It is also less error-prone, because you won't
forget to unlock the lock in an error path and you won't use "lock"
instead of "unlock" by mistake etc.
Moreover, it kind of promotes dividing the code into smaller
self-contained pieces, which is a plus too IMV.
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Thank you!
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard
2024-10-14 14:59 ` [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
@ 2024-10-24 8:13 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-24 8:13 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada, Linux PM
On 10/14/24 15:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Add and use a special guard for cooling devices.
>
> This allows quite a few error code paths to be simplified among
> other things and brings in code size reduction for a good measure.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> This is a new iteration of
>
> https://lore.kernel.org/linux-pm/1890654.atdPhlSkOF@rjwysocki.net/
>
> v2 -> v2.1: Add missing hunk in trans_table_show()
>
> v1 -> v2: Rearrange cur_state_store()
>
> ---
> drivers/thermal/gov_power_allocator.c | 21 ++++++--------
> drivers/thermal/gov_step_wise.c | 6 ++--
> drivers/thermal/thermal_core.c | 17 +++--------
> drivers/thermal/thermal_debugfs.c | 25 ++++++++++------
> drivers/thermal/thermal_helpers.c | 19 +++---------
> drivers/thermal/thermal_sysfs.c | 51 ++++++++++++----------------------
> include/linux/thermal.h | 3 ++
> 7 files changed, 59 insertions(+), 83 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -758,12 +758,10 @@ static int thermal_instance_add(struct t
>
> list_add_tail(&new_instance->trip_node, &td->thermal_instances);
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> list_add_tail(&new_instance->cdev_node, &cdev->thermal_instances);
>
> - mutex_unlock(&cdev->lock);
> -
> return 0;
> }
>
> @@ -872,11 +870,9 @@ static void thermal_instance_delete(stru
> {
> list_del(&instance->trip_node);
>
> - mutex_lock(&instance->cdev->lock);
> + guard(cooling_dev)(instance->cdev);
>
> list_del(&instance->cdev_node);
> -
> - mutex_unlock(&instance->cdev->lock);
> }
>
> /**
> @@ -1239,10 +1235,10 @@ void thermal_cooling_device_update(struc
> * Update under the cdev lock to prevent the state from being set beyond
> * the new limit concurrently.
> */
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> if (cdev->ops->get_max_state(cdev, &cdev->max_state))
> - goto unlock;
> + return;
>
> thermal_cooling_device_stats_reinit(cdev);
>
> @@ -1269,12 +1265,9 @@ void thermal_cooling_device_update(struc
> }
>
> if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
> - goto unlock;
> + return;
>
> thermal_cooling_device_stats_update(cdev, state);
> -
> -unlock:
> - mutex_unlock(&cdev->lock);
> }
> EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
>
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -140,6 +140,9 @@ struct thermal_cooling_device {
> #endif
> };
>
> +DEFINE_GUARD(cooling_dev, struct thermal_cooling_device *, mutex_lock(&_T->lock),
> + mutex_unlock(&_T->lock))
> +
> /* Structure to define Thermal Zone parameters */
> struct thermal_zone_params {
> const char *governor_name;
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -544,14 +544,15 @@ cur_state_store(struct device *dev, stru
> if (state > cdev->max_state)
> return -EINVAL;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> result = cdev->ops->set_cur_state(cdev, state);
> - if (!result)
> - thermal_cooling_device_stats_update(cdev, state);
> + if (result)
> + return result;
>
> - mutex_unlock(&cdev->lock);
> - return result ? result : count;
> + thermal_cooling_device_stats_update(cdev, state);
> +
> + return count;
> }
>
> static struct device_attribute
> @@ -625,21 +626,18 @@ static ssize_t total_trans_show(struct d
> {
> struct thermal_cooling_device *cdev = to_cooling_device(dev);
> struct cooling_dev_stats *stats;
> - int ret = 0;
> + int ret;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> if (!stats)
> - goto unlock;
> + return 0;
>
> spin_lock(&stats->lock);
> ret = sprintf(buf, "%u\n", stats->total_trans);
> spin_unlock(&stats->lock);
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return ret;
> }
>
> @@ -652,11 +650,11 @@ time_in_state_ms_show(struct device *dev
> ssize_t len = 0;
> int i;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> if (!stats)
> - goto unlock;
> + return 0;
>
> spin_lock(&stats->lock);
>
> @@ -668,9 +666,6 @@ time_in_state_ms_show(struct device *dev
> }
> spin_unlock(&stats->lock);
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return len;
> }
>
> @@ -682,11 +677,11 @@ reset_store(struct device *dev, struct d
> struct cooling_dev_stats *stats;
> int i, states;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> if (!stats)
> - goto unlock;
> + return count;
>
> states = cdev->max_state + 1;
>
> @@ -702,9 +697,6 @@ reset_store(struct device *dev, struct d
>
> spin_unlock(&stats->lock);
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return count;
> }
>
> @@ -716,13 +708,11 @@ static ssize_t trans_table_show(struct d
> ssize_t len = 0;
> int i, j;
>
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> stats = cdev->stats;
> - if (!stats) {
> - len = -ENODATA;
> - goto unlock;
> - }
> + if (!stats)
> + return -ENODATA;
>
> len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n");
> len += snprintf(buf + len, PAGE_SIZE - len, " : ");
> @@ -731,10 +721,8 @@ static ssize_t trans_table_show(struct d
> break;
> len += snprintf(buf + len, PAGE_SIZE - len, "state%2u ", i);
> }
> - if (len >= PAGE_SIZE) {
> - len = PAGE_SIZE;
> - goto unlock;
> - }
> + if (len >= PAGE_SIZE)
> + return PAGE_SIZE;
>
> len += snprintf(buf + len, PAGE_SIZE - len, "\n");
>
> @@ -760,9 +748,6 @@ static ssize_t trans_table_show(struct d
> len = -EFBIG;
> }
>
> -unlock:
> - mutex_unlock(&cdev->lock);
> -
> return len;
> }
>
> Index: linux-pm/drivers/thermal/thermal_helpers.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_helpers.c
> +++ linux-pm/drivers/thermal/thermal_helpers.c
> @@ -58,17 +58,10 @@ bool thermal_trip_is_bound_to_cdev(struc
> const struct thermal_trip *trip,
> struct thermal_cooling_device *cdev)
> {
> - bool ret;
> -
> guard(thermal_zone)(tz);
> + guard(cooling_dev)(cdev);
>
> - mutex_lock(&cdev->lock);
> -
> - ret = thermal_instance_present(tz, cdev, trip);
> -
> - mutex_unlock(&cdev->lock);
> -
> - return ret;
> + return thermal_instance_present(tz, cdev, trip);
> }
> EXPORT_SYMBOL_GPL(thermal_trip_is_bound_to_cdev);
>
> @@ -197,12 +190,12 @@ void __thermal_cdev_update(struct therma
> */
> void thermal_cdev_update(struct thermal_cooling_device *cdev)
> {
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
> +
> if (!cdev->updated) {
> __thermal_cdev_update(cdev);
> cdev->updated = true;
> }
> - mutex_unlock(&cdev->lock);
> }
>
> /**
> @@ -211,11 +204,9 @@ void thermal_cdev_update(struct thermal_
> */
> void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev)
> {
> - mutex_lock(&cdev->lock);
> + guard(cooling_dev)(cdev);
>
> __thermal_cdev_update(cdev);
> -
> - mutex_unlock(&cdev->lock);
> }
>
> /**
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -516,6 +516,19 @@ void thermal_debug_cdev_add(struct therm
> cdev->debugfs = thermal_dbg;
> }
>
> +static struct thermal_debugfs *thermal_debug_cdev_clear(struct thermal_cooling_device *cdev)
> +{
> + struct thermal_debugfs *thermal_dbg;
> +
> + guard(cooling_dev)(cdev);
> +
> + thermal_dbg = cdev->debugfs;
> + if (thermal_dbg)
> + cdev->debugfs = NULL;
> +
> + return thermal_dbg;
> +}
> +
> /**
> * thermal_debug_cdev_remove - Remove a cooling device debugfs entry
> *
> @@ -527,17 +540,9 @@ void thermal_debug_cdev_remove(struct th
> {
> struct thermal_debugfs *thermal_dbg;
>
> - mutex_lock(&cdev->lock);
> -
> - thermal_dbg = cdev->debugfs;
> - if (!thermal_dbg) {
> - mutex_unlock(&cdev->lock);
> + thermal_dbg = thermal_debug_cdev_clear(cdev);
> + if (!thermal_dbg)
> return;
> - }
> -
> - cdev->debugfs = NULL;
> -
> - mutex_unlock(&cdev->lock);
>
> mutex_lock(&thermal_dbg->lock);
>
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -549,18 +549,17 @@ static void allow_maximum_power(struct t
> cdev = instance->cdev;
>
> instance->target = 0;
> - mutex_lock(&cdev->lock);
> - /*
> - * Call for updating the cooling devices local stats and avoid
> - * periods of dozen of seconds when those have not been
> - * maintained.
> - */
> - cdev->ops->get_requested_power(cdev, &req_power);
> + scoped_guard(cooling_dev, cdev) {
> + /*
> + * Call for updating the cooling devices local stats and
> + * avoid periods of dozen of seconds when those have not
> + * been maintained.
> + */
> + cdev->ops->get_requested_power(cdev, &req_power);
>
> - if (params->update_cdevs)
> - __thermal_cdev_update(cdev);
> -
> - mutex_unlock(&cdev->lock);
> + if (params->update_cdevs)
> + __thermal_cdev_update(cdev);
> + }
> }
> }
>
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -97,9 +97,9 @@ static void thermal_zone_trip_update(str
>
> instance->initialized = true;
>
> - mutex_lock(&instance->cdev->lock);
> - instance->cdev->updated = false; /* cdev needs update */
> - mutex_unlock(&instance->cdev->lock);
> + scoped_guard(cooling_dev, instance->cdev) {
> + instance->cdev->updated = false; /* cdev needs update */
> + }
> }
> }
>
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard
2024-10-23 9:50 ` Rafael J. Wysocki
@ 2024-10-25 13:53 ` Lukasz Luba
0 siblings, 0 replies; 39+ messages in thread
From: Lukasz Luba @ 2024-10-25 13:53 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada
On 10/23/24 10:50, Rafael J. Wysocki wrote:
> Hi Lukasz,
>
> On Tue, Oct 22, 2024 at 11:01 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Rafael,
>>
>> On 10/10/24 23:05, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Add and use a guard for thermal zone locking.
>>>
>>> This allows quite a few error code paths to be simplified among
>>> other things and brings in a noticeable code size reduction for
>>> a good measure.
>>>
>>> No intentional functional impact.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> This is a new iteration of
>>>
>>> https://lore.kernel.org/linux-pm/3241904.5fSG56mABF@rjwysocki.net/
>>>
>>> that has been combined with
>>>
>>> https://lore.kernel.org/linux-pm/4613601.LvFx2qVVIh@rjwysocki.net/
>>>
>>> and rebased on top of
>>>
>>> https://lore.kernel.org/linux-pm/12549318.O9o76ZdvQC@rjwysocki.net/
>>>
>>> and
>>>
>>> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
>>>
>>> ---
>>> drivers/thermal/thermal_core.c | 61 +++++++---------------------
>>> drivers/thermal/thermal_core.h | 4 +
>>> drivers/thermal/thermal_debugfs.c | 25 +++++++----
>>> drivers/thermal/thermal_helpers.c | 17 ++-----
>>> drivers/thermal/thermal_hwmon.c | 5 --
>>> drivers/thermal/thermal_netlink.c | 21 ++-------
>>> drivers/thermal/thermal_sysfs.c | 81 ++++++++++++++++----------------------
>>> drivers/thermal/thermal_trip.c | 8 ---
>>> 8 files changed, 86 insertions(+), 136 deletions(-)
>>>
>>
>>
>> [snip]
>>
>> Surprise, how the code can look smaller using that
>> style with 'guard'.
>
> Yes, it gets more concise.
>
> Not only that, though. It is also less error-prone, because you won't
> forget to unlock the lock in an error path and you won't use "lock"
> instead of "unlock" by mistake etc.
>
> Moreover, it kind of promotes dividing the code into smaller
> self-contained pieces, which is a plus too IMV.
I cannot agree more!
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2024-10-25 13:51 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-10 21:59 [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
2024-10-10 22:05 ` [PATCH v2 01/11] thermal: core: Add and use thermal zone guard Rafael J. Wysocki
2024-10-21 11:40 ` Markus Elfring
2024-10-21 11:49 ` Rafael J. Wysocki
2024-10-21 12:02 ` [v2 " Markus Elfring
2024-10-21 12:03 ` Rafael J. Wysocki
2024-10-22 21:02 ` [PATCH v2 " Lukasz Luba
2024-10-23 9:50 ` Rafael J. Wysocki
2024-10-25 13:53 ` Lukasz Luba
2024-10-10 22:07 ` [PATCH v2 02/11] thermal: core: Add and use a reverse " Rafael J. Wysocki
2024-10-22 21:03 ` Lukasz Luba
2024-10-10 22:09 ` [PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock Rafael J. Wysocki
2024-10-22 21:09 ` Lukasz Luba
2024-10-10 22:10 ` [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard Rafael J. Wysocki
2024-10-22 22:50 ` Lukasz Luba
2024-10-10 22:12 ` [PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock Rafael J. Wysocki
2024-10-22 21:10 ` Lukasz Luba
2024-10-10 22:13 ` [PATCH v2 06/11] thermal: core: Introduce thermal_instance_add() Rafael J. Wysocki
2024-10-22 21:45 ` Lukasz Luba
2024-10-10 22:15 ` [PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete() Rafael J. Wysocki
2024-10-22 21:48 ` Lukasz Luba
2024-10-10 22:16 ` [PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck() Rafael J. Wysocki
2024-10-22 22:23 ` Lukasz Luba
2024-10-10 22:19 ` [PATCH v2 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
2024-10-14 12:26 ` Rafael J. Wysocki
2024-10-22 22:25 ` Lukasz Luba
2024-10-23 8:52 ` Rafael J. Wysocki
2024-10-10 22:20 ` [PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization Rafael J. Wysocki
2024-10-22 22:32 ` Lukasz Luba
2024-10-10 22:22 ` [PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard Rafael J. Wysocki
2024-10-22 22:35 ` Lukasz Luba
2024-10-11 18:51 ` [PATCH v2 00/11] thermal: core: Reimplement locking through guards Rafael J. Wysocki
2024-10-21 11:08 ` Rafael J. Wysocki
2024-10-21 22:03 ` Lukasz Luba
2024-10-22 9:57 ` Rafael J. Wysocki
2024-10-22 10:21 ` Lukasz Luba
2024-10-22 15:29 ` Rafael J. Wysocki
2024-10-14 14:59 ` [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard Rafael J. Wysocki
2024-10-24 8:13 ` Lukasz Luba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).