Re: [PATCH] ALSA: get rid of CONFIG_SND_VERBOSE_PRINTK

[Takashi Iwai <tiwai@suse.de>]

At Tue, 4 Jun 2013 16:54:12 -0400 (EDT),
Alan Stern wrote:
> 
> On Tue, 4 Jun 2013, Joe Perches wrote:
> 
> > A somewhat common convention is to use
> > 
> > 	prefix_dbg(level, fmt, ...)
> > 
> > where level is either a numeric value or bitmask,
> > and also is either a #define or a MODULE_PARAM
> > 
> > today sound/misc.c has:
> > 
> > ------------------------------------------
> > 
> > #ifdef CONFIG_SND_DEBUG
> > 
> > #ifdef CONFIG_SND_DEBUG_VERBOSE
> > #define DEFAULT_DEBUG_LEVEL	2
> > #else
> > #define DEFAULT_DEBUG_LEVEL	1
> > #endif
> > 
> > static int debug = DEFAULT_DEBUG_LEVEL;
> > module_param(debug, int, 0644);
> > MODULE_PARM_DESC(debug, "Debug level (0 = disable)");
> > 
> > #endif /* CONFIG_SND_DEBUG */
> 
> Yes, I have read this part of the code.
> 
> > ------------------------------------------
> > 
> > I suggest converting all the remaining
> > 
> > snd_printd(...) to snd_dbg(1, ...)
> > and
> > snd_printdd(...) to snd_dbg(2, ...)
> > 
> > so that debug module param control can be used for
> > all these and if the DEFAULT_DEBUG_LEVEL isn't
> > high enough, the various snd_dbg(2, ...) can be
> > completely optimized away if appropriate.
> 
> I don't see how DEFAULT_DEBUG_LEVEL can be used to optimize away
> anything.  The user can always change the value of the "debug" module
> parameter while the system is running.  The only valid opportunity for
> optimization would be if CONFIG_SND_DEBUG was disabled; then all these 
> messages would disappear.
> 
> You didn't respond to the first point I raised.  Since these messages
> are all meant for debugging, there's no point allowing them to have
> prefixes like KERN_ERR or KERN_INFO.  They should always be printed at
> the KERN_DEBUG level.  Or did you think this was so obviously true that
> it didn't require any comment?

Unfortunately, it's not so straightforward.  Many messages are better
with KERN_INFO indeed.  In such places, snd_printd() is used rather as
snd_chattier_printk_with_prefix().

This comes from the subsystem development history.  In the era of 2.4
kernels, many systems did load/unload the module frequently on demand.
Thus we didn't want to put a message from driver at each module probe
or removal.  Otherwise dmesg will be flood of such messages at each
time you play a WAV song or ring a beep via PCM.

Instead, we tended to put such informational messages as snd_printd(),
while keeping the driver itself reticent as much as possible for
"productive" systems.  This style was kept for a while even after
merged to 2.5 kernels until recently.

Also, some places use KERN_WARNING or KERN_ERR with snd_printd(),
mostly because they are in the context with CONFIG_SND_DEBUG.
They can be well pr_warning() or pr_err().

Last but not least, as mentioned in the reply to patch 1/2, many
messages miss proper prefixes.


These are the main reasons I stated that systematic replacements would
be difficult.  We need to take a look through the whole snd_print*()
and replace appropriately case by case...


thanks,

Takashi