| Message ID | 20240405165610.1537698-1-vchernou@cisco.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Add MO(mod objs) variable to process ext modules with subdirs | expand |
Hi Valerii, On 4/5/24 9:56 AM, Valerii Chernous wrote: > The change allow to build external modules with nested makefiles. > With current unofficial way(using "src" variable) it is possible to build > external(out of tree) kernel module with separate source and build > artifacts dirs but with nested makefiles it doesn't work properly. > Build system trap to recursion inside makefiles, artifacts output dir > path grow with each iteration until exceed max path len and build failed. > Providing "MO" variable and using "override" directive with declaring > "src" variable solves the problem > Usage example: > make -C KERNEL_SOURCE_TREE MO=BUILD_OUT_DIR M=EXT_MOD_SRC_DIR modules > > Cc: xe-linux-external@cisco.com > Cc: Valerii Chernous <vchernou@cisco.com> > Signed-off-by: Valerii Chernous <vchernou@cisco.com> > --- > Documentation/kbuild/kbuild.rst | 14 +++++++++++++- > Documentation/kbuild/modules.rst | 16 +++++++++++++++- > Makefile | 17 +++++++++++++++++ > scripts/Makefile.build | 7 +++++++ > 4 files changed, 52 insertions(+), 2 deletions(-) > I can read it now. There are still a few small things that I would change, but they aren't a big deal. I'll leave it for Masahiro or others to comment on. Thanks.
Hi Valerii, thanks for your patch. I know several non-upstream kernel developers that would like to see support for out-of-source builds of external kmods (and we developed several work-arounds at AVM as well); but please be aware that patches that don't fix or enhance the in-tree kernel/kmod build but only add feature for out-of-tree stuff, are rarely accepted. (Rational: better upstream your kmods to have them in-tree, especially if they are so complex that they need subdirs.) That said, I'll try to give some more concrete feedback below. On Fri, Apr 05, 2024 at 09:56:08AM -0700, Valerii Chernous wrote: > The change allow to build external modules with nested makefiles. better use imperative statements to the code itself: "Allow to build..." Does "nested makefile" mean that you want to support external kernel modules with subdirs and Makefiles within? > With current unofficial way(using "src" variable) it is possible to build > external(out of tree) kernel module with separate source and build > artifacts dirs but with nested makefiles it doesn't work properly. > Build system trap to recursion inside makefiles, artifacts output dir > path grow with each iteration until exceed max path len and build failed. I think I know what you want to say with this sentence, but I am not able to parse it correctly. Might you want to rephrase it a bit? > Providing "MO" variable and using "override" directive with declaring > "src" variable solves the problem > Usage example: > make -C KERNEL_SOURCE_TREE MO=BUILD_OUT_DIR M=EXT_MOD_SRC_DIR modules > > Cc: xe-linux-external@cisco.com > Cc: Valerii Chernous <vchernou@cisco.com> > Signed-off-by: Valerii Chernous <vchernou@cisco.com> > --- > Documentation/kbuild/kbuild.rst | 14 +++++++++++++- > Documentation/kbuild/modules.rst | 16 +++++++++++++++- > Makefile | 17 +++++++++++++++++ > scripts/Makefile.build | 7 +++++++ > 4 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst > index 9c8d1d046ea5..81413ddba2ad 100644 > --- a/Documentation/kbuild/kbuild.rst > +++ b/Documentation/kbuild/kbuild.rst > @@ -121,10 +121,22 @@ Setting "V=..." takes precedence over KBUILD_VERBOSE. > KBUILD_EXTMOD > ------------- > Set the directory to look for the kernel source when building external > -modules. > +modules. In case of using separate sources and module artifacts directories > +(KBUILD_EXTMOD + KBUILD_EXTMOD_SRC), KBUILD_EXTMOD working as output > +artifacts directory. That means, iff you use KBUILD_EXTMOD_SRC and let KBUILD_EXTMOD point to some other directory, you _have_ to be sure that your kernel supported KBUILD_EXTMOD_SRC properly or your module will not be build at all, right? > > Setting "M=..." takes precedence over KBUILD_EXTMOD. > > +KBUILD_EXTMOD_SRC > +----------------- > +Set the external module source directory in case when separate module > +sources and build artifacts directories are used. Working in pair with > +KBUILD_EXTMOD. If KBUILD_EXTMOD_SRC is set then KBUILD_EXTMOD is used as > +module build artifacts directory. > + > +Setting "MO=..." takes precedence over KBUILD_EXTMOD. > +Setting "M=..." takes precedence over KBUILD_EXTMOD_SRC. (Just a note, not a complaint: This confused me while reading it the first time, but I think it is correct for your patch. Personally, I do not like the semantical change of KBUILD_EXTMOD/M and would rather prefer the introduction of some more explicit names like KBUILD_EXTMOD_SOURCE and KBUILD_EXTMOD_BUILD instead.) > + > KBUILD_OUTPUT > ------------- > Specify the output directory when building the kernel. > diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst > index a1f3eb7a43e2..b6c30e76b314 100644 > --- a/Documentation/kbuild/modules.rst > +++ b/Documentation/kbuild/modules.rst > @@ -79,6 +79,14 @@ executed to make module versioning work. > The kbuild system knows that an external module is being built > due to the "M=<dir>" option given in the command. > > + To build an external module with separate src and artifacts dirs use:: > + > + $ make -C <path_to_kernel_src> M=$PWD MO=<output_dir> Is it really good to evaluate MO relative to the kernel source or build tree? make -C /lib/modules/$(uname -r)/build M=/somewhere/source MO=../build might accidentially lead to ugly inconsistencies in the kernel build tree (permissions presumed). > + > + The kbuild system knows that an external module with separate sources > + and build artifacts dirs is being built due to the "M=<dir>" and > + "MO=<output_dir>" options given in the command. > + > To build against the running kernel use:: > > $ make -C /lib/modules/`uname -r`/build M=$PWD > @@ -93,7 +101,7 @@ executed to make module versioning work. > > ($KDIR refers to the path of the kernel source directory.) > > - make -C $KDIR M=$PWD > + make -C $KDIR M=$PWD MO=<module_output_dir> > > -C $KDIR > The directory where the kernel source is located. > @@ -106,6 +114,12 @@ executed to make module versioning work. > directory where the external module (kbuild file) is > located. > > + MO=<module_output_dir> > + Informs kbuild that external module build artifacts > + should be placed into specific dir(<module_output_dir>). What about "should be placed into the specified directory <module_output_dir>." ? > + Parameter is optional. Without it "M" works as both > + source provider and build output location. > + > 2.3 Targets > =========== > > diff --git a/Makefile b/Makefile > index 4bef6323c47d..3d45a41737a6 100644 > --- a/Makefile > +++ b/Makefile > @@ -142,6 +142,7 @@ ifeq ("$(origin M)", "command line") > KBUILD_EXTMOD := $(M) > endif > > +define kbuild_extmod_check_TEMPLATE > $(if $(word 2, $(KBUILD_EXTMOD)), \ > $(error building multiple external modules is not supported)) > > @@ -152,9 +153,25 @@ $(foreach x, % :, $(if $(findstring $x, $(KBUILD_EXTMOD)), \ > ifneq ($(filter %/, $(KBUILD_EXTMOD)),) > KBUILD_EXTMOD := $(shell dirname $(KBUILD_EXTMOD).) > endif > +endef > +$(eval $(call kbuild_extmod_check_TEMPLATE)) Same checks make sense for KBUILD_EXTMOD_SRC, also if MO is not set. > > export KBUILD_EXTMOD > > +# Use make M=src_dir MO=ko_dir or set the environment variables: > +# KBUILD_EXTMOD_SRC, KBUILD_EXTMOD to specify separate directories of > +# external module sources and build artifacts. > +ifeq ("$(origin MO)", "command line") > +ifeq ($(KBUILD_EXTMOD),) > + $(error Ext module objects without module sources is not supported)) > +endif > +KBUILD_EXTMOD_SRC := $(KBUILD_EXTMOD) > +KBUILD_EXTMOD := $(MO) > +$(eval $(call kbuild_extmod_check_TEMPLATE)) > +endif > + > +export KBUILD_EXTMOD_SRC > + > # backward compatibility > KBUILD_EXTRA_WARN ?= $(KBUILD_ENABLE_EXTRA_GCC_CHECKS) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index baf86c0880b6..a293950e2e07 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -3,7 +3,14 @@ > # Building > # ========================================================================== > > +ifeq ($(KBUILD_EXTMOD_SRC),) > src := $(obj) I would leave the 'src := $(obj)' stand alone unconditionally, to clearly separate the oot-oos-kmod support from default in-tree kernel or kmod builds and in-source but out-of-tree kmod builds. > +else ifeq ($(KBUILD_EXTMOD),$(obj)) > +override src := $(KBUILD_EXTMOD_SRC) > +else > +src_subdir := $(patsubst $(KBUILD_EXTMOD)/%,%,$(obj)) > +override src := $(KBUILD_EXTMOD_SRC)/$(src_subdir) bike-shedding: see below > +endif > > PHONY := $(obj)/ > $(obj)/: > -- > 2.35.6 > Testing with a simple module w/ subdir, compilation fails for me: $ make M=../stupid-multidir-kmod/source V=2 MO=/tmp/build CC [M] /tmp/build/subdir/module.o - due to target missing CC [M] /tmp/build/hello.o - due to target missing scripts/Makefile.modpost:118: /tmp/build/Makefile: No such file or directory make[2]: *** No rule to make target '/tmp/build/Makefile'. Stop. make[1]: *** [/data/linux/oot-kmods-MO/Makefile:1897: modpost] Error 2 make: *** [Makefile:257: __sub-make] Error 2 (similar for 'make clean'.) The test kbuild files were as simple as: .../source/Kbuild: obj-m += subdir/ obj-m += hello.o .../source/subdir/Kbuild: obj-m += module.o Does something like this work for you? If I understand your proposed commit message correctly, you have some working example with subdirs? --- Bike-shedding for inspiration: While experimenting with similar solutions at AVM, we did the same src override but also auto-generated dummy Makefiles in $(obj) with something like: ifneq ($(if $(KBUILD_EXTMOD_SRC),$(filter $(KBUILD_EXTMOD)%,$(obj))),) # If KBUILD_EXTMOD_SOURCE is set, enable out-of-source kmod build # support, in general. But do not disturb the kbuild preparing targets # that need the original behaviour. # # Thus, iff also $(obj) is set and points to some path at $(KBUILD_EXTMOD) or # below, we really want to set (override) $(src). override src := $(obj:$(KBUILD_EXTMOD)%=$(KBUILD_EXTMOD_SRC)%) # Generate or update $(obj)/Makefile to include the original $(src)/Kbuild or # $(src)/Makefile. _kbuild_extmod_src_makefile = $(firstword $(wildcard $(src)/Kbuild $(src)/Makefile)) $(lastword $(MAKEFILE_LIST)): $(obj)/Makefile $(obj)/Makefile: $(_kbuild_extmod_src_makefile) FORCE $(call filechk,build_makefile) $(eval filechk_build_makefile = echo include $(_kbuild_extmod_src_makefile)) # Clean-up the variable space undefine $(filter _kbuild_extmod_%,$(.VARIABLES)) endif but we still had to add a dependency on $(subdir-ym) for all object files in any kmod subdir to enforce proper dependency handling. Fortunately, we almost stopped using such "enhanced" oot-oos kmod build things. HTH, kind regards, Nicolas
On Thu, Apr 11, 2024 at 01:38:20PM +0200, Nicolas Schier wrote: > Hi Valerii, > > thanks for your patch. I know several non-upstream kernel developers > that would like to see support for out-of-source builds of external > kmods (and we developed several work-arounds at AVM as well); but please > be aware that patches that don't fix or enhance the in-tree kernel/kmod > build but only add feature for out-of-tree stuff, are rarely accepted. > If that were true we would not have driver/uio/ for example. It seems like Cisco and NVM should work together produce a solution. You could run into this issue even with entirely in tree modules. For example, we may have a v6.6 kernel but we need some modules from v5.15 for some incompatibility reason in v6.6. Then we may build the v5.15 modules as out of tree modules against the v6.6 kernel. You also have just normal developers making kernel modules which always start as out of tree modules before they are upstreamed. Those modules could be any level of complexity. I don't think it make sense to view this as strictly enhancing large of the tree modules with no upstream potential. Daniel
On Thu, Apr 11, 2024 at 05:27:42PM +0000 Daniel Walker (danielwa) wrote: > On Thu, Apr 11, 2024 at 01:38:20PM +0200, Nicolas Schier wrote: > > Hi Valerii, > > > > thanks for your patch. I know several non-upstream kernel developers > > that would like to see support for out-of-source builds of external > > kmods (and we developed several work-arounds at AVM as well); but please > > be aware that patches that don't fix or enhance the in-tree kernel/kmod > > build but only add feature for out-of-tree stuff, are rarely accepted. "out-of-tree stuff" was meant to be "out-of-tree kernel modules". "Rarely" was chosen in explicit contrast to "never", but to hint on my personal expectation regarding the probability of acceptance. > If that were true we would not have driver/uio/ for example. It seems like > Cisco and NVM should work together produce a solution. > > You could run into this issue even with entirely in tree modules. For example, > we may have a v6.6 kernel but we need some modules from v5.15 for some incompatibility > reason in v6.6. Then we may build the v5.15 modules as out of tree modules > against the v6.6 kernel. If your in-tree module in question does compile and run properly in v5.15 and in v6.6: why don't you just compile it in-tree in v6.6? Which driver/module do you refer to? > You also have just normal developers making kernel modules which always start as > out of tree modules before they are upstreamed. Those modules could be any level > of complexity. I do not agree, but there is no need to convince me as I am not in the position to decide between acceptance or denial. I just thought it might be fair to warn that I do not expect acceptance. Kind regards, Nicolas > I don't think it make sense to view this as strictly enhancing large of the tree > modules with no upstream potential. > > Daniel
On Thu, Apr 11, 2024 at 09:25:35PM +0200, Nicolas Schier wrote: > On Thu, Apr 11, 2024 at 05:27:42PM +0000 Daniel Walker (danielwa) wrote: > > On Thu, Apr 11, 2024 at 01:38:20PM +0200, Nicolas Schier wrote: > > > Hi Valerii, > > > > > > thanks for your patch. I know several non-upstream kernel developers > > > that would like to see support for out-of-source builds of external > > > kmods (and we developed several work-arounds at AVM as well); but please > > > be aware that patches that don't fix or enhance the in-tree kernel/kmod > > > build but only add feature for out-of-tree stuff, are rarely accepted. > > "out-of-tree stuff" was meant to be "out-of-tree kernel modules". "Rarely" was > chosen in explicit contrast to "never", but to hint on my personal expectation > regarding the probability of acceptance. > > > If that were true we would not have driver/uio/ for example. It seems like > > Cisco and NVM should work together produce a solution. > > > > You could run into this issue even with entirely in tree modules. For example, > > we may have a v6.6 kernel but we need some modules from v5.15 for some incompatibility > > reason in v6.6. Then we may build the v5.15 modules as out of tree modules > > against the v6.6 kernel. All problems should be fixed or worked around. One bit of code maybe isn't the best choice or maybe another is, but not fixing or working around the problem is not really an option. > If your in-tree module in question does compile and run properly in v5.15 and > in v6.6: why don't you just compile it in-tree in v6.6? Which driver/module do > you refer to? I believe it was this driver drivers/crypto/marvell/octeontx2 . I don't recall every aspect of the issues but it has to do with what Marvell supported in their SDK and the exact hardware we were using and the bootloader we had on the product. > > You also have just normal developers making kernel modules which always start as > > out of tree modules before they are upstreamed. Those modules could be any level > > of complexity. > > I do not agree, but there is no need to convince me as I am not in the position > to decide between acceptance or denial. I just thought it might be fair to > warn that I do not expect acceptance. I think it's incorrect, unhealthy even, to look at it that way. If your using Linux to make a product and you have an issue, it should be consider as a real issue. Not something maintainer can just discard. Unless the maintainer has a suggestion to do what is needed or different code to do it. Daniel
On Thu, Apr 11, 2024 at 08:50:10PM +0000, Daniel Walker (danielwa) wrote: > On Thu, Apr 11, 2024 at 09:25:35PM +0200, Nicolas Schier wrote: > > On Thu, Apr 11, 2024 at 05:27:42PM +0000 Daniel Walker (danielwa) wrote: [...] > > > If that were true we would not have driver/uio/ for example. It seems like > > > Cisco and NVM should work together produce a solution. > > > > > > You could run into this issue even with entirely in tree modules. For example, > > > we may have a v6.6 kernel but we need some modules from v5.15 for some incompatibility > > > reason in v6.6. Then we may build the v5.15 modules as out of tree modules > > > against the v6.6 kernel. > > All problems should be fixed or worked around. One bit of code maybe isn't > the best choice or maybe another is, but not fixing or working around the > problem is not really an option. Let me sum up: It is possible to build out-of-tree kmods with subdirs in their source tree. The patch attempts to put support for _out-of-source builds_ of out-of-tree kmods with subdirs into kbuild itself. If you really out-of-source builds for your complex out-of-tree kmods, than, as a "work-around", you can simply put those 'src' override lines into your oot-Kbuild files. But you probably know that already, right? > > If your in-tree module in question does compile and run properly in v5.15 and > > in v6.6: why don't you just compile it in-tree in v6.6? Which driver/module do > > you refer to? > > I believe it was this driver drivers/crypto/marvell/octeontx2 . I don't recall > every aspect of the issues but it has to do with what Marvell supported in their > SDK and the exact hardware we were using and the bootloader we had on the > product. > > > > You also have just normal developers making kernel modules which always start as > > > out of tree modules before they are upstreamed. Those modules could be any level > > > of complexity. > > > > I do not agree, but there is no need to convince me as I am not in the position > > to decide between acceptance or denial. I just thought it might be fair to > > warn that I do not expect acceptance. > > I think it's incorrect, unhealthy even, to look at it that way. If your using > Linux to make a product and you have an issue, it should be consider as a real > issue. Not something maintainer can just discard. Unless the maintainer has > a suggestion to do what is needed or different code to do it. > > Daniel Daniel, I am confused about the outcome from your argumentation that you might expect. And I think, I as a spare-time reviewer (not maintainer), am not the one you want to argue with. If you have a concrete technical issue or bug, please explain it concretely to linux-kbuild and we will probably find someone trying to help you. If you want me to hide critical thoughts when reviewing patches under your pillow, then please tell me so. Have a nice weekend, Nicolas
Hi Nicolas, Thank you for your review and comments, let me answer to your comments: > Hi Valerii, > > thanks for your patch. I know several non-upstream kernel developers > that would like to see support for out-of-source builds of external > kmods (and we developed several work-arounds at AVM as well); but please > be aware that patches that don't fix or enhance the in-tree kernel/kmod > build but only add feature for out-of-tree stuff, are rarely accepted. > > (Rational: better upstream your kmods to have them in-tree, especially > if they are so complex that they need subdirs.) > > That said, I'll try to give some more concrete feedback below. > > On Fri, Apr 05, 2024 at 09:56:08AM -0700, Valerii Chernous wrote: > > The change allow to build external modules with nested makefiles. > > better use imperative statements to the code itself: "Allow to build..." > > Does "nested makefile" mean that you want to support external kernel > modules with subdirs and Makefiles within? Short answer "Yes". I tested my patch on couple of external modules with subdirs(Makefile included into each subdir) and it working well for me. > > With current unofficial way(using "src" variable) it is possible to build > > external(out of tree) kernel module with separate source and build > > artifacts dirs but with nested makefiles it doesn't work properly. > > Build system trap to recursion inside makefiles, artifacts output dir > > path grow with each iteration until exceed max path len and build failed. > > I think I know what you want to say with this sentence, but I am not > able to parse it correctly. Might you want to rephrase it a bit? Could you provide more details what you prefer to see into patch description? > > Providing "MO" variable and using "override" directive with declaring > > "src" variable solves the problem > > Usage example: > > make -C KERNEL_SOURCE_TREE MO=BUILD_OUT_DIR M=EXT_MOD_SRC_DIR modules > > > > Cc: xe-linux-external@cisco.com > > Cc: Valerii Chernous <vchernou@cisco.com> > > Signed-off-by: Valerii Chernous <vchernou@cisco.com> > > --- > > Documentation/kbuild/kbuild.rst | 14 +++++++++++++- > > Documentation/kbuild/modules.rst | 16 +++++++++++++++- > > Makefile | 17 +++++++++++++++++ > > scripts/Makefile.build | 7 +++++++ > > 4 files changed, 52 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst > > index 9c8d1d046ea5..81413ddba2ad 100644 > > --- a/Documentation/kbuild/kbuild.rst > > +++ b/Documentation/kbuild/kbuild.rst > > @@ -121,10 +121,22 @@ Setting "V=..." takes precedence over KBUILD_VERBOSE. > > KBUILD_EXTMOD > > ------------- > > Set the directory to look for the kernel source when building external > > -modules. > > +modules. In case of using separate sources and module artifacts directories > > +(KBUILD_EXTMOD + KBUILD_EXTMOD_SRC), KBUILD_EXTMOD working as output > > +artifacts directory. > > That means, iff you use KBUILD_EXTMOD_SRC and let KBUILD_EXTMOD point to > some other directory, you _have_ to be sure that your kernel supported > KBUILD_EXTMOD_SRC properly or your module will not be build at all, > right? If you will try to use KBUILD_EXTMOD_SRC but kernel doesn't support it then module will not be build at all because module sources with "Makefile" located at KBUILD_EXTMOD_SRC and "make" can't use this "Makefile"(it try to use Makefile from KBUILD_EXTMOD but it actually point to artifacts dir with absent or empty Makefile) > > Setting "M=..." takes precedence over KBUILD_EXTMOD. > > > > +KBUILD_EXTMOD_SRC > > +----------------- > > +Set the external module source directory in case when separate module > > +sources and build artifacts directories are used. Working in pair with > > +KBUILD_EXTMOD. If KBUILD_EXTMOD_SRC is set then KBUILD_EXTMOD is used as > > +module build artifacts directory. > > + > > +Setting "MO=..." takes precedence over KBUILD_EXTMOD. > > +Setting "M=..." takes precedence over KBUILD_EXTMOD_SRC. > > (Just a note, not a complaint: This confused me while reading it the > first time, but I think it is correct for your patch. Personally, I do > not like the semantical change of KBUILD_EXTMOD/M and would rather > prefer the introduction of some more explicit names like > KBUILD_EXTMOD_SOURCE and KBUILD_EXTMOD_BUILD instead.) Yes, this semantical change little bit confusing. We discussed this issue internally (inside my team) but at this point decided to try upstream current patch version because doing this work without this semantical chages require much bigger patch(maybe even patch series) and will take much more reviewer time and work > > + > > KBUILD_OUTPUT > > ------------- > > Specify the output directory when building the kernel. > > diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst > > index a1f3eb7a43e2..b6c30e76b314 100644 > > --- a/Documentation/kbuild/modules.rst > > +++ b/Documentation/kbuild/modules.rst > > @@ -79,6 +79,14 @@ executed to make module versioning work. > > The kbuild system knows that an external module is being built > > due to the "M=<dir>" option given in the command. > > > > + To build an external module with separate src and artifacts dirs use:: > > + > > + $ make -C <path_to_kernel_src> M=$PWD MO=<output_dir> > > Is it really good to evaluate MO relative to the kernel source or build tree? At this moment there is not so much restrictions on "MO" presented. You can provide absolute or relative path via "MO" and, yes, it is possible to provide some ugly configuration but developers should understand what they provide to kernel build system, is not it? > make -C /lib/modules/$(uname -r)/build M=/somewhere/source MO=../build > > might accidentially lead to ugly inconsistencies in the kernel build > tree (permissions presumed). > > > + > > + The kbuild system knows that an external module with separate sources > > + and build artifacts dirs is being built due to the "M=<dir>" and > > + "MO=<output_dir>" options given in the command. > > + > > To build against the running kernel use:: > > > > $ make -C /lib/modules/`uname -r`/build M=$PWD > > @@ -93,7 +101,7 @@ executed to make module versioning work. > > > > ($KDIR refers to the path of the kernel source directory.) > > > > - make -C $KDIR M=$PWD > > + make -C $KDIR M=$PWD MO=<module_output_dir> > > > > -C $KDIR > > The directory where the kernel source is located. > > @@ -106,6 +114,12 @@ executed to make module versioning work. > > directory where the external module (kbuild file) is > > located. > > > > + MO=<module_output_dir> > > + Informs kbuild that external module build artifacts > > + should be placed into specific dir(<module_output_dir>). > > What about "should be placed into the specified directory <module_output_dir>." ? Do you think it will be better to use "will" instead "should", whole phrase: Informs kbuild that external module build artifacts will be placed into specific dir(<module_output_dir>). > > + Parameter is optional. Without it "M" works as both > > + source provider and build output location. > > + > > 2.3 Targets > > =========== > > > > diff --git a/Makefile b/Makefile > > index 4bef6323c47d..3d45a41737a6 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -142,6 +142,7 @@ ifeq ("$(origin M)", "command line") > > KBUILD_EXTMOD := $(M) > > endif > > > > +define kbuild_extmod_check_TEMPLATE > > $(if $(word 2, $(KBUILD_EXTMOD)), \ > > $(error building multiple external modules is not supported)) > > > > @@ -152,9 +153,25 @@ $(foreach x, % :, $(if $(findstring $x, $(KBUILD_EXTMOD)), \ > > ifneq ($(filter %/, $(KBUILD_EXTMOD)),) > > KBUILD_EXTMOD := $(shell dirname $(KBUILD_EXTMOD).) > > endif > > +endef > > +$(eval $(call kbuild_extmod_check_TEMPLATE)) > > Same checks make sense for KBUILD_EXTMOD_SRC, also if MO is not set. Make sence, but it will require more changes with defining template kbuild_extmod_check_TEMPLATE. Will update in next patch version. > > > > export KBUILD_EXTMOD > > > > +# Use make M=src_dir MO=ko_dir or set the environment variables: > > +# KBUILD_EXTMOD_SRC, KBUILD_EXTMOD to specify separate directories of > > +# external module sources and build artifacts. > > +ifeq ("$(origin MO)", "command line") > > +ifeq ($(KBUILD_EXTMOD),) > > + $(error Ext module objects without module sources is not supported)) > > +endif > > +KBUILD_EXTMOD_SRC := $(KBUILD_EXTMOD) > > +KBUILD_EXTMOD := $(MO) > > +$(eval $(call kbuild_extmod_check_TEMPLATE)) > > +endif > > + > > +export KBUILD_EXTMOD_SRC > > + > > # backward compatibility > > KBUILD_EXTRA_WARN ?= $(KBUILD_ENABLE_EXTRA_GCC_CHECKS) > > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > > index baf86c0880b6..a293950e2e07 100644 > > --- a/scripts/Makefile.build > > +++ b/scripts/Makefile.build > > @@ -3,7 +3,14 @@ > > # Building > > # ========================================================================== > > > > +ifeq ($(KBUILD_EXTMOD_SRC),) > > src := $(obj) > > I would leave the 'src := $(obj)' stand alone unconditionally, to > clearly separate the oot-oos-kmod support from default in-tree kernel or > kmod builds and in-source but out-of-tree kmod builds. In case of separating in-tree and out-of-tree src processing it will be not couple of line patch :) Main idea of patch is create some small change to provide required functionality. Current processing of "src" do required work, it only need to have proper value of this variable. Also, please, don't forget that each new line of code require engineers time for supporting it into the feature. My opinon - patch must provide require functionality and should be as small as it possible > > +else ifeq ($(KBUILD_EXTMOD),$(obj)) > > +override src := $(KBUILD_EXTMOD_SRC) > > +else > > +src_subdir := $(patsubst $(KBUILD_EXTMOD)/%,%,$(obj)) > > +override src := $(KBUILD_EXTMOD_SRC)/$(src_subdir) > > bike-shedding: see below I think it's a good idea to use next line instead of my couples of lines: override src := $(obj:$(KBUILD_EXTMOD)%=$(KBUILD_EXTMOD_SRC)%) but creating separate Makefile and do additional changes it's not required overhead. It increase code size and request additional resources/time on supporting changes > > +endif > > > > PHONY := $(obj)/ > > $(obj)/: > > -- > > 2.35.6 > > > > Testing with a simple module w/ subdir, compilation fails for me: > > $ make M=../stupid-multidir-kmod/source V=2 MO=/tmp/build > CC [M] /tmp/build/subdir/module.o - due to target missing > CC [M] /tmp/build/hello.o - due to target missing > scripts/Makefile.modpost:118: /tmp/build/Makefile: No such file or directory > make[2]: *** No rule to make target '/tmp/build/Makefile'. Stop. > make[1]: *** [/data/linux/oot-kmods-MO/Makefile:1897: modpost] Error 2 > make: *** [Makefile:257: __sub-make] Error 2 > > (similar for 'make clean'.) > The test kbuild files were as simple as: > > .../source/Kbuild: > obj-m += subdir/ > obj-m += hello.o > > .../source/subdir/Kbuild: > obj-m += module.o > > Does something like this work for you? If I understand your proposed > commit message correctly, you have some working example with subdirs? Looks like working example, try to run next cmds before run build: mkdir -p /tmp/build && \ touch /tmp/build/Makefile Probably, this hack still required like into unofficial variant with using "M" and "src" > --- > > Bike-shedding for inspiration: > > While experimenting with similar solutions at AVM, we did the same src > override but also auto-generated dummy Makefiles in $(obj) with something > like: > > ifneq ($(if $(KBUILD_EXTMOD_SRC),$(filter $(KBUILD_EXTMOD)%,$(obj))),) > # If KBUILD_EXTMOD_SOURCE is set, enable out-of-source kmod build > # support, in general. But do not disturb the kbuild preparing targets > # that need the original behaviour. > # > # Thus, iff also $(obj) is set and points to some path at $(KBUILD_EXTMOD) or > # below, we really want to set (override) $(src). > > override src := $(obj:$(KBUILD_EXTMOD)%=$(KBUILD_EXTMOD_SRC)%) > > # Generate or update $(obj)/Makefile to include the original $(src)/Kbuild or > # $(src)/Makefile. > > _kbuild_extmod_src_makefile = $(firstword $(wildcard $(src)/Kbuild $(src)/Makefile)) > > $(lastword $(MAKEFILE_LIST)): $(obj)/Makefile > > $(obj)/Makefile: $(_kbuild_extmod_src_makefile) FORCE > $(call filechk,build_makefile) > > $(eval filechk_build_makefile = echo include $(_kbuild_extmod_src_makefile)) > > # Clean-up the variable space > undefine $(filter _kbuild_extmod_%,$(.VARIABLES)) > > endif > > but we still had to add a dependency on $(subdir-ym) for all object > files in any kmod subdir to enforce proper dependency handling. > Fortunately, we almost stopped using such "enhanced" oot-oos kmod build > things. I don't saw any additional requirements or dependencies into my builds, all that required is processed by current rules into Kbuild, it only that required into my case it's provide proper value of "src" variable > HTH, > kind regards, > Nicolas Best regards, Valerii
On Fri, Apr 12, 2024 at 08:34:31AM +0200, Nicolas Schier wrote: > On Thu, Apr 11, 2024 at 08:50:10PM +0000, Daniel Walker (danielwa) wrote: > > On Thu, Apr 11, 2024 at 09:25:35PM +0200, Nicolas Schier wrote: > > > On Thu, Apr 11, 2024 at 05:27:42PM +0000 Daniel Walker (danielwa) wrote: > [...] > > > > If that were true we would not have driver/uio/ for example. It seems like > > > > Cisco and NVM should work together produce a solution. > > > > > > > > You could run into this issue even with entirely in tree modules. For example, > > > > we may have a v6.6 kernel but we need some modules from v5.15 for some incompatibility > > > > reason in v6.6. Then we may build the v5.15 modules as out of tree modules > > > > against the v6.6 kernel. > > > > All problems should be fixed or worked around. One bit of code maybe isn't > > the best choice or maybe another is, but not fixing or working around the > > problem is not really an option. > > Let me sum up: It is possible to build out-of-tree kmods with subdirs > in their source tree. > The patch attempts to put support for _out-of-source builds_ of > out-of-tree kmods with subdirs into kbuild itself. > > If you really out-of-source builds for your complex out-of-tree kmods, > than, as a "work-around", you can simply put those 'src' override lines > into your oot-Kbuild files. But you probably know that already, right? I tried "make src=..." and Valerii also tried it. I think that's what your referring to. There must have been a defect added which prevents that from working. It has some sort of recursion issue. It seems this method was not "official" and only work by accident. > > > If your in-tree module in question does compile and run properly in v5.15 and > > > in v6.6: why don't you just compile it in-tree in v6.6? Which driver/module do > > > you refer to? > > > > I believe it was this driver drivers/crypto/marvell/octeontx2 . I don't recall > > every aspect of the issues but it has to do with what Marvell supported in their > > SDK and the exact hardware we were using and the bootloader we had on the > > product. > > > > > > You also have just normal developers making kernel modules which always start as > > > > out of tree modules before they are upstreamed. Those modules could be any level > > > > of complexity. > > > > > > I do not agree, but there is no need to convince me as I am not in the position > > > to decide between acceptance or denial. I just thought it might be fair to > > > warn that I do not expect acceptance. > > > > I think it's incorrect, unhealthy even, to look at it that way. If your using > > Linux to make a product and you have an issue, it should be consider as a real > > issue. Not something maintainer can just discard. Unless the maintainer has > > a suggestion to do what is needed or different code to do it. > > > > Daniel > > Daniel, > > I am confused about the outcome from your argumentation that you might > expect. And I think, I as a spare-time reviewer (not maintainer), am > not the one you want to argue with. I don't care if your a maintainer or not. > If you have a concrete technical issue or bug, please explain it > concretely to linux-kbuild and we will probably find someone trying to > help you. If you want me to hide critical thoughts when reviewing > patches under your pillow, then please tell me so. I don't think it's critical thinking to effectively tell someone to stop submitting code. Daniel
The workaround described in the commit message
(overwrite 'src') is different from what I know.
As I explained to Daniel before, the point is,
O= refers to the kernel output directory, and
M= specifies a relative path to your downstream
module directory.
Say, you have a linux source tree and external modules
under ~/my-project-src/, and you want to output all
build artifacts under ~/my-build-dir/.
my-project-src
|-- linux
\-- my-modules
masahiro@zoe:~/my-project-src/my-modules$ tree
.
|-- Kbuild
|-- dir1
| |-- Kbuild
| |-- bar.c
| `-- dir2
| |-- Kbuild
| `-- baz.c
`-- foo.c
3 directories, 6 files
masahiro@zoe:~/my-project-src/my-modules$ cat Kbuild
obj-m += foo.o
obj-m += dir1/
masahiro@zoe:~/my-project-src/my-modules$ cat dir1/Kbuild
obj-m += bar.o
obj-m += dir2/
masahiro@zoe:~/my-project-src/my-modules$ cat dir1/dir2/Kbuild
obj-m += baz.o
First, build the kernel and external modules
in separate output directories.
masahiro@zoe:~/my-project-src/linux$ make O=~/my-build-dir/linux defconfig all
[ snip ]
masahiro@zoe:~/my-project-src/linux$ make O=~/my-build-dir/linux M=../my-modules
make[1]: Entering directory '/home/masahiro/my-build-dir/linux'
CC [M] ../my-modules/dir1/dir2/baz.o
CC [M] ../my-modules/dir1/bar.o
CC [M] ../my-modules/foo.o
MODPOST ../my-modules/Module.symvers
CC [M] ../my-modules/foo.mod.o
LD [M] ../my-modules/foo.ko
CC [M] ../my-modules/dir1/bar.mod.o
LD [M] ../my-modules/dir1/bar.ko
CC [M] ../my-modules/dir1/dir2/baz.mod.o
LD [M] ../my-modules/dir1/dir2/baz.ko
make[1]: Leaving directory '/home/masahiro/my-build-dir/linux'
masahiro@zoe:~/my-build-dir/my-modules$ tree
.
|-- Module.symvers
|-- dir1
| |-- bar.ko
| |-- bar.mod
| |-- bar.mod.c
| |-- bar.mod.o
| |-- bar.o
| |-- dir2
| | |-- baz.ko
| | |-- baz.mod
| | |-- baz.mod.c
| | |-- baz.mod.o
| | |-- baz.o
| | `-- modules.order
| `-- modules.order
|-- foo.ko
|-- foo.mod
|-- foo.mod.c
|-- foo.mod.o
|-- foo.o
`-- modules.order
3 directories, 19 files
I saw this before somewhere.
I believe it is a well-known workaround
that works with recursion.
This patch submission is not helpful.
Kbuild does not support the external module builds
in a separate output directory.
Most people know this limitation for a long time.
You are not the first person to discover it.
Second, anybody can write a patch like yours
in several minutes.
There already exists a similar (but more correct) patch:
https://lore.kernel.org/linux-kbuild/e58cba84c90c40108ac678500f33655e@xiaomi.com/
That one works more correctly than yours, because modpost
works with no error, and 'make clean' works too.
I am not suggesting to fix scripts/{Makefile.modpost,Makefile.clean}.
If such a patch had been acceptable,
it would have been accepted many years before.
Things are, the decision is postponed until
we are confident about a solution.
I must avoid a situation where a bad solution
is upstreamed as official.
That is worse than nothing.
And, I am pretty sure that your patch is not
the right solution.
--
Best Regards
Masahiro Yamada
Hi Masahiro, Thank you for your response and for description of relative method with "O" and "M" variables. I'm tried it before start upstreaming my patch. It working but have strong restriction on external modules location correspond to kernel sources/output objects. Unfortunately, proper external module location for your method can't be guarantee into all projects and my not exceptional one. > The workaround described in the commit message > (overwrite 'src') is different from what I know. Yes, your method is relative method with using "O" and "M" variables and it different from one that I used > As I explained to Daniel before, the point is, > O= refers to the kernel output directory, and > M= specifies a relative path to your downstream > module directory. Thank you, again, for your explanation and I'm tried this before starting discussion with You and other guys from kernel team > Say, you have a linux source tree and external modules > under ~/my-project-src/, and you want to output all > build artifacts under ~/my-build-dir/. > my-project-src > |-- linux > \-- my-modules > masahiro@zoe:~/my-project-src/my-modules$ tree > . > |-- Kbuild > |-- dir1 > | |-- Kbuild > | |-- bar.c > | `-- dir2 > | |-- Kbuild > | `-- baz.c > `-- foo.c > 3 directories, 6 files > masahiro@zoe:~/my-project-src/my-modules$ cat Kbuild > obj-m += foo.o > obj-m += dir1/ > masahiro@zoe:~/my-project-src/my-modules$ cat dir1/Kbuild > obj-m += bar.o > obj-m += dir2/ > masahiro@zoe:~/my-project-src/my-modules$ cat dir1/dir2/Kbuild > obj-m += baz.o > First, build the kernel and external modules > in separate output directories. > masahiro@zoe:~/my-project-src/linux$ make O=~/my-build-dir/linux defconfig all > [ snip ] > masahiro@zoe:~/my-project-src/linux$ make O=~/my-build-dir/linux M=../my-modules > make[1]: Entering directory '/home/masahiro/my-build-dir/linux' > CC [M] ../my-modules/dir1/dir2/baz.o > CC [M] ../my-modules/dir1/bar.o > CC [M] ../my-modules/foo.o > MODPOST ../my-modules/Module.symvers > CC [M] ../my-modules/foo.mod.o > LD [M] ../my-modules/foo.ko > CC [M] ../my-modules/dir1/bar.mod.o > LD [M] ../my-modules/dir1/bar.ko > CC [M] ../my-modules/dir1/dir2/baz.mod.o > LD [M] ../my-modules/dir1/dir2/baz.ko > make[1]: Leaving directory '/home/masahiro/my-build-dir/linux' > masahiro@zoe:~/my-build-dir/my-modules$ tree > . > |-- Module.symvers > |-- dir1 > | |-- bar.ko > | |-- bar.mod > | |-- bar.mod.c > | |-- bar.mod.o > | |-- bar.o > | |-- dir2 > | | |-- baz.ko > | | |-- baz.mod > | | |-- baz.mod.c > | | |-- baz.mod.o > | | |-- baz.o > | | `-- modules.order > | `-- modules.order > |-- foo.ko > |-- foo.mod > |-- foo.mod.c > |-- foo.mod.o > |-- foo.o > `-- modules.order > 3 directories, 19 files > I saw this before somewhere. > I believe it is a well-known workaround > that works with recursion. I agree with You, Masahiro, this workaround works but it contain significant restriction that can't be implemented into all projects and as you told - it's workaround but not official method > This patch submission is not helpful. > Kbuild does not support the external module builds > in a separate output directory. > Most people know this limitation for a long time. > You are not the first person to discover it. Problem will be raised again and again, until somebody will implement it properly :). Of cause I'm not first who trap to this issue but I'm also not the last :) > Second, anybody can write a patch like yours > in several minutes. I'm totally agree with you, it's not a complex patch and that's a good news because small change can provide required functionality :). > There already exists a similar (but more correct) patch: > https://lore.kernel.org/linux-kbuild/e58cba84c90c40108ac678500f33655e@xiaomi.com/ > That one works more correctly than yours, because modpost > works with no error, and 'make clean' works too. > I am not suggesting to fix scripts/{Makefile.modpost,Makefile.clean}. > If such a patch had been acceptable, > it would have been accepted many years before. > Things are, the decision is postponed until > we are confident about a solution. > I must avoid a situation where a bad solution > is upstreamed as official. > That is worse than nothing. Ok, I'm understand your opinion. > And, I am pretty sure that your patch is not > the right solution. > -- > Best Regards > Masahiro Yamada Ok, maybe one line patch that I submitted today will be more acceptable compare to current one. It don't create any new variables, it only fix "src" variable recursion. Best regards, Valerii
diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst index 9c8d1d046ea5..81413ddba2ad 100644 --- a/Documentation/kbuild/kbuild.rst +++ b/Documentation/kbuild/kbuild.rst @@ -121,10 +121,22 @@ Setting "V=..." takes precedence over KBUILD_VERBOSE. KBUILD_EXTMOD ------------- Set the directory to look for the kernel source when building external -modules. +modules. In case of using separate sources and module artifacts directories +(KBUILD_EXTMOD + KBUILD_EXTMOD_SRC), KBUILD_EXTMOD working as output +artifacts directory. Setting "M=..." takes precedence over KBUILD_EXTMOD. +KBUILD_EXTMOD_SRC +----------------- +Set the external module source directory in case when separate module +sources and build artifacts directories are used. Working in pair with +KBUILD_EXTMOD. If KBUILD_EXTMOD_SRC is set then KBUILD_EXTMOD is used as +module build artifacts directory. + +Setting "MO=..." takes precedence over KBUILD_EXTMOD. +Setting "M=..." takes precedence over KBUILD_EXTMOD_SRC. + KBUILD_OUTPUT ------------- Specify the output directory when building the kernel. diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst index a1f3eb7a43e2..b6c30e76b314 100644 --- a/Documentation/kbuild/modules.rst +++ b/Documentation/kbuild/modules.rst @@ -79,6 +79,14 @@ executed to make module versioning work. The kbuild system knows that an external module is being built due to the "M=<dir>" option given in the command. + To build an external module with separate src and artifacts dirs use:: + + $ make -C <path_to_kernel_src> M=$PWD MO=<output_dir> + + The kbuild system knows that an external module with separate sources + and build artifacts dirs is being built due to the "M=<dir>" and + "MO=<output_dir>" options given in the command. + To build against the running kernel use:: $ make -C /lib/modules/`uname -r`/build M=$PWD @@ -93,7 +101,7 @@ executed to make module versioning work. ($KDIR refers to the path of the kernel source directory.) - make -C $KDIR M=$PWD + make -C $KDIR M=$PWD MO=<module_output_dir> -C $KDIR The directory where the kernel source is located. @@ -106,6 +114,12 @@ executed to make module versioning work. directory where the external module (kbuild file) is located. + MO=<module_output_dir> + Informs kbuild that external module build artifacts + should be placed into specific dir(<module_output_dir>). + Parameter is optional. Without it "M" works as both + source provider and build output location. + 2.3 Targets =========== diff --git a/Makefile b/Makefile index 4bef6323c47d..3d45a41737a6 100644 --- a/Makefile +++ b/Makefile @@ -142,6 +142,7 @@ ifeq ("$(origin M)", "command line") KBUILD_EXTMOD := $(M) endif +define kbuild_extmod_check_TEMPLATE $(if $(word 2, $(KBUILD_EXTMOD)), \ $(error building multiple external modules is not supported)) @@ -152,9 +153,25 @@ $(foreach x, % :, $(if $(findstring $x, $(KBUILD_EXTMOD)), \ ifneq ($(filter %/, $(KBUILD_EXTMOD)),) KBUILD_EXTMOD := $(shell dirname $(KBUILD_EXTMOD).) endif +endef +$(eval $(call kbuild_extmod_check_TEMPLATE)) export KBUILD_EXTMOD +# Use make M=src_dir MO=ko_dir or set the environment variables: +# KBUILD_EXTMOD_SRC, KBUILD_EXTMOD to specify separate directories of +# external module sources and build artifacts. +ifeq ("$(origin MO)", "command line") +ifeq ($(KBUILD_EXTMOD),) + $(error Ext module objects without module sources is not supported)) +endif +KBUILD_EXTMOD_SRC := $(KBUILD_EXTMOD) +KBUILD_EXTMOD := $(MO) +$(eval $(call kbuild_extmod_check_TEMPLATE)) +endif + +export KBUILD_EXTMOD_SRC + # backward compatibility KBUILD_EXTRA_WARN ?= $(KBUILD_ENABLE_EXTRA_GCC_CHECKS) diff --git a/scripts/Makefile.build b/scripts/Makefile.build index baf86c0880b6..a293950e2e07 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -3,7 +3,14 @@ # Building # ========================================================================== +ifeq ($(KBUILD_EXTMOD_SRC),) src := $(obj) +else ifeq ($(KBUILD_EXTMOD),$(obj)) +override src := $(KBUILD_EXTMOD_SRC) +else +src_subdir := $(patsubst $(KBUILD_EXTMOD)/%,%,$(obj)) +override src := $(KBUILD_EXTMOD_SRC)/$(src_subdir) +endif PHONY := $(obj)/ $(obj)/:
The change allow to build external modules with nested makefiles. With current unofficial way(using "src" variable) it is possible to build external(out of tree) kernel module with separate source and build artifacts dirs but with nested makefiles it doesn't work properly. Build system trap to recursion inside makefiles, artifacts output dir path grow with each iteration until exceed max path len and build failed. Providing "MO" variable and using "override" directive with declaring "src" variable solves the problem Usage example: make -C KERNEL_SOURCE_TREE MO=BUILD_OUT_DIR M=EXT_MOD_SRC_DIR modules Cc: xe-linux-external@cisco.com Cc: Valerii Chernous <vchernou@cisco.com> Signed-off-by: Valerii Chernous <vchernou@cisco.com> --- Documentation/kbuild/kbuild.rst | 14 +++++++++++++- Documentation/kbuild/modules.rst | 16 +++++++++++++++- Makefile | 17 +++++++++++++++++ scripts/Makefile.build | 7 +++++++ 4 files changed, 52 insertions(+), 2 deletions(-)