[PATCH 1/2] gpio: sim: use sysfs_streq() and avoid an strdup()

[PATCH 1/2] gpio: sim: use sysfs_streq() and avoid an strdup()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

When comparing strings passed to us from configfs, we can pass the page
argument directly to sysfs_streq() and avoid manual string trimming.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-sim.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 8b49b0abacd5..dc4097dc0fbc 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -1272,7 +1272,6 @@ gpio_sim_hog_config_direction_store(struct config_item *item,
 {
 	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
 	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
-	char *trimmed;
 	int dir;
 
 	mutex_lock(&dev->lock);
@@ -1282,23 +1281,15 @@ gpio_sim_hog_config_direction_store(struct config_item *item,
 		return -EBUSY;
 	}
 
-	trimmed = gpio_sim_strdup_trimmed(page, count);
-	if (!trimmed) {
-		mutex_unlock(&dev->lock);
-		return -ENOMEM;
-	}
-
-	if (strcmp(trimmed, "input") == 0)
+	if (sysfs_streq(page, "input"))
 		dir = GPIOD_IN;
-	else if (strcmp(trimmed, "output-high") == 0)
+	else if (sysfs_streq(page, "output-high"))
 		dir = GPIOD_OUT_HIGH;
-	else if (strcmp(trimmed, "output-low") == 0)
+	else if (sysfs_streq(page, "output-low"))
 		dir = GPIOD_OUT_LOW;
 	else
 		dir = -EINVAL;
 
-	kfree(trimmed);
-
 	if (dir < 0) {
 		mutex_unlock(&dev->lock);
 		return dir;
-- 
2.39.2

[PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use macros defined in linux/cleanup.h to automate resource lifetime
control in the gpio-simulator.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-sim.c | 224 ++++++++++++++--------------------------
 1 file changed, 79 insertions(+), 145 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index dc4097dc0fbc..715e79dc3978 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -8,6 +8,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/bitmap.h>
+#include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/configfs.h>
 #include <linux/device.h>
@@ -68,7 +69,7 @@ static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
 	gc = &chip->gc;
 	desc = &gc->gpiodev->descs[offset];
 
-	mutex_lock(&chip->lock);
+	guard(mutex)(&chip->lock);
 
 	if (test_bit(FLAG_REQUESTED, &desc->flags) &&
 	    !test_bit(FLAG_IS_OUT, &desc->flags)) {
@@ -104,7 +105,6 @@ static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
 
 set_pull:
 	__assign_bit(offset, chip->pull_map, value);
-	mutex_unlock(&chip->lock);
 	return 0;
 }
 
@@ -113,9 +113,8 @@ static int gpio_sim_get(struct gpio_chip *gc, unsigned int offset)
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 	int ret;
 
-	mutex_lock(&chip->lock);
-	ret = !!test_bit(offset, chip->value_map);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		ret = !!test_bit(offset, chip->value_map);
 
 	return ret;
 }
@@ -124,9 +123,8 @@ static void gpio_sim_set(struct gpio_chip *gc, unsigned int offset, int value)
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
-	mutex_lock(&chip->lock);
-	__assign_bit(offset, chip->value_map, value);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		__assign_bit(offset, chip->value_map, value);
 }
 
 static int gpio_sim_get_multiple(struct gpio_chip *gc,
@@ -134,9 +132,8 @@ static int gpio_sim_get_multiple(struct gpio_chip *gc,
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
-	mutex_lock(&chip->lock);
-	bitmap_replace(bits, bits, chip->value_map, mask, gc->ngpio);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		bitmap_replace(bits, bits, chip->value_map, mask, gc->ngpio);
 
 	return 0;
 }
@@ -146,9 +143,9 @@ static void gpio_sim_set_multiple(struct gpio_chip *gc,
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
-	mutex_lock(&chip->lock);
-	bitmap_replace(chip->value_map, chip->value_map, bits, mask, gc->ngpio);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		bitmap_replace(chip->value_map, chip->value_map, bits, mask,
+			       gc->ngpio);
 }
 
 static int gpio_sim_direction_output(struct gpio_chip *gc,
@@ -156,10 +153,10 @@ static int gpio_sim_direction_output(struct gpio_chip *gc,
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
-	mutex_lock(&chip->lock);
-	__clear_bit(offset, chip->direction_map);
-	__assign_bit(offset, chip->value_map, value);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock) {
+		__clear_bit(offset, chip->direction_map);
+		__assign_bit(offset, chip->value_map, value);
+	}
 
 	return 0;
 }
@@ -168,9 +165,8 @@ static int gpio_sim_direction_input(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
-	mutex_lock(&chip->lock);
-	__set_bit(offset, chip->direction_map);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		__set_bit(offset, chip->direction_map);
 
 	return 0;
 }
@@ -180,9 +176,8 @@ static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 	int direction;
 
-	mutex_lock(&chip->lock);
-	direction = !!test_bit(offset, chip->direction_map);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		direction = !!test_bit(offset, chip->direction_map);
 
 	return direction ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
 }
@@ -215,9 +210,9 @@ static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
-	mutex_lock(&chip->lock);
-	__assign_bit(offset, chip->value_map, !!test_bit(offset, chip->pull_map));
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		__assign_bit(offset, chip->value_map,
+			     !!test_bit(offset, chip->pull_map));
 }
 
 static ssize_t gpio_sim_sysfs_val_show(struct device *dev,
@@ -227,9 +222,8 @@ static ssize_t gpio_sim_sysfs_val_show(struct device *dev,
 	struct gpio_sim_chip *chip = dev_get_drvdata(dev);
 	int val;
 
-	mutex_lock(&chip->lock);
-	val = !!test_bit(line_attr->offset, chip->value_map);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		val = !!test_bit(line_attr->offset, chip->value_map);
 
 	return sysfs_emit(buf, "%d\n", val);
 }
@@ -258,9 +252,8 @@ static ssize_t gpio_sim_sysfs_pull_show(struct device *dev,
 	struct gpio_sim_chip *chip = dev_get_drvdata(dev);
 	int pull;
 
-	mutex_lock(&chip->lock);
-	pull = !!test_bit(line_attr->offset, chip->pull_map);
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock)
+		pull = !!test_bit(line_attr->offset, chip->pull_map);
 
 	return sysfs_emit(buf, "%s\n", gpio_sim_sysfs_pull_strings[pull]);
 }
@@ -661,13 +654,13 @@ static ssize_t gpio_sim_device_config_dev_name_show(struct config_item *item,
 	struct platform_device *pdev;
 	int ret;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
+
 	pdev = dev->pdev;
 	if (pdev)
 		ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
 	else
 		ret = sprintf(page, "gpio-sim.%d\n", dev->id);
-	mutex_unlock(&dev->lock);
 
 	return ret;
 }
@@ -680,9 +673,8 @@ gpio_sim_device_config_live_show(struct config_item *item, char *page)
 	struct gpio_sim_device *dev = to_gpio_sim_device(item);
 	bool live;
 
-	mutex_lock(&dev->lock);
-	live = gpio_sim_device_is_live_unlocked(dev);
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		live = gpio_sim_device_is_live_unlocked(dev);
 
 	return sprintf(page, "%c\n", live ? '1' : '0');
 }
