Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

[Doug Anderson]

Hi,

On Sat, Jan 30, 2021 at 3:24 AM Stephen Zhang <stephenzhangzsd@xxxxxxxxx> 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