[Patch] debugfs: remove module_exit()

[Patch] debugfs: remove module_exit()

[Amerigo Wang]

debugfs can't be a module, so module_exit() is meaningless
for it.

Signed-off-by: WANG Cong <amwang@redhat.com>

---
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 37a8ca7..d38c88f 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -13,9 +13,6 @@
  *
  */
 
-/* uncomment to get debug messages from the debug filesystem, ah the irony. */
-/* #define DEBUG */
-
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/mount.h>
@@ -540,17 +537,5 @@ static int __init debugfs_init(void)
 
 	return retval;
 }
-
-static void __exit debugfs_exit(void)
-{
-	debugfs_registered = false;
-
-	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
-	unregister_filesystem(&debug_fs_type);
-	kobject_put(debug_kobj);
-}
-
 core_initcall(debugfs_init);
-module_exit(debugfs_exit);
-MODULE_LICENSE("GPL");
 

Re: [Patch] debugfs: remove module_exit()

[Greg KH]

On Tue, Nov 09, 2010 at 04:19:58AM -0500, Amerigo Wang wrote:
> debugfs can't be a module, so module_exit() is meaningless
> for it.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> 
> ---
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 37a8ca7..d38c88f 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -13,9 +13,6 @@
>   *
>   */
>  
> -/* uncomment to get debug messages from the debug filesystem, ah the irony. */
> -/* #define DEBUG */

Why did you remove these lines?  They don't pertain to this patch.

> -
>  #include <linux/module.h>
>  #include <linux/fs.h>
>  #include <linux/mount.h>
> @@ -540,17 +537,5 @@ static int __init debugfs_init(void)
>  
>  	return retval;
>  }
> -
> -static void __exit debugfs_exit(void)
> -{
> -	debugfs_registered = false;
> -
> -	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
> -	unregister_filesystem(&debug_fs_type);
> -	kobject_put(debug_kobj);
> -}

When the code is built into the kernel, the __exit function should go
away, so this isn't costing us any extra memory, right?

And debugfs used to be able to be built as a module, perhaps it will be
in the future?  I don't think this patch is really needed.

thanks,

greg k-h

Re: [Patch] debugfs: remove module_exit()

[Cong Wang]

On 12/01/10 09:35, Greg KH wrote:
> On Tue, Nov 09, 2010 at 04:19:58AM -0500, Amerigo Wang wrote:
>> debugfs can't be a module, so module_exit() is meaningless
>> for it.
>>
>> Signed-off-by: WANG Cong<amwang@redhat.com>
>>
>> ---
>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> index 37a8ca7..d38c88f 100644
>> --- a/fs/debugfs/inode.c
>> +++ b/fs/debugfs/inode.c
>> @@ -13,9 +13,6 @@
>>    *
>>    */
>>
>> -/* uncomment to get debug messages from the debug filesystem, ah the irony. */
>> -/* #define DEBUG */
>
> Why did you remove these lines?  They don't pertain to this patch.

These lines are obsolete.


>
>> -
>>   #include<linux/module.h>
>>   #include<linux/fs.h>
>>   #include<linux/mount.h>
>> @@ -540,17 +537,5 @@ static int __init debugfs_init(void)
>>
>>   	return retval;
>>   }
>> -
>> -static void __exit debugfs_exit(void)
>> -{
>> -	debugfs_registered = false;
>> -
>> -	simple_release_fs(&debugfs_mount,&debugfs_mount_count);
>> -	unregister_filesystem(&debug_fs_type);
>> -	kobject_put(debug_kobj);
>> -}
>
> When the code is built into the kernel, the __exit function should go
> away, so this isn't costing us any extra memory, right?


Perhaps, but this can still reduce the vmlinux size, right?

>
> And debugfs used to be able to be built as a module, perhaps it will be
> in the future?  I don't think this patch is really needed.
>

Huh? Wasn't it a module before?

I think the problem is tracers use debugfs, it needs to depends on DEBUGFS=y.

Thanks.

Re: [Patch] debugfs: remove module_exit()

[Greg KH]

On Wed, Dec 01, 2010 at 02:33:12PM +0800, Cong Wang wrote:
> On 12/01/10 09:35, Greg KH wrote:
> >On Tue, Nov 09, 2010 at 04:19:58AM -0500, Amerigo Wang wrote:
> >>debugfs can't be a module, so module_exit() is meaningless
> >>for it.
> >>
> >>Signed-off-by: WANG Cong<amwang@redhat.com>
> >>
> >>---
> >>diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> >>index 37a8ca7..d38c88f 100644
> >>--- a/fs/debugfs/inode.c
> >>+++ b/fs/debugfs/inode.c
> >>@@ -13,9 +13,6 @@
> >>   *
> >>   */
> >>
> >>-/* uncomment to get debug messages from the debug filesystem, ah the irony. */
> >>-/* #define DEBUG */
> >
> >Why did you remove these lines?  They don't pertain to this patch.
> 
> These lines are obsolete.

