[alsa-devel] [PATCH 1/2] ALSA: convert "snd_printk(KERN_INFO" to "pr_info("
stern at rowland.harvard.edu
Fri Jun 7 17:51:14 CEST 2013
On Fri, 7 Jun 2013, Takashi Iwai wrote:
> > > 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.
For the messages that get converted to "snd_printk(KERN_ERR", the
prefix added by snd_printk() when CONFIG_SND_VERBOSE_PRINTK is enabled
should be sufficient. Following the 2/2 patch in this series, that
symbol will effectively _always_ be enabled. Therefore these messages
shouldn't need any extra prefix.
The ones that get converted to pr_info() probably will need a short
prefix of some kind, though.
> > > 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 ""
> #define PREFIX "miro: "
> snd_printk(KERN_WARNING PFX "found unsupported aci card\n");
There's no point using PREFIX (or PFX) here, since the 2/2 patch would
eliminate the second #define anyway.
> 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");
Ultimately, it will be better to change these to dev_warn() or
something equivalent. Then the "miro:" prefix wouldn't be needed.
However, doing what you suggest might be a reasonable intermediate
step, aside from for all the additional work required.
> 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
I'm willing to do the first step for the messages that were altered in
the 1/2 patch (but I don't want to audit every message under sound/).
The second step shouldn't be needed, according to the reasoning above.
The third step is something that will have to be done on a
driver-by-driver basis, since it requires looking through all the
messages in each driver at once to determine if there are any
duplicates. I don't want to tackle that (except perhaps for the USB
Finally, typos can be fixed separately.
More information about the Alsa-devel