diff mbox series

[6/8] kbuild: move more module installation code to scripts/Makefile.modinst

Message ID 20230823115048.823011-6-masahiroy@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/8] kbuild: do not run depmod for 'make modules_sign' | expand

Commit Message

Masahiro Yamada Aug. 23, 2023, 11:50 a.m. UTC 
Move more relevant code to scripts/Makefile.modinst.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 Makefile                 | 34 +++++++--------------------------
 scripts/Makefile.modinst | 41 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 30 deletions(-)

Comments

Nicolas Schier Aug. 28, 2023, 2:25 p.m. UTC | #1 
On Wed 23 Aug 2023 20:50:46 GMT, Masahiro Yamada wrote:
> Move more relevant code to scripts/Makefile.modinst.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  Makefile                 | 34 +++++++--------------------------
>  scripts/Makefile.modinst | 41 +++++++++++++++++++++++++++++++++++++---
>  2 files changed, 45 insertions(+), 30 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 7d9cab3d2186..82d22debf6c9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1477,24 +1477,6 @@ endif
>  
>  endif # CONFIG_MODULES
>  
> -modinst_pre :=
> -ifneq ($(filter modules_install,$(MAKECMDGOALS)),)
> -modinst_pre := __modinst_pre
> -endif
> -
> -modules_install: $(modinst_pre)
> -PHONY += __modinst_pre
> -__modinst_pre:
> -	@rm -rf $(MODLIB)/kernel
> -	@rm -f $(MODLIB)/build
> -	@mkdir -p $(MODLIB)
> -ifdef CONFIG_MODULES
> -	@ln -s $(CURDIR) $(MODLIB)/build
> -	@sed 's:^\(.*\)\.o$$:kernel/\1.ko:' modules.order > $(MODLIB)/modules.order
> -endif
> -	@cp -f modules.builtin $(MODLIB)/
> -	@cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
> -
>  ###
>  # Cleaning is done on three levels.
>  # make clean     Delete most generated files
> @@ -1836,12 +1818,15 @@ help:
>  	@echo  '  clean           - remove generated files in module directory only'
>  	@echo  ''
>  
> +ifndef CONFIG_MODULES
> +modules modules_install: __external_modules_error
>  __external_modules_error:
>  	@echo >&2 '***'
>  	@echo >&2 '*** The present kernel disabled CONFIG_MODULES.'
>  	@echo >&2 '*** You cannot build or install external modules.'
>  	@echo >&2 '***'
>  	@false
> +endif
>  
>  endif # KBUILD_EXTMOD
>  
> @@ -1850,6 +1835,9 @@ endif # KBUILD_EXTMOD
>  
>  PHONY += modules modules_install modules_prepare
>  
> +modules_install:
> +	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst

I was a bit surprised to see 'modules_install' being allowed 
unconditionally for in-tree usage (thus, even if CONFIG_MODULES=n), but 
then realised that this is the same behaviour as we had before.  Out of 
curiosity:  _why_ do we need to install 
$(MODLIB)/modules.builtin{,.modinfo} also for configs w/ 
CONFIG_MODULES=n?

