[PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Andy Shevchenko]

Currently we have a slightly twisted logic in swnode_register().
It frees resources that it doesn't allocate on error path and
in once case it relies on the ->release() implementation.

Untwist the logic by freeing resources explicitly when swnode_register()
fails. Currently it happens only in fwnode_create_software_node().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 drivers/base/swnode.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index fa3719ef80e4..456f5fe58b58 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -767,22 +767,19 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 	int ret;
 
 	swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
-	if (!swnode) {
-		ret = -ENOMEM;
-		goto out_err;
-	}
+	if (!swnode)
+		return ERR_PTR(-ENOMEM);
 
 	ret = ida_simple_get(parent ? &parent->child_ids : &swnode_root_ids,
 			     0, 0, GFP_KERNEL);
 	if (ret < 0) {
 		kfree(swnode);
-		goto out_err;
+		return ERR_PTR(ret);
 	}
 
 	swnode->id = ret;
 	swnode->node = node;
 	swnode->parent = parent;
-	swnode->allocated = allocated;
 	swnode->kobj.kset = swnode_kset;
 	fwnode_init(&swnode->fwnode, &software_node_ops);
 
@@ -803,16 +800,17 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 		return ERR_PTR(ret);
 	}
 
+	/*
+	 * Assign the flag only in the successful case, so
+	 * the above kobject_put() won't mess up with properties.
+	 */
+	swnode->allocated = allocated;
+
 	if (parent)
 		list_add_tail(&swnode->entry, &parent->children);
 
 	kobject_uevent(&swnode->kobj, KOBJ_ADD);
 	return &swnode->fwnode;
