Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Seth Forshee]

On Tue, Mar 12, 2013 at 07:54:33PM +0000, Matt Fleming wrote:
> On Mon, 2013-03-11 at 16:17 -0500, Seth Forshee wrote:
> > Here's a patch that does the command line option. I'm happy with either
> > one.
> 
> I like the idea, but isn't the logic backwards? I would have expected
> s/EFI_VARS_PSTORE_DEFAULT_DISABLE/EFI_VARS_PSTORE/g and then 'default y'
> in the Kconfig file to maintain backward compatibility?
> 
> Is there a reason that wouldn't work?

It should work, just a change of perspective I guess. When the feature
is enabled by default a foo_disable parameter feels more natural to me
than foo_enable, but it's just semantics. I've switched it to your
suggestion in the patch below. I must admit that the name of the config
option is much better this way.

> I know that Linus has previously denounced setting new Kconfig symbols
> to 'y' by default, but I think there's a case here for doing exactly
> that since the previous behaviour was always enabled. The networking
> folks did something similar recently, where they introduced new Kconfig
> symbols for existing functionality that was previously on by default.
> 
> [...]
> 
> > +#ifdef CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE
> > +static bool efivars_pstore_disable = true;
> > +#else
> > +static bool efivars_pstore_disable = false;
> > +#endif
> > +module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644);
> > +
> 
> static bool efivars_pstore_enable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE) ?

Yes, that's much nicer.