Even if they were (and hint, I don't think they are), they have nothing
to do with the patch you created so they don't belong here.  The rule is
"one patch per logical change" and you didn't even describe that you
were removing these lines in the changelog entry, so that's two strikes
against removing these lines.

> >>-
> >>  #include<linux/module.h>
> >>  #include<linux/fs.h>
> >>  #include<linux/mount.h>
> >>@@ -540,17 +537,5 @@ static int __init debugfs_init(void)
> >>
> >>  	return retval;
> >>  }
> >>-
> >>-static void __exit debugfs_exit(void)
> >>-{
> >>-	debugfs_registered = false;
> >>-
> >>-	simple_release_fs(&debugfs_mount,&debugfs_mount_count);
> >>-	unregister_filesystem(&debug_fs_type);
> >>-	kobject_put(debug_kobj);
> >>-}
> >
> >When the code is built into the kernel, the __exit function should go
> >away, so this isn't costing us any extra memory, right?
> 
> 
> Perhaps, but this can still reduce the vmlinux size, right?

Which really doesn't matter, right?  How much is it reduced?

> >And debugfs used to be able to be built as a module, perhaps it will be
> >in the future?  I don't think this patch is really needed.
> >
> 
> Huh? Wasn't it a module before?

Yes it was.

> I think the problem is tracers use debugfs, it needs to depends on DEBUGFS=y.

So if you disable tracing, then you could use debugfs as a module,
right?  So the patch should not be applied.

thanks,

greg k-h

Re: [Patch] debugfs: remove module_exit()

[Cong Wang]

On 12/01/10 23:56, Greg KH wrote:
> On Wed, Dec 01, 2010 at 02:33:12PM +0800, Cong Wang wrote:
>> On 12/01/10 09:35, Greg KH wrote:
>>> On Tue, Nov 09, 2010 at 04:19:58AM -0500, Amerigo Wang wrote:
>>>> debugfs can't be a module, so module_exit() is meaningless
>>>> for it.
>>>>
>>>> Signed-off-by: WANG Cong<amwang@redhat.com>
>>>>
>>>> ---
>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>>>> index 37a8ca7..d38c88f 100644
>>>> --- a/fs/debugfs/inode.c
>>>> +++ b/fs/debugfs/inode.c
>>>> @@ -13,9 +13,6 @@
>>>>    *
>>>>    */
>>>>
>>>> -/* uncomment to get debug messages from the debug filesystem, ah the irony. */
>>>> -/* #define DEBUG */
>>>
>>> Why did you remove these lines?  They don't pertain to this patch.
>>
>> These lines are obsolete.
>
> Even if they were (and hint, I don't think they are), they have nothing
> to do with the patch you created so they don't belong here.  The rule is
> "one patch per logical change" and you didn't even describe that you
> were removing these lines in the changelog entry, so that's two strikes
> against removing these lines.
>

Ok, teach me where DEBUG is used in that file?


>>>> -
>>>>   #include<linux/module.h>
>>>>   #include<linux/fs.h>
>>>>   #include<linux/mount.h>
>>>> @@ -540,17 +537,5 @@ static int __init debugfs_init(void)
>>>>
>>>>   	return retval;
>>>>   }
>>>> -
>>>> -static void __exit debugfs_exit(void)
>>>> -{
>>>> -	debugfs_registered = false;
>>>> -
>>>> -	simple_release_fs(&debugfs_mount,&debugfs_mount_count);
>>>> -	unregister_filesystem(&debug_fs_type);
>>>> -	kobject_put(debug_kobj);
>>>> -}
>>>
>>> When the code is built into the kernel, the __exit function should go
>>> away, so this isn't costing us any extra memory, right?
>>
>>
>> Perhaps, but this can still reduce the vmlinux size, right?
>
> Which really doesn't matter, right?  How much is it reduced?
>
>>> And debugfs used to be able to be built as a module, perhaps it will be
>>> in the future?  I don't think this patch is really needed.
>>>
>>
>> Huh? Wasn't it a module before?
>
> Yes it was.
>
>> I think the problem is tracers use debugfs, it needs to depends on DEBUGFS=y.
>
> So if you disable tracing, then you could use debugfs as a module,
> right?  So the patch should not be applied.
>

