On Tue 2022-03-08 08:36 +0000, Christophe Leroy wrote:
>
>
> Le 07/03/2022 à 18:47, Aaron Tomlin a écrit :
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index 0852a537dad4..f3a30cd5037f 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -26,7 +26,6 @@
> > #include <linux/utsname.h>
> > #include <linux/vmalloc.h>
> > #include <linux/atomic.h>
> > -#include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/mm.h>
> > #include <linux/init.h>
> No need of linux/module.h here anymore ?
Hi Christophe,
Correct.
> In that case, I see several other files in kernel/debug/kdb/ that
> include linux/module.h
>
> Should it be removed in those files as well ?
I did not review the other kernel/debug/kdb/.*c files.
Anyhow, yes it can be removed from each, since it is entirely redundant.
> > diff --git a/kernel/module/kdb.c b/kernel/module/kdb.c
> > new file mode 100644
> > index 000000000000..60baeebea3e0
> > --- /dev/null
> > +++ b/kernel/module/kdb.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Module kdb support
> > + *
> > + * Copyright (C) 2010 Jason Wessel
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kdb.h>
> > +#include "internal.h"
> > +
> > +/*
> > + * kdb_lsmod - This function implements the 'lsmod' command. Lists
> > + * currently loaded kernel modules.
> > + * Mostly taken from userland lsmod.
> > + */
> > +int kdb_lsmod(int argc, const char **argv)
> > +{
> > + struct module *mod;
> > +
> > + if (argc != 0)
> > + return KDB_ARGCOUNT;
> > +
> > + kdb_printf("Module Size modstruct Used by\n");
> > + list_for_each_entry(mod, &modules, list) {
> > + if (mod->state == MODULE_STATE_UNFORMED)
> > + continue;
> > +
> > + kdb_printf("%-20s%8u 0x%px ", mod->name,
> > + mod->core_layout.size, (void *)mod);
> > +#ifdef CONFIG_MODULE_UNLOAD
> > + kdb_printf("%4d ", module_refcount(mod));
> > +#endif
> > + if (mod->state == MODULE_STATE_GOING)
> > + kdb_printf(" (Unloading)");
> > + else if (mod->state == MODULE_STATE_COMING)
> > + kdb_printf(" (Loading)");
> > + else
> > + kdb_printf(" (Live)");
> > + kdb_printf(" 0x%px", mod->core_layout.base);
> > +
> > +#ifdef CONFIG_MODULE_UNLOAD
> > + {
> > + struct module_use *use;
> > + kdb_printf(" [ ");
> > + list_for_each_entry(use, &mod->source_list,
> > + source_list)
> > + kdb_printf("%s ", use->target->name);
> > + kdb_printf("]\n");
> > + }
> > +#endif
>
> That's a ugly construct. Could it be a function instead that you call
> from this loop,
Fair enough and I agree; albeit, as you know, this was simply a migration
to kernel/module/kdb.c. We could indeed address this format/or style
concern later.
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index b8a59b5c3e3a..bcc4f7a82649 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -108,10 +108,6 @@ static void mod_update_bounds(struct module *mod)
> > __mod_update_bounds(mod->init_layout.base, mod->init_layout.size);
> > }
> >
> > -#ifdef CONFIG_KGDB_KDB
> > -struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
>
> It should be removed from kernel/debug/kdb/kdb_private.h as well.
Agreed - this was missed.
Thanks,
--
Aaron Tomlin
