Re: [PATCH v10 13/14] module: Move kdb module related code out of main kdb code

[Aaron Tomlin <atomlin@redhat.com>]

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