Re: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Mar 15, 2022 at 03:21:12PM +0530, Maninder Singh wrote:
> print module information when KALLSYMS is disabled.
> 
> No change for %pB, as it needs to know symbol name to adjust address
> value which can't be done without KALLSYMS.

...

> +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.

> +}

...

> +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.

> +{
> +	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?

> +			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?

> +	len += fill_name_build_id(buf, modname, modbuildid, buildid, len);
> +	return len;

	return len + ...;

?

> +}

-- 
With Best Regards,
Andy Shevchenko