@@ -837,8 +829,8 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 {
 	struct property_entry properties[GPIO_SIM_PROP_MAX];
 	unsigned int prop_idx = 0, line_names_size = 0;
+	char **line_names __free(kfree) = NULL;
 	struct fwnode_handle *swnode;
-	char **line_names;
 
 	memset(properties, 0, sizeof(properties));
 
@@ -858,7 +850,6 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 						line_names, line_names_size);
 
 	swnode = fwnode_create_software_node(properties, parent);
-	kfree(line_names);
 	return swnode;
 }
 
@@ -984,7 +975,7 @@ gpio_sim_device_config_live_store(struct config_item *item,
 	if (ret)
 		return ret;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
 	if ((!live && !gpio_sim_device_is_live_unlocked(dev)) ||
 	    (live && gpio_sim_device_is_live_unlocked(dev)))
@@ -994,8 +985,6 @@ gpio_sim_device_config_live_store(struct config_item *item,
 	else
 		gpio_sim_device_deactivate_unlocked(dev);
 
-	mutex_unlock(&dev->lock);
-
 	return ret ?: count;
 }
 
@@ -1034,13 +1023,13 @@ static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item,
 	struct gpio_sim_chip_name_ctx ctx = { bank->swnode, page };
 	int ret;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
+
 	if (gpio_sim_device_is_live_unlocked(dev))
 		ret = device_for_each_child(&dev->pdev->dev, &ctx,
 					    gpio_sim_emit_chip_name);
 	else
 		ret = sprintf(page, "none\n");
-	mutex_unlock(&dev->lock);
 
 	return ret;
 }
