| Message ID | 1429572637-30234-5-git-send-email-mcgrof@do-not-panic.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
Hi Luis, You made a spelling mistake: On Tue, Apr 21, 2015 at 9:30 AM, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote: > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > This adds a couple of bool module_param_config_*() helpers > which are designed to let us easily associate a booloean > module parameter with an associated kernel configuration > option, and to help us remove #ifdef'ery eyesores. > > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Hannes Reinecke <hare@suse.de> > Cc: Kees Cook <keescook@chromium.org> > Cc: Tejun Heo <tj@kernel.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: linux-kernel@vger.kernel.org > Cc: cocci@systeme.lip6.fr > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> > --- > include/linux/moduleparam.h | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h > index 7e00799..fdf7b87 100644 > --- a/include/linux/moduleparam.h > +++ b/include/linux/moduleparam.h > @@ -155,6 +155,43 @@ struct kparam_array > __MODULE_PARM_TYPE(name, #type) > > /** > + * module_param_config_on_off - bool parameter with run time override > + * @name: a valid C identifier which is the parameter name. > + * @value: the actual lvalue to alter. > + * @perm: visibility in sysfs. > + * @config: kernel parameter which will enable this option if this > + * kernel configuration option has been enabled. > + * > + * This lets you define a bool module paramter which by default will be s/paramter/parameter/ > + * set to true if the config option has been set on your kernel's > + * configuration, otherwise it is set to false. > + */ > +#define module_param_config_on_off(name, var, perm, config) \ > + static bool var = IS_ENABLED(config); \ > + module_param_named(name, var, bool, perm); > + > +/** > + * module_param_config_on - bool parameter with run time enablement override > + * @name: a valid C identifier which is the parameter name. > + * @value: the actual lvalue to alter. > + * @perm: visibility in sysfs. > + * @config: kernel parameter which will enable this option if this > + * kernel configuration option has been enabled. > + * > + * This lets you define a bool module paramter which by default will be Here too. Thanks,
On Mon, Apr 20, 2015 at 04:30:35PM -0700, Luis R. Rodriguez wrote: > /** > + * module_param_config_on_off - bool parameter with run time override > + * @name: a valid C identifier which is the parameter name. > + * @value: the actual lvalue to alter. > + * @perm: visibility in sysfs. > + * @config: kernel parameter which will enable this option if this > + * kernel configuration option has been enabled. > + * > + * This lets you define a bool module paramter which by default will be > + * set to true if the config option has been set on your kernel's > + * configuration, otherwise it is set to false. > + */ > +#define module_param_config_on_off(name, var, perm, config) \ > + static bool var = IS_ENABLED(config); \ > + module_param_named(name, var, bool, perm); Maybe we want to make @config just a boolean initializer? e.g. something like #define module_param_config_on_off(name, var, perm, on_off) \ static bool var = on_off; \ module_param_named(name, var, bool, perm); so that the caller does IS_ENABLED() or whatever that's necessary? It just seems a bit too restricted. Thanks.
On Tue, Apr 21, 2015 at 09:42:21AM +1000, Julian Calaby wrote: > Hi Luis, > > You made a spelling mistake: > > On Tue, Apr 21, 2015 at 9:30 AM, Luis R. Rodriguez > <mcgrof@do-not-panic.com> wrote: > > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > > > This adds a couple of bool module_param_config_*() helpers > > which are designed to let us easily associate a booloean > > module parameter with an associated kernel configuration > > option, and to help us remove #ifdef'ery eyesores. > > > > Cc: Rusty Russell <rusty@rustcorp.com.au> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Christoph Hellwig <hch@infradead.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: Hannes Reinecke <hare@suse.de> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Tejun Heo <tj@kernel.org> > > Cc: Ingo Molnar <mingo@kernel.org> > > Cc: linux-kernel@vger.kernel.org > > Cc: cocci@systeme.lip6.fr > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> > > --- > > include/linux/moduleparam.h | 37 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h > > index 7e00799..fdf7b87 100644 > > --- a/include/linux/moduleparam.h > > +++ b/include/linux/moduleparam.h > > @@ -155,6 +155,43 @@ struct kparam_array > > __MODULE_PARM_TYPE(name, #type) > > > > /** > > + * module_param_config_on_off - bool parameter with run time override > > + * @name: a valid C identifier which is the parameter name. > > + * @value: the actual lvalue to alter. > > + * @perm: visibility in sysfs. > > + * @config: kernel parameter which will enable this option if this > > + * kernel configuration option has been enabled. > > + * > > + * This lets you define a bool module paramter which by default will be > > s/paramter/parameter/ > > > + * set to true if the config option has been set on your kernel's > > + * configuration, otherwise it is set to false. > > + */ > > +#define module_param_config_on_off(name, var, perm, config) \ > > + static bool var = IS_ENABLED(config); \ > > + module_param_named(name, var, bool, perm); > > + > > +/** > > + * module_param_config_on - bool parameter with run time enablement override > > + * @name: a valid C identifier which is the parameter name. > > + * @value: the actual lvalue to alter. > > + * @perm: visibility in sysfs. > > + * @config: kernel parameter which will enable this option if this > > + * kernel configuration option has been enabled. > > + * > > + * This lets you define a bool module paramter which by default will be > > Here too. Fixed, thanks. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 21, 2015 at 11:21:36AM -0400, Tejun Heo wrote: > On Mon, Apr 20, 2015 at 04:30:35PM -0700, Luis R. Rodriguez wrote: > > /** > > + * module_param_config_on_off - bool parameter with run time override > > + * @name: a valid C identifier which is the parameter name. > > + * @value: the actual lvalue to alter. > > + * @perm: visibility in sysfs. > > + * @config: kernel parameter which will enable this option if this > > + * kernel configuration option has been enabled. > > + * > > + * This lets you define a bool module paramter which by default will be > > + * set to true if the config option has been set on your kernel's > > + * configuration, otherwise it is set to false. > > + */ > > +#define module_param_config_on_off(name, var, perm, config) \ > > + static bool var = IS_ENABLED(config); \ > > + module_param_named(name, var, bool, perm); > > Maybe we want to make @config just a boolean initializer? > e.g. something like > > #define module_param_config_on_off(name, var, perm, on_off) \ > static bool var = on_off; \ > module_param_named(name, var, bool, perm); > > so that the caller does IS_ENABLED() or whatever that's necessary? It > just seems a bit too restricted. A use then would be for instance: module_param_config_on_off(power_efficient, wq_power_efficient, 0444, IS_ENABLED(CONFIG_WQ_POWER_EFFICIENT_DEFAULT)); this as an alternative would enable use of other static / global variables but I'm not sure if these are good use cases to promote, given that all this is to help with initial set up, so I believe the restrictions are for the better. Let me know, we already have early_param_on_off() relying on @config so we should consider both and/or address early_param_on_off() as well while at it. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Luis. On Tue, Apr 21, 2015 at 06:55:16PM +0200, Luis R. Rodriguez wrote: > A use then would be for instance: > > module_param_config_on_off(power_efficient, wq_power_efficient, 0444, > IS_ENABLED(CONFIG_WQ_POWER_EFFICIENT_DEFAULT)); > > this as an alternative would enable use of other static / global variables but > I'm not sure if these are good use cases to promote, given that all this is to > help with initial set up, so I believe the restrictions are for the better. I was thinking more of cases where CONFIG should be inverted or and/or'd. In general I don't think we conventionally embed IS_ENABLED() in this sort of macros. It just jumps at me as a weird restriction. What do others think? Thanks.
"Luis R. Rodriguez" <mcgrof@do-not-panic.com> writes: > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > This adds a couple of bool module_param_config_*() helpers > which are designed to let us easily associate a booloean > module parameter with an associated kernel configuration > option, and to help us remove #ifdef'ery eyesores. But they don't. And I had to read the descriptions twice to understand what you're doing. eg you use it like this: -#ifdef CONFIG_WQ_POWER_EFFICIENT_DEFAULT -static bool wq_power_efficient = true; -#else -static bool wq_power_efficient; -#endif - -module_param_named(power_efficient, wq_power_efficient, bool, 0444); +module_param_config_on_off(power_efficient, wq_power_efficient, 0444, CONFIG_WQ_POWER_EFFICIENT_DEFAULT); It would be much clearer to do this: -#ifdef CONFIG_WQ_POWER_EFFICIENT_DEFAULT -static bool wq_power_efficient = true; -#else -static bool wq_power_efficient; -#endif +static bool wq_power_efficient = IS_ENABLED(CONFIG_WQ_POWER_EFFICIENT_DEFAULT); I know exactly what that does without having to notice the difference between module_param_config_on_off() and module_param_config_on(). Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 22, 2015 at 04:45:04PM +0930, Rusty Russell wrote: > "Luis R. Rodriguez" <mcgrof@do-not-panic.com> writes: > > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > > > This adds a couple of bool module_param_config_*() helpers > > which are designed to let us easily associate a booloean > > module parameter with an associated kernel configuration > > option, and to help us remove #ifdef'ery eyesores. > > But they don't. And I had to read the descriptions twice to understand > what you're doing. > > eg you use it like this: > > -#ifdef CONFIG_WQ_POWER_EFFICIENT_DEFAULT > -static bool wq_power_efficient = true; > -#else > -static bool wq_power_efficient; > -#endif > - > -module_param_named(power_efficient, wq_power_efficient, bool, 0444); > +module_param_config_on_off(power_efficient, wq_power_efficient, 0444, > CONFIG_WQ_POWER_EFFICIENT_DEFAULT); > > It would be much clearer to do this: > > -#ifdef CONFIG_WQ_POWER_EFFICIENT_DEFAULT > -static bool wq_power_efficient = true; > -#else > -static bool wq_power_efficient; > -#endif > +static bool wq_power_efficient = IS_ENABLED(CONFIG_WQ_POWER_EFFICIENT_DEFAULT); > > I know exactly what that does without having to notice the difference > between module_param_config_on_off() and module_param_config_on(). You're right, I forgot a small step patch in between to make the change clearer. I can add that in my next respin, anything else or do the other changes look OK? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h index 7e00799..fdf7b87 100644 --- a/include/linux/moduleparam.h +++ b/include/linux/moduleparam.h @@ -155,6 +155,43 @@ struct kparam_array __MODULE_PARM_TYPE(name, #type) /** + * module_param_config_on_off - bool parameter with run time override + * @name: a valid C identifier which is the parameter name. + * @value: the actual lvalue to alter. + * @perm: visibility in sysfs. + * @config: kernel parameter which will enable this option if this + * kernel configuration option has been enabled. + * + * This lets you define a bool module paramter which by default will be + * set to true if the config option has been set on your kernel's + * configuration, otherwise it is set to false. + */ +#define module_param_config_on_off(name, var, perm, config) \ + static bool var = IS_ENABLED(config); \ + module_param_named(name, var, bool, perm); + +/** + * module_param_config_on - bool parameter with run time enablement override + * @name: a valid C identifier which is the parameter name. + * @value: the actual lvalue to alter. + * @perm: visibility in sysfs. + * @config: kernel parameter which will enable this option if this + * kernel configuration option has been enabled. + * + * This lets you define a bool module paramter which by default will be + * set to true if the config option has been set on your kernel's + * configuration, otherwise it is set to false. This particular helper + * will ensure that if the kernel configuration has been set you will not + * be able to disable this kernel parameter. You can only use this to let + * the an option that was disabled on your kernel configuration be enabled + * at run time. + */ +#define module_param_config_on(name, var, perm, config) \ + static bool var = IS_ENABLED(config); \ + module_param_named(name, var, bool_enable_only, perm); + + +/** * module_param_cb - general callback for a module/cmdline parameter * @name: a valid C identifier which is the parameter name. * @ops: the set & get operations for this parameter.