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,

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>

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!

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!

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

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/

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

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.