-
-out_err:
-	if (allocated)
-		property_entries_free(node->properties);
-	return ERR_PTR(ret);
 }
 
 /**
@@ -963,6 +961,7 @@ struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent)
 {
+	struct fwnode_handle *fwnode;
 	struct software_node *node;
 	struct swnode *p = NULL;
 	int ret;
@@ -987,7 +986,13 @@ fwnode_create_software_node(const struct property_entry *properties,
 
 	node->parent = p ? p->node : NULL;
 
-	return swnode_register(node, p, 1);
+	fwnode = swnode_register(node, p, 1);
+	if (IS_ERR(fwnode)) {
+		property_entries_free(node->properties);
+		kfree(node);
+	}
+
+	return fwnode;
 }
 EXPORT_SYMBOL_GPL(fwnode_create_software_node);
 
-- 
2.30.2

[PATCH v2 2/6] software node: Introduce software_node_alloc()/software_node_free()

[Andy Shevchenko]

Introduce software_node_alloc() and software_node_free() helpers.
This will help with code readability and maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 drivers/base/swnode.c | 47 ++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 456f5fe58b58..19aa44bc2628 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -720,19 +720,30 @@ software_node_find_by_name(const struct software_node *parent, const char *name)
 }
 EXPORT_SYMBOL_GPL(software_node_find_by_name);
 
-static int
-software_node_register_properties(struct software_node *node,
-				  const struct property_entry *properties)
+static struct software_node *software_node_alloc(const struct property_entry *properties)
 {
 	struct property_entry *props;
+	struct software_node *node;
 
 	props = property_entries_dup(properties);
 	if (IS_ERR(props))
-		return PTR_ERR(props);
+		return ERR_CAST(props);
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node) {
+		property_entries_free(props);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	node->properties = props;
 
-	return 0;
+	return node;
+}
+
+static void software_node_free(const struct software_node *node)
+{
+	property_entries_free(node->properties);
+	kfree(node);
 }
 
 static void software_node_release(struct kobject *kobj)
@@ -746,10 +757,9 @@ static void software_node_release(struct kobject *kobj)
 		ida_simple_remove(&swnode_root_ids, swnode->id);
 	}
 
-	if (swnode->allocated) {
-		property_entries_free(swnode->node->properties);
-		kfree(swnode->node);
-	}
+	if (swnode->allocated)
+		software_node_free(swnode->node);
+
 	ida_destroy(&swnode->child_ids);
 	kfree(swnode);
 }
@@ -964,7 +974,6 @@ fwnode_create_software_node(const struct property_entry *properties,
 	struct fwnode_handle *fwnode;
 	struct software_node *node;
 	struct swnode *p = NULL;
-	int ret;
 
 	if (parent) {
 		if (IS_ERR(parent))
@@ -974,23 +983,15 @@ fwnode_create_software_node(const struct property_entry *properties,
 		p = to_swnode(parent);
 	}
 
-	node = kzalloc(sizeof(*node), GFP_KERNEL);
-	if (!node)
-		return ERR_PTR(-ENOMEM);
-
-	ret = software_node_register_properties(node, properties);
-	if (ret) {
-		kfree(node);
-		return ERR_PTR(ret);
-	}
+	node = software_node_alloc(properties);
+	if (IS_ERR(node))
+		return ERR_CAST(node);
 
 	node->parent = p ? p->node : NULL;
 
 	fwnode = swnode_register(node, p, 1);
-	if (IS_ERR(fwnode)) {
-		property_entries_free(node->properties);
-		kfree(node);
-	}
+	if (IS_ERR(fwnode))
+		software_node_free(node);
 
 	return fwnode;
 }
-- 
2.30.2

[PATCH v2 3/6] software node: Deduplicate code in fwnode_create_software_node()

[Andy Shevchenko]

Deduplicate conditional and assignment in fwnode_create_software_node(),
i.e. parent is checked in two out of three cases and parent software node
is assigned by to_swnode() call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 drivers/base/swnode.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 19aa44bc2628..db982859171e 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -973,15 +973,14 @@ fwnode_create_software_node(const struct property_entry *properties,
 {
 	struct fwnode_handle *fwnode;
 	struct software_node *node;
-	struct swnode *p = NULL;
-
-	if (parent) {
-		if (IS_ERR(parent))
-			return ERR_CAST(parent);
-		if (!is_software_node(parent))
-			return ERR_PTR(-EINVAL);
-		p = to_swnode(parent);
-	}
+	struct swnode *p;
+
+	if (IS_ERR(parent))
+		return ERR_CAST(parent);
+
+	p = to_swnode(parent);
+	if (parent && !p)
+		return ERR_PTR(-EINVAL);
 
 	node = software_node_alloc(properties);
 	if (IS_ERR(node))
-- 
2.30.2

Re: [PATCH v2 3/6] software node: Deduplicate code in fwnode_create_software_node()

[Daniel Scally]

Hi Andy

On 29/03/2021 16:12, Andy Shevchenko wrote:
> Deduplicate conditional and assignment in fwnode_create_software_node(),
> i.e. parent is checked in two out of three cases and parent software node
> is assigned by to_swnode() call.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


Reviewed-by: Daniel Scally <djrscally@gmail.com>

> ---
> v2: no changes
>  drivers/base/swnode.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 19aa44bc2628..db982859171e 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -973,15 +973,14 @@ fwnode_create_software_node(const struct property_entry *properties,
>  {
>  	struct fwnode_handle *fwnode;
>  	struct software_node *node;
> -	struct swnode *p = NULL;
> -
> -	if (parent) {
> -		if (IS_ERR(parent))
> -			return ERR_CAST(parent);
> -		if (!is_software_node(parent))
> -			return ERR_PTR(-EINVAL);
> -		p = to_swnode(parent);
> -	}
> +	struct swnode *p;
> +
> +	if (IS_ERR(parent))
> +		return ERR_CAST(parent);
> +
> +	p = to_swnode(parent);
> +	if (parent && !p)
> +		return ERR_PTR(-EINVAL);
>  
>  	node = software_node_alloc(properties);
>  	if (IS_ERR(node))

[PATCH v2 4/6] software node: Imply kobj_to_swnode() to be no-op

[Andy Shevchenko]

Since we don't use structure field layout randomization
the manual shuffling can affect some macros, in particular
kobj_to_swnode(), which becomes a no-op when kobj member
is the first one in the struct swnode.

Bloat-o-meter statistics for swnode.o:

  add/remove: 0/0 grow/shrink: 2/10 up/down: 9/-100 (-91)
  Total: Before=7217, After=7126, chg -1.26%

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: added the object file name against which bloat-o-meter was run (Greg)
 drivers/base/swnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index db982859171e..e83028e11f50 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -12,10 +12,10 @@
 #include <linux/slab.h>
 
 struct swnode {
-	int id;
 	struct kobject kobj;
 	struct fwnode_handle fwnode;
 	const struct software_node *node;
+	int id;
 
 	/* hierarchy */
 	struct ida child_ids;
-- 
2.30.2

[PATCH v2 5/6] software node: Introduce SOFTWARE_NODE_REFERENCE() helper macro

[Andy Shevchenko]