@@ -1054,9 +1043,8 @@ gpio_sim_bank_config_label_show(struct config_item *item, char *page)
 	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
 	int ret;
 
-	mutex_lock(&dev->lock);
-	ret = sprintf(page, "%s\n", bank->label ?: "");
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		ret = sprintf(page, "%s\n", bank->label ?: "");
 
 	return ret;
 }
@@ -1068,23 +1056,18 @@ static ssize_t gpio_sim_bank_config_label_store(struct config_item *item,
 	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
 	char *trimmed;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
-	if (gpio_sim_device_is_live_unlocked(dev)) {
-		mutex_unlock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
 		return -EBUSY;
-	}
 
 	trimmed = gpio_sim_strdup_trimmed(page, count);
-	if (!trimmed) {
-		mutex_unlock(&dev->lock);
+	if (!trimmed)
 		return -ENOMEM;
-	}
 
 	kfree(bank->label);
 	bank->label = trimmed;
 
-	mutex_unlock(&dev->lock);
 	return count;
 }
 
@@ -1097,9 +1080,8 @@ gpio_sim_bank_config_num_lines_show(struct config_item *item, char *page)
 	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
 	int ret;
 
-	mutex_lock(&dev->lock);
-	ret = sprintf(page, "%u\n", bank->num_lines);
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		ret = sprintf(page, "%u\n", bank->num_lines);
 
 	return ret;
 }
@@ -1120,16 +1102,13 @@ gpio_sim_bank_config_num_lines_store(struct config_item *item,
 	if (num_lines == 0)
 		return -EINVAL;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
-	if (gpio_sim_device_is_live_unlocked(dev)) {
-		mutex_unlock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
 		return -EBUSY;
-	}
 
 	bank->num_lines = num_lines;
 
-	mutex_unlock(&dev->lock);
 	return count;
 }
 
@@ -1149,9 +1128,8 @@ gpio_sim_line_config_name_show(struct config_item *item, char *page)
 	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
 	int ret;
 
-	mutex_lock(&dev->lock);
-	ret = sprintf(page, "%s\n", line->name ?: "");
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		ret = sprintf(page, "%s\n", line->name ?: "");
 
 	return ret;
 }
@@ -1163,24 +1141,18 @@ static ssize_t gpio_sim_line_config_name_store(struct config_item *item,
 	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
 	char *trimmed;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
-	if (gpio_sim_device_is_live_unlocked(dev)) {
-		mutex_unlock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
 		return -EBUSY;
-	}
 
 	trimmed = gpio_sim_strdup_trimmed(page, count);
-	if (!trimmed) {
-		mutex_unlock(&dev->lock);
+	if (!trimmed)
 		return -ENOMEM;
-	}
 
 	kfree(line->name);
 	line->name = trimmed;
 
-	mutex_unlock(&dev->lock);
-
 	return count;
 }
 
@@ -1198,9 +1170,8 @@ static ssize_t gpio_sim_hog_config_name_show(struct config_item *item,
 	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
 	int ret;
 
-	mutex_lock(&dev->lock);
-	ret = sprintf(page, "%s\n", hog->name ?: "");
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		ret = sprintf(page, "%s\n", hog->name ?: "");
 
 	return ret;
 }
@@ -1212,24 +1183,18 @@ static ssize_t gpio_sim_hog_config_name_store(struct config_item *item,
 	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
 	char *trimmed;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
-	if (gpio_sim_device_is_live_unlocked(dev)) {
-		mutex_unlock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
 		return -EBUSY;
-	}
 
 	trimmed = gpio_sim_strdup_trimmed(page, count);
-	if (!trimmed) {
-		mutex_unlock(&dev->lock);
+	if (!trimmed)
 		return -ENOMEM;
-	}
 
 	kfree(hog->name);
 	hog->name = trimmed;
 
-	mutex_unlock(&dev->lock);
-
 	return count;
 }
 
