* [PATCH v3] kdb: kdb_support: Fix debugging information problem
@ 2021-01-30 11:24 Stephen Zhang
2021-02-02 0:27 ` Doug Anderson
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Zhang @ 2021-01-30 11:24 UTC (permalink / raw)
To: jason.wessel, daniel.thompson, dianders, gustavoars
Cc: kgdb-bugreport, linux-kernel, Stephen Zhang
There are several common patterns.
0:
kdb_printf("...",...);
which is the normal one.
1:
kdb_printf("%s: "...,__func__,...)
We could improve '1' to this :
#define kdb_func_printf(format, args...) \
kdb_printf("%s: " format, __func__, ## args)
2:
if(KDB_DEBUG(AR))
kdb_printf("%s "...,__func__,...);
We could improve '2' to this :
#define kdb_dbg_printf(mask, format, args...) \
do { \
if (KDB_DEBUG(mask)) \
kdb_func_printf(format, ## args); \
} while (0)
In additon, we changed the format code of size_t to %zu.
Signed-off-by: Stephen Zhang <stephenzhangzsd@gmail.com>
---
v1->v2 Changelog:
- Add 'mask' parameter in kdb_dbg_printf()
Thanks to Daniel and Doug's suggestions and review.
v2->v3 Changelog:
- Adjust alignment for some lines.
Suggested by Douglas Anderson.
kernel/debug/kdb/kdb_private.h | 10 ++++++++
kernel/debug/kdb/kdb_support.c | 53 ++++++++++++++++++------------------------
2 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index a4281fb..0a56d35 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -254,4 +254,14 @@ extern unsigned long kdb_task_state(const struct task_struct *p,
#define KDB_WORD_SIZE ((int)sizeof(unsigned long))
#endif /* CONFIG_KGDB_KDB */
+
+#define kdb_func_printf(format, args...) \
+ kdb_printf("%s: " format, __func__, ## args)
+
+#define kdb_dbg_printf(mask, format, args...) \
+ do { \
+ if (KDB_DEBUG(mask)) \
+ kdb_func_printf(format, ## args); \
+ } while (0)
+
#endif /* !_KDBPRIVATE_H */
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 6226502..99a6232 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -39,20 +39,15 @@
*/
int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
{
- if (KDB_DEBUG(AR))
- kdb_printf("kdbgetsymval: symname=%s, symtab=%px\n", symname,
- symtab);
+ kdb_dbg_printf(AR, "symname=%s, symtab=%px\n", symname, symtab);
memset(symtab, 0, sizeof(*symtab));
symtab->sym_start = kallsyms_lookup_name(symname);
if (symtab->sym_start) {
- if (KDB_DEBUG(AR))
- kdb_printf("kdbgetsymval: returns 1, "
- "symtab->sym_start=0x%lx\n",
- symtab->sym_start);
+ kdb_dbg_printf(AR, "returns 1,symtab->sym_start=0x%lx\n",
+ symtab->sym_start);
return 1;
}
- if (KDB_DEBUG(AR))
- kdb_printf("kdbgetsymval: returns 0\n");
+ kdb_dbg_printf(AR, "returns 0\n");
return 0;
}
EXPORT_SYMBOL(kdbgetsymval);
@@ -87,15 +82,14 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
#define knt1_size 128 /* must be >= kallsyms table size */
char *knt1 = NULL;
- if (KDB_DEBUG(AR))
- kdb_printf("kdbnearsym: addr=0x%lx, symtab=%px\n", addr, symtab);
+ kdb_dbg_printf(AR, "addr=0x%lx, symtab=%px\n", addr, symtab);
memset(symtab, 0, sizeof(*symtab));
if (addr < 4096)
goto out;
knt1 = debug_kmalloc(knt1_size, GFP_ATOMIC);
if (!knt1) {
- kdb_printf("kdbnearsym: addr=0x%lx cannot kmalloc knt1\n",
+ kdb_func_printf("addr=0x%lx cannot kmalloc knt1\n",
addr);
goto out;
}
@@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
if (symtab->mod_name == NULL)
symtab->mod_name = "kernel";
- if (KDB_DEBUG(AR))
- kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
- "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
- symtab->sym_start, symtab->mod_name, symtab->sym_name,
- symtab->sym_name);
+ kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
+ "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
+ symtab->sym_start, symtab->mod_name, symtab->sym_name,
+ symtab->sym_name);
out:
debug_kfree(knt1);
@@ -328,7 +321,7 @@ int kdb_getarea_size(void *res, unsigned long addr, size_t size)
int ret = copy_from_kernel_nofault((char *)res, (char *)addr, size);
if (ret) {
if (!KDB_STATE(SUPPRESS)) {
- kdb_printf("kdb_getarea: Bad address 0x%lx\n", addr);
+ kdb_func_printf("Bad address 0x%lx\n", addr);
KDB_STATE_SET(SUPPRESS);
}
ret = KDB_BADADDR;
@@ -353,7 +346,7 @@ int kdb_putarea_size(unsigned long addr, void *res, size_t size)
int ret = copy_from_kernel_nofault((char *)addr, (char *)res, size);
if (ret) {
if (!KDB_STATE(SUPPRESS)) {
- kdb_printf("kdb_putarea: Bad address 0x%lx\n", addr);
+ kdb_func_printf("Bad address 0x%lx\n", addr);
KDB_STATE_SET(SUPPRESS);
}
ret = KDB_BADADDR;
@@ -435,7 +428,7 @@ int kdb_getphysword(unsigned long *word, unsigned long addr, size_t size)
fallthrough;
default:
diag = KDB_BADWIDTH;
- kdb_printf("kdb_getphysword: bad width %ld\n", (long) size);
+ kdb_func_printf("bad width %zu\n", size);
}
return diag;
}
@@ -484,7 +477,7 @@ int kdb_getword(unsigned long *word, unsigned long addr, size_t size)
fallthrough;
default:
diag = KDB_BADWIDTH;
- kdb_printf("kdb_getword: bad width %ld\n", (long) size);
+ kdb_func_printf("bad width %zu\n", size);
}
return diag;
}
@@ -528,7 +521,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
fallthrough;
default:
diag = KDB_BADWIDTH;
- kdb_printf("kdb_putword: bad width %ld\n", (long) size);
+ kdb_func_printf("bad width %zu\n", size);
}
return diag;
}
@@ -602,8 +595,7 @@ unsigned long kdb_task_state_string(const char *s)
res = ~0UL;
break;
default:
- kdb_printf("%s: unknown flag '%c' ignored\n",
- __func__, *s);
+ kdb_func_printf("unknown flag '%c' ignored\n", *s);
break;
}
++s;
@@ -884,18 +876,17 @@ void debug_kusage(void)
if (!debug_kusage_one_time)
goto out;
debug_kusage_one_time = 0;
- kdb_printf("%s: debug_kmalloc memory leak dah_first %d\n",
- __func__, dah_first);
+ kdb_func_printf("debug_kmalloc memory leak dah_first %d\n", dah_first);
if (dah_first) {
h_used = (struct debug_alloc_header *)debug_alloc_pool;
- kdb_printf("%s: h_used %px size %d\n", __func__, h_used,
+ kdb_func_printf("h_used %px size %d\n", h_used,
h_used->size);
}
do {
h_used = (struct debug_alloc_header *)
((char *)h_free + dah_overhead + h_free->size);
- kdb_printf("%s: h_used %px size %d caller %px\n",
- __func__, h_used, h_used->size, h_used->caller);
+ kdb_func_printf("h_used %px size %d caller %px\n",
+ h_used, h_used->size, h_used->caller);
h_free = (struct debug_alloc_header *)
(debug_alloc_pool + h_free->next);
} while (h_free->next);
@@ -903,8 +894,8 @@ void debug_kusage(void)
((char *)h_free + dah_overhead + h_free->size);
if ((char *)h_used - debug_alloc_pool !=
sizeof(debug_alloc_pool_aligned))
- kdb_printf("%s: h_used %px size %d caller %px\n",
- __func__, h_used, h_used->size, h_used->caller);
+ kdb_func_printf("h_used %px size %d caller %px\n",
+ h_used, h_used->size, h_used->caller);
out:
spin_unlock(&dap_lock);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem
2021-01-30 11:24 [PATCH v3] kdb: kdb_support: Fix debugging information problem Stephen Zhang
@ 2021-02-02 0:27 ` Doug Anderson
[not found] ` <CALuz2=erXGa2q_ODOpARK9yb_GQo0nOLWnP-PuBMCc+pv8Cp4Q@mail.gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Doug Anderson @ 2021-02-02 0:27 UTC (permalink / raw)
To: Stephen Zhang
Cc: Jason Wessel, Daniel Thompson, Gustavo A. R. Silva, kgdb-bugreport, LKML
Hi,
On Sat, Jan 30, 2021 at 3:24 AM Stephen Zhang <stephenzhangzsd@gmail.com> wrote:
>
> int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
> {
> - if (KDB_DEBUG(AR))
> - kdb_printf("kdbgetsymval: symname=%s, symtab=%px\n", symname,
> - symtab);
> + kdb_dbg_printf(AR, "symname=%s, symtab=%px\n", symname, symtab);
> memset(symtab, 0, sizeof(*symtab));
> symtab->sym_start = kallsyms_lookup_name(symname);
> if (symtab->sym_start) {
> - if (KDB_DEBUG(AR))
> - kdb_printf("kdbgetsymval: returns 1, "
> - "symtab->sym_start=0x%lx\n",
> - symtab->sym_start);
> + kdb_dbg_printf(AR, "returns 1,symtab->sym_start=0x%lx\n",
nit: There used to be a space after the "," and there no longer is.
> + symtab->sym_start);
nit: I think it was supposed to be indented 1 more space so it's under "AR".
> EXPORT_SYMBOL(kdbgetsymval);
> @@ -87,15 +82,14 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
> #define knt1_size 128 /* must be >= kallsyms table size */
> char *knt1 = NULL;
>
> - if (KDB_DEBUG(AR))
> - kdb_printf("kdbnearsym: addr=0x%lx, symtab=%px\n", addr, symtab);
> + kdb_dbg_printf(AR, "addr=0x%lx, symtab=%px\n", addr, symtab);
> memset(symtab, 0, sizeof(*symtab));
>
> if (addr < 4096)
> goto out;
> knt1 = debug_kmalloc(knt1_size, GFP_ATOMIC);
> if (!knt1) {
> - kdb_printf("kdbnearsym: addr=0x%lx cannot kmalloc knt1\n",
> + kdb_func_printf("addr=0x%lx cannot kmalloc knt1\n",
> addr);
nit: "addr" used to be indented properly before your change and now
it's not. It could also move up to the previous line.
> @@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>
> if (symtab->mod_name == NULL)
> symtab->mod_name = "kernel";
> - if (KDB_DEBUG(AR))
> - kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
> - "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
> - symtab->sym_start, symtab->mod_name, symtab->sym_name,
> - symtab->sym_name);
> + kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
> + "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
> + symtab->sym_start, symtab->mod_name, symtab->sym_name,
> + symtab->sym_name);
This indention doesn't line up, but it didn't before. I guess I'd let
Daniel say if he likes this or would prefer different wrapping /
indentation.
> @@ -884,18 +876,17 @@ void debug_kusage(void)
> if (!debug_kusage_one_time)
> goto out;
> debug_kusage_one_time = 0;
> - kdb_printf("%s: debug_kmalloc memory leak dah_first %d\n",
> - __func__, dah_first);
> + kdb_func_printf("debug_kmalloc memory leak dah_first %d\n", dah_first);
> if (dah_first) {
> h_used = (struct debug_alloc_header *)debug_alloc_pool;
> - kdb_printf("%s: h_used %px size %d\n", __func__, h_used,
> + kdb_func_printf("h_used %px size %d\n", h_used,
> h_used->size);
nit: "h_used->size" used to be indented properly before your change
and now it's not. It could also move up to the previous line.
> }
> do {
> h_used = (struct debug_alloc_header *)
> ((char *)h_free + dah_overhead + h_free->size);
> - kdb_printf("%s: h_used %px size %d caller %px\n",
> - __func__, h_used, h_used->size, h_used->caller);
> + kdb_func_printf("h_used %px size %d caller %px\n",
> + h_used, h_used->size, h_used->caller);
nit: "h_used" used to be indented properly before your change and now it's not.
> @@ -903,8 +894,8 @@ void debug_kusage(void)
> ((char *)h_free + dah_overhead + h_free->size);
> if ((char *)h_used - debug_alloc_pool !=
> sizeof(debug_alloc_pool_aligned))
> - kdb_printf("%s: h_used %px size %d caller %px\n",
> - __func__, h_used, h_used->size, h_used->caller);
> + kdb_func_printf("h_used %px size %d caller %px\n",
> + h_used, h_used->size, h_used->caller);
nit: "h_used" used to be indented properly before your change and now it's not.
It's possible that Daniel would prefer to fix these word-wrapping and
indention things when he applies your change?
I know the above is all pretty nitty, but given that the whole point
of the change is to clean up the code it seems like it's fair game to
make sure the code touched is fully clean...
-Doug
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-03 16:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-30 11:24 [PATCH v3] kdb: kdb_support: Fix debugging information problem Stephen Zhang
2021-02-02 0:27 ` Doug Anderson
[not found] ` <CALuz2=erXGa2q_ODOpARK9yb_GQo0nOLWnP-PuBMCc+pv8Cp4Q@mail.gmail.com>
2021-02-02 17:39 ` Doug Anderson
[not found] ` <CALuz2=cApyqB2FWvf8GVhAtGJrBGVWxTvQmiiOFUN+zouqZOUA@mail.gmail.com>
2021-02-03 15:57 ` Doug Anderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).