[PATCH v2 00/11] thermal: core: Reimplement locking through guards

[PATCH v2 00/11] thermal: core: Reimplement locking through guards

[Rafael J. Wysocki]

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!

[PATCH v2 01/11] thermal: core: Add and use thermal zone guard

[Rafael J. Wysocki]

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,

Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard

[Markus Elfring]

…
> +++ 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

Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard

[Rafael J. Wysocki]

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/

Re: [v2 01/11] thermal: core: Add and use thermal zone guard

[Markus Elfring]

>> …
>>> +++ 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

Re: [v2 01/11] thermal: core: Add and use thermal zone guard

[Rafael J. Wysocki]

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.

Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard

[Lukasz Luba]

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>

Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard

[Rafael J. Wysocki]

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!

Re: [PATCH v2 01/11] thermal: core: Add and use thermal zone guard

[Lukasz Luba]

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!

[PATCH v2 02/11] thermal: core: Add and use a reverse thermal zone guard

[Rafael J. Wysocki]

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
 

Re: [PATCH v2 02/11] thermal: core: Add and use a reverse thermal zone guard

[Lukasz Luba]

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>

[PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock

[Rafael J. Wysocki]

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);
 

Re: [PATCH v2 03/11] thermal: core: Separate code running under thermal_list_lock

[Lukasz Luba]

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>

[PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard

[Rafael J. Wysocki]

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;

Re: [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard

[Lukasz Luba]

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>

[PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock

[Rafael J. Wysocki]

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);

Re: [PATCH v2 05/11] thermal: core: Call thermal_governor_update_tz() outside of cdev lock

[Lukasz Luba]

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>

[PATCH v2 06/11] thermal: core: Introduce thermal_instance_add()

[Rafael J. Wysocki]

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);

Re: [PATCH v2 06/11] thermal: core: Introduce thermal_instance_add()

[Lukasz Luba]

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>

[PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete()

[Rafael J. Wysocki]

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;
 

Re: [PATCH v2 07/11] thermal: core: Introduce thermal_instance_delete()

[Lukasz Luba]

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>

[PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck()

[Rafael J. Wysocki]

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
  *

Re: [PATCH v2 08/11] thermal: core: Introduce thermal_cdev_update_nocheck()

[Lukasz Luba]

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>

[PATCH v2 09/11] thermal: core: Add and use cooling device guard

[Rafael J. Wysocki]

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 */
+		}
 	}
 }
 

Re: [PATCH v2 09/11] thermal: core: Add and use cooling device guard

[Rafael J. Wysocki]

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 */
> +               }
>         }
>  }
>
>
>
>
>

Re: [PATCH v2 09/11] thermal: core: Add and use cooling device guard

[Lukasz Luba]

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.

Re: [PATCH v2 09/11] thermal: core: Add and use cooling device guard

[Rafael J. Wysocki]

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/

[PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization

[Rafael J. Wysocki]

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);

Re: [PATCH v2 10/11] thermal: core: Separate thermal zone governor initialization

[Lukasz Luba]

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>

[PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard

[Rafael J. Wysocki]

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)

Re: [PATCH v2 11/11] thermal: core: Manage thermal_governor_lock using a mutex guard

[Lukasz Luba]

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>

Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards

[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.

Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards

[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.

Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards

[Lukasz Luba]

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

Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards

[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!

Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards

[Lukasz Luba]

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.

Re: [PATCH v2 00/11] thermal: core: Reimplement locking through guards

[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!

[PATCH v2.1 09/11] thermal: core: Add and use cooling device guard

[Rafael J. Wysocki]

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 */
+		}
 	}
 }
 

Re: [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard

[Lukasz Luba]

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>