From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH] ALSA: get rid of CONFIG_SND_VERBOSE_PRINTK Date: Wed, 05 Jun 2013 08:04:59 +0200 Message-ID: References: <1370375134.2385.64.camel@joe-AO722> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 65C942608E8 for ; Wed, 5 Jun 2013 08:04:21 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Alan Stern Cc: Joe Perches , alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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