[Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build
Erico Nunes
nunes.erico at gmail.com
Fri Nov 2 13:57:10 UTC 2018
On Fri, Nov 2, 2018 at 1:37 PM Sumit Garg <sumit.garg at linaro.org> wrote:
>
> Firmware test suite does provides efi_runtime kernel module required
> to run UEFI tests. So optionally enable this module build.
Thanks for working on this. I ended up never adding support for it in
the package because efi_runtime can also be enabled in the kernel
config (config EFI_TEST) rather than being provided by fwts. But I
think it's not bad to also have the support here for the source
shipped with the given fwts version.
I have some suggestions below.
>
> Signed-off-by: Sumit Garg <sumit.garg at linaro.org>
> ---
> package/fwts/Config.in | 8 ++++++++
> package/fwts/fwts.mk | 6 ++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/package/fwts/Config.in b/package/fwts/Config.in
> index 959d871..3ddb989 100644
> --- a/package/fwts/Config.in
> +++ b/package/fwts/Config.in
> @@ -28,3 +28,11 @@ comment "fwts needs a glibc toolchain w/ wchar, threads"
> depends on BR2_USE_MMU
> depends on !BR2_USE_WCHAR || !BR2_TOOLCHAIN_HAS_THREADS || \
> !BR2_TOOLCHAIN_USES_GLIBC
> +
> +config BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
> + bool "efi_runtime_module"
For a slightly better looking menu, I'd suggest dropping the second
underscore and making it "efi_runtime module".
> + depends on BR2_PACKAGE_FWTS
Looking at most packages, I think a more clear way to show this option
only then fwts is selected, is to wrap it inside a 'if
BR2_PACKAGE_FWTS' instead of a 'depends on' for this case. Not sure if
it changes anything in practice but it seems more appropriate than a
'depends on' to me.
> + depends on BR2_LINUX_KERNEL
> + help
> + Firmware Test Suite (FWTS) also provides EFI runtime kernel
> + module required to run UEFI tests.
As it is, the option looks confusing in the menu as it doesn't look
like a fwts suboption. It looks like this:
[*] fwts
[ ] efi_runtime_module
I don't fully understand the Kconfig inner workings, but if
BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE is moved to right after
BR2_PACKAGE_FWTS (before the comment) then it becomes automatically
indented and to me much more easier to understand that it is a fwts
suboption:
[*] fwts
[ ] efi_runtime_module
> diff --git a/package/fwts/fwts.mk b/package/fwts/fwts.mk
> index 15f0afc..840190e 100644
> --- a/package/fwts/fwts.mk
> +++ b/package/fwts/fwts.mk
> @@ -14,3 +14,9 @@ FWTS_DEPENDENCIES = host-bison host-flex host-pkgconf json-c libglib2 libbsd \
> $(if $(BR2_PACKAGE_DTC),dtc)
>
> $(eval $(autotools-package))
> +
> +ifdef BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE
> +FWTS_DEPENDENCIES += linux
You don't need to add linux as an explicit dependency if you are using
$(kernel-module).
> +FWTS_MODULE_SUBDIRS = efi_runtime
> +$(eval $(kernel-module))
> +endif
This doesn't work for me, it doesn't trigger linux as a dependency and
fails to build from a clean build. The entire block including the
$(eval $(kernel-module)) needs to happen before $(eval
$(autotools-package)) so that it works as intended.
Erico
More information about the buildroot
mailing list