Re: [RFC/PATCH] unregister_node() for hotplug use
On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote:
> On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote:
> > +#ifdef CONFIG_HOTPLUG
> > +void unregister_node(struct node *node)
> > +{
> > + sysdev_remove_file(>sysdev, _cpumap);
> > + sysdev_remove_file(>sysdev, _meminfo);
> > + sysdev_remove_file(>sysdev, _numastat);
> > + sysdev_remove_file(>sysdev, _distance);
> > +
> > + sysdev_unregister(>sysdev);
> > +}
> > +EXPORT_SYMBOL_GPL(register_node);
> > +EXPORT_SYMBOL_GPL(unregister_node);
> > +#else /* !CONFIG_HOTPLUG */
> > +void unregister_node(struct node *node)
> > +{
> > +}
> > +#endif /* !CONFIG_HOTPLUG */
>
> Oops, you only exported the symbols if CONFIG_HOTPLUG is enabled, not a
> good idea. Either make the null functions in a .h file if the option is
> not enabled, or always export them.
My bad...
> And the comment for the #endif isn't really needed, as the whole #ifdef
> fits on a single screen :)
I see:) That makes sense.
> And hey, what's the real big deal here, why not always have this
> function no matter if CONFIG_HOTPLUG is enabled or not? I really want
> to just make that an option that is always enabled anyway, but changable
> if you are using CONFIG_TINY or something...
I put the #ifdef there for users who don't need hotplug
stuffs, but I want to make the option always enabled, too.
Also a good side effect, the code would be cleaner:) I
will be updating my patch without the #ifdef and sending
it here.
Thanks!
Keiichiro Tokunaga
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH] unregister_node() for hotplug use
On Wed, 20 Apr 2005 10:32:35 -0700 Greg KH wrote:
> On Wed, Apr 20, 2005 at 09:07:44PM +0900, Keiichiro Tokunaga wrote:
> > This is to add a generic function 'unregister_node()'.
> > It is used to remove objects of a node going away for
> > hotplug. If CONFIG_HOTPLUG=y, it becomes available.
> > This is against 2.6.12-rc2-mm3.
>
> Please CC: this kind of stuff to the driver core maintainer, otherwise
> it can get dropped...
I will for sure CC appropriate maintainers next time...
> Anyway, comments below:
>
> > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c
> > --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14
> > 20:49:37.0 +0900
> > +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-14
> > 20:49:37.0 +0900
> > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no
> > *
> > * Initialize and register the node device.
> > */
> > -int __init register_node(struct node *node, int num, struct node *parent)
> > +int __devinit register_node(struct node *node, int num, struct node
> > *parent)
> > {
> > int error;
> >
> > @@ -145,6 +145,9 @@ int __init register_node(struct node *no
> > error = sysdev_register(>sysdev);
> >
> > if (!error){
> > + /*
> > +* If you add new object here, delete it when unregistering.
> > +*/
>
> Comment really isn't needed.
I removed the comments.
> > +/*
> > + * unregister_node - Remove objects of a node going away from sysfs.
> > + * @node - node going away
> > + *
> > + * This is used only for hotplug.
> > + */
>
> If you are going to create function comments, at least use the proper
> kerneldoc format.
I removed it. I thought about its necessity again and
it seems that the comments isn't really necessary because
the purpose of the function is obvious anyway.
> > +#ifdef CONFIG_HOTPLUG
>
> You don't provide function prototype for when CONFIG_HOTPLUG is not
> enabled.
>
> > +void unregister_node(struct node *node)
> > +{
> > + if (node == NULL)
> > + return;
>
> How can this happen?
It's a redundant check, so I removed it.
> > +
> > + sysdev_remove_file(>sysdev, _cpumap);
> > + sysdev_remove_file(>sysdev, _meminfo);
> > + sysdev_remove_file(>sysdev, _numastat);
> > + sysdev_remove_file(>sysdev, _distance);
> > +
> > + sysdev_unregister(>sysdev);
> > +}
> > +EXPORT_SYMBOL(register_node);
> > +EXPORT_SYMBOL(unregister_node);
>
> All of sysfs and the driver core are EXPORT_SYMBOL_GPL(). Please follow
> that convention.
OK, I made that change.
> > +#endif /* CONFIG_HOTPLUG */
> >
> > -int __init register_node_type(void)
> > +static int __init register_node_type(void)
>
> Are you sure no one calls this?
Yes, no one calls this today.
> > {
> > return sysdev_class_register(_class);
> > }
> > diff -puN include/linux/node.h~numa_hp_base include/linux/node.h
> > --- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14
> > 20:49:37.0 +0900
> > +++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-14
> > 20:49:37.0 +0900
> > @@ -21,12 +21,16 @@
> >
> > #include
> > #include
> > +#include
>
> Why?
>
> >
> > struct node {
> > struct sys_device sysdev;
> > };
> >
> > -extern int register_node(struct node *, int, struct node *);
> > +extern int __devinit register_node(struct node *, int, struct node *);
>
> __devinit is not needed on a function prototype.
>
> > +#ifdef CONFIG_HOTPLUG
> > +extern void unregister_node(struct node *node);
> > +#endif
>
> Not needed for a function prototype.
I corrected them. Thanks for all the comments!
I am attaching the updated patch. Please take a look.
Thanks,
Keiichiro Tokunaga
Signed-off-by: Keiichiro Tokunaga <[EMAIL PROTECTED]>
---
linux-2.6.12-rc2-mm3-kei/drivers/base/node.c | 21 +++--
linux-2.6.12-rc2-mm3-kei/include/linux/node.h |1 +
2 files changed, 20 insertions(+), 2 deletions(-)
diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c
--- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-21
19:20:41.0 +0900
@@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no
*
* Initialize and register the node device.
*/
-int __init register_node(struct node *node, int num, stru
Re: [RFC/PATCH] unregister_node() for hotplug use
On Wed, 20 Apr 2005 14:35:10 +0200 Arjan van de Ven wrote: > > > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > > --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 > > 20:49:37.0 +0900 > > +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-14 > > 20:49:37.0 +0900 > > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > > > > +EXPORT_SYMBOL(register_node); > > +EXPORT_SYMBOL(unregister_node); > > +#endif /* CONFIG_HOTPLUG */ > > > > please make these EXPORT_SYMBOL_GPL; the rest of sysfs is too and this > is very much deep kernel internals that are linux specific. OK, I will make that change. Thanks for the comments! Thanks, Keiichiro Tokunaga - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH] unregister_node() for hotplug use
On Wed, 20 Apr 2005 14:35:10 +0200 Arjan van de Ven wrote: diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 20:49:37.0 +0900 +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-14 20:49:37.0 +0900 @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no +EXPORT_SYMBOL(register_node); +EXPORT_SYMBOL(unregister_node); +#endif /* CONFIG_HOTPLUG */ please make these EXPORT_SYMBOL_GPL; the rest of sysfs is too and this is very much deep kernel internals that are linux specific. OK, I will make that change. Thanks for the comments! Thanks, Keiichiro Tokunaga - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH] unregister_node() for hotplug use
On Wed, 20 Apr 2005 10:32:35 -0700 Greg KH wrote:
On Wed, Apr 20, 2005 at 09:07:44PM +0900, Keiichiro Tokunaga wrote:
This is to add a generic function 'unregister_node()'.
It is used to remove objects of a node going away for
hotplug. If CONFIG_HOTPLUG=y, it becomes available.
This is against 2.6.12-rc2-mm3.
Please CC: this kind of stuff to the driver core maintainer, otherwise
it can get dropped...
I will for sure CC appropriate maintainers next time...
Anyway, comments below:
diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c
--- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-14
20:49:37.0 +0900
@@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no
*
* Initialize and register the node device.
*/
-int __init register_node(struct node *node, int num, struct node *parent)
+int __devinit register_node(struct node *node, int num, struct node
*parent)
{
int error;
@@ -145,6 +145,9 @@ int __init register_node(struct node *no
error = sysdev_register(node-sysdev);
if (!error){
+ /*
+* If you add new object here, delete it when unregistering.
+*/
Comment really isn't needed.
I removed the comments.
+/*
+ * unregister_node - Remove objects of a node going away from sysfs.
+ * @node - node going away
+ *
+ * This is used only for hotplug.
+ */
If you are going to create function comments, at least use the proper
kerneldoc format.
I removed it. I thought about its necessity again and
it seems that the comments isn't really necessary because
the purpose of the function is obvious anyway.
+#ifdef CONFIG_HOTPLUG
You don't provide function prototype for when CONFIG_HOTPLUG is not
enabled.
+void unregister_node(struct node *node)
+{
+ if (node == NULL)
+ return;
How can this happen?
It's a redundant check, so I removed it.
+
+ sysdev_remove_file(node-sysdev, attr_cpumap);
+ sysdev_remove_file(node-sysdev, attr_meminfo);
+ sysdev_remove_file(node-sysdev, attr_numastat);
+ sysdev_remove_file(node-sysdev, attr_distance);
+
+ sysdev_unregister(node-sysdev);
+}
+EXPORT_SYMBOL(register_node);
+EXPORT_SYMBOL(unregister_node);
All of sysfs and the driver core are EXPORT_SYMBOL_GPL(). Please follow
that convention.
OK, I made that change.
+#endif /* CONFIG_HOTPLUG */
-int __init register_node_type(void)
+static int __init register_node_type(void)
Are you sure no one calls this?
Yes, no one calls this today.
{
return sysdev_class_register(node_class);
}
diff -puN include/linux/node.h~numa_hp_base include/linux/node.h
--- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-14
20:49:37.0 +0900
@@ -21,12 +21,16 @@
#include linux/sysdev.h
#include linux/cpumask.h
+#include linux/module.h
Why?
struct node {
struct sys_device sysdev;
};
-extern int register_node(struct node *, int, struct node *);
+extern int __devinit register_node(struct node *, int, struct node *);
__devinit is not needed on a function prototype.
+#ifdef CONFIG_HOTPLUG
+extern void unregister_node(struct node *node);
+#endif
Not needed for a function prototype.
I corrected them. Thanks for all the comments!
I am attaching the updated patch. Please take a look.
Thanks,
Keiichiro Tokunaga
Signed-off-by: Keiichiro Tokunaga [EMAIL PROTECTED]
---
linux-2.6.12-rc2-mm3-kei/drivers/base/node.c | 21 +++--
linux-2.6.12-rc2-mm3-kei/include/linux/node.h |1 +
2 files changed, 20 insertions(+), 2 deletions(-)
diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c
--- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-21
19:20:41.0 +0900
@@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no
*
* Initialize and register the node device.
*/
-int __init register_node(struct node *node, int num, struct node *parent)
+int __devinit register_node(struct node *node, int num, struct node *parent)
{
int error;
@@ -153,8 +153,25 @@ int __init register_node(struct node *no
return error;
}
+#ifdef CONFIG_HOTPLUG
+void unregister_node(struct node *node)
+{
+ sysdev_remove_file(node-sysdev, attr_cpumap);
+ sysdev_remove_file(node-sysdev, attr_meminfo);
+ sysdev_remove_file(node-sysdev, attr_numastat);
+ sysdev_remove_file(node-sysdev, attr_distance);
+
+ sysdev_unregister(node-sysdev);
+}
+EXPORT_SYMBOL_GPL(register_node);
+EXPORT_SYMBOL_GPL(unregister_node);
+#else /* !CONFIG_HOTPLUG */
+void unregister_node
Re: [RFC/PATCH] unregister_node() for hotplug use
On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote:
On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote:
+#ifdef CONFIG_HOTPLUG
+void unregister_node(struct node *node)
+{
+ sysdev_remove_file(node-sysdev, attr_cpumap);
+ sysdev_remove_file(node-sysdev, attr_meminfo);
+ sysdev_remove_file(node-sysdev, attr_numastat);
+ sysdev_remove_file(node-sysdev, attr_distance);
+
+ sysdev_unregister(node-sysdev);
+}
+EXPORT_SYMBOL_GPL(register_node);
+EXPORT_SYMBOL_GPL(unregister_node);
+#else /* !CONFIG_HOTPLUG */
+void unregister_node(struct node *node)
+{
+}
+#endif /* !CONFIG_HOTPLUG */
Oops, you only exported the symbols if CONFIG_HOTPLUG is enabled, not a
good idea. Either make the null functions in a .h file if the option is
not enabled, or always export them.
My bad...
And the comment for the #endif isn't really needed, as the whole #ifdef
fits on a single screen :)
I see:) That makes sense.
And hey, what's the real big deal here, why not always have this
function no matter if CONFIG_HOTPLUG is enabled or not? I really want
to just make that an option that is always enabled anyway, but changable
if you are using CONFIG_TINY or something...
I put the #ifdef there for users who don't need hotplug
stuffs, but I want to make the option always enabled, too.
Also a good side effect, the code would be cleaner:) I
will be updating my patch without the #ifdef and sending
it here.
Thanks!
Keiichiro Tokunaga
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC/PATCH] unregister_node() for hotplug use
This is to add a generic function 'unregister_node()'.
It is used to remove objects of a node going away for
hotplug. If CONFIG_HOTPLUG=y, it becomes available.
This is against 2.6.12-rc2-mm3.
Thanks,
Keiichiro Tokunaga
Signed-off-by: Keiichiro Tokunaga <[EMAIL PROTECTED]>
---
linux-2.6.12-rc2-mm3-kei/drivers/base/node.c | 29 --
linux-2.6.12-rc2-mm3-kei/include/linux/node.h |6 -
2 files changed, 32 insertions(+), 3 deletions(-)
diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c
--- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-14
20:49:37.0 +0900
@@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no
*
* Initialize and register the node device.
*/
-int __init register_node(struct node *node, int num, struct node *parent)
+int __devinit register_node(struct node *node, int num, struct node *parent)
{
int error;
@@ -145,6 +145,9 @@ int __init register_node(struct node *no
error = sysdev_register(>sysdev);
if (!error){
+ /*
+* If you add new object here, delete it when unregistering.
+*/
sysdev_create_file(>sysdev, _cpumap);
sysdev_create_file(>sysdev, _meminfo);
sysdev_create_file(>sysdev, _numastat);
@@ -153,8 +156,30 @@ int __init register_node(struct node *no
return error;
}
+/*
+ * unregister_node - Remove objects of a node going away from sysfs.
+ * @node - node going away
+ *
+ * This is used only for hotplug.
+ */
+#ifdef CONFIG_HOTPLUG
+void unregister_node(struct node *node)
+{
+ if (node == NULL)
+ return;
+
+ sysdev_remove_file(>sysdev, _cpumap);
+ sysdev_remove_file(>sysdev, _meminfo);
+ sysdev_remove_file(>sysdev, _numastat);
+ sysdev_remove_file(>sysdev, _distance);
+
+ sysdev_unregister(>sysdev);
+}
+EXPORT_SYMBOL(register_node);
+EXPORT_SYMBOL(unregister_node);
+#endif /* CONFIG_HOTPLUG */
-int __init register_node_type(void)
+static int __init register_node_type(void)
{
return sysdev_class_register(_class);
}
diff -puN include/linux/node.h~numa_hp_base include/linux/node.h
--- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-14
20:49:37.0 +0900
@@ -21,12 +21,16 @@
#include
#include
+#include
struct node {
struct sys_device sysdev;
};
-extern int register_node(struct node *, int, struct node *);
+extern int __devinit register_node(struct node *, int, struct node *);
+#ifdef CONFIG_HOTPLUG
+extern void unregister_node(struct node *node);
+#endif
#define to_node(sys_device) container_of(sys_device, struct node, sysdev)
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC/PATCH] unregister_node() for hotplug use
This is to add a generic function 'unregister_node()'.
It is used to remove objects of a node going away for
hotplug. If CONFIG_HOTPLUG=y, it becomes available.
This is against 2.6.12-rc2-mm3.
Thanks,
Keiichiro Tokunaga
Signed-off-by: Keiichiro Tokunaga [EMAIL PROTECTED]
---
linux-2.6.12-rc2-mm3-kei/drivers/base/node.c | 29 --
linux-2.6.12-rc2-mm3-kei/include/linux/node.h |6 -
2 files changed, 32 insertions(+), 3 deletions(-)
diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c
--- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c2005-04-14
20:49:37.0 +0900
@@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no
*
* Initialize and register the node device.
*/
-int __init register_node(struct node *node, int num, struct node *parent)
+int __devinit register_node(struct node *node, int num, struct node *parent)
{
int error;
@@ -145,6 +145,9 @@ int __init register_node(struct node *no
error = sysdev_register(node-sysdev);
if (!error){
+ /*
+* If you add new object here, delete it when unregistering.
+*/
sysdev_create_file(node-sysdev, attr_cpumap);
sysdev_create_file(node-sysdev, attr_meminfo);
sysdev_create_file(node-sysdev, attr_numastat);
@@ -153,8 +156,30 @@ int __init register_node(struct node *no
return error;
}
+/*
+ * unregister_node - Remove objects of a node going away from sysfs.
+ * @node - node going away
+ *
+ * This is used only for hotplug.
+ */
+#ifdef CONFIG_HOTPLUG
+void unregister_node(struct node *node)
+{
+ if (node == NULL)
+ return;
+
+ sysdev_remove_file(node-sysdev, attr_cpumap);
+ sysdev_remove_file(node-sysdev, attr_meminfo);
+ sysdev_remove_file(node-sysdev, attr_numastat);
+ sysdev_remove_file(node-sysdev, attr_distance);
+
+ sysdev_unregister(node-sysdev);
+}
+EXPORT_SYMBOL(register_node);
+EXPORT_SYMBOL(unregister_node);
+#endif /* CONFIG_HOTPLUG */
-int __init register_node_type(void)
+static int __init register_node_type(void)
{
return sysdev_class_register(node_class);
}
diff -puN include/linux/node.h~numa_hp_base include/linux/node.h
--- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14
20:49:37.0 +0900
+++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-14
20:49:37.0 +0900
@@ -21,12 +21,16 @@
#include linux/sysdev.h
#include linux/cpumask.h
+#include linux/module.h
struct node {
struct sys_device sysdev;
};
-extern int register_node(struct node *, int, struct node *);
+extern int __devinit register_node(struct node *, int, struct node *);
+#ifdef CONFIG_HOTPLUG
+extern void unregister_node(struct node *node);
+#endif
#define to_node(sys_device) container_of(sys_device, struct node, sysdev)
_
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
