On Mon, Feb 28, 2022 at 11:04:47AM +0530, Maninder Singh wrote: > with commit '82b37e632513 ("kallsyms: print module name in %ps/S > case when KALLSYMS is disabled"), module name printing was enhanced. > > As per suggestion from Petr Mladek <pmladek@xxxxxxxx>, covering > other flavours also to print build id also. > > for %pB no change as it needs to know symbol name to adjust address > value which can't be done without KALLSYMS. > > original output with KALLSYMS: > [8.842129] ps function_1 [crash] > [8.842735] pS function_1+0x4/0x2c [crash] > [8.842890] pSb function_1+0x4/0x2c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > [8.843175] pB function_1+0x4/0x2c [crash] > [8.843362] pBb function_1+0x4/0x2c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > > original output without KALLSYMS: > [12.487424] ps 0xffff800000eb008c > [12.487598] pS 0xffff800000eb008c > [12.487723] pSb 0xffff800000eb008c > [12.487850] pB 0xffff800000eb008c > [12.487967] pBb 0xffff800000eb008c > > With patched kernel without KALLSYMS: > [9.205207] ps 0xffff800000eb008c [crash] > [9.205564] pS 0xffff800000eb0000+0x8c [crash] > [9.205757] pSb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > [9.206066] pB 0xffff800000eb0000+0x8c [crash] > [9.206257] pBb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] ... > +static int sprint_module_info(char *buf, char *end, unsigned long value, > + const char *fmt) > +{ > + struct module *mod; > + unsigned long offset = 1; > + unsigned long base; > + int ret = 0; This is hard to find if it's not close to the first use. Since you are using positive numbers... > + const char *modname; > + int modbuildid = 0; > + int len; > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + const unsigned char *buildid = NULL; > +#endif > + > + if (is_ksym_addr(value)) > + return 0; > + if (*fmt == 'B' && fmt[1] == 'b') > + modbuildid = 1; > + else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) Why not to split to two conditionals? Would be easier to get, > + modbuildid = 1; > + else if (*fmt != 's') { These all are inconsistent, please switch to fmt[0]. > + /* > + * do nothing. > + */ > + } else > + offset = 0; > + > + preempt_disable(); > + mod = __module_address(value); > + if (mod) { > + ret = 1; > + modname = mod->name; > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + if (modbuildid) > + buildid = mod->build_id; > +#endif > + if (offset) { > + base = (unsigned long)mod->core_layout.base; > + offset = value - base; > + } > + } > + > + preempt_enable(); > + if (!ret) This looks a bit strange, but okay, I'm not familiar with the function of this code. > + return 0; > + > + /* address belongs to module */ > + if (offset) > + len = sprintf(buf, "0x%lx+0x%lx", base, offset); > + else > + len = sprintf(buf, "0x%lx", value); > + > + len += sprintf(buf + len, " [%s", modname); > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + if (modbuildid && buildid) { > + /* build ID should match length of sprintf */ > + static_assert(sizeof(typeof_member(struct module, build_id)) == 20); > + len += sprintf(buf + len, " %20phN", buildid); > + } > +#endif > + len += sprintf(buf + len, "]"); > + > + return len; > +} -- With Best Regards, Andy Shevchenko