Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0350c21bcfdc896f2b912363f221958d41ebf1e1.camel@perches.com>
Subject: Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*()
 macro
From:   Joe Perches <joe@perches.com>
To:     Takashi Iwai <tiwai@suse.de>, linux-fsdevel@vger.kernel.org
Cc:     Namjae Jeon <linkinjeon@kernel.org>,
        Sungjong Seo <sj1557.seo@samsung.com>,
        linux-kernel@vger.kernel.org
Date:   Sat, 23 Jul 2022 00:42:12 -0700
In-Reply-To: <20220722142916.29435-4-tiwai@suse.de>
References: <20220722142916.29435-1-tiwai@suse.de>
         <20220722142916.29435-4-tiwai@suse.de>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.44.1-0ubuntu1 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2022-07-22 at 16:29 +0200, Takashi Iwai wrote:
> Currently the error and info messages handled by exfat_err() and co
> are tossed to exfat_msg() function that does nothing but passes the
> strings with printk() invocation.  Not only that this is more overhead
> by the indirect calls, but also this makes harder to extend for the
> debug print usage; because of the direct printk() call, you cannot
> make it for dynamic debug or without debug like the standard helpers
> such as pr_debug() or dev_dbg().
> 
> For addressing the problem, this patch replaces exfat_msg() function
> with a macro to expand to pr_*() directly.  This allows us to create
> exfat_debug() macro that is expanded to pr_debug() (which output can
> gracefully suppressed via dyndbg).
[]
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
[]
> @@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
>  #define exfat_fs_error_ratelimit(sb, fmt, args...) \
>  		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
>  		fmt, ## args)
> -void exfat_msg(struct super_block *sb, const char *lv, const char *fmt, ...)
> -		__printf(3, 4) __cold;
> +
> +/* expand to pr_xxx() with prefix */
> +#define exfat_msg(sb, lv, fmt, ...) \
> +	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> +
>  #define exfat_err(sb, fmt, ...)						\
> -	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
> +	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
>  #define exfat_warn(sb, fmt, ...)					\
> -	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
> +	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
>  #define exfat_info(sb, fmt, ...)					\
> -	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
> +	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
> +#define exfat_debug(sb, fmt, ...)					\
> +	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)

I think this would be clearer using pr_<level> directly instead
of an indirecting macro that uses concatenation of <level> that
obscures the actual use of pr_<level>

Either: (and this first option would be my preference)

#define exfat_err(sb, fmt, ...) \
	pr_err("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
#define exfat_warn(sb, fmt, ...) \
	pr_warn("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
etc...

or using an indirecting macro:

#define exfat_printk(pr_level, sb, fmt, ...)	\
	pr_level("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)

#define exfat_err(sb, fmt, ...)
	exfat_printk(pr_err, sb, fmt, ##__VA_ARGS)
#define exfat_warn(sb, fmt, ...)
	exfat_printk(pr_warn, sb, fmt, ##__VA_ARGS)
etc...

and btw, there are multiple uses of exfat_<level> output with a
unnecessary and duplicated '\n' that the macro already adds that
should be removed:

$ git grep -P -n '\bexfat_(err|warn|info).*\\n' fs/exfat/
fs/exfat/fatent.c:334:                  exfat_err(sb, "sbi->clu_srch_ptr is invalid (%u)\n",
fs/exfat/nls.c:674:                     exfat_err(sb, "failed to read sector(0x%llx)\n",
fs/exfat/super.c:467:           exfat_err(sb, "bogus sector size bits : %u\n",
fs/exfat/super.c:476:           exfat_err(sb, "bogus sectors bits per cluster : %u\n",