[PATCH 0/3] pwm: Use guards instead of explicity mutex_lock + mutex_unlock

[PATCH 0/3] pwm: Use guards instead of explicity mutex_lock + mutex_unlock

[Uwe Kleine-König]

Hello,

I consider this a nice cleanup. It makes the code more compact and it's less
prone to error, because you cannot forget an unlock in an error path.

Best regards
Uwe

Uwe Kleine-König (3):
  pwm: Use guards for pwm_lock instead of explicity mutex_lock + mutex_unlock
  pwm: Use guards for export->lock instead of explicity mutex_lock + mutex_unlock
  pwm: Use guards for pwm_lookup_lock instead of explicity mutex_lock + mutex_unlock

 drivers/pwm/core.c | 128 +++++++++++++++++----------------------------
 1 file changed, 48 insertions(+), 80 deletions(-)

base-commit: 888564d8d708d1c91900ed3a11761f46297fd748
-- 
2.43.0

[PATCH 1/3] pwm: Use guards for pwm_lock instead of explicity mutex_lock + mutex_unlock

[Uwe Kleine-König]

With the compiler caring for unlocking the mutex several functions can
be simplified. Benefit from that.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c | 52 +++++++++++++---------------------------------
 1 file changed, 15 insertions(+), 37 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 5c1d20985148..58e868b56530 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -293,19 +293,15 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
 int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 		unsigned long timeout)
 {
-	int err;
-
 	if (!pwm || !pwm->chip->ops)
 		return -EINVAL;
 
 	if (!pwm->chip->ops->capture)
 		return -ENOSYS;
 
-	mutex_lock(&pwm_lock);
-	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
-	mutex_unlock(&pwm_lock);
+	guard(mutex)(&pwm_lock);
 
-	return err;
+	return pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
 }
 EXPORT_SYMBOL_GPL(pwm_capture);
 
@@ -317,19 +313,15 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name)
 	if (!name)
 		return NULL;
 
-	mutex_lock(&pwm_lock);
+	guard(mutex)(&pwm_lock);
 
 	idr_for_each_entry_ul(&pwm_chips, chip, tmp, id) {
 		const char *chip_name = dev_name(pwmchip_parent(chip));
 
-		if (chip_name && strcmp(chip_name, name) == 0) {
-			mutex_unlock(&pwm_lock);
+		if (chip_name && strcmp(chip_name, name) == 0)
 			return chip;
-		}
 	}
 
-	mutex_unlock(&pwm_lock);
-
 	return NULL;
 }
 
@@ -406,14 +398,14 @@ static struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 	if (!chip || index >= chip->npwm)
 		return ERR_PTR(-EINVAL);
 
-	mutex_lock(&pwm_lock);
+	guard(mutex)(&pwm_lock);
+
 	pwm = &chip->pwms[index];
 
 	err = pwm_device_request(pwm, label);
 	if (err < 0)
-		pwm = ERR_PTR(err);
+		return ERR_PTR(err);
 
-	mutex_unlock(&pwm_lock);
 	return pwm;
 }
 
@@ -1102,11 +1094,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 
 	chip->owner = owner;
 
-	mutex_lock(&pwm_lock);
+	guard(mutex)(&pwm_lock);
 
 	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
 	if (ret < 0)
-		goto err_idr_alloc;
+		return ret;
 
 	chip->id = ret;
 
@@ -1119,8 +1111,6 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	if (ret)
 		goto err_device_add;
 
-	mutex_unlock(&pwm_lock);
-
 	return 0;
 
 err_device_add:
@@ -1128,9 +1118,6 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 		of_pwmchip_remove(chip);
 
 	idr_remove(&pwm_chips, chip->id);
-err_idr_alloc:
-
-	mutex_unlock(&pwm_lock);
 
 	return ret;
 }
@@ -1149,11 +1136,8 @@ void pwmchip_remove(struct pwm_chip *chip)
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_remove(chip);
 
-	mutex_lock(&pwm_lock);
-
-	idr_remove(&pwm_chips, chip->id);
-
-	mutex_unlock(&pwm_lock);
+	scoped_guard(mutex, &pwm_lock)
+		idr_remove(&pwm_chips, chip->id);
 
 	device_del(&chip->dev);
 }
@@ -1209,15 +1193,11 @@ static struct pwm_chip *fwnode_to_pwmchip(struct fwnode_handle *fwnode)
 	struct pwm_chip *chip;
 	unsigned long id, tmp;
 
-	mutex_lock(&pwm_lock);
+	guard(mutex)(&pwm_lock);
 
 	idr_for_each_entry_ul(&pwm_chips, chip, tmp, id)
-		if (pwmchip_parent(chip) && device_match_fwnode(pwmchip_parent(chip), fwnode)) {
-			mutex_unlock(&pwm_lock);
+		if (pwmchip_parent(chip) && device_match_fwnode(pwmchip_parent(chip), fwnode))
 			return chip;
-		}
-
-	mutex_unlock(&pwm_lock);
 
 	return ERR_PTR(-EPROBE_DEFER);
 }
@@ -1532,11 +1512,11 @@ void pwm_put(struct pwm_device *pwm)
 
 	chip = pwm->chip;
 
-	mutex_lock(&pwm_lock);
+	guard(mutex)(&pwm_lock);
 
 	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
 		pr_warn("PWM device already freed\n");
-		goto out;
+		return;
 	}
 
 	if (chip->ops->free)
@@ -1547,8 +1527,6 @@ void pwm_put(struct pwm_device *pwm)
 	put_device(&chip->dev);
 
 	module_put(chip->owner);
-out:
-	mutex_unlock(&pwm_lock);
 }
 EXPORT_SYMBOL_GPL(pwm_put);
 
