[PATCH v2 0/2] gpio: sim: improve the usage of __free()

[PATCH v2 0/2] gpio: sim: improve the usage of __free()

[Bartosz Golaszewski]

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

Hi Linus et al,

As discussed here's an improved fix for the invalid usage of __free() in
gpio-sim. I based it on your "maybe-sane" suggestion but unfortunately it
missed a couple details that make it impossible to avoid a conditional
initialization of the managed pointer without repeating the call to
fwnode_create_software_node().

What we're doing here is: we're creating the string array for the standard
"gpio-line-names" device property. It can look like this:

    { "foo", "bar", "baz" }
    
In which case lines 0, 1 and 2 are named but it can also look like this:

    { "foo", NULL, NULL, "bar", NULL, "baz" }
    
Where only lines 0, 3 and 5 have assigned names.

So the `has_line_names` boolean set when encountering the first line with a
name is there for a reason, namely: it's possible that only the line at
offset 0 will have a name, leaving max_offset at 0 but we still need to
create an array of size 1 in this case.

If the array is created and filled, then it needs to live until a deep
copy is completed in fwnode_create_software_node() so it has to be defined
at the top of the function.

I think this still results in clearer code then if we called
`return fwnode_create_software_node();` twice with the same arguments.

I also changed the naming to reflect the purpose of the array: it can be
sparse so it's not really "number of lines", it's the "size of the array
holding the names". The array can be of size 10 but we can only have 3
named lines.

To atone for the above, I've added a second patch which changes the other
instance of __free() in this driver to be initialized in place.

If this is alright for you, please consider applying it directly to your
tree for v6.6-rc2.

Best regards,
Bartosz Golaszewski

v1 -> v2:
- split the line name setting into two parts
- add a patch improving the second instance of using __free()

Bartosz Golaszewski (2):
  gpio: sim: fix an invalid __free() usage
  gpio: sim: initialize a managed pointer when declaring it

 drivers/gpio/gpio-sim.c | 68 +++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 36 deletions(-)

-- 
2.39.2

[PATCH v2 1/2] gpio: sim: fix an invalid __free() usage

[Bartosz Golaszewski]

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

gpio_sim_make_line_names() returns NULL or ERR_PTR() so we must not use
__free(kfree) on the returned address. Split this function into two, one
that determines the size of the "gpio-line-names" array to allocate and
one that actually sets the names at correct offsets. The allocation and
assignment of the managed pointer happens in between.

Fixes: 3faf89f27aab ("gpio: sim: simplify code with cleanup helpers")
Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
Closes: https://lore.kernel.org/all/07c32bf1-6c1a-49d9-b97d-f0ae4a2b42ab@p183/
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-sim.c | 64 +++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 34 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 59cba5b5f54a..4d74ea3a4269 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -21,6 +21,7 @@
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/list.h>
+#include <linux/minmax.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -718,52 +719,42 @@ gpio_sim_device_config_live_show(struct config_item *item, char *page)
 	return sprintf(page, "%c\n", live ? '1' : '0');
 }
 
-static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
-				       unsigned int *line_names_size)
+static unsigned int gpio_sim_get_line_names_size(struct gpio_sim_bank *bank)
 {
 	unsigned int max_offset = 0;
 	bool has_line_names = false;
 	struct gpio_sim_line *line;
-	char **line_names;
 
 	list_for_each_entry(line, &bank->line_list, siblings) {
-		if (line->offset >= bank->num_lines)
+		if (!line->name)
 			continue;
 
-		if (line->name) {
-			if (line->offset > max_offset)
-				max_offset = line->offset;
-
-			/*
-			 * max_offset can stay at 0 so it's not an indicator
-			 * of whether line names were configured at all.
-			 */
-			has_line_names = true;
-		}
+		has_line_names = true;
+		max_offset = max(line->offset, max_offset);
 	}
 
-	if (!has_line_names)
-		/*
-		 * This is not an error - NULL means, there are no line
-		 * names configured.
-		 */
-		return NULL;
+	/*
+	 * It's possible that only the line at offset 0 has a name in which
+	 * case max_offset will be 0 but we still want to allocate an array
+	 * of size 1.
+	 */
+	if (has_line_names)
+		max_offset++;
 
-	*line_names_size = max_offset + 1;
+	return max_offset;
+}
 
