Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 30 Jun 2020 11:37:54 +0200
From:   Jessica Yu <jeyu@kernel.org>
To:     Jim Cromie <jim.cromie@gmail.com>
Cc:     tobyboy0@gmail.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kernel/module:  add name size info to pr_debug() calls
Message-ID: <20200630093754.GA13271@linux-8ccs.fritz.box>
References: <20200611142009.1098058-1-jim.cromie@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20200611142009.1098058-1-jim.cromie@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+++ Jim Cromie [11/06/20 08:20 -0600]:
>when booted with arg: module.dyndbg=+p
>dmesg gets volumes of info about loaded modules.
>This adds module & symbol names, and sizes where pertinent.
>Now I can know which module's info Im looking at.

Hi,

Could you please fix the changelog formatting according to
Documentation/process/submitting-patches.rst? More specifically,
complete sentences, line wrapped at 75 columns, and a Signed-off-by:
line at the end. There are plenty of examples if you look through the
lkml mailing list.

>---
> kernel/module.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index e8a198588f26..d871d9cee9eb 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2294,8 +2294,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>
> 		case SHN_ABS:
> 			/* Don't need to do anything */
>-			pr_debug("Absolute symbol: 0x%08lx\n",
>-			       (long)sym[i].st_value);
>+			pr_debug("Absolute symbol: 0x%08lx %s\n",
>+				 (long)sym[i].st_value, name);

I would prefer "Absolute symbol %s:" rather than putting the symbol
name at the end.

> 			break;
>
> 		case SHN_LIVEPATCH:
>@@ -2409,7 +2409,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
> 	for (i = 0; i < info->hdr->e_shnum; i++)
> 		info->sechdrs[i].sh_entsize = ~0UL;
>
>-	pr_debug("Core section allocation order:\n");
>+	pr_debug("Core section allocation order for: %s\n", mod->name);

I would slightly prefer "Core section allocation order for %s:", but
it's a matter of taste.

> 	for (m = 0; m < ARRAY_SIZE(masks); ++m) {
> 		for (i = 0; i < info->hdr->e_shnum; ++i) {
> 			Elf_Shdr *s = &info->sechdrs[i];
>@@ -2442,7 +2442,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
> 		}
> 	}
>
>-	pr_debug("Init section allocation order:\n");
>+	pr_debug("Init section allocation order for: %s\n", mod->name);

Same here.

> 	for (m = 0; m < ARRAY_SIZE(masks); ++m) {
> 		for (i = 0; i < info->hdr->e_shnum; ++i) {
> 			Elf_Shdr *s = &info->sechdrs[i];
>@@ -3276,7 +3276,7 @@ static int move_module(struct module *mod, struct load_info *info)
> 		mod->init_layout.base = NULL;
>
> 	/* Transfer each section which specifies SHF_ALLOC */
>-	pr_debug("final section addresses:\n");
>+	pr_debug("final section addresses for: %s\n", mod->name);

Let's capitalize the "f" in "final section addresses" to be consistent
with the other two above.

> 	for (i = 0; i < info->hdr->e_shnum; i++) {
> 		void *dest;
> 		Elf_Shdr *shdr = &info->sechdrs[i];
>@@ -3294,8 +3294,8 @@ static int move_module(struct module *mod, struct load_info *info)
> 			memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
> 		/* Update sh_addr to point to copy in image. */
> 		shdr->sh_addr = (unsigned long)dest;
>-		pr_debug("\t0x%lx %s\n",
>-			 (long)shdr->sh_addr, info->secstrings + shdr->sh_name);
>+		pr_debug("\t0x%lx 0x%.8lx %s\n", (long)shdr->sh_addr,
>+			 (long)shdr->sh_size, info->secstrings + shdr->sh_name);

Any reason why you added sh_size here? Is it really needed? You can
usually deduce how much space a section is taking up in a module by
looking at the final section address printout. Plus the elf section
size is not entirely accurate here as each module section is aligned
to page boundaries when the module loader allocates memory for each
section. I would prefer to just leave it out.

Thanks,

Jessica