gpio: sim: simplify gpio_sim_device_config_live_store()

PATCH Applied Bartosz Golaszewski 2 participants 8 messages Aug 11, 2023 – Aug 12, 2023 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2023-08-11 13:14 UTC #0 lore raw

PATCH 1/2 gpio: sim: simplify gpio_sim_device_config_live_store()

From: Bartosz Golaszewski <[email protected]>

Simplify the logic when checking the current live value against the user input.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
This is the same as what Andy suggested for gpio-consumer. Let's save a line.

 drivers/gpio/gpio-sim.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/gpio/gpio-sim.c +1 -2

index dc4097dc0fbc..0177b41e0d72 100644
@@ -986,8 +986,7 @@ gpio_sim_device_config_live_store(struct config_item *item,
 
 	mutex_lock(&dev->lock);
 
-	if ((!live && !gpio_sim_device_is_live_unlocked(dev)) ||
-	    (live && gpio_sim_device_is_live_unlocked(dev)))
+	if (live == gpio_sim_device_is_live_unlocked(dev))
 		ret = -EPERM;
 	else if (live)
 		ret = gpio_sim_device_activate_unlocked(dev);

Signature · 1 line

2.39.2

Bartosz Golaszewski

author

2023-08-11 13:14 UTC #1 lore raw

PATCH v2 2/2 gpio: sim: simplify code with cleanup helpers

From: Bartosz Golaszewski <[email protected]>

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

Signed-off-by Bartosz Golaszewski <[email protected]>

---
v1 -> v2:
- reorder code to return earlier from functions whenever possible, for instance when the last line is just returning a value from snprintf(), this saves us int ret; local variables
- tweak the commit message

 drivers/gpio/gpio-sim.c | 250 ++++++++++++++--------------------------
 1 file changed, 85 insertions(+), 165 deletions(-)

drivers/gpio/gpio-sim.c +85 ~~-165~~

index 0177b41e0d72..5f6c2826c70e 100644
@@ -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]);
 }

@@ -659,17 +652,14 @@ static ssize_t gpio_sim_device_config_dev_name_show(struct config_item *item,
 {
 	struct gpio_sim_device *dev = to_gpio_sim_device(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 sprintf(page, "%s\n", dev_name(&pdev->dev));
 
-	return ret;
+	return sprintf(page, "gpio-sim.%d\n", dev->id);
 }
 
 CONFIGFS_ATTR_RO(gpio_sim_device_config_, dev_name);

@@ -680,9 +670,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 +826,7 @@ 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;
-	struct fwnode_handle *swnode;
-	char **line_names;
+	char **line_names __free(kfree) = NULL;
 
 	memset(properties, 0, sizeof(properties));

@@ -857,9 +845,7 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 						"gpio-line-names",
 						line_names, line_names_size);
 
-	swnode = fwnode_create_software_node(properties, parent);
-	kfree(line_names);
-	return swnode;
+	return fwnode_create_software_node(properties, parent);
 }
 
 static void gpio_sim_remove_swnode_recursive(struct fwnode_handle *swnode)

@@ -984,7 +970,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))
 		ret = -EPERM;

@@ -993,8 +979,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;
 }

@@ -1031,17 +1015,14 @@ static ssize_t gpio_sim_bank_config_chip_name_show(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);
 	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 device_for_each_child(&dev->pdev->dev, &ctx,
+					     gpio_sim_emit_chip_name);
 
-	return ret;
+	return sprintf(page, "none\n");
 }
 
 CONFIGFS_ATTR_RO(gpio_sim_bank_config_, chip_name);

@@ -1051,13 +1032,10 @@ gpio_sim_bank_config_label_show(struct config_item *item, char *page)
 {
 	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
 	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);
+	guard(mutex)(&dev->lock);
 
-	return ret;
+	return sprintf(page, "%s\n", bank->label ?: "");
 }
 
 static ssize_t gpio_sim_bank_config_label_store(struct config_item *item,

@@ -1067,23 +1045,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;
 }

