[KJ] [PATCH] convert airo wireless driver to module_param

[KJ] [PATCH] convert airo wireless driver to module_param

[Stefan Sperling]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]


Replaces MODULE_PARM with module_param calls in airo wireless driver.

--- drivers/net/wireless/airo.c.orig	2004-11-09 20:40:21.867942880 +0100
+++ drivers/net/wireless/airo.c	2004-11-08 23:09:27.000000000 +0100
@@ -246,36 +246,36 @@ MODULE_DESCRIPTION("Support for Cisco/Ai
 		   for PCMCIA when used with airo_cs.");
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_SUPPORTED_DEVICE("Aironet 4500, 4800 and Cisco 340/350");
-MODULE_PARM(io,"1-4i");
-MODULE_PARM(irq,"1-4i");
-MODULE_PARM(basic_rate,"i");
-MODULE_PARM(rates,"1-8i");
-MODULE_PARM(ssids,"1-3s");
-MODULE_PARM(auto_wep,"i");
+module_param_array(io, int, NULL, 0444);
+module_param_array(irq, int, NULL, 0444);
+module_param(basic_rate, int, 0644);
+module_param_array(rates, int, NULL, 0644);
+module_param_array(ssids, int, NULL, 0644);
+module_param(auto_wep, int, 0644);
 MODULE_PARM_DESC(auto_wep, "If non-zero, the driver will keep looping through \
 the authentication options until an association is made.  The value of \
 auto_wep is number of the wep keys to check.  A value of 2 will try using \
 the key at index 0 and index 1.");
-MODULE_PARM(aux_bap,"i");
+module_param(aux_bap, int, 0644);
 MODULE_PARM_DESC(aux_bap, "If non-zero, the driver will switch into a mode \
 than seems to work better for older cards with some older buses.  Before \
 switching it checks that the switch is needed.");
-MODULE_PARM(maxencrypt, "i");
+module_param(maxencrypt, int, 0644);
 MODULE_PARM_DESC(maxencrypt, "The maximum speed that the card can do \
 encryption.  Units are in 512kbs.  Zero (default) means there is no limit. \
 Older cards used to be limited to 2mbs (4).");
-MODULE_PARM(adhoc, "i");
+module_param(adhoc, int, 0644);
 MODULE_PARM_DESC(adhoc, "If non-zero, the card will start in adhoc mode.");
-MODULE_PARM(probe, "i");
+module_param(probe, int, 0644);
 MODULE_PARM_DESC(probe, "If zero, the driver won't start the card.");
 
-MODULE_PARM(proc_uid, "i");
+module_param(proc_uid, int, 0644);
 MODULE_PARM_DESC(proc_uid, "The uid that the /proc files will belong to.");
-MODULE_PARM(proc_gid, "i");
+module_param(proc_gid, int, 0644);
 MODULE_PARM_DESC(proc_gid, "The gid that the /proc files will belong to.");
-MODULE_PARM(airo_perm, "i");
+module_param(airo_perm, int, 0644);
 MODULE_PARM_DESC(airo_perm, "The permission bits of /proc/[driver/]aironet.");
-MODULE_PARM(proc_perm, "i");
+module_param(proc_perm, int, 0644);
 MODULE_PARM_DESC(proc_perm, "The permission bits of the files in /proc");
 
 /* This is a kind of sloppy hack to get this information to OUT4500 and

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Randy.Dunlap]

Stefan Sperling wrote:
> Replaces MODULE_PARM with module_param calls in airo wireless driver.
> 
> --- drivers/net/wireless/airo.c.orig	2004-11-09 20:40:21.867942880 +0100
> +++ drivers/net/wireless/airo.c	2004-11-08 23:09:27.000000000 +0100
> @@ -246,36 +246,36 @@ MODULE_DESCRIPTION("Support for Cisco/Ai
>  		   for PCMCIA when used with airo_cs.");
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_SUPPORTED_DEVICE("Aironet 4500, 4800 and Cisco 340/350");
> -MODULE_PARM(io,"1-4i");
> -MODULE_PARM(irq,"1-4i");
> -MODULE_PARM(basic_rate,"i");
> -MODULE_PARM(rates,"1-8i");
> -MODULE_PARM(ssids,"1-3s");
> -MODULE_PARM(auto_wep,"i");
> +module_param_array(io, int, NULL, 0444);
> +module_param_array(irq, int, NULL, 0444);
> +module_param(basic_rate, int, 0644);
> +module_param_array(rates, int, NULL, 0644);
> +module_param_array(ssids, int, NULL, 0644);
Why make these writable, even by root?
Why make them visible?

Rusty's first set of module_param() conversion patches, which
I mostly followed, use permission (last parameter) of 0
(not visible in sysfs) except for fields that can be modified.
Often this is a "debug" or "verbose" flag or bitmask.

Many of these shouldn't be exposed in sysfs IMO.

> +module_param(auto_wep, int, 0644);
>  MODULE_PARM_DESC(auto_wep, "If non-zero, the driver will keep looping through \
>  the authentication options until an association is made.  The value of \
>  auto_wep is number of the wep keys to check.  A value of 2 will try using \
>  the key at index 0 and index 1.");
> -MODULE_PARM(aux_bap,"i");
> +module_param(aux_bap, int, 0644);
>  MODULE_PARM_DESC(aux_bap, "If non-zero, the driver will switch into a mode \
>  than seems to work better for older cards with some older buses.  Before \
>  switching it checks that the switch is needed.");
> -MODULE_PARM(maxencrypt, "i");
> +module_param(maxencrypt, int, 0644);
>  MODULE_PARM_DESC(maxencrypt, "The maximum speed that the card can do \
>  encryption.  Units are in 512kbs.  Zero (default) means there is no limit. \
>  Older cards used to be limited to 2mbs (4).");
> -MODULE_PARM(adhoc, "i");
> +module_param(adhoc, int, 0644);
>  MODULE_PARM_DESC(adhoc, "If non-zero, the card will start in adhoc mode.");
> -MODULE_PARM(probe, "i");
> +module_param(probe, int, 0644);
>  MODULE_PARM_DESC(probe, "If zero, the driver won't start the card.");
>  
> -MODULE_PARM(proc_uid, "i");
> +module_param(proc_uid, int, 0644);
>  MODULE_PARM_DESC(proc_uid, "The uid that the /proc files will belong to.");
> -MODULE_PARM(proc_gid, "i");
> +module_param(proc_gid, int, 0644);
>  MODULE_PARM_DESC(proc_gid, "The gid that the /proc files will belong to.");
> -MODULE_PARM(airo_perm, "i");
> +module_param(airo_perm, int, 0644);
>  MODULE_PARM_DESC(airo_perm, "The permission bits of /proc/[driver/]aironet.");
> -MODULE_PARM(proc_perm, "i");
> +module_param(proc_perm, int, 0644);
>  MODULE_PARM_DESC(proc_perm, "The permission bits of the files in /proc");


-- 
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Stefan Sperling]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Tue, Nov 09, 2004 at 01:38:18PM -0800, Randy.Dunlap wrote:
> Why make these writable, even by root?
> Why make them visible?
> 
> Rusty's first set of module_param() conversion patches, which
> I mostly followed, use permission (last parameter) of 0
> (not visible in sysfs) except for fields that can be modified.
> Often this is a "debug" or "verbose" flag or bitmask.
> 
> Many of these shouldn't be exposed in sysfs IMO.

OK, I wasn't too sure about permissions anyway.
I thought the general idea was to allow autoconfiguration through sysfs,
and expose a driver's internal state to diagnostic tools in user space.
So I only 0444'd parameters that should obviously not be changed at runtime,
such as IO ports and IRQs.

I'll try again and make everything invisible except debug/verbosity flags.
Since you mentioned bitmasks, is it safe to make irq bitmasks writable as well?
I made them read-only.

thanks
stefan
-- 
Give me a fish and I will eat today.
Teach me to fish and I will eat forever.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Stefan Sperling]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On Tue, Nov 09, 2004 at 01:38:18PM -0800, Randy.Dunlap wrote:
> Stefan Sperling wrote:
 
> Rusty's first set of module_param() conversion patches, which
> I mostly followed, use permission (last parameter) of 0
> (not visible in sysfs) except for fields that can be modified.
> Often this is a "debug" or "verbose" flag or bitmask.

Not in this one:

http://marc.theaimsgroup.com/?l=linux-kernel&m=109826168201622&w=2

This is the reference link from the janitors TODO file.
Everything 0444, except a debug flag, which is 0644.

Can someone please clarify the "official guidelines" for
module parameter permissions in sysfs? Shall I use 0 or 0444
for parameters that should not be modified, i.e. everything
except debug and verbosity flags?

thanks
stefan
-- 
Give me a fish and I will eat today.
Teach me to fish and I will eat forever.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Randy.Dunlap]

Stefan Sperling wrote:
> On Tue, Nov 09, 2004 at 01:38:18PM -0800, Randy.Dunlap wrote:
> 
>>Stefan Sperling wrote:
> 
>  
> 
>>Rusty's first set of module_param() conversion patches, which
>>I mostly followed, use permission (last parameter) of 0
>>(not visible in sysfs) except for fields that can be modified.
>>Often this is a "debug" or "verbose" flag or bitmask.
> 
> 
> Not in this one:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x109826168201622&w=2

True.  That's why we post patches and have review (one reason).

> This is the reference link from the janitors TODO file.
> Everything 0444, except a debug flag, which is 0644.
> 
> Can someone please clarify the "official guidelines" for
> module parameter permissions in sysfs? Shall I use 0 or 0444
> for parameters that should not be modified, i.e. everything
> except debug and verbosity flags?

Compare with Rusty's (other) conversion patch (with wrap):

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc1/2.6.10-rc1-mm2/broken-out/remove-module_parm-from-allyesconfig-almost.patch

Greg KH just told me (paraphrased):
if it can be modified on the fly, put it into sysfs (permissions
non-zero).  If it cannot be modified, make permissions = 0.

-- 
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Tue, Nov 09, 2004 at 11:21:06PM +0100, Stefan Sperling wrote:
> On Tue, Nov 09, 2004 at 01:38:18PM -0800, Randy.Dunlap wrote:
> > Stefan Sperling wrote:
>  
> > Rusty's first set of module_param() conversion patches, which
> > I mostly followed, use permission (last parameter) of 0
> > (not visible in sysfs) except for fields that can be modified.
> > Often this is a "debug" or "verbose" flag or bitmask.
> 
> Not in this one:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=109826168201622&w=2
> 
> This is the reference link from the janitors TODO file.
> Everything 0444, except a debug flag, which is 0644.
> 
> Can someone please clarify the "official guidelines" for
> module parameter permissions in sysfs?

"When it makes sense to have it exposed to userspace"

Yeah, it's vague, sorry, but it all depends.

For things that can be changed on the fly, expose it.
For things that don't really matter, and no one will ever look them up,
don't.  I think the irq stuff is in the "don't" category, as almost no
one messes with them anymore.

Does this help?

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Randy.Dunlap]

Stefan Sperling wrote:
> On Tue, Nov 09, 2004 at 01:38:18PM -0800, Randy.Dunlap wrote:
> 
>>Why make these writable, even by root?
>>Why make them visible?
>>
>>Rusty's first set of module_param() conversion patches, which
>>I mostly followed, use permission (last parameter) of 0
>>(not visible in sysfs) except for fields that can be modified.
>>Often this is a "debug" or "verbose" flag or bitmask.
>>
>>Many of these shouldn't be exposed in sysfs IMO.
> 
> 
> OK, I wasn't too sure about permissions anyway.
> I thought the general idea was to allow autoconfiguration through sysfs,
> and expose a driver's internal state to diagnostic tools in user space.
> So I only 0444'd parameters that should obviously not be changed at runtime,
> such as IO ports and IRQs.
> 
> I'll try again and make everything invisible except debug/verbosity flags.
> Since you mentioned bitmasks, is it safe to make irq bitmasks writable as well?
> I made them read-only.

I mentioned bitmasks because some debug variables aren't boolean,
they are bit flags, or sometimes they are an integer (debug level).

No, I don't think that irq bitmasks should be writeable.
Having any system reource data writeable (like IRQ) is asking for
trouble IMO.


-- 
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Stefan Sperling]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Tue, Nov 09, 2004 at 02:29:07PM -0800, Randy.Dunlap wrote:
> >I'll try again and make everything invisible except debug/verbosity flags.
> >Since you mentioned bitmasks, is it safe to make irq bitmasks writable as 
> >well?
> >I made them read-only.
> 
> I mentioned bitmasks because some debug variables aren't boolean,
> they are bit flags, or sometimes they are an integer (debug level).

Oops, I misread that one, sorry.
I only found IRQ bitmasks among the parameters, thus I mixed that up...

> No, I don't think that irq bitmasks should be writeable.
> Having any system reource data writeable (like IRQ) is asking for
> trouble IMO.

no way! ;-)

thanks
stefan
-- 
Give me a fish and I will eat today.
Teach me to fish and I will eat forever.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Stefan Sperling]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Tue, Nov 09, 2004 at 02:29:01PM -0800, Greg KH wrote:
> > Can someone please clarify the "official guidelines" for
> > module parameter permissions in sysfs?
> 
> "When it makes sense to have it exposed to userspace"
> 
> Yeah, it's vague, sorry, but it all depends.
> 
> For things that can be changed on the fly, expose it.
> For things that don't really matter, and no one will ever look them up,
> don't.  I think the irq stuff is in the "don't" category, as almost no
> one messes with them anymore.
> 
> Does this help?

It does.

> thanks,
> 
> greg k-h

thanks
stefan
-- 
Give me a fish and I will eat today.
Teach me to fish and I will eat forever.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] convert airo wireless driver to module_param

[Stefan Sperling]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

On Tue, Nov 09, 2004 at 02:28:15PM -0800, Randy.Dunlap wrote:
> >>Rusty's first set of module_param() conversion patches, which
> >>I mostly followed, use permission (last parameter) of 0
> >>(not visible in sysfs) except for fields that can be modified.
> >>Often this is a "debug" or "verbose" flag or bitmask.
> >
> >
> >Not in this one:
> >
> >http://marc.theaimsgroup.com/?l=linux-kernel&m=109826168201622&w=2
> 
> True.  That's why we post patches and have review (one reason).
> 
> >This is the reference link from the janitors TODO file.
> >Everything 0444, except a debug flag, which is 0644.
> >
> >Can someone please clarify the "official guidelines" for
> >module parameter permissions in sysfs? Shall I use 0 or 0444
> >for parameters that should not be modified, i.e. everything
> >except debug and verbosity flags?
> 
> Compare with Rusty's (other) conversion patch (with wrap):
> 
> http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc1/2.6.10-rc1-mm2/broken-out/remove-module_parm-from-allyesconfig-almost.patch


Yikes! He's quick.
He's already converted some of the drivers I have been converting.
Last time I looked he hadn't touched the wireless drivers yet...

Well, at least he did not convert all of them yet... ;-)
I'll post the missing ones soon so.

stefan
-- 
Give me a fish and I will eat today.
Teach me to fish and I will eat forever.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors