| Message ID | 20220308105203.2981099-1-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | kdb: Remove redundant module related references | expand |
On Tue, Mar 08, 2022 at 10:52:03AM +0000, Aaron Tomlin wrote: > Hi Luis, Christoph, Daniel, > > Is this patch ok or would you rather another iteration of the series? > Either way is fine for me. Thanks. Another iteration makes more sense to me. The removal of kdb_modules is semantically part of your module clean up patch set and should certainly be included in it. The removal of the spurious #include's in other kdb files is a good change but it is fully independent of the module rework. AFAICT those fixes are good with or without your changes. This suggests these changes can be separate from the main patch set. Daniel. > > > No functional change. > > There is no need to include linux/module.h. > This patch addresses the above. Furthermore, we remove the list of known > loaded modules i.e. stored in 'kdb_modules', since it is now redundant. > > Fixes: 260681b3763f ("module: Move kdb module related code out of main kdb code") > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > --- > kernel/debug/kdb/kdb_io.c | 1 - > kernel/debug/kdb/kdb_keyboard.c | 1 - > kernel/debug/kdb/kdb_private.h | 4 ---- > kernel/debug/kdb/kdb_support.c | 1 - > 4 files changed, 7 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c > index 6735ac36b718..67d3c48a1522 100644 > --- a/kernel/debug/kdb/kdb_io.c > +++ b/kernel/debug/kdb/kdb_io.c > @@ -9,7 +9,6 @@ > * Copyright (c) 2009 Wind River Systems, Inc. All Rights Reserved. > */ > > -#include <linux/module.h> > #include <linux/types.h> > #include <linux/ctype.h> > #include <linux/kernel.h> > diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c > index f877a0a0d7cf..f87c750d3eb3 100644 > --- a/kernel/debug/kdb/kdb_keyboard.c > +++ b/kernel/debug/kdb/kdb_keyboard.c > @@ -11,7 +11,6 @@ > #include <linux/kdb.h> > #include <linux/keyboard.h> > #include <linux/ctype.h> > -#include <linux/module.h> > #include <linux/io.h> > > /* Keyboard Controller Registers on normal PCs. */ > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h > index 0d2f9feea0a4..1f8c519a5f81 100644 > --- a/kernel/debug/kdb/kdb_private.h > +++ b/kernel/debug/kdb/kdb_private.h > @@ -226,10 +226,6 @@ extern void kdb_kbd_cleanup_state(void); > #define kdb_kbd_cleanup_state() > #endif /* ! CONFIG_KDB_KEYBOARD */ > > -#ifdef CONFIG_MODULES > -extern struct list_head *kdb_modules; > -#endif /* CONFIG_MODULES */ > - > extern char kdb_prompt_str[]; > > #define KDB_WORD_SIZE ((int)sizeof(unsigned long)) > diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c > index df2bface866e..08229ffb6b5e 100644 > --- a/kernel/debug/kdb/kdb_support.c > +++ b/kernel/debug/kdb/kdb_support.c > @@ -17,7 +17,6 @@ > #include <linux/stddef.h> > #include <linux/vmalloc.h> > #include <linux/ptrace.h> > -#include <linux/module.h> > #include <linux/highmem.h> > #include <linux/hardirq.h> > #include <linux/delay.h> > -- > 2.34.1 >
On Wed 2022-03-09 12:06 +0000, Daniel Thompson wrote: > On Tue, Mar 08, 2022 at 10:52:03AM +0000, Aaron Tomlin wrote: > Another iteration makes more sense to me. Hi Daniel, Thanks for your feedback. > The removal of the spurious #include's in other kdb files is a > good change but it is fully independent of the module rework. AFAICT > those fixes are good with or without your changes. This suggests > these changes can be separate from the main patch set. I will include these changes in the series too, as it cannot hurt. Kind regards,
On Wed, Mar 09, 2022 at 12:06:40PM +0000, Daniel Thompson wrote: > On Tue, Mar 08, 2022 at 10:52:03AM +0000, Aaron Tomlin wrote: > > Hi Luis, Christoph, Daniel, > > > > Is this patch ok or would you rather another iteration of the series? > > Either way is fine for me. Thanks. > > Another iteration makes more sense to me. Iteration yes, but separating the patches no into another series no. > The removal of kdb_modules is semantically part of your module clean > up patch set and should certainly be included in it. > > The removal of the spurious #include's in other kdb files is a > good change but it is fully independent of the module rework. AFAICT > those fixes are good with or without your changes. This suggests > these changes can be separate from the main patch set. Small fixes get piled in first on the series. But this is not a fix. This effort will not be merged separately too. This won't go into the next merge window either, because: 1) There is no rush 2) It is too late as all this needs proper testing and its too late to claim enough testing So given this is all related to the move I see no reason to treat this as a separate series. Your review of the v11 would be nice. Luis
On Thu, Mar 10, 2022 at 01:07:57PM -0800, Luis Chamberlain wrote: > On Wed, Mar 09, 2022 at 12:06:40PM +0000, Daniel Thompson wrote: > > On Tue, Mar 08, 2022 at 10:52:03AM +0000, Aaron Tomlin wrote: > > > Hi Luis, Christoph, Daniel, > > > > > > Is this patch ok or would you rather another iteration of the series? > > > Either way is fine for me. Thanks. > > > > Another iteration makes more sense to me. > > Iteration yes, but separating the patches no into another series no. > > > The removal of kdb_modules is semantically part of your module clean > > up patch set and should certainly be included in it. > > > > The removal of the spurious #include's in other kdb files is a > > good change but it is fully independent of the module rework. AFAICT > > those fixes are good with or without your changes. This suggests > > these changes can be separate from the main patch set. > > Small fixes get piled in first on the series. But this is not a fix. > This effort will not be merged separately too. This won't go into the > next merge window either, because: > > 1) There is no rush > 2) It is too late as all this needs proper testing and > its too late to claim enough testing > > So given this is all related to the move I see no reason to treat > this as a separate series. Your review of the v11 would be nice. The reason to suggest separation was that the changes to the other files in kernel/debug/ are entirely independent of the module rework and would usually be landed via a different tree. On the whole it doesn't really matter much... but landing the independent parts via the normal route for kgdb code reduces what I have to remember acking. Daniel.
On Fri 2022-03-11 15:47 +0000, Daniel Thompson wrote: > On the whole it doesn't really matter much... but landing the > independent parts via the normal route for kgdb code reduces what I > have to remember acking. Hi Daniel, Any thoughts on this [1]? Unfortunately, the wrong In-Reply-To was specified. So sorry about that. I will send another iteration of the series so that it'll be easier for Luis to apply to mcgrof/modules-testing or mcgrof/modules-next but after we have more feedback. [1]: https://lore.kernel.org/lkml/20220310102625.3441888-2-atomlin@redhat.com/ Kind regards,
On Fri, Mar 11, 2022 at 04:01:53PM +0000, Aaron Tomlin wrote: > On Fri 2022-03-11 15:47 +0000, Daniel Thompson wrote: > > On the whole it doesn't really matter much... but landing the > > independent parts via the normal route for kgdb code reduces what I > > have to remember acking. > > Hi Daniel, > > Any thoughts on this [1]? Unfortunately, the wrong In-Reply-To was > specified. So sorry about that. I will send another iteration of the series > so that it'll be easier for Luis to apply to mcgrof/modules-testing or > mcgrof/modules-next but after we have more feedback. > > [1]: https://lore.kernel.org/lkml/20220310102625.3441888-2-atomlin@redhat.com/ As above. I would rather not have to remember acking the header changes as well... but it's not like the sky is going to fall in. Daniel.
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 6735ac36b718..67d3c48a1522 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -9,7 +9,6 @@ * Copyright (c) 2009 Wind River Systems, Inc. All Rights Reserved. */ -#include <linux/module.h> #include <linux/types.h> #include <linux/ctype.h> #include <linux/kernel.h> diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c index f877a0a0d7cf..f87c750d3eb3 100644 --- a/kernel/debug/kdb/kdb_keyboard.c +++ b/kernel/debug/kdb/kdb_keyboard.c @@ -11,7 +11,6 @@ #include <linux/kdb.h> #include <linux/keyboard.h> #include <linux/ctype.h> -#include <linux/module.h> #include <linux/io.h> /* Keyboard Controller Registers on normal PCs. */ diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index 0d2f9feea0a4..1f8c519a5f81 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -226,10 +226,6 @@ extern void kdb_kbd_cleanup_state(void); #define kdb_kbd_cleanup_state() #endif /* ! CONFIG_KDB_KEYBOARD */ -#ifdef CONFIG_MODULES -extern struct list_head *kdb_modules; -#endif /* CONFIG_MODULES */ - extern char kdb_prompt_str[]; #define KDB_WORD_SIZE ((int)sizeof(unsigned long)) diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c index df2bface866e..08229ffb6b5e 100644 --- a/kernel/debug/kdb/kdb_support.c +++ b/kernel/debug/kdb/kdb_support.c @@ -17,7 +17,6 @@ #include <linux/stddef.h> #include <linux/vmalloc.h> #include <linux/ptrace.h> -#include <linux/module.h> #include <linux/highmem.h> #include <linux/hardirq.h> #include <linux/delay.h>
Hi Luis, Christoph, Daniel, Is this patch ok or would you rather another iteration of the series? Either way is fine for me. Thanks. No functional change. There is no need to include linux/module.h. This patch addresses the above. Furthermore, we remove the list of known loaded modules i.e. stored in 'kdb_modules', since it is now redundant. Fixes: 260681b3763f ("module: Move kdb module related code out of main kdb code") Signed-off-by: Aaron Tomlin <atomlin@redhat.com> --- kernel/debug/kdb/kdb_io.c | 1 - kernel/debug/kdb/kdb_keyboard.c | 1 - kernel/debug/kdb/kdb_private.h | 4 ---- kernel/debug/kdb/kdb_support.c | 1 - 4 files changed, 7 deletions(-)