Re: [PATCH 1/2] ALSA: convert "snd_printk(KERN_INFO" to "pr_info("

[Takashi Iwai <tiwai@suse.de>]

At Thu, 6 Jun 2013 16:54:06 -0400 (EDT),
Alan Stern wrote:
> 
> On Wed, 5 Jun 2013, Takashi Iwai wrote:
> 
> > At Tue, 4 Jun 2013 13:20:40 -0400 (EDT),
> > Alan Stern wrote:
> > > 
> > > The snd_printk() function prints kernel log messages, including the
> > > filename and line number if CONFIG_SND_PRINTK_VERBOSE is enabled.
> > > This may make sense for errors and warnings, but not for informational
> > > messages.  For those, a simple pr_info() is what we want.
> > > 
> > > This patch mechanically converts all occurrences of
> > > "snd_printk(KERN_INFO" to "pr_info(".  It doesn't try to tell whether
> > > the message really is informational; it relies on the existing
> > > KERN_INFO tag.
> > 
> > I agree conversion in this way.  But looking at the patch, some places
> > should be better convert with pr_warning() or pr_err().
> 
> How often do those places get printed?  I would assume that warnings 
> and errors are relatively infrequent, in which case there would be no 
> harm in changing "snd_printk(KERN_INFO" to "snd_printk(KERN_WARN" or 
> "snd_printk(KERN_ERR".

Yes.  That's my intention, too.  These shouldn't have been KERN_INFO
from the beginning.

> > Also, many places miss proper prefix, thus you'll still see the
> 
> What exactly do you mean by "proper prefix"?  KBUILD_MODNAME?  The 
> filename and line number?  The device and driver names?  Or something 
> else?

I don't mind any way if it's unique enough to identify from where the
message comes, but some people prefer (over-)unification.
Usually KBUILD_MODNAME should suffice, I guess, but still function
name or file name might be needed in addition if similar messages
appear multiple times in the same driver.  This must be all
case-by-case decision.

> > original issue the thread started from (no clue who prints the stuff).
> > This is because originally snd_printk() printed the prefix and line
> > number always.  CONFIG_SND_VERBOSE_PRINTK was introduced later since
> > some people complained about too verbose output, IIRC, while many
> > codes weren't fixed to give a proper prefix with
> > CONFIG_SND_VERBOSE_PRINTK=n.
> 
> After this patch (or after an updated version of this patch), none of 
> the snd_printk calls will be informational.  They will all be things 
> like errors and warnings.  This means they won't get printed very 
> often, so always using the verbose form shouldn't make things too bad.

Yes, it's the reason I suggested to apply always
CONFIG_SND_VERBOSE_PRINTK and drop the kconfig :)

> > Just taking a quick glance:
> > 
> > > --- usb-3.10.orig/sound/isa/opti9xx/miro.c
> > > +++ usb-3.10/sound/isa/opti9xx/miro.c
> > > @@ -1346,11 +1346,11 @@ static int snd_miro_probe(struct snd_car
> > >  		default:
> > >  			sprintf(card->shortname, 
> > >  				"unknown miro");
> > > -			snd_printk(KERN_INFO "unknown miro aci id\n");
> > > +			pr_info("unknown miro aci id\n");
> > >  			break;
> > >  		}
> > >  	} else {
> > > -		snd_printk(KERN_INFO "found unsupported aci card\n");
> > > +		pr_info("found unsupported aci card\n");
> > >  		sprintf(card->shortname, "unknown Cardinal Technologies");
> > 
> > These need proper prefix, and should be rather pr_warning().
> 
> I don't understand.  pr_warning doesn't print any prefixes, by default.  
> They would have to added to the format string.  Why not change it to 
> "snd_printk(KERN_WARN"?

Yes, this should be snd_printk(KERN_WARN) *and* it needs a proper
prefix depending on CONFIG_SND_VERBOSE_PRINTK:

#ifdef CONFIG_SND_VERBOSE_PRINTK
#define PREFIX	""
#else
#define PREFIX	"miro: "
#endif
....
	snd_printk(KERN_WARNING PFX "found unsupported aci card\n");

The same check should be applied to all snd_printk() lines.

Once after all snd_printk() have the unified prefix, we can convert
straightforwardly like

#define pr_fmt	"miro: "
....
	pr_warn("found unsupported aci card\n");


In other words, toward the final conversions, we need to audit:
- Whether each message is really marked properly with KERN_* level,
- Whether messages have consistent prefix through the whole module,
- whether messages are unique enough to be identified,
- optionally, check typos


Takashi