@@ -1094,13 +1067,10 @@ gpio_sim_bank_config_num_lines_show(struct config_item *item, char *page)
 {
 	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
 	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);
+	guard(mutex)(&dev->lock);
 
-	return ret;
+	return sprintf(page, "%u\n", bank->num_lines);
 }
 
 static ssize_t

@@ -1119,16 +1089,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;
 }

@@ -1146,13 +1113,10 @@ gpio_sim_line_config_name_show(struct config_item *item, char *page)
 {
 	struct gpio_sim_line *line = to_gpio_sim_line(item);
 	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);
+	guard(mutex)(&dev->lock);
 
-	return ret;
+	return sprintf(page, "%s\n", line->name ?: "");
 }
 
 static ssize_t gpio_sim_line_config_name_store(struct config_item *item,

@@ -1162,24 +1126,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;
 }

@@ -1197,9 +1155,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;
 }

@@ -1211,24 +1168,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;
 }

@@ -1242,9 +1193,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:

@@ -1273,12 +1223,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;

@@ -1287,17 +1235,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;
 }

@@ -1315,9 +1256,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);

@@ -1343,13 +1283,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);

@@ -1359,8 +1297,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;
 }

@@ -1369,9 +1305,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);

@@ -1406,18 +1341,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);

@@ -1426,8 +1357,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;
 }

@@ -1436,9 +1365,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);

@@ -1466,18 +1394,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);

@@ -1486,8 +1410,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;
 }

@@ -1495,10 +1417,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);

@@ -1523,7 +1445,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);

@@ -1531,10 +1453,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);

@@ -1545,7 +1465,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 = {

Signature · 1 line

2.39.2

Andy Shevchenko

2023-08-11 14:24 UTC #2 lore raw

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

Quoted 4 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
Use macros defined in linux/cleanup.h to automate resource lifetime control in gpio-sim.

...

drivers/gpio/gpio-sim.c static int gpio_sim_get(struct gpio_chip *gc, unsigned int offset) · line 119

Quoted 10 lines

drivers/gpio/gpio-sim.c +2 -3

 	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;

Isn't the same approach applicable here?

...

Quoted 2 lines

{
struct gpio_sim_chip *chip = gpiochip_get_data(gc);

With

	unsigned long *map = ...->value_map;

drivers/gpio/gpio-sim.c static void gpio_sim_set_multiple(struct gpio_chip *gc, · line 148

Quoted 6 lines

drivers/gpio/gpio-sim.c +3 -3

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

...you can satisfy me as well :-)

		bitmap_replace(map, map, bits, mask, gc->ngpio);

Quoted 1 line

}

...

drivers/gpio/gpio-sim.c static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) · line 215

Quoted 9 lines

{

drivers/gpio/gpio-sim.c +3 -3

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

Ditto (I checked the line size).

Quoted 1 line

}

...

Quoted 8 lines

drivers/gpio/gpio-sim.c +2 -2

 	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
 	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))

drivers/gpio/gpio-sim.c static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item, · line 1025

Quoted 10 lines

drivers/gpio/gpio-sim.c +3 -6

-		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 device_for_each_child(&dev->pdev->dev, &ctx,
+					     gpio_sim_emit_chip_name);
 
-	return ret;
+	return sprintf(page, "none\n");

I looked at the original and at the change and maybe it could be done as

	struct device *parent = &dev->pdev->dev; // Naming?
	bool live;

	live = gpio_sim_device_is_live_unlocked(dev);
	if (!live)
		return sprintf(page, "none\n");

	return device_for_each_child(parent, &ctx, gpio_sim_emit_chip_name);

...

