| Message ID | 20210329151207.36619-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 3f6b6536a73fad0c2c82db1672fc500cc15ff551 |
| Headers | show |
| Series | [v2,1/6] software node: Free resources explicitly when swnode_register() fails | expand |
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))
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); >
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
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!
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
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.
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
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);
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(-)