On Sat, 23 Jul 2022 09:42:12 +0200, Joe Perches wrote: > > 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... IMO, it's a matter of taste, and I don't mind either way. Just let me know. > or using an indirecting macro: > > #define exfat_printk(pr_level, sb, fmt, ...) \ > pr_level("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__) Is pr_level() defined anywhere...? > > 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", Right, that should be addressed in another patch. thanks, Takashi