Hi, > > +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id, > > + int backtrace, int symbol) > > +{ > > + if (backtrace) > > + return __sprint_symbol(buffer, address, -1, 1, build_id); > > > + else if (symbol) > > + return __sprint_symbol(buffer, address, 0, 1, build_id); > > + else > > + return __sprint_symbol(buffer, address, 0, 0, 0); > > Redundant 'else' in both cases. > Ok, will change it > > +} > > ... > > > +static int sprint_module_info(char *buf, char *end, unsigned long value, > > + const char *fmt, int modbuildid, int backtrace, int symbol) > > fmt is not used. Yes, did not notice it.(will remove both end and gmt) > > +{ > > + struct module *mod; > > + unsigned long offset = 0; > > > + unsigned long base; > > Can it be the same type as core_layout.base? Why not? > > > + char *modname; > > + int len; > > + const unsigned char *buildid = NULL; > > + > > + if (is_ksym_addr(value)) > > + return 0; > > + > > + if (backtrace || symbol) > > + offset = 1; > > I would expect here to have > > else > offset = 0; > > But see below. > > > + preempt_disable(); > > + mod = __module_address(value); > > + if (mod) { > > + modname = mod->name; > > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > > + if (modbuildid) > > + buildid = mod->build_id; > > +#endif > > > + if (offset) { > > This seems quite confusing because semantically you use offset as a boolean > flag and offset. Why not add a boolean variable with a clear name? > Ok, will add 2 separate variables. > > + base = (unsigned long)mod->core_layout.base; > > + offset = value - base; > > + } > > + } > > > + > > Probably you can drop this blank line to group entire critical section, > or add a blank line above. > > > + preempt_enable(); > > + if (!mod) > > + return 0; > > + > > + /* address belongs to module */ > > + if (offset) > > + len = sprintf(buf, "0x%p+0x%lx", (void *)base, offset); > > + else > > > + len = sprintf(buf, "0x%lx", (void *)value); > > What this casting is for? Don't you have a compilation warning? My Bad, earlier I made patch with hashing this value also (%p), but after that changed it to %lx to have same original behavior in case of %ps, forgot to update final patch to remove typecast. > > > + len += fill_name_build_id(buf, modname, modbuildid, buildid, len); > > + return len; > > return len + ...; > > ? > > > +} Will modify patch with all changes. Thanks, Maninder Singh
Attachment:
rcptInfo.txt
Description: Binary data