| Message ID | 20210329151207.36619-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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: > 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,
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?
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
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.
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(-)