-- 
2.43.0

[PATCH 2/3] pwm: Use guards for export->lock instead of explicity mutex_lock + mutex_unlock

[Uwe Kleine-König]

With the compiler caring for unlocking the mutex several functions can
be simplified. Benefit from that.

There is just one caller left for mutex_lock(&export->lock). The code
flow is too complicated there to convert it to the compiler assisted
variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 58e868b56530..a0b1aee373bf 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -503,11 +503,11 @@ static ssize_t period_store(struct device *pwm_dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&export->lock);
+	guard(mutex)(&export->lock);
+
 	pwm_get_state(pwm, &state);
 	state.period = val;
 	ret = pwm_apply_might_sleep(pwm, &state);
-	mutex_unlock(&export->lock);
 
 	return ret ? : size;
 }
@@ -538,11 +538,11 @@ static ssize_t duty_cycle_store(struct device *pwm_dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&export->lock);
+	guard(mutex)(&export->lock);
+
 	pwm_get_state(pwm, &state);
 	state.duty_cycle = val;
 	ret = pwm_apply_might_sleep(pwm, &state);
-	mutex_unlock(&export->lock);
 
 	return ret ? : size;
 }
@@ -572,7 +572,7 @@ static ssize_t enable_store(struct device *pwm_dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&export->lock);
+	guard(mutex)(&export->lock);
 
 	pwm_get_state(pwm, &state);
 
@@ -584,14 +584,11 @@ static ssize_t enable_store(struct device *pwm_dev,
 		state.enabled = true;
 		break;
 	default:
-		ret = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
 
 	ret = pwm_apply_might_sleep(pwm, &state);
 
-unlock:
-	mutex_unlock(&export->lock);
 	return ret ? : size;
 }
 
@@ -635,11 +632,11 @@ static ssize_t polarity_store(struct device *pwm_dev,
 	else
 		return -EINVAL;
 
-	mutex_lock(&export->lock);
+	guard(mutex)(&export->lock);
+
 	pwm_get_state(pwm, &state);
 	state.polarity = polarity;
 	ret = pwm_apply_might_sleep(pwm, &state);
-	mutex_unlock(&export->lock);
 
 	return ret ? : size;
 }
-- 
2.43.0

[PATCH 3/3] pwm: Use guards for pwm_lookup_lock instead of explicity mutex_lock + mutex_unlock

[Uwe Kleine-König]

With the compiler caring for unlocking the mutex several functions can
be simplified. Benefit from that.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c | 57 ++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a0b1aee373bf..a79828f563d4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -1343,14 +1343,12 @@ static LIST_HEAD(pwm_lookup_list);
  */
 void pwm_add_table(struct pwm_lookup *table, size_t num)
 {
-	mutex_lock(&pwm_lookup_lock);
+	guard(mutex)(&pwm_lookup_lock);
 
 	while (num--) {
 		list_add_tail(&table->list, &pwm_lookup_list);
 		table++;
 	}
-
-	mutex_unlock(&pwm_lookup_lock);
 }
 
 /**
@@ -1360,14 +1358,12 @@ void pwm_add_table(struct pwm_lookup *table, size_t num)
  */
 void pwm_remove_table(struct pwm_lookup *table, size_t num)
 {
-	mutex_lock(&pwm_lookup_lock);
+	guard(mutex)(&pwm_lookup_lock);
 
 	while (num--) {
 		list_del(&table->list);
 		table++;
 	}
-
-	mutex_unlock(&pwm_lookup_lock);
 }
 
 /**
@@ -1428,37 +1424,34 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 	 * Then we take the most specific entry - with the following order
 	 * of precedence: dev+con > dev only > con only.
 	 */
-	mutex_lock(&pwm_lookup_lock);
+	scoped_guard(mutex, &pwm_lookup_lock)
+		list_for_each_entry(p, &pwm_lookup_list, list) {
+			match = 0;
 
-	list_for_each_entry(p, &pwm_lookup_list, list) {
-		match = 0;
+			if (p->dev_id) {
+				if (!dev_id || strcmp(p->dev_id, dev_id))
+					continue;
 
-		if (p->dev_id) {
-			if (!dev_id || strcmp(p->dev_id, dev_id))
-				continue;
+				match += 2;
+			}
 
-			match += 2;
+			if (p->con_id) {
+				if (!con_id || strcmp(p->con_id, con_id))
+					continue;
+
+				match += 1;
+			}
+
+			if (match > best) {
+				chosen = p;
+
+				if (match != 3)
+					best = match;
+				else
+					break;
+			}
 		}
 
-		if (p->con_id) {
-			if (!con_id || strcmp(p->con_id, con_id))
-				continue;
-
-			match += 1;
-		}
-
-		if (match > best) {
-			chosen = p;
-
-			if (match != 3)
-				best = match;
-			else
-				break;
-		}
-	}
-
-	mutex_unlock(&pwm_lookup_lock);
-
 	if (!chosen)
 		return ERR_PTR(-ENODEV);
 
-- 
2.43.0

Re: [PATCH 0/3] pwm: Use guards instead of explicity mutex_lock + mutex_unlock

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

Hello,

On Thu, Jun 27, 2024 at 10:31:18PM +0200, Uwe Kleine-König wrote:
> I consider this a nice cleanup. It makes the code more compact and it's less
> prone to error, because you cannot forget an unlock in an error path.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (3):
>   pwm: Use guards for pwm_lock instead of explicity mutex_lock + mutex_unlock
>   pwm: Use guards for export->lock instead of explicity mutex_lock + mutex_unlock
>   pwm: Use guards for pwm_lookup_lock instead of explicity mutex_lock + mutex_unlock

I applied the series to my pwm/for-next branch.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]