Skip to contentSkip to conversation

gpio: sim: use sysfs_streq() and avoid an strdup()

PATCH In review Bartosz Golaszewski 5 participants 17 messages Aug 9, 2023 – Aug 17, 2023 GPIO

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

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

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
---
 drivers/gpio/gpio-sim.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)
drivers/gpio/gpio-sim.c +3 -12
index 8b49b0abacd5..dc4097dc0fbc 100644
@@ -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;
Signature · 1 line
2.39.2
author
#1 lore raw

PATCH 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 the gpio-simulator.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/gpio/gpio-sim.c | 224 ++++++++++++++--------------------------
 1 file changed, 79 insertions(+), 145 deletions(-)
drivers/gpio/gpio-sim.c +79 -145
index dc4097dc0fbc..715e79dc3978 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]);
 }

@@ -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 = {
Signature · 1 line
2.39.2
#2 lore raw

Re: [PATCH 2/2] 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 the gpio-simulator.

gpio-sim ?

...

drivers/gpio/gpio-sim.c static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, · line 72

Quoted 2 lines
drivers/gpio/gpio-sim.c +1 -1
-	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?

...

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

Quoted 3 lines
drivers/gpio/gpio-sim.c +3 -0
+	scoped_guard(mutex, &chip->lock)
+		bitmap_replace(chip->value_map, chip->value_map, bits, mask,
+			       gc->ngpio);

Perhaps with {} ?

...

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

Quoted 13 lines
drivers/gpio/gpio-sim.c +2 -2
 	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 ...

...

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

Quoted 13 lines
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))
 		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.

...

drivers/gpio/gpio-sim.c gpio_sim_line_config_name_show(struct config_item *item, char *page) · line 1134

Quoted 9 lines
drivers/gpio/gpio-sim.c +2 -3
 	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(...);

?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#3 lore raw

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

Quoted 9 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 the gpio-simulator.

gpio-sim ?

Meh, if you insist...

Quoted 5 lines

...

drivers/gpio/gpio-sim.c static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, · line 72

Quoted 2 lines
drivers/gpio/gpio-sim.c +1 -1
-     mutex_lock(&chip->lock);
+     guard(mutex)(&chip->lock);
Quoted 7 lines

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.

Quoted 8 lines

...

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

Quoted 3 lines
drivers/gpio/gpio-sim.c +3 -0
+     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 {}.

Quoted 37 lines

...

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

Quoted 13 lines
      int ret;
drivers/gpio/gpio-sim.c +2 -2
-     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 ...

...

Quoted 12 lines
      int ret;
drivers/gpio/gpio-sim.c +2 -2
-     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);
Quoted 4 lines
Quoted 1 line
      return ret;

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

Sure.

Quoted 10 lines

...

Quoted 8 lines
      int ret;
drivers/gpio/gpio-sim.c +2 -3
-     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 ?: "");
Quoted 8 lines
Quoted 1 line
      return ret;

Why not

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

?

I'll change that too.

Bart

Quoted 6 lines

--
With Best Regards,
Andy Shevchenko

#4 lore raw

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

Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · Andy Shevchenko

On Wed, Aug 09, 2023 at 03:14:42PM +0200, Bartosz Golaszewski wrote:

...

Quoted 9 lines
Quoted 5 lines

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

Quoted 3 lines
drivers/gpio/gpio-sim.c +3 -0
+     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 {}.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#5 lore raw

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

Quoted 12 lines · Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · Andy Shevchenko

On Wed, Aug 09, 2023 at 03:14:42PM +0200, Bartosz Golaszewski wrote:

...

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
drivers/gpio/gpio-sim.c +3 -0
+     scoped_guard(mutex, &chip->lock)
+             bitmap_replace(chip->value_map, chip->value_map, bits, mask,
+                            gc->ngpio);
Quoted 8 lines
Quoted 5 lines
Quoted 1 line

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

#6 lore raw

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

In reply to Bartosz Golaszewski

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/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230811/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

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  }
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#7 lore raw

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

Quoted 2 lines · Dan Carpenter

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?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#8 lore raw

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

Quoted 7 lines · Andy Shevchenko
Quoted 2 lines · Dan Carpenter

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

Acked-by on v1 2/2
#9 lore raw

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

In reply to Bartosz Golaszewski
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

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

(...)

Quoted 2 lines
drivers/gpio/gpio-sim.c +1 -1
-       mutex_lock(&chip->lock);
+       guard(mutex)(&chip->lock);

(...)

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

Quoted 5 lines
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);

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 <[email protected]>

Yours,
Linus Walleij

#10 lore raw

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

Quoted 12 lines · Linus Walleij
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

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

(...)

Quoted 2 lines
drivers/gpio/gpio-sim.c +1 -1
-       mutex_lock(&chip->lock);
+       guard(mutex)(&chip->lock);

(...)

Quoted 8 lines

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

Quoted 5 lines
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);

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.

Quoted 2 lines

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 :/

#11 lore raw

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

Quoted 8 lines · Peter Zijlstra
Quoted 5 lines · Linus Walleij

drivers/gpio/gpio-sim.c static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, · line 72

Quoted 2 lines · Bartosz Golaszewski
drivers/gpio/gpio-sim.c +1 -1
-       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?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#12 lore raw

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

Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Peter Zijlstra
Quoted 5 lines · Linus Walleij

drivers/gpio/gpio-sim.c static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, · line 72

Quoted 2 lines · Bartosz Golaszewski
drivers/gpio/gpio-sim.c +1 -1
-       mutex_lock(&chip->lock);
+       guard(mutex)(&chip->lock);

Looks about right.

Quoted 7 lines

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/[email protected]

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.

Quoted 2 lines

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.

#13 lore raw

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

Quoted 7 lines · Peter Zijlstra
Quoted 2 lines

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 ?

#14 lore raw

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

Quoted 5 lines · Peter Zijlstra
Quoted 3 lines · Peter Zijlstra
Quoted 2 lines

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

Quoted 7 lines
Quoted 4 lines

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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko