From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 1/2] ALSA: convert "snd_printk(KERN_INFO" to "pr_info(" Date: Fri, 07 Jun 2013 07:41:14 +0200 Message-ID: References: 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 0C4192655D3 for ; Fri, 7 Jun 2013 07:40:40 +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 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