drivers/gpio/gpio-sim.c static ssize_t gpio_sim_hog_config_name_show(struct config_item *item, · line 1161

Quoted 9 lines

drivers/gpio/gpio-sim.c +2 -3

 	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;

guard() ?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-11 14:28 UTC #3 lore raw

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

Quoted 31 lines · Andy Shevchenko

Quoted 4 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Use macros defined in linux/cleanup.h to automate resource lifetime control in gpio-sim.
...
drivers/gpio/gpio-sim.c static int gpio_sim_get(struct gpio_chip *gc, unsigned int offset) · line 119
Quoted 10 lines
      struct gpio_sim_chip *chip = gpiochip_get_data(gc);
      int ret;
drivers/gpio/gpio-sim.c +2 -3
-     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;
Isn't the same approach applicable here?
...
Quoted 2 lines
 {
      struct gpio_sim_chip *chip = gpiochip_get_data(gc);
With
        unsigned long *map = ...->value_map;

Quoted 9 lines

drivers/gpio/gpio-sim.c static void gpio_sim_set_multiple(struct gpio_chip *gc, · line 148
Quoted 6 lines
drivers/gpio/gpio-sim.c +3 -3
-     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);
...you can satisfy me as well :-)

You are a tough negotiator...

Quoted 38 lines

                bitmap_replace(map, map, bits, mask, gc->ngpio);

Quoted 1 line

}

...

drivers/gpio/gpio-sim.c static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) · line 215

Quoted 9 lines

 {
      struct gpio_sim_chip *chip = gpiochip_get_data(gc);

drivers/gpio/gpio-sim.c +3 -3

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

Ditto (I checked the line size).

Quoted 1 line

}

...

Quoted 16 lines

      struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
      struct gpio_sim_chip_name_ctx ctx = { bank->swnode, page };

drivers/gpio/gpio-sim.c +4 -7

-     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 device_for_each_child(&dev->pdev->dev, &ctx,
+                                          gpio_sim_emit_chip_name);

Quoted 5 lines

drivers/gpio/gpio-sim.c static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item, · line 1025
Quoted 2 lines
drivers/gpio/gpio-sim.c +1 -1
-     return ret;
+     return sprintf(page, "none\n");
I looked at the original and at the change and maybe it could be done as

What's the difference?!

Quoted 19 lines

        struct device *parent = &dev->pdev->dev; // Naming?
        bool live;

        live = gpio_sim_device_is_live_unlocked(dev);
        if (!live)
                return sprintf(page, "none\n");

        return device_for_each_child(parent, &ctx, gpio_sim_emit_chip_name);

...

Quoted 8 lines

      int ret;

drivers/gpio/gpio-sim.c +2 -3

-     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 ?: "");

Quoted 9 lines

Quoted 1 line
      return ret;
guard() ?
--
With Best Regards,
Andy Shevchenko

Bart

Andy Shevchenko

2023-08-11 14:31 UTC #4 lore raw

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

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko
On Fri, Aug 11, 2023 at 03:14:27PM +0200, Bartosz Golaszewski wrote:

...

Quoted 16 lines

      struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
      struct gpio_sim_chip_name_ctx ctx = { bank->swnode, page };

drivers/gpio/gpio-sim.c +4 -7

-     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 device_for_each_child(&dev->pdev->dev, &ctx,
+                                          gpio_sim_emit_chip_name);

Quoted 16 lines

Quoted 5 lines
drivers/gpio/gpio-sim.c static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item, · line 1025
Quoted 2 lines
drivers/gpio/gpio-sim.c +1 -1
-     return ret;
+     return sprintf(page, "none\n");
I looked at the original and at the change and maybe it could be done as
What's the difference?!
Quoted 8 lines
        struct device *parent = &dev->pdev->dev; // Naming?
        bool live;

        live = gpio_sim_device_is_live_unlocked(dev);
        if (!live)
                return sprintf(page, "none\n");

        return device_for_each_child(parent, &ctx, gpio_sim_emit_chip_name);

No wrapped lines.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-11 14:41 UTC #5 lore raw

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

In reply to Andy Shevchenko

Quoted 49 lines · Andy Shevchenko