@@ -1243,9 +1208,8 @@ static ssize_t gpio_sim_hog_config_direction_show(struct config_item *item,
 	char *repr;
 	int dir;
 
-	mutex_lock(&dev->lock);
-	dir = hog->dir;
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		dir = hog->dir;
 
 	switch (dir) {
 	case GPIOD_IN:
@@ -1274,12 +1238,10 @@ gpio_sim_hog_config_direction_store(struct config_item *item,
 	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
 	int dir;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
-	if (gpio_sim_device_is_live_unlocked(dev)) {
-		mutex_unlock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
 		return -EBUSY;
-	}
 
 	if (sysfs_streq(page, "input"))
 		dir = GPIOD_IN;
@@ -1288,17 +1250,10 @@ gpio_sim_hog_config_direction_store(struct config_item *item,
 	else if (sysfs_streq(page, "output-low"))
 		dir = GPIOD_OUT_LOW;
 	else
-		dir = -EINVAL;
-
-	if (dir < 0) {
-		mutex_unlock(&dev->lock);
-		return dir;
-	}
+		return -EINVAL;
 
 	hog->dir = dir;
 
-	mutex_unlock(&dev->lock);
-
 	return count;
 }
 
@@ -1316,9 +1271,8 @@ static void gpio_sim_hog_config_item_release(struct config_item *item)
 	struct gpio_sim_line *line = hog->parent;
 	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
 
-	mutex_lock(&dev->lock);
-	line->hog = NULL;
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		line->hog = NULL;
 
 	kfree(hog->name);
 	kfree(hog);
@@ -1344,13 +1298,11 @@ gpio_sim_line_config_make_hog_item(struct config_group *group, const char *name)
 	if (strcmp(name, "hog") != 0)
 		return ERR_PTR(-EINVAL);
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
 	hog = kzalloc(sizeof(*hog), GFP_KERNEL);
-	if (!hog) {
-		mutex_unlock(&dev->lock);
+	if (!hog)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	config_item_init_type_name(&hog->item, name,
 				   &gpio_sim_hog_config_type);
@@ -1360,8 +1312,6 @@ gpio_sim_line_config_make_hog_item(struct config_group *group, const char *name)
 	hog->parent = line;
 	line->hog = hog;
 
-	mutex_unlock(&dev->lock);
-
 	return &hog->item;
 }
 
@@ -1370,9 +1320,8 @@ static void gpio_sim_line_config_group_release(struct config_item *item)
 	struct gpio_sim_line *line = to_gpio_sim_line(item);
 	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
 
-	mutex_lock(&dev->lock);
-	list_del(&line->siblings);
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		list_del(&line->siblings);
 
 	kfree(line->name);
 	kfree(line);
@@ -1407,18 +1356,14 @@ gpio_sim_bank_config_make_line_group(struct config_group *group,
 	if (ret != 1 || nchar != strlen(name))
 		return ERR_PTR(-EINVAL);
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
-	if (gpio_sim_device_is_live_unlocked(dev)) {
-		mutex_unlock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
 		return ERR_PTR(-EBUSY);
-	}
 
 	line = kzalloc(sizeof(*line), GFP_KERNEL);
-	if (!line) {
-		mutex_unlock(&dev->lock);
+	if (!line)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	config_group_init_type_name(&line->group, name,
 				    &gpio_sim_line_config_type);
@@ -1427,8 +1372,6 @@ gpio_sim_bank_config_make_line_group(struct config_group *group,
 	line->offset = offset;
 	list_add_tail(&line->siblings, &bank->line_list);
 
-	mutex_unlock(&dev->lock);
-
 	return &line->group;
 }
 
@@ -1437,9 +1380,8 @@ static void gpio_sim_bank_config_group_release(struct config_item *item)
 	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
 	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
 
-	mutex_lock(&dev->lock);
-	list_del(&bank->siblings);
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock)
+		list_del(&bank->siblings);
 
 	kfree(bank->label);
 	kfree(bank);
@@ -1467,18 +1409,14 @@ gpio_sim_device_config_make_bank_group(struct config_group *group,
 	struct gpio_sim_device *dev = to_gpio_sim_device(&group->cg_item);
 	struct gpio_sim_bank *bank;
 
-	mutex_lock(&dev->lock);
+	guard(mutex)(&dev->lock);
 
-	if (gpio_sim_device_is_live_unlocked(dev)) {
-		mutex_unlock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
 		return ERR_PTR(-EBUSY);
-	}
 
 	bank = kzalloc(sizeof(*bank), GFP_KERNEL);
-	if (!bank) {
-		mutex_unlock(&dev->lock);
+	if (!bank)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	config_group_init_type_name(&bank->group, name,
 				    &gpio_sim_bank_config_group_type);
@@ -1487,8 +1425,6 @@ gpio_sim_device_config_make_bank_group(struct config_group *group,
 	INIT_LIST_HEAD(&bank->line_list);
 	list_add_tail(&bank->siblings, &dev->bank_list);
 
-	mutex_unlock(&dev->lock);
-
 	return &bank->group;
 }
 
@@ -1496,10 +1432,10 @@ static void gpio_sim_device_config_group_release(struct config_item *item)
 {
 	struct gpio_sim_device *dev = to_gpio_sim_device(item);
 
-	mutex_lock(&dev->lock);
-	if (gpio_sim_device_is_live_unlocked(dev))
-		gpio_sim_device_deactivate_unlocked(dev);
-	mutex_unlock(&dev->lock);
+	scoped_guard(mutex, &dev->lock) {
+		if (gpio_sim_device_is_live_unlocked(dev))
+			gpio_sim_device_deactivate_unlocked(dev);
+	}
 
 	mutex_destroy(&dev->lock);
 	ida_free(&gpio_sim_ida, dev->id);
@@ -1524,7 +1460,7 @@ static const struct config_item_type gpio_sim_device_config_group_type = {
 static struct config_group *
 gpio_sim_config_make_device_group(struct config_group *group, const char *name)
 {
-	struct gpio_sim_device *dev;
+	struct gpio_sim_device *dev __free(kfree) = NULL;
 	int id;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
@@ -1532,10 +1468,8 @@ gpio_sim_config_make_device_group(struct config_group *group, const char *name)
 		return ERR_PTR(-ENOMEM);
 
 	id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);
-	if (id < 0) {
-		kfree(dev);
+	if (id < 0)
 		return ERR_PTR(id);
-	}
 
 	config_group_init_type_name(&dev->group, name,
 				    &gpio_sim_device_config_group_type);
@@ -1546,7 +1480,7 @@ gpio_sim_config_make_device_group(struct config_group *group, const char *name)
 	dev->bus_notifier.notifier_call = gpio_sim_bus_notifier_call;
 	init_completion(&dev->probe_completion);
 
-	return &dev->group;
+	return &no_free_ptr(dev)->group;
 }
 
 static struct configfs_group_operations gpio_sim_config_group_ops = {
-- 
2.39.2

Re: [PATCH 1/2] gpio: sim: use sysfs_streq() and avoid an strdup()

[Andy Shevchenko]

On Wed, Aug 09, 2023 at 03:14:41PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> When comparing strings passed to us from configfs, we can pass the page
> argument directly to sysfs_streq() and avoid manual string trimming.

Good one!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Andy Shevchenko]

On Wed, Aug 09, 2023 at 03:14:42PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use macros defined in linux/cleanup.h to automate resource lifetime
> control in the gpio-simulator.

gpio-sim ?

...

> -	mutex_lock(&chip->lock);
> +	guard(mutex)(&chip->lock);

I hoped to see somehing like

	guard_mutex(...);

But looking into cleanup.h it seems to me that the lock itself on GPIO library
can be defined with respective class, no?

...

> +	scoped_guard(mutex, &chip->lock)
> +		bitmap_replace(chip->value_map, chip->value_map, bits, mask,
> +			       gc->ngpio);

Perhaps with {} ?

...

>  	int ret;
>  
> -	mutex_lock(&dev->lock);
> +	guard(mutex)(&dev->lock);
> +
>  	pdev = dev->pdev;
>  	if (pdev)
>  		ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
>  	else
>  		ret = sprintf(page, "gpio-sim.%d\n", dev->id);
> -	mutex_unlock(&dev->lock);
>  
>  	return ret;

Now can be

	if (...)
		return ...
	else // if you wish (not needed)
		return ...

...

>  	int ret;
>  
> -	mutex_lock(&dev->lock);
> +	guard(mutex)(&dev->lock);
> +
>  	if (gpio_sim_device_is_live_unlocked(dev))
>  		ret = device_for_each_child(&dev->pdev->dev, &ctx,
>  					    gpio_sim_emit_chip_name);
>  	else
>  		ret = sprintf(page, "none\n");
> -	mutex_unlock(&dev->lock);
>  
>  	return ret;

As per above. And may be other functions as well.

...

>  	int ret;
>  
> -	mutex_lock(&dev->lock);
> -	ret = sprintf(page, "%s\n", line->name ?: "");
> -	mutex_unlock(&dev->lock);
> +	scoped_guard(mutex, &dev->lock)
> +		ret = sprintf(page, "%s\n", line->name ?: "");
>  
>  	return ret;

Why not

	guard(...);
	return sprintf(...);

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Bartosz Golaszewski]

On Thu, Aug 10, 2023 at 4:42 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Aug 09, 2023 at 03:14:42PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use macros defined in linux/cleanup.h to automate resource lifetime
> > control in the gpio-simulator.
>
> gpio-sim ?
>

Meh, if you insist...

> ...
>
> > -     mutex_lock(&chip->lock);
> > +     guard(mutex)(&chip->lock);
>
> I hoped to see somehing like
>
>         guard_mutex(...);
>
> But looking into cleanup.h it seems to me that the lock itself on GPIO library
> can be defined with respective class, no?
>

Why though? This is perfectly clear and concise as it is. It's similar
to going bare mutex_lock() everywhere instead of wrapping it with
foo_lock() which requires you to go and check what you're locking.

> ...
>
> > +     scoped_guard(mutex, &chip->lock)
> > +             bitmap_replace(chip->value_map, chip->value_map, bits, mask,
> > +                            gc->ngpio);
>
> Perhaps with {} ?
>

This scoped_guard() thing is in essence a for loop, so I believe
kernel coding style applies and a single statement doesn't require a
{}.

> ...
>
> >       int ret;
> >
> > -     mutex_lock(&dev->lock);
> > +     guard(mutex)(&dev->lock);
> > +
> >       pdev = dev->pdev;
> >       if (pdev)
> >               ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
> >       else
> >               ret = sprintf(page, "gpio-sim.%d\n", dev->id);
> > -     mutex_unlock(&dev->lock);
> >
> >       return ret;
>
> Now can be
>
>         if (...)
>                 return ...
>         else // if you wish (not needed)
>                 return ...
>
> ...
>
> >       int ret;
> >
> > -     mutex_lock(&dev->lock);
> > +     guard(mutex)(&dev->lock);
> > +
> >       if (gpio_sim_device_is_live_unlocked(dev))
> >               ret = device_for_each_child(&dev->pdev->dev, &ctx,
> >                                           gpio_sim_emit_chip_name);
> >       else
> >               ret = sprintf(page, "none\n");
> > -     mutex_unlock(&dev->lock);
> >
> >       return ret;
>
> As per above. And may be other functions as well.
>

Sure.

> ...
>
> >       int ret;
> >
> > -     mutex_lock(&dev->lock);
> > -     ret = sprintf(page, "%s\n", line->name ?: "");
> > -     mutex_unlock(&dev->lock);
> > +     scoped_guard(mutex, &dev->lock)
> > +             ret = sprintf(page, "%s\n", line->name ?: "");
> >
> >       return ret;
>
> Why not
>
>         guard(...);
>         return sprintf(...);
>
> ?

I'll change that too.

Bart

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Dan Carpenter]

Hi Bartosz,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-sim-simplify-code-with-cleanup-helpers/20230809-211601
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20230809131442.25524-2-brgl%40bgdev.pl
patch subject: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers
config: i386-randconfig-m021-20230809 (https://download.01.org/0day-ci/archive/20230811/202308110253.R2TUMfFr-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230811/202308110253.R2TUMfFr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202308110253.R2TUMfFr-lkp@intel.com/

smatch warnings:
drivers/gpio/gpio-sim.c:1472 gpio_sim_config_make_device_group() warn: possible memory leak of 'dev'

vim +/dev +1472 drivers/gpio/gpio-sim.c

cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1460  static struct config_group *
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1461  gpio_sim_config_make_device_group(struct config_group *group, const char *name)
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1462  {
c7a663cdcfc698 Bartosz Golaszewski 2023-08-09  1463  	struct gpio_sim_device *dev __free(kfree) = NULL;
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1464  	int id;
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1465  
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1466  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1467  	if (!dev)
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1468  		return ERR_PTR(-ENOMEM);
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1469  
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1470  	id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);
c7a663cdcfc698 Bartosz Golaszewski 2023-08-09  1471  	if (id < 0)
cb8c474e79be45 Bartosz Golaszewski 2021-12-07 @1472  		return ERR_PTR(id);

kfree(dev);

cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1473  
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1474  	config_group_init_type_name(&dev->group, name,
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1475  				    &gpio_sim_device_config_group_type);
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1476  	dev->id = id;
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1477  	mutex_init(&dev->lock);
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1478  	INIT_LIST_HEAD(&dev->bank_list);
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1479  
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1480  	dev->bus_notifier.notifier_call = gpio_sim_bus_notifier_call;
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1481  	init_completion(&dev->probe_completion);
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1482  
c7a663cdcfc698 Bartosz Golaszewski 2023-08-09  1483  	return &no_free_ptr(dev)->group;
cb8c474e79be45 Bartosz Golaszewski 2021-12-07  1484  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Andy Shevchenko]

On Thu, Aug 10, 2023 at 09:04:12PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 10, 2023 at 4:42 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Aug 09, 2023 at 03:14:42PM +0200, Bartosz Golaszewski wrote:

...

> > > +     scoped_guard(mutex, &chip->lock)
> > > +             bitmap_replace(chip->value_map, chip->value_map, bits, mask,
> > > +                            gc->ngpio);
> >
> > Perhaps with {} ?
> 
> This scoped_guard() thing is in essence a for loop, so I believe
> kernel coding style applies and a single statement doesn't require a
> {}.

You have two lines (or single wrapped line). I found to read it better with {}.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Andy Shevchenko]

On Fri, Aug 11, 2023 at 08:20:11AM +0300, Dan Carpenter wrote:

> smatch warnings:
> drivers/gpio/gpio-sim.c:1472 gpio_sim_config_make_device_group() warn: possible memory leak of 'dev'

Isn't smatch a bit dumb about cleanup.h?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Dan Carpenter]

On Fri, Aug 11, 2023 at 12:14:52PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 11, 2023 at 08:20:11AM +0300, Dan Carpenter wrote:
> 
> > smatch warnings:
> > drivers/gpio/gpio-sim.c:1472 gpio_sim_config_make_device_group() warn: possible memory leak of 'dev'
> 
> Isn't smatch a bit dumb about cleanup.h?
> 

Aw.  Crud.  I hadn't seen that this was a cleanup.h thing.

I did do some work to suppoort cleanup.h but probably it will take a
while to work out the kinks.  Let me figure this out.

regards,
dan carpenter

Re: [PATCH 1/2] gpio: sim: use sysfs_streq() and avoid an strdup()

[Bartosz Golaszewski]

On Wed, Aug 9, 2023 at 3:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> When comparing strings passed to us from configfs, we can pass the page
> argument directly to sysfs_streq() and avoid manual string trimming.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

I applied this one, I'll send a v2 for other one.

Bart

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Bartosz Golaszewski]

On Fri, Aug 11, 2023 at 11:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 10, 2023 at 09:04:12PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 10, 2023 at 4:42 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Aug 09, 2023 at 03:14:42PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > > +     scoped_guard(mutex, &chip->lock)
> > > > +             bitmap_replace(chip->value_map, chip->value_map, bits, mask,
> > > > +                            gc->ngpio);
> > >
> > > Perhaps with {} ?
> >
> > This scoped_guard() thing is in essence a for loop, so I believe
> > kernel coding style applies and a single statement doesn't require a
> > {}.
>
> You have two lines (or single wrapped line). I found to read it better with {}.
>

It's just a broken line, not a compound statement. Matter of personal taste. :)

Bart

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Linus Walleij]

On Wed, Aug 9, 2023 at 3:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Use macros defined in linux/cleanup.h to automate resource lifetime
> control in the gpio-simulator.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
(...)
> -       mutex_lock(&chip->lock);
> +       guard(mutex)(&chip->lock);
(...)
> -       mutex_lock(&chip->lock);
> -       ret = !!test_bit(offset, chip->value_map);
> -       mutex_unlock(&chip->lock);
> +       scoped_guard(mutex, &chip->lock)
> +               ret = !!test_bit(offset, chip->value_map);

This is really neat. When I grep:ed around in linux-next this seemed like
the first user of the scoped guards, so maybe Peter Z want to take a look?
I bet there is other code using it coming for this next merge window as
well, but this is really the first that will land in linux-next as it seems.

It looks good to me FWIW:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Peter Zijlstra]

On Tue, Aug 15, 2023 at 10:04:32AM +0200, Linus Walleij wrote:
> On Wed, Aug 9, 2023 at 3:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use macros defined in linux/cleanup.h to automate resource lifetime
> > control in the gpio-simulator.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> (...)
> > -       mutex_lock(&chip->lock);
> > +       guard(mutex)(&chip->lock);
> (...)
> > -       mutex_lock(&chip->lock);
> > -       ret = !!test_bit(offset, chip->value_map);
> > -       mutex_unlock(&chip->lock);
> > +       scoped_guard(mutex, &chip->lock)
> > +               ret = !!test_bit(offset, chip->value_map);
> 
> This is really neat. When I grep:ed around in linux-next this seemed like
> the first user of the scoped guards, so maybe Peter Z want to take a look?

Looks about right.

> I bet there is other code using it coming for this next merge window as
> well, but this is really the first that will land in linux-next as it seems.

There's more people starting to use it indeed.

There should be some in tip/sched/core as well. I have more pending, but
got side-tracked a bit with other things :/

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Andy Shevchenko]