This is useful to assign software node reference with arguments
in a common way. Moreover, we have already couple of users that
may be converted. And by the fact, one of them is moved right here
to use the helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 drivers/base/test/property-entry-test.c | 11 ++---------
 include/linux/property.h                | 13 ++++++++-----
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/base/test/property-entry-test.c b/drivers/base/test/property-entry-test.c
index abe03315180f..c2e455d46ffd 100644
--- a/drivers/base/test/property-entry-test.c
+++ b/drivers/base/test/property-entry-test.c
@@ -370,15 +370,8 @@ static void pe_test_reference(struct kunit *test)
 	};
 
 	static const struct software_node_ref_args refs[] = {
-		{
-			.node = &nodes[0],
-			.nargs = 0,
-		},
-		{
-			.node = &nodes[1],
-			.nargs = 2,
-			.args = { 3, 4 },
-		},
+		SOFTWARE_NODE_REFERENCE(&nodes[0]),
+		SOFTWARE_NODE_REFERENCE(&nodes[1], 3, 4),
 	};
 
 	const struct property_entry entries[] = {
diff --git a/include/linux/property.h b/include/linux/property.h
index dd4687b56239..0d876316e61d 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -254,6 +254,13 @@ struct software_node_ref_args {
 	u64 args[NR_FWNODE_REFERENCE_ARGS];
 };
 
+#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
+(const struct software_node_ref_args) {				\
+	.node = _ref_,						\
+	.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
+	.args = { __VA_ARGS__ },				\
+}
+
 /**
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
@@ -362,11 +369,7 @@ struct property_entry {
 	.name = _name_,							\
 	.length = sizeof(struct software_node_ref_args),		\
 	.type = DEV_PROP_REF,						\
-	{ .pointer = &(const struct software_node_ref_args) {		\
-		.node = _ref_,						\
-		.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
-		.args = { __VA_ARGS__ },				\
-	} },								\
+	{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), },	\
 }
 
 struct property_entry *
-- 
2.30.2

Re: [PATCH v2 5/6] software node: Introduce SOFTWARE_NODE_REFERENCE() helper macro

[Daniel Scally]

Hi Andy

On 29/03/2021 16:12, Andy Shevchenko wrote:
> This is useful to assign software node reference with arguments
> in a common way. Moreover, we have already couple of users that
> may be converted. And by the fact, one of them is moved right here
> to use the helper.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: no changes
>  drivers/base/test/property-entry-test.c | 11 ++---------
>  include/linux/property.h                | 13 ++++++++-----
>  2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/test/property-entry-test.c b/drivers/base/test/property-entry-test.c
> index abe03315180f..c2e455d46ffd 100644
> --- a/drivers/base/test/property-entry-test.c
> +++ b/drivers/base/test/property-entry-test.c
> @@ -370,15 +370,8 @@ static void pe_test_reference(struct kunit *test)
>  	};
>  
>  	static const struct software_node_ref_args refs[] = {
> -		{
> -			.node = &nodes[0],
> -			.nargs = 0,
> -		},
> -		{
> -			.node = &nodes[1],
> -			.nargs = 2,
> -			.args = { 3, 4 },
> -		},
> +		SOFTWARE_NODE_REFERENCE(&nodes[0]),
> +		SOFTWARE_NODE_REFERENCE(&nodes[1], 3, 4),
>  	};
>  
>  	const struct property_entry entries[] = {
> diff --git a/include/linux/property.h b/include/linux/property.h
> index dd4687b56239..0d876316e61d 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -254,6 +254,13 @@ struct software_node_ref_args {
>  	u64 args[NR_FWNODE_REFERENCE_ARGS];
>  };
>  
> +#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
> +(const struct software_node_ref_args) {				\
> +	.node = _ref_,						\
> +	.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
> +	.args = { __VA_ARGS__ },				\
> +}
> +
>  /**
>   * struct property_entry - "Built-in" device property representation.
>   * @name: Name of the property.
> @@ -362,11 +369,7 @@ struct property_entry {
>  	.name = _name_,							\
>  	.length = sizeof(struct software_node_ref_args),		\
>  	.type = DEV_PROP_REF,						\
> -	{ .pointer = &(const struct software_node_ref_args) {		\
> -		.node = _ref_,						\
> -		.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
> -		.args = { __VA_ARGS__ },				\
> -	} },								\
> +	{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), },	\
>  }


What are the .args intended to be used for? I actually had it in mind to
replace this with a simple pointer to a struct software_node, because I
can't see any users of them and the fact that it's actually storing a
pointer to a new variable is something that confused me for a good long
time when I wrote the cio2-bridge (though that's mostly due to my
relative inexperience of course, but still)

>  
>  struct property_entry *

Re: [PATCH v2 5/6] software node: Introduce SOFTWARE_NODE_REFERENCE() helper macro

[Andy Shevchenko]

On Mon, Mar 29, 2021 at 11:45:29PM +0100, Daniel Scally wrote:
> On 29/03/2021 16:12, Andy Shevchenko wrote:
> > This is useful to assign software node reference with arguments
> > in a common way. Moreover, we have already couple of users that
> > may be converted. And by the fact, one of them is moved right here
> > to use the helper.

...

> > +		SOFTWARE_NODE_REFERENCE(&nodes[0]),
> > +		SOFTWARE_NODE_REFERENCE(&nodes[1], 3, 4),

...

> > +#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
> > +(const struct software_node_ref_args) {				\
> > +	.node = _ref_,						\
> > +	.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
> > +	.args = { __VA_ARGS__ },				\
> > +}

...

> > +	{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), },	\
> 
> What are the .args intended to be used for? I actually had it in mind to
> replace this with a simple pointer to a struct software_node, because I
> can't see any users of them and the fact that it's actually storing a
> pointer to a new variable is something that confused me for a good long
> time when I wrote the cio2-bridge (though that's mostly due to my
> relative inexperience of course, but still)

It's to be in align with DT phandle references that can take arguments. While
for now, indeed, we have no users of this, it might be changed in the future
(I hadn't checked DesignWare DMA where I would like to transform the code to
 use device properties eventually and there it might be the case).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] software node: Introduce SOFTWARE_NODE_REFERENCE() helper macro

[Daniel Scally]

Hi Andy

On 30/03/2021 10:26, Andy Shevchenko wrote:
>>> +	{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), },	\
>> What are the .args intended to be used for? I actually had it in mind to
>> replace this with a simple pointer to a struct software_node, because I
>> can't see any users of them and the fact that it's actually storing a
>> pointer to a new variable is something that confused me for a good long
>> time when I wrote the cio2-bridge (though that's mostly due to my
>> relative inexperience of course, but still)
> It's to be in align with DT phandle references that can take arguments. While
> for now, indeed, we have no users of this, it might be changed in the future
> (I hadn't checked DesignWare DMA where I would like to transform the code to
>  use device properties eventually and there it might be the case).


Ah yeah I see - haven't come across phandles before but having looked
them up now I see what this is meant to emulate. Consistency is good; in
that case, for this and 6/6:


Reviewed-by: Daniel Scally <djrscally@gmail.com>

and

Tested-by: Daniel Scally <djrscally@gmail.com>



>

[PATCH v2 6/6] media: ipu3-cio2: Switch to use SOFTWARE_NODE_REFERENCE()

[Andy Shevchenko]

This is useful to assign software node reference with arguments
in a common way. Switch to use SOFTWARE_NODE_REFERENCE() here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 drivers/media/pci/intel/ipu3/cio2-bridge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index c2199042d3db..e8511787c1e4 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -79,8 +79,8 @@ static void cio2_bridge_create_fwnode_properties(
 {
 	sensor->prop_names = prop_names;
 
-	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
-	sensor->remote_ref[0].node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT];
+	sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CIO2_ENDPOINT]);
+	sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
 
 	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(
 					sensor->prop_names.clock_frequency,
-- 
2.30.2

Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Daniel Scally]

Hi Andy

On 29/03/2021 16:12, Andy Shevchenko wrote:
> Currently we have a slightly twisted logic in swnode_register().
> It frees resources that it doesn't allocate on error path and
> in once case it relies on the ->release() implementation.
>
> Untwist the logic by freeing resources explicitly when swnode_register()
> fails. Currently it happens only in fwnode_create_software_node().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


Reviewed-by: Daniel Scally <djrscally@gmail.com>

and

Tested-by: Daniel Scally <djrscally@gmail.com>

> ---
> v2: no changes
>  drivers/base/swnode.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index fa3719ef80e4..456f5fe58b58 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -767,22 +767,19 @@ swnode_register(const struct software_node *node, struct swnode *parent,
>  	int ret;
>  
>  	swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
> -	if (!swnode) {
> -		ret = -ENOMEM;
> -		goto out_err;
> -	}
> +	if (!swnode)
> +		return ERR_PTR(-ENOMEM);
>  
>  	ret = ida_simple_get(parent ? &parent->child_ids : &swnode_root_ids,
>  			     0, 0, GFP_KERNEL);
>  	if (ret < 0) {
>  		kfree(swnode);
> -		goto out_err;
> +		return ERR_PTR(ret);
>  	}
>  
>  	swnode->id = ret;
>  	swnode->node = node;
>  	swnode->parent = parent;
> -	swnode->allocated = allocated;
>  	swnode->kobj.kset = swnode_kset;
>  	fwnode_init(&swnode->fwnode, &software_node_ops);
>  
> @@ -803,16 +800,17 @@ swnode_register(const struct software_node *node, struct swnode *parent,
>  		return ERR_PTR(ret);
>  	}
>  
> +	/*
> +	 * Assign the flag only in the successful case, so
> +	 * the above kobject_put() won't mess up with properties.
> +	 */
> +	swnode->allocated = allocated;
> +
>  	if (parent)
>  		list_add_tail(&swnode->entry, &parent->children);
>  
>  	kobject_uevent(&swnode->kobj, KOBJ_ADD);
>  	return &swnode->fwnode;
> -
> -out_err:
> -	if (allocated)
> -		property_entries_free(node->properties);
> -	return ERR_PTR(ret);
>  }
>  
>  /**
> @@ -963,6 +961,7 @@ struct fwnode_handle *
>  fwnode_create_software_node(const struct property_entry *properties,
>  			    const struct fwnode_handle *parent)
>  {
> +	struct fwnode_handle *fwnode;
>  	struct software_node *node;
>  	struct swnode *p = NULL;
>  	int ret;
> @@ -987,7 +986,13 @@ fwnode_create_software_node(const struct property_entry *properties,
>  
>  	node->parent = p ? p->node : NULL;
>  
> -	return swnode_register(node, p, 1);
> +	fwnode = swnode_register(node, p, 1);
> +	if (IS_ERR(fwnode)) {
> +		property_entries_free(node->properties);
> +		kfree(node);
> +	}
> +
> +	return fwnode;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_create_software_node);
>  

Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Heikki Krogerus]

On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> Currently we have a slightly twisted logic in swnode_register().
> It frees resources that it doesn't allocate on error path and
> in once case it relies on the ->release() implementation.
> 
> Untwist the logic by freeing resources explicitly when swnode_register()
> fails. Currently it happens only in fwnode_create_software_node().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

It all looks OK to me. FWIW, for the whole series:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> v2: no changes
>  drivers/base/swnode.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index fa3719ef80e4..456f5fe58b58 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -767,22 +767,19 @@ swnode_register(const struct software_node *node, struct swnode *parent,
>  	int ret;
>  
>  	swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
> -	if (!swnode) {
> -		ret = -ENOMEM;
> -		goto out_err;
> -	}
> +	if (!swnode)
> +		return ERR_PTR(-ENOMEM);
>  
>  	ret = ida_simple_get(parent ? &parent->child_ids : &swnode_root_ids,
>  			     0, 0, GFP_KERNEL);
>  	if (ret < 0) {
>  		kfree(swnode);
> -		goto out_err;
> +		return ERR_PTR(ret);
>  	}
>  
>  	swnode->id = ret;
>  	swnode->node = node;
>  	swnode->parent = parent;
> -	swnode->allocated = allocated;
>  	swnode->kobj.kset = swnode_kset;
>  	fwnode_init(&swnode->fwnode, &software_node_ops);
>  
> @@ -803,16 +800,17 @@ swnode_register(const struct software_node *node, struct swnode *parent,
>  		return ERR_PTR(ret);
>  	}
>  
> +	/*
> +	 * Assign the flag only in the successful case, so
> +	 * the above kobject_put() won't mess up with properties.
> +	 */
> +	swnode->allocated = allocated;
> +
>  	if (parent)
>  		list_add_tail(&swnode->entry, &parent->children);
>  
>  	kobject_uevent(&swnode->kobj, KOBJ_ADD);
>  	return &swnode->fwnode;
> -
> -out_err:
> -	if (allocated)
> -		property_entries_free(node->properties);
> -	return ERR_PTR(ret);
>  }
>  
>  /**
> @@ -963,6 +961,7 @@ struct fwnode_handle *
>  fwnode_create_software_node(const struct property_entry *properties,
>  			    const struct fwnode_handle *parent)
>  {
> +	struct fwnode_handle *fwnode;
>  	struct software_node *node;
>  	struct swnode *p = NULL;
>  	int ret;
> @@ -987,7 +986,13 @@ fwnode_create_software_node(const struct property_entry *properties,
>  
>  	node->parent = p ? p->node : NULL;
>  
> -	return swnode_register(node, p, 1);
> +	fwnode = swnode_register(node, p, 1);
> +	if (IS_ERR(fwnode)) {
> +		property_entries_free(node->properties);
> +		kfree(node);
> +	}
> +
> +	return fwnode;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_create_software_node);
>  
> -- 
> 2.30.2

thanks,

-- 
heikki

Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Rafael J. Wysocki]

On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > Currently we have a slightly twisted logic in swnode_register().
> > It frees resources that it doesn't allocate on error path and
> > in once case it relies on the ->release() implementation.
> >
> > Untwist the logic by freeing resources explicitly when swnode_register()
> > fails. Currently it happens only in fwnode_create_software_node().
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> It all looks OK to me. FWIW, for the whole series:
>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Whole series applied (with some minor changelog edits) as 5.13 material, thanks!

Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Andy Shevchenko]

On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote:
> On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > > Currently we have a slightly twisted logic in swnode_register().
> > > It frees resources that it doesn't allocate on error path and
> > > in once case it relies on the ->release() implementation.
> > >
> > > Untwist the logic by freeing resources explicitly when swnode_register()
> > > fails. Currently it happens only in fwnode_create_software_node().
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > It all looks OK to me. FWIW, for the whole series:
> >
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Whole series applied (with some minor changelog edits) as 5.13 material, thanks!

It seems Greg applied it already. Was it dropped there?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Rafael J. Wysocki]

On Thu, Apr 8, 2021 at 4:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > > > Currently we have a slightly twisted logic in swnode_register().
> > > > It frees resources that it doesn't allocate on error path and
> > > > in once case it relies on the ->release() implementation.
> > > >
> > > > Untwist the logic by freeing resources explicitly when swnode_register()
> > > > fails. Currently it happens only in fwnode_create_software_node().
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > It all looks OK to me. FWIW, for the whole series:
> > >
> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > Whole series applied (with some minor changelog edits) as 5.13 material, thanks!
>
> It seems Greg applied it already. Was it dropped there?

Did he?

OK, so please let me know if it's still there in the Greg's tree.

Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Andy Shevchenko]

On Thu, Apr 08, 2021 at 05:04:32PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 8, 2021 at 4:50 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > > > > Currently we have a slightly twisted logic in swnode_register().
> > > > > It frees resources that it doesn't allocate on error path and
> > > > > in once case it relies on the ->release() implementation.
> > > > >
> > > > > Untwist the logic by freeing resources explicitly when swnode_register()
> > > > > fails. Currently it happens only in fwnode_create_software_node().
> > > > >
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > >
> > > > It all looks OK to me. FWIW, for the whole series:
> > > >
> > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >
> > > Whole series applied (with some minor changelog edits) as 5.13 material, thanks!
> >
> > It seems Greg applied it already. Was it dropped there?
> 
> Did he?
> 
> OK, so please let me know if it's still there in the Greg's tree.

Here [1] what I see. Seems still there.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=6e11b376fd74356e32d842be588e12dc9bf6e197

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

[Rafael J. Wysocki]

On Thu, Apr 8, 2021 at 5:18 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 08, 2021 at 05:04:32PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 4:50 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > > > > > Currently we have a slightly twisted logic in swnode_register().
> > > > > > It frees resources that it doesn't allocate on error path and
> > > > > > in once case it relies on the ->release() implementation.
> > > > > >
> > > > > > Untwist the logic by freeing resources explicitly when swnode_register()
> > > > > > fails. Currently it happens only in fwnode_create_software_node().
> > > > > >
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > >
> > > > > It all looks OK to me. FWIW, for the whole series:
> > > > >
> > > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > >
> > > > Whole series applied (with some minor changelog edits) as 5.13 material, thanks!
> > >
> > > It seems Greg applied it already. Was it dropped there?
> >
> > Did he?
> >
> > OK, so please let me know if it's still there in the Greg's tree.
>
> Here [1] what I see. Seems still there.
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=6e11b376fd74356e32d842be588e12dc9bf6e197

I will not be applying it then, sorry for the confusion.