Quoted 4 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Use macros defined in linux/cleanup.h to automate resource lifetime control in gpio-sim.
...
drivers/gpio/gpio-sim.c static int gpio_sim_get(struct gpio_chip *gc, unsigned int offset) · line 119
Quoted 10 lines
      struct gpio_sim_chip *chip = gpiochip_get_data(gc);
      int ret;
drivers/gpio/gpio-sim.c +2 -3
-     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;
Isn't the same approach applicable here?
...
Quoted 2 lines
 {
      struct gpio_sim_chip *chip = gpiochip_get_data(gc);
With
        unsigned long *map = ...->value_map;
drivers/gpio/gpio-sim.c static void gpio_sim_set_multiple(struct gpio_chip *gc, · line 148
Quoted 6 lines
drivers/gpio/gpio-sim.c +3 -3
-     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);
...you can satisfy me as well :-)
                bitmap_replace(map, map, bits, mask, gc->ngpio);
Quoted 1 line
}
...
Quoted 3 lines
 {
      struct gpio_sim_chip *chip = gpiochip_get_data(gc);

Quoted 9 lines

drivers/gpio/gpio-sim.c static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) · line 215
Quoted 6 lines
drivers/gpio/gpio-sim.c +3 -3
-     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));
Ditto (I checked the line size).

Sorry but no. This is bikeshedding. I'm not going to add temp variables to only use them once.

Bart

Quoted 53 lines

Quoted 1 line

}

...

Quoted 8 lines

      struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
      struct gpio_sim_chip_name_ctx ctx = { bank->swnode, page };

drivers/gpio/gpio-sim.c +2 -2

-     int ret;
 
-     mutex_lock(&dev->lock);
+     guard(mutex)(&dev->lock);
+
      if (gpio_sim_device_is_live_unlocked(dev))

drivers/gpio/gpio-sim.c static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item, · line 1025

Quoted 10 lines

drivers/gpio/gpio-sim.c +3 -6

-             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 device_for_each_child(&dev->pdev->dev, &ctx,
+                                          gpio_sim_emit_chip_name);
 
-     return ret;
+     return sprintf(page, "none\n");

I looked at the original and at the change and maybe it could be done as

        struct device *parent = &dev->pdev->dev; // Naming?
        bool live;

        live = gpio_sim_device_is_live_unlocked(dev);
        if (!live)
                return sprintf(page, "none\n");

        return device_for_each_child(parent, &ctx, gpio_sim_emit_chip_name);

...

drivers/gpio/gpio-sim.c static ssize_t gpio_sim_hog_config_name_show(struct config_item *item, · line 1161

Quoted 9 lines

      int ret;

drivers/gpio/gpio-sim.c +2 -3

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

guard() ?

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko

Reviewed-by on v1 1/2

2023-08-11 14:04 UTC #6 lore raw

In reply to Bartosz Golaszewski

Quoted 4 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
Simplify the logic when checking the current live value against the user input.

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 4 lines

Signed-off-by Bartosz Golaszewski <[email protected]>
---
This is the same as what Andy suggested for gpio-consumer. Let's save a line.

Right.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author Applied

2023-08-12 18:32 UTC #7 lore raw

In reply to Bartosz Golaszewski

Quoted 29 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Simplify the logic when checking the current live value against the user input.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
This is the same as what Andy suggested for gpio-consumer. Let's save a line.

 drivers/gpio/gpio-sim.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/gpio/gpio-sim.c +1 -3

index dc4097dc0fbc..0177b41e0d72 100644
@@ -986,8 +986,7 @@ gpio_sim_device_config_live_store(struct config_item *item,
 
        mutex_lock(&dev->lock);
 
-       if ((!live && !gpio_sim_device_is_live_unlocked(dev)) ||
-           (live && gpio_sim_device_is_live_unlocked(dev)))
+       if (live == gpio_sim_device_is_live_unlocked(dev))
                ret = -EPERM;
        else if (live)
                ret = gpio_sim_device_activate_unlocked(dev);
--

Quoted 2 lines

2.39.2

Applied.

Bart