Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Mar 2022 16:17:40 +0530
Message-ID: <2032885049.3816543.1647341260934@mail-kr5-2>
Mime-Version: 1.0
Subject: RE: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is
 disabled
Reply-To: maninder1.s@samsung.com
Sender: Maninder Singh <maninder1.s@samsung.com>
From:   Maninder Singh <maninder1.s@samsung.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC:     "mcgrof@kernel.org" <mcgrof@kernel.org>,
        "pmladek@suse.com" <pmladek@suse.com>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "senozhatsky@chromium.org" <senozhatsky@chromium.org>,
        "linux@rasmusvillemoes.dk" <linux@rasmusvillemoes.dk>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "wangkefeng.wang@huawei.com" <wangkefeng.wang@huawei.com>,
        Vaneet Narang <v.narang@samsung.com>,
        "swboyd@chromium.org" <swboyd@chromium.org>,
        "ojeda@kernel.org" <ojeda@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        "avimalin@gmail.com" <avimalin@gmail.com>,
        "atomlin@redhat.com" <atomlin@redhat.com>
In-Reply-To: <YjBq1jg9cNgEqB8T@smile.fi.intel.com>
Content-Type: multipart/mixed;
        boundary="----=_Part_3816542_2122723889.1647341260934"
CMS-TYPE: 105P
References: <YjBq1jg9cNgEqB8T@smile.fi.intel.com>
        <20220315095112.439580-1-maninder1.s@samsung.com>
        <CGME20220315095129epcas5p230b94ea10517d148c9cae0669229b0fc@epcms5p5>
Precedence: bulk

------=_Part_3816542_2122723889.1647341260934
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"

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
------=_Part_3816542_2122723889.1647341260934
Content-Type: application/octet-stream
Content-Disposition: attachment; filename="rcptInfo.txt"
Content-Transfer-Encoding: base64

DQogICA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT0NCiAgICAgIFN1YmplY3QgICAgOiBSZTogW1BBVENIIHYyXSBr
YWxsc3ltczogZW5oYW5jZSAlcFMvcy9iIHByaW50aW5nIHdoZW4gS0FMTFNZU01TIGlzIGRpc2Fi
bGVkDQogICAgICBGcm9tICAgICAgIDogbnVsbA0KICAgICAgU2VudCBEYXRlICA6IDIwMjItMDMt
MTUgMTY6MDIgIEdNVCs1OjMwDQogICA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCiAgICAgICAgICAgICAgICAg
IE5hbWUgICAgICAgICAgICAgICAgVHlwZSAgICAgICAgICBKb2IgVGl0bGUgICAgICAgICAgICAg
ICAgICAgICAgIERlcHQuICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIENvbXBhbnkgICAg
ICAgICAgICAgICAgDQogICA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCiAgICAgIE1hbmluZGVyIFNpbmdoICAg
ICAgICAgICAgICAgICBUTyAgICAgICAgIFN0YWZmIEVuZ2luZWVyICAgICAgICAgICAgIFN5c3Rl
bSBTL1cgR3JvdXAgL1NSSS1EZWxoaSAgICAgICAgICAgICAgIFNhbXN1bmfCoEVsZWN0cm9uaWNz
wqANCiAgICAgIG1jZ3JvZkBrZXJuZWwub3JnICAgICAgICAgICAgICBDQw0KICAgICAgcG1sYWRl
a0BzdXNlLmNvbSAgICAgICAgICAgICAgIENDDQogICAgICByb3N0ZWR0QGdvb2RtaXMub3JnICAg
ICAgICAgICAgQ0MNCiAgICAgIHNlbm96aGF0c2t5QGNocm9taXVtLm9yZyAgICAgICBDQw0KICAg
ICAgbGludXhAcmFzbXVzdmlsbGVtb2VzLmRrICAgICAgIENDDQogICAgICBha3BtQGxpbnV4LWZv
dW5kYXRpb24ub3JnICAgICAgQ0MNCiAgICAgIHdhbmdrZWZlbmcud2FuZ0BodWF3ZWkuY29tICAg
ICBDQw0KICAgICAgVmFuZWV0IE5hcmFuZyAgICAgICAgICAgICAgICAgIENDICAgICAgICAgQXNz
b2NpYXRlIEFyY2hpdGVjdCAgICAgICAgU3lzdGVtIFMvVyBHcm91cCAvU1JJLURlbGhpICAgICAg
ICAgICAgICAgU2Ftc3VuZyBFbGVjdHJvbmljcw0KICAgICAgc3dib3lkQGNocm9taXVtLm9yZyAg
ICAgICAgICAgIENDDQogICAgICBvamVkYUBrZXJuZWwub3JnICAgICAgICAgICAgICAgQ0MNCiAg
ICAgIGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmcgICBDQw0KICAgICAgbGludXgtbW9kdWxl
c0B2Z2VyLmtlcm5lbC4uLiAgIENDDQogICAgICBhdmltYWxpbkBnbWFpbC5jb20gICAgICAgICAg
ICAgQ0MNCiAgICAgIGF0b21saW5AcmVkaGF0LmNvbSAgICAgICAgICAgICBDQw0KICAgPT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09DQo=

------=_Part_3816542_2122723889.1647341260934--