Re: [PATCHv3] Make CONFIG_EXPERT select CONFIG_DEBUG_KERNEL to unhide debug options

[Josh Triplett <josh@joshtriplett.org>]

On Mon, Jun 06, 2011 at 08:16:09PM +0200, Frederic Weisbecker wrote:
> On Mon, Jun 06, 2011 at 09:51:30AM -0700, Josh Triplett wrote:
> > Several debugging options currently default to y, such as
> > CONFIG_DEBUG_BUGVERBOSE and CONFIG_DEBUG_RODATA.  Embedded users might
> > want to turn those options off to save space; however, turning them off
> > requires turning on CONFIG_DEBUG_KERNEL to unhide them.  Since
> > CONFIG_DEBUG_KERNEL exists specifically to unhide debugging options, and
> > CONFIG_EXPERT exists specifically to unhide options potentially needed
> > by experts and/or embedded users, make CONFIG_EXPERT automatically imply
> > CONFIG_DEBUG_KERNEL.
> > 
> > Change various debugging options to only reference DEBUG_KERNEL, not
> > EXPERT.
> 
> Ok, so ideally this should have been done in two patches. They are both
> different logical changes that don't imply the same.

Fair enough; I'd considered whether to split this patch or not, and
ended up with "not" on the theory that this seemed like a case of "fix
an API and fix up the callers", but I can easily split it in v4.

> v2 was fine, and should be an independent change.
> 
> But some comment on what was added in this v3 below:
> 
> > Note that DEBUG_MEMORY_INIT defaulted to !EXPERT, which seemed wrong,
> > since EXPERT should not directly affect anything except the availability
> > of other options.  (And EXPERT definitely shouldn't mean "implicitly
> > turn off default safety features".)  Change it to default to y, which
> > means that turning on EXPERT does not automatically disable it, but will
> > provide the option to disable it.
> > 
> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> > ---
> > 
> > v3: Changed other debugging options to stop referencing EXPERT.
> > 
> >  arch/tile/Kconfig.debug |    2 +-
> >  init/Kconfig            |    2 ++
> >  lib/Kconfig.debug       |    6 +++---
> >  3 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/tile/Kconfig.debug b/arch/tile/Kconfig.debug
> > index ddbfc33..afc9cf3 100644
> > --- a/arch/tile/Kconfig.debug
> > +++ b/arch/tile/Kconfig.debug
> > @@ -3,7 +3,7 @@ menu "Kernel hacking"
> >  source "lib/Kconfig.debug"
> >  
> >  config EARLY_PRINTK
> > -	bool "Early printk" if EXPERT && DEBUG_KERNEL
> > +	bool "Early printk" if DEBUG_KERNEL
> >  	default y
> 
> I don't understand why early_printk is default y. But that's debatable,
> we may want to have any user able to run a raw vga console for debugging
> requests.
> 
> Either:
> 
> - if we consider it's very important and mandatory, let's keep it "if EXPERT"
> and default y.
> 
> - if we consider it's only used by kernel developers, it should be off by
> default and depend on DEBUG_KERNEL only.

Note that this change occurs in arch/tile/Kconfig.debug; you'd have to
ask the Tile architecture developers about the rationale there.  For the
purposes of this change, I'd prefer not to make any semantic change
there.  Based on your explanation, that probably means I should change
it to "if EXPERT" rather than "if DEBUG_KERNEL", keeping the current
behavior of "only experts can turn this off".  I'll do that in v4.

> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -694,7 +694,7 @@ config DEBUG_HIGHMEM
> >  	  Disable for production systems.
> >  
> >  config DEBUG_BUGVERBOSE
> > -	bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL && EXPERT
> > +	bool "Verbose BUG() reporting (adds 70K)" if DEBUG_KERNEL
> 
> I believe this should not be changed, let keep it how it is, or change to
> "if EXPERT" because EXPERT selects DEBUG_KERNEL anyway.
> 
> There are no good reason to disable this option, except for some embedded
> system perhaps, so EXPERT still make sense here. 

I guess this matches the rationale above for tile's EARLY_PRINTK; I'll
use "if EXPERT" to preserve the behavior of "only experts can turn this
off".  To me it seems equivalent to any other debug option, and I don't
really see the harm in allowing users to turn it off if they turned on
DEBUG_KERNEL, but nevertheless this patch shouldn't make that kind of
semantic change.

- Josh Triplett