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