> +
>  ifdef CONFIG_MODULES
>  
>  $(MODORDER): $(build-dir)
> @@ -1866,17 +1854,9 @@ PHONY += modules_check
>  modules_check: $(MODORDER)
>  	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/modules-check.sh $<
>  
> -modules_install:
> -	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
> -
>  else # CONFIG_MODULES
>  
> -# Modules not configured
> -# ---------------------------------------------------------------------------
> -
> -PHONY += __external_modules_error
> -
> -modules modules_install: __external_modules_error
> +modules:
>  	@:
>  
>  KBUILD_MODULES :=
> diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
> index 5d687a453d90..dc7c54669082 100644
> --- a/scripts/Makefile.modinst
> +++ b/scripts/Makefile.modinst
> @@ -13,9 +13,41 @@ install-y :=
>  
>  PHONY += prepare
>  
> +ifeq ($(KBUILD_EXTMOD)$(modules_sign_only),)
> +
> +# Install more files for in-tree modules_install
> +
> +prepare:
> +	$(Q)rm -fr $(MODLIB)/kernel $(MODLIB)/build
> +	$(Q)mkdir -p $(sort $(dir $(install-y)))
> +
> +install-$(CONFIG_MODULES) += $(addprefix $(MODLIB)/, build modules.order)
> +
> +$(MODLIB)/build: FORCE
> +	$(call cmd,symlink)
> +
> +quiet_cmd_symlink = SYMLINK $@
> +      cmd_symlink = ln -s $(CURDIR) $@
> +
> +$(MODLIB)/modules.order: modules.order FORCE
> +	$(call cmd,install_modorder)
> +
> +quiet_cmd_install_modorder = INSTALL $@
> +      cmd_install_modorder = sed 's:^\(.*\)\.o$$:kernel/\1.ko:' $< > $@
> +
> +# Install modules.builtin(.modinfo) even when CONFIG_MODULES is disabled.
> +install-y += $(addprefix $(MODLIB)/, modules.builtin modules.builtin.modinfo)
> +
> +$(addprefix $(MODLIB)/, modules.builtin modules.builtin.modinfo): $(MODLIB)/%: % FORCE
> +	$(call cmd,install)
> +
> +else
> +
>  prepare:
>  	$(Q)mkdir -p $(sort $(dir $(install-y)))
>  
> +endif
> +
>  modules := $(call read-file, $(MODORDER))
>  
>  ifeq ($(KBUILD_EXTMOD),)
> @@ -34,9 +66,10 @@ suffix-$(CONFIG_MODULE_COMPRESS_XZ)	:= .xz
>  suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)	:= .zst
>  
>  modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
> -install-y += $(modules)
>  
> -__modinst: $(modules)
> +install-$(CONFIG_MODULES) += $(modules)
> +
> +__modinst: $(install-y)
>  	@:
>  
>  #
> @@ -94,14 +127,16 @@ $(dst)/%.ko: $(extmod_prefix)%.ko FORCE
>  	$(call cmd,strip)
>  	$(call cmd,sign)
>  
> +ifdef CONFIG_MODULES
>  __modinst: depmod
>  
>  PHONY += depmod
> -depmod: $(modules)
> +depmod: $(install-y)
>  	$(call cmd,depmod)
>  
>  quiet_cmd_depmod = DEPMOD  $(MODLIB)
>        cmd_depmod = $(srctree)/scripts/depmod.sh $(KERNELRELEASE)
> +endif
>  
>  $(install-y): prepare
>  
> -- 
> 2.39.2

Thanks for cleaning up.  For me, the new rules look better than the 
original ones.

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>
Masahiro Yamada Aug. 29, 2023, 2:35 a.m. UTC | #2 
On Tue, Aug 29, 2023 at 11:15 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Wed 23 Aug 2023 20:50:46 GMT, Masahiro Yamada wrote:
> > Move more relevant code to scripts/Makefile.modinst.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  Makefile                 | 34 +++++++--------------------------
> >  scripts/Makefile.modinst | 41 +++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 45 insertions(+), 30 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 7d9cab3d2186..82d22debf6c9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1477,24 +1477,6 @@ endif
> >
> >  endif # CONFIG_MODULES
> >
> > -modinst_pre :=
> > -ifneq ($(filter modules_install,$(MAKECMDGOALS)),)
> > -modinst_pre := __modinst_pre
> > -endif
> > -
> > -modules_install: $(modinst_pre)
> > -PHONY += __modinst_pre
> > -__modinst_pre:
> > -     @rm -rf $(MODLIB)/kernel
> > -     @rm -f $(MODLIB)/build
> > -     @mkdir -p $(MODLIB)
> > -ifdef CONFIG_MODULES
> > -     @ln -s $(CURDIR) $(MODLIB)/build
> > -     @sed 's:^\(.*\)\.o$$:kernel/\1.ko:' modules.order > $(MODLIB)/modules.order
> > -endif
> > -     @cp -f modules.builtin $(MODLIB)/
> > -     @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
> > -
> >  ###
> >  # Cleaning is done on three levels.
> >  # make clean     Delete most generated files
> > @@ -1836,12 +1818,15 @@ help:
> >       @echo  '  clean           - remove generated files in module directory only'
> >       @echo  ''
> >
> > +ifndef CONFIG_MODULES
> > +modules modules_install: __external_modules_error
> >  __external_modules_error:
> >       @echo >&2 '***'
> >       @echo >&2 '*** The present kernel disabled CONFIG_MODULES.'
> >       @echo >&2 '*** You cannot build or install external modules.'
> >       @echo >&2 '***'
> >       @false
> > +endif
> >
> >  endif # KBUILD_EXTMOD
> >
> > @@ -1850,6 +1835,9 @@ endif # KBUILD_EXTMOD
> >
> >  PHONY += modules modules_install modules_prepare
> >
> > +modules_install:
> > +     $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
>
> I was a bit surprised to see 'modules_install' being allowed
> unconditionally for in-tree usage (thus, even if CONFIG_MODULES=n), but
> then realised that this is the same behaviour as we had before.  Out of
> curiosity:  _why_ do we need to install
> $(MODLIB)/modules.builtin{,.modinfo} also for configs w/
> CONFIG_MODULES=n?


