Suppress "Device <device name> does not have a release() function" warning

PATCH In review Yasuaki Ishimatsu 4 participants 11 messages Oct 11, 2012 – Oct 19, 2012

Subsystems (3)

Memory management Driver core Memory hot(un)plug

lore

Share & tools

lore

Yasuaki Ishimatsu

author

2012-10-11 05:19 UTC #0 lore raw

v2 cover Suppress "Device <device name> does not have a release() function" warning

This patch-set is patches to which [1] and [2] are updated
[1] memory-hotplug: add memory_block_release
[2] memory-hotplug: add node_device_release
from following patch-set.

https://lkml.org/lkml/2012/9/27/39

So the patch-set version is v2.

v1 -> v2
[PATCH 1/2]
- change subject to Suppress "Device memoryX does not have a release() function" warning.
- Add detail information into description
- change function name from release_memory_block() to memory_block_release(), because other device release() function is named to <device_name>_release()
[PATCH 2/2]
- change subject to Suppress "Device nodeX does not have a release() function" warning.
- Add detail information into description
- Remove memset() to initialize a node struct from node_device_release()
- Add memset() to initialize a node struct into register_node()

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yasuaki Ishimatsu

author

2012-10-11 05:22 UTC #1 lore raw

PATCH v2 1/2 suppress "Device memoryX does not have a release() function" warning

When calling remove_memory_block(), the function shows following message at device_release().

"Device 'memory528' does not have a release() function, it is broken and must be fixed."

The reason is memory_block's device struct does not have a release() function.

So the patch registers memory_block_release() to the device's release() function for suppressing the warning message. Additionally, the patch moves kfree(mem) into the release function since the release function is prepared as a means to free a memory_block struct.

CC David Rientjes <[email protected]>

CC Jiang Liu <[email protected]>

Cc Minchan Kim <[email protected]>

CC Andrew Morton <[email protected]>

CC KOSAKI Motohiro <[email protected]>

CC Wen Congyang <[email protected]>

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
 drivers/base/memory.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/base/memory.c +8 -1

Index: linux-3.6/drivers/base/memory.c
===================================================================
@@ -70,6 +70,13 @@ void unregister_memory_isolate_notifier(
 }
 EXPORT_SYMBOL(unregister_memory_isolate_notifier);
 
+static void memory_block_release(struct device *dev)
+{
+	struct memory_block *mem = container_of(dev, struct memory_block, dev);
+
+	kfree(mem);
+}
+
 /*
  * register_memory - Setup a sysfs device for a memory block
  */

