diff mbox

[v1,4/6] moduleparam.h: add module_param_config_*() helpers

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

Commit Message

Luis R. Rodriguez April 20, 2015, 11:30 p.m. UTC
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(+)

Comments

Julian Calaby April 20, 2015, 11:42 p.m. UTC | #1
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,
Tejun Heo April 21, 2015, 3:21 p.m. UTC | #2
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.
Luis Chamberlain April 21, 2015, 4:41 p.m. UTC | #3
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
Luis Chamberlain April 21, 2015, 4:55 p.m. UTC | #4
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
Tejun Heo April 21, 2015, 8:58 p.m. UTC | #5
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.
Rusty Russell April 22, 2015, 7:15 a.m. UTC | #6
"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
Luis Chamberlain April 22, 2015, 3:42 p.m. UTC | #7
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 mbox

Patch

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.