On Tue, Aug 15, 2023 at 05:52:53PM +0200, Peter Zijlstra wrote:
> On Tue, Aug 15, 2023 at 10:04:32AM +0200, Linus Walleij wrote:
> > On Wed, Aug 9, 2023 at 3:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:


> > > -       mutex_lock(&chip->lock);
> > > +       guard(mutex)(&chip->lock);

> Looks about right.

Btw, why don't we have something like

	guard_mutex()

to be used as

	guard_mutex(&chip->lock);

Moreover, maybe some macro that can predict the API call from the type of
the parameter?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Peter Zijlstra]

On Tue, Aug 15, 2023 at 06:58:10PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 15, 2023 at 05:52:53PM +0200, Peter Zijlstra wrote:
> > On Tue, Aug 15, 2023 at 10:04:32AM +0200, Linus Walleij wrote:
> > > On Wed, Aug 9, 2023 at 3:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> 
> > > > -       mutex_lock(&chip->lock);
> > > > +       guard(mutex)(&chip->lock);
> 
> > Looks about right.
> 
> Btw, why don't we have something like
> 
> 	guard_mutex()
> 
> to be used as
> 
> 	guard_mutex(&chip->lock);

Because this way I can write:

DEFINE_LOCK_GUARD_1(rq_lock_irqsave, struct rq,
		    rq_lock_irqsave(_T->lock, &_T->rf),
		    rq_unlock_irqrestore(_T->lock, &_T->rf),
		    struct rq_flags rf);