I see your tags in commit
8ae071fc216a25f4f797f33c56857f4dd6b4408e    :)


Some drivers need to load firmware.

To make such drivers working in initrd,
mkinitramfs needs to copy necessary firmware files
into the initrd.
So, the tool needs to know which drivers are enabled.

That is my understanding why modules.builtin(.modinfo)
is needed even with CONFIG_MODULES=n.
Nicolas Schier Aug. 29, 2023, 3:50 a.m. UTC | #3 
On Tue 29 Aug 2023 11:35:41 GMT, Masahiro Yamada wrote:
> On Tue, Aug 29, 2023 at 11:15 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> > On Wed 23 Aug 2023 20:50:46 GMT, Masahiro Yamada wrote:
> > > Move more relevant code to scripts/Makefile.modinst.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > >  Makefile                 | 34 +++++++--------------------------
> > >  scripts/Makefile.modinst | 41 +++++++++++++++++++++++++++++++++++++---
> > >  2 files changed, 45 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 7d9cab3d2186..82d22debf6c9 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1477,24 +1477,6 @@ endif
> > >
> > >  endif # CONFIG_MODULES
> > >
> > > -modinst_pre :=
> > > -ifneq ($(filter modules_install,$(MAKECMDGOALS)),)
> > > -modinst_pre := __modinst_pre
> > > -endif
> > > -
> > > -modules_install: $(modinst_pre)
> > > -PHONY += __modinst_pre
> > > -__modinst_pre:
> > > -     @rm -rf $(MODLIB)/kernel
> > > -     @rm -f $(MODLIB)/build
> > > -     @mkdir -p $(MODLIB)
> > > -ifdef CONFIG_MODULES
> > > -     @ln -s $(CURDIR) $(MODLIB)/build
> > > -     @sed 's:^\(.*\)\.o$$:kernel/\1.ko:' modules.order > $(MODLIB)/modules.order
> > > -endif
> > > -     @cp -f modules.builtin $(MODLIB)/
> > > -     @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
> > > -
> > >  ###
> > >  # Cleaning is done on three levels.
> > >  # make clean     Delete most generated files
> > > @@ -1836,12 +1818,15 @@ help:
> > >       @echo  '  clean           - remove generated files in module directory only'
> > >       @echo  ''
> > >
> > > +ifndef CONFIG_MODULES
> > > +modules modules_install: __external_modules_error
> > >  __external_modules_error:
> > >       @echo >&2 '***'
> > >       @echo >&2 '*** The present kernel disabled CONFIG_MODULES.'
> > >       @echo >&2 '*** You cannot build or install external modules.'
> > >       @echo >&2 '***'
> > >       @false
> > > +endif
> > >
> > >  endif # KBUILD_EXTMOD
> > >
> > > @@ -1850,6 +1835,9 @@ endif # KBUILD_EXTMOD
> > >
> > >  PHONY += modules modules_install modules_prepare
> > >
> > > +modules_install:
> > > +     $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
> >
> > I was a bit surprised to see 'modules_install' being allowed
> > unconditionally for in-tree usage (thus, even if CONFIG_MODULES=n), but
> > then realised that this is the same behaviour as we had before.  Out of
> > curiosity:  _why_ do we need to install
> > $(MODLIB)/modules.builtin{,.modinfo} also for configs w/
> > CONFIG_MODULES=n?
> 
> 
> I see your tags in commit
> 8ae071fc216a25f4f797f33c56857f4dd6b4408e    :)
> 
> 
> Some drivers need to load firmware.
> 
> To make such drivers working in initrd,
> mkinitramfs needs to copy necessary firmware files
> into the initrd.
> So, the tool needs to know which drivers are enabled.
> 
> That is my understanding why modules.builtin(.modinfo)
> is needed even with CONFIG_MODULES=n.

Ups, yes.  Thanks for the reminder!

Kind regards,
Nicolas
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 7d9cab3d2186..82d22debf6c9 100644
--- a/Makefile
+++ b/Makefile
@@ -1477,24 +1477,6 @@  endif
 
 endif # CONFIG_MODULES
 
