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;
snd_printk(KERN_INFO "found unsupported aci card\n");
pr_info("found unsupported aci card\n");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