And have:

	guard(rq_lock_irqsave)(rq);

and

	scoped_guard (rq_lock_irqsave, rq) {
	}

just work.

And if you look in tip/sched/core, you'll find exactly this.

Or look here:

  https://lkml.kernel.org/r/20230612090713.652690195@infradead.org

for a bunch more examples -- I've wanted to get more of that merged, but
alas, only 24h in a day and life got in the way. Defining local guard
types is very useful.

> Moreover, maybe some macro that can predict the API call from the type of
> the parameter?

The whole type inferrence in C is not extensible. That is, you get to
write a single _Generic() statement, and every case that is included in
it will work, but the moment you use a new type, one that is not
included in your giant _Generic() statement, you're out of luck.

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Peter Zijlstra]

On Tue, Aug 15, 2023 at 10:31:17PM +0200, Peter Zijlstra wrote:

> > Moreover, maybe some macro that can predict the API call from the type of
> > the parameter?
> 
> The whole type inferrence in C is not extensible. That is, you get to
> write a single _Generic() statement, and every case that is included in
> it will work, but the moment you use a new type, one that is not
> included in your giant _Generic() statement, you're out of luck.

Additionally, spinlock_t, does that map to spinlock, spinlock_irq or
spinlock_irqsave ?

Re: [PATCH 2/2] gpio: sim: simplify code with cleanup helpers

[Andy Shevchenko]

On Wed, Aug 16, 2023 at 02:47:57PM +0200, Peter Zijlstra wrote:
> On Tue, Aug 15, 2023 at 10:31:17PM +0200, Peter Zijlstra wrote:
> 
> > > Moreover, maybe some macro that can predict the API call from the type of
> > > the parameter?
> > 
> > The whole type inferrence in C is not extensible. That is, you get to
> > write a single _Generic() statement, and every case that is included in
> > it will work, but the moment you use a new type, one that is not
> > included in your giant _Generic() statement, you're out of luck.
> 
> Additionally, spinlock_t, does that map to spinlock, spinlock_irq or
> spinlock_irqsave ?

Thank you for a good explanation.

-- 
With Best Regards,
Andy Shevchenko