@@ -80,6 +87,7 @@ int register_memory(struct memory_block 
 
 	memory->dev.bus = &memory_subsys;
 	memory->dev.id = memory->start_section_nr / sections_per_block;
+	memory->dev.release = memory_block_release;
 
 	error = device_register(&memory->dev);
 	return error;

@@ -630,7 +638,6 @@ int remove_memory_block(unsigned long no
 		mem_remove_simple_file(mem, phys_device);
 		mem_remove_simple_file(mem, removable);
 		unregister_memory(mem);
-		kfree(mem);
 	} else
 		kobject_put(&mem->dev.kobj);

David Rientjes

Acked-by on v2 1/2

2012-10-11 20:23 UTC #2 lore raw

Re: [PATCH 1/2]suppress "Device memoryX does not have a release() function" warning

Quoted 13 lines · Yasuaki Ishimatsu

When calling remove_memory_block(), the function shows following message at device_release().
"Device 'memory528' does not have a release() function, it is broken and must be fixed."
The reason is memory_block's device struct does not have a release() function.
So the patch registers memory_block_release() to the device's release() function for suppressing the warning message. Additionally, the patch moves kfree(mem) into the release function since the release function is prepared as a means to free a memory_block struct.

Quoted 7 lines

CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
CC Wen Congyang <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>

Acked-by David Rientjes <[email protected]>

KOSAKI Motohiro

Acked-by on v2 1/2

2012-10-11 22:31 UTC #3 lore raw

Re: [PATCH 1/2]suppress "Device memoryX does not have a release() function" warning

In reply to Yasuaki Ishimatsu

Quoted 13 lines · Yasuaki Ishimatsu

When calling remove_memory_block(), the function shows following message at device_release().
"Device 'memory528' does not have a release() function, it is broken and must be fixed."
The reason is memory_block's device struct does not have a release() function.
So the patch registers memory_block_release() to the device's release() function for suppressing the warning message. Additionally, the patch moves kfree(mem) into the release function since the release function is prepared as a means to free a memory_block struct.

Quoted 7 lines

CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
CC Wen Congyang <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>

Acked-by KOSAKI Motohiro <[email protected]>

Yasuaki Ishimatsu

author

2012-10-11 05:27 UTC #4 lore raw

PATCH v2 2/2 suppress "Device nodeX does not have a release() function" warning

In reply to Yasuaki Ishimatsu

When calling unregister_node(), the function shows following message at device_release().

"Device 'node2' does not have a release() function, it is broken and must be fixed."

The reason is node's device struct does not have a release() function.

So the patch registers node_device_release() to the device's release() function for suppressing the warning message. Additionally, the patch adds memset() to initialize a node struct into register_node(). Because the node struct is part of node_devices[] array and it cannot be freed by node_device_release(). So if system reuses the node struct, it has a garbage.

CC David Rientjes <[email protected]>

CC Jiang Liu <[email protected]>

Cc Minchan Kim <[email protected]>

CC Andrew Morton <[email protected]>

CC KOSAKI Motohiro <[email protected]>

Signed-off-by Yasuaki Ishimatsu <[email protected]>

Signed-off-by Wen Congyang <[email protected]>

---
 drivers/base/node.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/base/node.c +11 -0

Index: linux-3.6/drivers/base/node.c
===================================================================
@@ -252,6 +252,14 @@ static inline void hugetlb_register_node
 static inline void hugetlb_unregister_node(struct node *node) {}
 #endif
 
+static void node_device_release(struct device *dev)
+{
+#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
+	struct node *node_dev = to_node(dev);
+
+	flush_work(&node_dev->node_work);
+#endif
+}
 
 /*
  * register_node - Setup a sysfs device for a node.

@@ -263,8 +271,11 @@ int register_node(struct node *node, int
 {
 	int error;
 
+	memset(node, 0, sizeof(*node));
+
 	node->dev.id = num;
 	node->dev.bus = &node_subsys;
+	node->dev.release = node_device_release;
 	error = device_register(&node->dev);
 
 	if (!error){

David Rientjes

Acked-by on v2 2/2

2012-10-11 20:32 UTC #5 lore raw

Re: [PATCH 2/2]suppress "Device nodeX does not have a release() function" warning

Quoted 6 lines · Yasuaki Ishimatsu

When calling unregister_node(), the function shows following message at device_release().
"Device 'node2' does not have a release() function, it is broken and must be fixed."

Quoted 8 lines

The reason is node's device struct does not have a release() function.
So the patch registers node_device_release() to the device's release() function for suppressing the warning message. Additionally, the patch adds memset() to initialize a node struct into register_node(). Because the node struct is part of node_devices[] array and it cannot be freed by node_device_release(). So if system reuses the node struct, it has a garbage.

Nice catch on reuse of the statically allocated node_devices[] for node hotplug.

Quoted 7 lines

CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>

Can register_node() be made static in drivers/base/node.c and its declaration removed from linux/node.h?

Acked-by David Rientjes <[email protected]>

Yasuaki Ishimatsu

author

2012-10-12 00:14 UTC #6 lore raw

Re: [PATCH 2/2]suppress "Device nodeX does not have a release() function" warning

Quoted 28 lines · David Rientjes

Quoted 6 lines · Yasuaki Ishimatsu
When calling unregister_node(), the function shows following message at device_release().
"Device 'node2' does not have a release() function, it is broken and must be fixed."
Quoted 8 lines
The reason is node's device struct does not have a release() function.
So the patch registers node_device_release() to the device's release() function for suppressing the warning message. Additionally, the patch adds memset() to initialize a node struct into register_node(). Because the node struct is part of node_devices[] array and it cannot be freed by node_device_release(). So if system reuses the node struct, it has a garbage.
Nice catch on reuse of the statically allocated node_devices[] for node hotplug.
Quoted 7 lines
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>

Quoted 2 lines

Can register_node() be made static in drivers/base/node.c and its declaration removed from linux/node.h?

Yah. I'll fix it.

Thanks,
Yasuaki Ishimatsu

Quoted 3 lines

Acked-by David Rientjes <[email protected]>

KOSAKI Motohiro

2012-10-11 22:33 UTC #7 lore raw

Re: [PATCH 2/2]suppress "Device nodeX does not have a release() function" warning

In reply to Yasuaki Ishimatsu

Quoted 31 lines · Yasuaki Ishimatsu

When calling unregister_node(), the function shows following message at device_release().
"Device 'node2' does not have a release() function, it is broken and must be fixed."
The reason is node's device struct does not have a release() function.
So the patch registers node_device_release() to the device's release() function for suppressing the warning message. Additionally, the patch adds memset() to initialize a node struct into register_node(). Because the node struct is part of node_devices[] array and it cannot be freed by node_device_release(). So if system reuses the node struct, it has a garbage.
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
---
 drivers/base/node.c |   11 +++++++++++
 1 file changed, 11 insertions(+)
Index: linux-3.6/drivers/base/node.c
===================================================================
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +0 -0
@@ -252,6 +252,14 @@ static inline void hugetlb_register_node
 static inline void hugetlb_unregister_node(struct node *node) {}

drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 static inline void hugetlb_register_node · line 262

Quoted 10 lines

drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +8 -0
 #endif
 
+static void node_device_release(struct device *dev)
+{
+#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
+       struct node *node_dev = to_node(dev);
+
+       flush_work(&node_dev->node_work);
+#endif
+}

The patch description don't explain why this flush_work() is needed.

drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 int register_node(struct node *node, int · line 275

Quoted 8 lines

drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +2 -0
 /*
  * register_node - Setup a sysfs device for a node.
@@ -263,8 +271,11 @@ int register_node(struct node *node, int
 {
        int error;
 
+       memset(node, 0, sizeof(*node));
+

You should add a comment why we need initialize a node here. A lot of developers don't have hotplug knowledge.

Wen Congyang

2012-10-17 06:18 UTC #8 lore raw

Re: [PATCH 2/2]suppress "Device nodeX does not have a release() function" warning

At 10/12/2012 06:33 AM, KOSAKI Motohiro Wrote:

Quoted 35 lines

Quoted 33 lines · Yasuaki Ishimatsu
When calling unregister_node(), the function shows following message at device_release().
"Device 'node2' does not have a release() function, it is broken and must be fixed."
The reason is node's device struct does not have a release() function.
So the patch registers node_device_release() to the device's release() function for suppressing the warning message. Additionally, the patch adds memset() to initialize a node struct into register_node(). Because the node struct is part of node_devices[] array and it cannot be freed by node_device_release(). So if system reuses the node struct, it has a garbage.
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
---
 drivers/base/node.c |   11 +++++++++++
 1 file changed, 11 insertions(+)
Index: linux-3.6/drivers/base/node.c
===================================================================
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +0 -0
@@ -252,6 +252,14 @@ static inline void hugetlb_register_node
 static inline void hugetlb_unregister_node(struct node *node) {}
 #endif
 

Quoted 10 lines

drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 static inline void hugetlb_register_node · line 262
Quoted 8 lines
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +8 -0
+static void node_device_release(struct device *dev)
+{
+#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
+       struct node *node_dev = to_node(dev);
+
+       flush_work(&node_dev->node_work);
+#endif
+}
The patch description don't explain why this flush_work() is needed.

If the node is onlined after it is offlined, we will clear the memory, so we should flush_work() before node_dev is set to 0.

Thanks
Wen Congyang

Quoted 8 lines

Quoted 8 lines
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +0 -0
 /*
  * register_node - Setup a sysfs device for a node.
@@ -263,8 +271,11 @@ int register_node(struct node *node, int
 {
        int error;
 

Quoted 6 lines

drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 int register_node(struct node *node, int · line 275
Quoted 2 lines
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +2 -0
+       memset(node, 0, sizeof(*node));
+
You should add a comment why we need initialize a node here. A lot of developers don't have hotplug knowledge.

KOSAKI Motohiro

2012-10-17 08:51 UTC #9 lore raw

Re: [PATCH 2/2]suppress "Device nodeX does not have a release() function" warning

Quoted 47 lines · Wen Congyang

At 10/12/2012 06:33 AM, KOSAKI Motohiro Wrote:
Quoted 35 lines
Quoted 33 lines · Yasuaki Ishimatsu
When calling unregister_node(), the function shows following message at device_release().
"Device 'node2' does not have a release() function, it is broken and must be fixed."
The reason is node's device struct does not have a release() function.
So the patch registers node_device_release() to the device's release() function for suppressing the warning message. Additionally, the patch adds memset() to initialize a node struct into register_node(). Because the node struct is part of node_devices[] array and it cannot be freed by node_device_release(). So if system reuses the node struct, it has a garbage.
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
---
 drivers/base/node.c |   11 +++++++++++
 1 file changed, 11 insertions(+)
Index: linux-3.6/drivers/base/node.c
===================================================================
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +0 -0
@@ -252,6 +252,14 @@ static inline void hugetlb_register_node
 static inline void hugetlb_unregister_node(struct node *node) {}
 #endif
 
Quoted 10 lines
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 static inline void hugetlb_register_node · line 262
Quoted 8 lines
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +8 -0
+static void node_device_release(struct device *dev)
+{
+#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
+       struct node *node_dev = to_node(dev);
+
+       flush_work(&node_dev->node_work);
+#endif
+}
The patch description don't explain why this flush_work() is needed.

Quoted 2 lines

If the node is onlined after it is offlined, we will clear the memory, so we should flush_work() before node_dev is set to 0.

So then, it is irrelevant from warning supressness. You should make an another patch.

Wen Congyang

2012-10-19 05:36 UTC #10 lore raw

Re: [PATCH 2/2]suppress "Device nodeX does not have a release() function" warning

At 10/17/2012 04:50 PM, KOSAKI Motohiro Wrote:

Quoted 48 lines · Wen Congyang

Quoted 47 lines
At 10/12/2012 06:33 AM, KOSAKI Motohiro Wrote:
Quoted 45 lines
Quoted 31 lines · Yasuaki Ishimatsu
When calling unregister_node(), the function shows following message at device_release().
"Device 'node2' does not have a release() function, it is broken and must be fixed."
The reason is node's device struct does not have a release() function.
So the patch registers node_device_release() to the device's release() function for suppressing the warning message. Additionally, the patch adds memset() to initialize a node struct into register_node(). Because the node struct is part of node_devices[] array and it cannot be freed by node_device_release(). So if system reuses the node struct, it has a garbage.
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
---
 drivers/base/node.c |   11 +++++++++++
 1 file changed, 11 insertions(+)
Index: linux-3.6/drivers/base/node.c
===================================================================
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +0 -0
@@ -252,6 +252,14 @@ static inline void hugetlb_register_node
 static inline void hugetlb_unregister_node(struct node *node) {}
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 static inline void hugetlb_register_node · line 262
Quoted 10 lines
drivers/base/node.c 2012-10-11 10:20:34.111806931 +0900 renamed from drivers/base/node.c 2012-10-11 10:04:02.149758748 +0900 +8 -0
 #endif
 
+static void node_device_release(struct device *dev)
+{
+#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
+       struct node *node_dev = to_node(dev);
+
+       flush_work(&node_dev->node_work);
+#endif
+}
The patch description don't explain why this flush_work() is needed.

Quoted 6 lines

Quoted 2 lines
If the node is onlined after it is offlined, we will clear the memory, so we should flush_work() before node_dev is set to 0.
So then, it is irrelevant from warning supressness. You should make an another patch.

OK, I will update it soon.

Thanks
Wen Congyang