-	line_names = kcalloc(*line_names_size, sizeof(*line_names), GFP_KERNEL);
-	if (!line_names)
-		return ERR_PTR(-ENOMEM);
+static void
+gpio_sim_set_line_names(struct gpio_sim_bank *bank, char **line_names)
+{
+	struct gpio_sim_line *line;
 
 	list_for_each_entry(line, &bank->line_list, siblings) {
-		if (line->offset >= bank->num_lines)
+		if (!line->name)
 			continue;
 
-		if (line->name && (line->offset <= max_offset))
-			line_names[line->offset] = line->name;
+		line_names[line->offset] = line->name;
 	}
-
-	return line_names;
 }
 
 static void gpio_sim_remove_hogs(struct gpio_sim_device *dev)
@@ -867,7 +858,7 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 			  struct fwnode_handle *parent)
 {
 	struct property_entry properties[GPIO_SIM_PROP_MAX];
-	unsigned int prop_idx = 0, line_names_size = 0;
+	unsigned int prop_idx = 0, line_names_size;
 	char **line_names __free(kfree) = NULL;
 
 	memset(properties, 0, sizeof(properties));
@@ -878,14 +869,19 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 		properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
 							       bank->label);
 
-	line_names = gpio_sim_make_line_names(bank, &line_names_size);
-	if (IS_ERR(line_names))
-		return ERR_CAST(line_names);
+	line_names_size = gpio_sim_get_line_names_size(bank);
+	if (line_names_size) {
+		line_names = kcalloc(line_names_size, sizeof(*line_names),
+				     GFP_KERNEL);
+		if (!line_names)
+			return ERR_PTR(-ENOMEM);
+
+		gpio_sim_set_line_names(bank, line_names);
 
-	if (line_names)
 		properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
 						"gpio-line-names",
 						line_names, line_names_size);
+	}
 
 	return fwnode_create_software_node(properties, parent);
 }
-- 
2.39.2

[PATCH v2 2/2] gpio: sim: initialize a managed pointer when declaring it

[Bartosz Golaszewski]

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

Variables managed with __free() should typically be initialized where
they are declared so that the __free() callback is paired with its
counterpart resource allocator. Fix the second instance of using
__free() in gpio-sim to follow this pattern.

Fixes: 3faf89f27aab ("gpio: sim: simplify code with cleanup helpers")
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-sim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 4d74ea3a4269..24c19c7c50a0 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -1481,10 +1481,10 @@ 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 __free(kfree) = NULL;
 	int id;
 
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	struct gpio_sim_device *dev __free(kfree) = kzalloc(sizeof(*dev),
+							    GFP_KERNEL);
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.39.2

Re: [PATCH v2 1/2] gpio: sim: fix an invalid __free() usage

[Bartosz Golaszewski]

On Sun, Sep 17, 2023 at 10:58 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> gpio_sim_make_line_names() returns NULL or ERR_PTR() so we must not use
> __free(kfree) on the returned address. Split this function into two, one
> that determines the size of the "gpio-line-names" array to allocate and
> one that actually sets the names at correct offsets. The allocation and
> assignment of the managed pointer happens in between.
>
> Fixes: 3faf89f27aab ("gpio: sim: simplify code with cleanup helpers")
> Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
> Closes: https://lore.kernel.org/all/07c32bf1-6c1a-49d9-b97d-f0ae4a2b42ab@p183/
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

I'm sorry for the noise, I realized I dropped the `if (line->offset >=
bank->num_lines)` check that were added by commit d7459efc9276 ("gpio:
sim: quietly ignore configured lines outside the bank") for a reason.
I need to send a v3 with that restored.

Bartosz

Re: [PATCH v2 2/2] gpio: sim: initialize a managed pointer when declaring it

[Bartosz Golaszewski]

On Sun, Sep 17, 2023 at 10:58 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Variables managed with __free() should typically be initialized where
> they are declared so that the __free() callback is paired with its
> counterpart resource allocator. Fix the second instance of using
> __free() in gpio-sim to follow this pattern.
>
> Fixes: 3faf89f27aab ("gpio: sim: simplify code with cleanup helpers")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpio-sim.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index 4d74ea3a4269..24c19c7c50a0 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -1481,10 +1481,10 @@ 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 __free(kfree) = NULL;
>         int id;
>
> -       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +       struct gpio_sim_device *dev __free(kfree) = kzalloc(sizeof(*dev),
> +                                                           GFP_KERNEL);
>         if (!dev)
>                 return ERR_PTR(-ENOMEM);
>
> --
> 2.39.2
>

Queued for v6.7.

Bartosz