Skip to contentSkip to conversation

node: fix device cleanups in error handling code

PATCH In review Dan Carpenter 2 participants 2 messages Apr 9, 2021 – Apr 16, 2021

Subsystems (2)
Kernel janitors Driver core
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 node: fix device cleanups in error handling code

We can't use kfree() to free device managed resources so the kfree(dev) is against the rules.

It's easier to write this code if we open code the device_register() as a device_initialize() and device_add(). That way if dev_set_name() set name fails we can call put_device() and it will clean up correctly.

Fixes acc02a109b04 ("node: Add memory-side caching attributes")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/base/node.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
drivers/base/node.c +12 -14
index f449dbb2c746..2c36f61d30bc 100644
@@ -268,21 +268,20 @@ static void node_init_cache_dev(struct node *node)
 	if (!dev)
 		return;
 
+	device_initialize(dev);
 	dev->parent = &node->dev;
 	dev->release = node_cache_release;
 	if (dev_set_name(dev, "memory_side_cache"))
-		goto free_dev;
+		goto put_device;
 
-	if (device_register(dev))
-		goto free_name;
+	if (device_add(dev))
+		goto put_device;
 
 	pm_runtime_no_callbacks(dev);
 	node->cache_dev = dev;
 	return;
-free_name:
-	kfree_const(dev->kobj.name);
-free_dev:
-	kfree(dev);
+put_device:
+	put_device(dev);
 }
 
 /**

@@ -319,25 +318,24 @@ void node_add_cache(unsigned int nid, struct node_cache_attrs *cache_attrs)
 		return;
 
 	dev = &info->dev;
+	device_initialize(dev);
 	dev->parent = node->cache_dev;
 	dev->release = node_cacheinfo_release;
 	dev->groups = cache_groups;
 	if (dev_set_name(dev, "index%d", cache_attrs->level))
-		goto free_cache;
+		goto put_device;
 
 	info->cache_attrs = *cache_attrs;
-	if (device_register(dev)) {
+	if (device_add(dev)) {
 		dev_warn(&node->dev, "failed to add cache level:%d\n",
 			 cache_attrs->level);
-		goto free_name;
+		goto put_device;
 	}
 	pm_runtime_no_callbacks(dev);
 	list_add_tail(&info->node, &node->cache_attrs);
 	return;
-free_name:
-	kfree_const(dev->kobj.name);
-free_cache:
-	kfree(info);
+put_device:
+	put_device(dev);
 }
 
 static void node_remove_caches(struct node *node)
Signature · 1 line
2.30.2
Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines · Dan Carpenter

We can't use kfree() to free device managed resources so the kfree(dev) is against the rules.

It's easier to write this code if we open code the device_register() as a device_initialize() and device_add(). That way if dev_set_name() set name fails we can call put_device() and it will clean up correctly.

Quoted 5 lines
Fixes acc02a109b04 ("node: Add memory-side caching attributes")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/base/node.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Wow, yikes, that "kfree_const(dev->kobj.name);" is really creative

Reviewed-by Jason Gunthorpe <[email protected]>

Though I dislike ignoring the error code from dev_set_name(), I think Greg would prefer this:

drivers/base/node.c +5 -9
index f449dbb2c74666..80079e440add12 100644
@@ -140,20 +140,16 @@ static struct node_access_nodes *node_init_node_access(struct node *node,
 	dev->parent = &node->dev;
 	dev->release = node_access_release;
 	dev->groups = node_access_node_groups;
-	if (dev_set_name(dev, "access%u", access))
-		goto free;
 
-	if (device_register(dev))
-		goto free_name;
+	dev_set_name(dev, "access%u", access);
+	if (device_register(dev)) {
+		put_device(dev);
+		return NULL;
+	}
 
 	pm_runtime_no_callbacks(dev);
 	list_add_tail(&access_node->list_node, &node->access_list);
 	return access_node;
-free_name:
-	kfree_const(dev->kobj.name);
-free:
-	kfree(access_node);
-	return NULL;
 }

(arguably a device_register_name() would be even better, if you are handy with coccinelle there could quickly be alot of users)

Jason