No, that CONFIG is a bool, no way to make it as a module.
Since you insist, I will send a patch to make it as a module.

Thanks.

Re: [Patch] debugfs: remove module_exit()

[Greg KH]

On Thu, Dec 02, 2010 at 11:17:34AM +0800, Cong Wang wrote:
> On 12/01/10 23:56, Greg KH wrote:
> >On Wed, Dec 01, 2010 at 02:33:12PM +0800, Cong Wang wrote:
> >>On 12/01/10 09:35, Greg KH wrote:
> >>>On Tue, Nov 09, 2010 at 04:19:58AM -0500, Amerigo Wang wrote:
> >>>>debugfs can't be a module, so module_exit() is meaningless
> >>>>for it.
> >>>>
> >>>>Signed-off-by: WANG Cong<amwang@redhat.com>
> >>>>
> >>>>---
> >>>>diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> >>>>index 37a8ca7..d38c88f 100644
> >>>>--- a/fs/debugfs/inode.c
> >>>>+++ b/fs/debugfs/inode.c
> >>>>@@ -13,9 +13,6 @@
> >>>>   *
> >>>>   */
> >>>>
> >>>>-/* uncomment to get debug messages from the debug filesystem, ah the irony. */
> >>>>-/* #define DEBUG */
> >>>
> >>>Why did you remove these lines?  They don't pertain to this patch.
> >>
> >>These lines are obsolete.
> >
> >Even if they were (and hint, I don't think they are), they have nothing
> >to do with the patch you created so they don't belong here.  The rule is
> >"one patch per logical change" and you didn't even describe that you
> >were removing these lines in the changelog entry, so that's two strikes
> >against removing these lines.
> >
> 
> Ok, teach me where DEBUG is used in that file?

It is.  And even if it isn't, it still shouldn't be done in this patch,
which is my main point here.

> >>>>-
> >>>>  #include<linux/module.h>
> >>>>  #include<linux/fs.h>
> >>>>  #include<linux/mount.h>
> >>>>@@ -540,17 +537,5 @@ static int __init debugfs_init(void)
> >>>>
> >>>>  	return retval;
> >>>>  }
> >>>>-
> >>>>-static void __exit debugfs_exit(void)
> >>>>-{
> >>>>-	debugfs_registered = false;
> >>>>-
> >>>>-	simple_release_fs(&debugfs_mount,&debugfs_mount_count);
> >>>>-	unregister_filesystem(&debug_fs_type);
> >>>>-	kobject_put(debug_kobj);
> >>>>-}
> >>>
> >>>When the code is built into the kernel, the __exit function should go
> >>>away, so this isn't costing us any extra memory, right?
> >>
> >>
> >>Perhaps, but this can still reduce the vmlinux size, right?
> >
> >Which really doesn't matter, right?  How much is it reduced?
> >
> >>>And debugfs used to be able to be built as a module, perhaps it will be
> >>>in the future?  I don't think this patch is really needed.
> >>>
> >>
> >>Huh? Wasn't it a module before?
> >
> >Yes it was.
> >
> >>I think the problem is tracers use debugfs, it needs to depends on DEBUGFS=y.
> >
> >So if you disable tracing, then you could use debugfs as a module,
> >right?  So the patch should not be applied.
> >
> 
> No, that CONFIG is a bool, no way to make it as a module.
> Since you insist, I will send a patch to make it as a module.

{sigh}  No, that's not what I ment at all.

greg k-h

Re: [Patch] debugfs: remove module_exit()

[Cong Wang]

On 12/02/10 11:59, Greg KH wrote:
>>> Even if they were (and hint, I don't think they are), they have nothing
>>> to do with the patch you created so they don't belong here.  The rule is
>>> "one patch per logical change" and you didn't even describe that you
>>> were removing these lines in the changelog entry, so that's two strikes
>>> against removing these lines.
>>>
>>
>> Ok, teach me where DEBUG is used in that file?
>
> It is.  And even if it isn't, it still shouldn't be done in this patch,
> which is my main point here.
>

I see.

>>>> Huh? Wasn't it a module before?
>>>
>>> Yes it was.
>>>
>>>> I think the problem is tracers use debugfs, it needs to depends on DEBUGFS=y.
>>>
>>> So if you disable tracing, then you could use debugfs as a module,
>>> right?  So the patch should not be applied.
>>>
>>
>> No, that CONFIG is a bool, no way to make it as a module.
>> Since you insist, I will send a patch to make it as a module.
>
> {sigh}  No, that's not what I ment at all.
>

Totally confused. :(

If you want to make it as a module, then we should not use 'bool'
and tracers should depend on =y.

If not, then this patch applies.