Re: [PATCH] ALSA: get rid of CONFIG_SND_VERBOSE_PRINTK

[Takashi Iwai <tiwai@suse.de>]

At Thu, 6 Jun 2013 17:28:52 -0400 (EDT),
Alan Stern wrote:
> 
> On Wed, 5 Jun 2013, Takashi Iwai wrote:
> 
> > > 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().
> 
> I don't understand the point of this.  The idea of using snd_printd() 
> is to prevent these messages from being printed unless debugging is 
> enabled.

Originally, yes, but differently developed along time.

>  Since the only time you are going to see these messages is 
> when CONFIG_SND_DEBUG is on, why shouldn't they use KERN_DEBUG?

KERN_DEBUG appears in dmesg normally, no?

> > 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.
> 
> Changing snd_printd() to use KERN_DEBUG always won't make the driver 
> any less reticent.
> 
> > 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().
> 
> Okay.  Those can be changed separately.
> 
> > 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...
> 
> I'm not sure if my original point behind all this is still clear.  I
> don't really want to make a lot of small updates to the logging API for
> the sound subsystem, or clean up messages that have the wrong logging
> level.
> 
> The important point is that these files are _drivers_; what matters at
> runtime is the device name.  On systems with more than one sound card
> of the same type, the device name is vital, but even on others it still
> helps a lot.
> 
> What I want to do is make the messages include the device and driver
> names.  Basically this means calling dev_info() and friends instead of
> "snd_printk(KERN_INFO" or pr_info().  As part of the fallout from this
> change, it should no longer be necessary to print the filename or
> module name.  If a driver contains multiple identical messages, they
> can be made non-identical (assuming it really matters; messages like
> "can't allocate memory" usually don't need to be pinned down exactly).  
> Line numbers are even less useful because they can change from one
> kernel version to the next.
> 
> I can't convert the entire sound subsystem at once -- it's way too big.  
> But I can at least do the sound/usb subsystem.  If this means that two
> different logging APIs are used by different sets of sound drivers, so
> be it.
> 
> In theory, this would be more or less independent of the patches posted 
> so far.  In fact, when taken to its logical extreme, _all_ the messages 
> would use dev_*() or some variant.  There wouldn't be any snd_printk(), 
> snd_printd(), pr_info(), ... messages left to worry about.
> 
> I don't expect that to happen any time soon.  Still, I think it makes
> more sense to concentrate on converting the messages below sound/usb
> rather than fiddling around with the existing API, which is inadequate
> anyway since it doesn't include the device.

Yeah, dev_*() is more appropriate in many places.
Feel free to work on replacement with dev_*() variants for
sound/usb/*.  I prefer such bottom up cleanups, actually.


thanks,

Takashi