-modinst_pre :=
-ifneq ($(filter modules_install,$(MAKECMDGOALS)),)
-modinst_pre := __modinst_pre
-endif
-
-modules_install: $(modinst_pre)
-PHONY += __modinst_pre
-__modinst_pre:
-	@rm -rf $(MODLIB)/kernel
-	@rm -f $(MODLIB)/build
-	@mkdir -p $(MODLIB)
-ifdef CONFIG_MODULES
-	@ln -s $(CURDIR) $(MODLIB)/build
-	@sed 's:^\(.*\)\.o$$:kernel/\1.ko:' modules.order > $(MODLIB)/modules.order
-endif
-	@cp -f modules.builtin $(MODLIB)/
-	@cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
-
 ###
 # Cleaning is done on three levels.
 # make clean     Delete most generated files
@@ -1836,12 +1818,15 @@  help:
 	@echo  '  clean           - remove generated files in module directory only'
 	@echo  ''
 
+ifndef CONFIG_MODULES
+modules modules_install: __external_modules_error
 __external_modules_error:
 	@echo >&2 '***'
 	@echo >&2 '*** The present kernel disabled CONFIG_MODULES.'
 	@echo >&2 '*** You cannot build or install external modules.'
 	@echo >&2 '***'
 	@false
+endif
 
 endif # KBUILD_EXTMOD
 
@@ -1850,6 +1835,9 @@  endif # KBUILD_EXTMOD
 
 PHONY += modules modules_install modules_prepare
 
+modules_install:
+	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
+
 ifdef CONFIG_MODULES
 
 $(MODORDER): $(build-dir)
@@ -1866,17 +1854,9 @@  PHONY += modules_check
 modules_check: $(MODORDER)
 	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/modules-check.sh $<
 
-modules_install:
-	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
-
 else # CONFIG_MODULES
 
-# Modules not configured
-# ---------------------------------------------------------------------------
-
-PHONY += __external_modules_error
-
-modules modules_install: __external_modules_error
+modules:
 	@:
 
 KBUILD_MODULES :=
diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
index 5d687a453d90..dc7c54669082 100644
--- a/scripts/Makefile.modinst
+++ b/scripts/Makefile.modinst
@@ -13,9 +13,41 @@  install-y :=
 
 PHONY += prepare
 
+ifeq ($(KBUILD_EXTMOD)$(modules_sign_only),)
+
+# Install more files for in-tree modules_install
+
+prepare:
+	$(Q)rm -fr $(MODLIB)/kernel $(MODLIB)/build
+	$(Q)mkdir -p $(sort $(dir $(install-y)))
+
+install-$(CONFIG_MODULES) += $(addprefix $(MODLIB)/, build modules.order)
+
+$(MODLIB)/build: FORCE
+	$(call cmd,symlink)
+
+quiet_cmd_symlink = SYMLINK $@
+      cmd_symlink = ln -s $(CURDIR) $@
+
+$(MODLIB)/modules.order: modules.order FORCE
+	$(call cmd,install_modorder)
+
+quiet_cmd_install_modorder = INSTALL $@
+      cmd_install_modorder = sed 's:^\(.*\)\.o$$:kernel/\1.ko:' $< > $@
+
+# Install modules.builtin(.modinfo) even when CONFIG_MODULES is disabled.
+install-y += $(addprefix $(MODLIB)/, modules.builtin modules.builtin.modinfo)
+
+$(addprefix $(MODLIB)/, modules.builtin modules.builtin.modinfo): $(MODLIB)/%: % FORCE
+	$(call cmd,install)
+
+else
+
 prepare:
 	$(Q)mkdir -p $(sort $(dir $(install-y)))
 
+endif
+
 modules := $(call read-file, $(MODORDER))
 
 ifeq ($(KBUILD_EXTMOD),)
@@ -34,9 +66,10 @@  suffix-$(CONFIG_MODULE_COMPRESS_XZ)	:= .xz
 suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)	:= .zst
 
 modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
-install-y += $(modules)
 
-__modinst: $(modules)
+install-$(CONFIG_MODULES) += $(modules)
+
+__modinst: $(install-y)
 	@:
 
 #
@@ -94,14 +127,16 @@  $(dst)/%.ko: $(extmod_prefix)%.ko FORCE
 	$(call cmd,strip)
 	$(call cmd,sign)
 
+ifdef CONFIG_MODULES
 __modinst: depmod
 
 PHONY += depmod
-depmod: $(modules)
+depmod: $(install-y)
 	$(call cmd,depmod)
 
 quiet_cmd_depmod = DEPMOD  $(MODLIB)
       cmd_depmod = $(srctree)/scripts/depmod.sh $(KERNELRELEASE)
+endif
 
 $(install-y): prepare