Re: [PATCH v9 13/14] module: Move kdb_modules list out of core code

[Luis Chamberlain]

On Wed, Mar 02, 2022 at 08:56:23PM +0000, Christophe Leroy wrote:
> 
> 
> Le 02/03/2022 à 21:31, Aaron Tomlin a écrit :
> > On Wed 2022-03-02 16:19 +0000, Daniel Thompson wrote:
> >> On Mon, Feb 28, 2022 at 11:43:21PM +0000, Aaron Tomlin wrote:
> >>> No functional change.
> >>>
> >>> This patch migrates kdb_modules list to core kdb code
> >>> since the list of added/or loaded modules is no longer
> >>> private.
> >>>
> >>> Reviewed-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> >>> Signed-off-by: Aaron Tomlin <atomlin@xxxxxxxxxx>
> >>> ---
> >>>   kernel/debug/kdb/kdb_main.c    | 5 +++++
> >>>   kernel/debug/kdb/kdb_private.h | 4 ----
> >>>   kernel/module/main.c           | 4 ----
> >>>   3 files changed, 5 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> >>> index 0852a537dad4..5369bf45c5d4 100644
> >>> --- a/kernel/debug/kdb/kdb_main.c
> >>> +++ b/kernel/debug/kdb/kdb_main.c
> >>> @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag);
> >>>   int kdb_grep_leading;
> >>>   int kdb_grep_trailing;
> >>>   
> >>> +#ifdef CONFIG_MODULES
> >>> +extern struct list_head modules;
> >>> +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
> > 
> > Hi Daniel,
> > 
> >> If modules is no longer static then why do we kdb_modules at all?
> >> kdb_modules is used exactly once and it can now simply be replaced
> >> with &modules.
> > 
> > In my opinion, I would prefer to avoid an explicit include of "internal.h"
> > in kernel/module. By definition it should be reserved for internal use to
> > kernel/module only. Please keep to the above logic.
> > 
> > Christophe, Luis,
> > 
> > Thoughts?
> > 
> 
> Do we really want to hide the 'struct list_head modules' from external 
> world ?

> Otherwise we could declare it in include/linux/module.h ?

Since we are doing this to help with the cleaning this crap up
the natural thing to do is have the code be a helper which only
built-in code can use, so writing a helper starting with
list_for_each_entry() which prints the modules out. I'm
surprised we have no other users of this. There is nothing
kdb specific about the functionality in that code. So it should
just be moved.

Exposing just the list_head was a bad idea to begin with. So
let's do away with that. This can be a preamble change to the
series.

  Luis