* [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build @ 2018-11-02 12:36 Sumit Garg 2018-11-02 13:57 ` Erico Nunes 2018-11-02 18:18 ` Thomas Petazzoni 0 siblings, 2 replies; 6+ messages in thread From: Sumit Garg @ 2018-11-02 12:36 UTC (permalink / raw) To: buildroot Firmware test suite does provides efi_runtime kernel module required to run UEFI tests. So optionally enable this module build. Signed-off-by: Sumit Garg <sumit.garg@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" + depends on BR2_PACKAGE_FWTS + depends on BR2_LINUX_KERNEL + help + Firmware Test Suite (FWTS) also provides EFI runtime kernel + module required to run UEFI tests. 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 +FWTS_MODULE_SUBDIRS = efi_runtime +$(eval $(kernel-module)) +endif -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build 2018-11-02 12:36 [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build Sumit Garg @ 2018-11-02 13:57 ` Erico Nunes 2018-11-02 14:19 ` Yann E. MORIN 2018-11-02 18:18 ` Thomas Petazzoni 1 sibling, 1 reply; 6+ messages in thread From: Erico Nunes @ 2018-11-02 13:57 UTC (permalink / raw) To: buildroot On Fri, Nov 2, 2018 at 1:37 PM Sumit Garg <sumit.garg@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@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build 2018-11-02 13:57 ` Erico Nunes @ 2018-11-02 14:19 ` Yann E. MORIN 2018-11-05 4:15 ` Sumit Garg 0 siblings, 1 reply; 6+ messages in thread From: Yann E. MORIN @ 2018-11-02 14:19 UTC (permalink / raw) To: buildroot Erico, Sumit, All, On 2018-11-02 14:57 +0100, Erico Nunes spake thusly: > On Fri, Nov 2, 2018 at 1:37 PM Sumit Garg <sumit.garg@linaro.org> wrote: > > +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". ACK. > > + 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. It depends (pun intended). While I do prefer an if-block as you suggest, some package do use 'depends on', especially when there is only one or two sub-options. So, historically, both exist, but using an the if-block makes it more consistent. > > + 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 Exactly: options are only indented when they are right below the option they depend on. This is an idiosyncracy of kconfig. And using 'depends on' or an if-block is basically just equivalent. > > 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. Exactly, see: https://buildroot.org/downloads/manual/manual.html#_infrastructure_for_packages_building_kernel_modules Unlike other package infrastructures, [kernel-module] is not stand-alone, and requires any of the other *-package macros be called after it. Regards, Yann E. MORIN. -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------' ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build 2018-11-02 14:19 ` Yann E. MORIN @ 2018-11-05 4:15 ` Sumit Garg 0 siblings, 0 replies; 6+ messages in thread From: Sumit Garg @ 2018-11-05 4:15 UTC (permalink / raw) To: buildroot On Fri, 2 Nov 2018 at 19:50, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Erico, Sumit, All, > > On 2018-11-02 14:57 +0100, Erico Nunes spake thusly: > > On Fri, Nov 2, 2018 at 1:37 PM Sumit Garg <sumit.garg@linaro.org> wrote: > > > +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". > > ACK. > Ok. > > > + 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. > > It depends (pun intended). While I do prefer an if-block as you suggest, > some package do use 'depends on', especially when there is only one or > two sub-options. So, historically, both exist, but using an the if-block > makes it more consistent. > Ok will use if block instead. > > > + 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 > > Exactly: options are only indented when they are right below the option > they depend on. This is an idiosyncracy of kconfig. > Ok will move this option right below "fwts". > And using 'depends on' or an if-block is basically just equivalent. > > > > 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). > > Ok will remove linux as a dependency. > > > +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. > Ok. > Exactly, see: > https://buildroot.org/downloads/manual/manual.html#_infrastructure_for_packages_building_kernel_modules > > Unlike other package infrastructures, [kernel-module] is not > stand-alone, and requires any of the other *-package macros be > called after it. > Thanks for the explanation. Regards, Sumit > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build 2018-11-02 12:36 [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build Sumit Garg 2018-11-02 13:57 ` Erico Nunes @ 2018-11-02 18:18 ` Thomas Petazzoni 2018-11-05 4:16 ` Sumit Garg 1 sibling, 1 reply; 6+ messages in thread From: Thomas Petazzoni @ 2018-11-02 18:18 UTC (permalink / raw) To: buildroot Hello, On Fri, 2 Nov 2018 18:06:47 +0530, Sumit Garg wrote: > Firmware test suite does provides efi_runtime kernel module required > to run UEFI tests. So optionally enable this module build. > > Signed-off-by: Sumit Garg <sumit.garg@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" > + depends on BR2_PACKAGE_FWTS > + depends on BR2_LINUX_KERNEL > + help > + Firmware Test Suite (FWTS) also provides EFI runtime kernel > + module required to run UEFI tests. In addition to the other comments being made: when there is a "depends on BR2_LINUX_KERNEL", we typically need add something like this: comment "efi-runtime module needs a Linux kernel to be built" depends on !BR2_LINUX_KERNEL Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build 2018-11-02 18:18 ` Thomas Petazzoni @ 2018-11-05 4:16 ` Sumit Garg 0 siblings, 0 replies; 6+ messages in thread From: Sumit Garg @ 2018-11-05 4:16 UTC (permalink / raw) To: buildroot On Fri, 2 Nov 2018 at 23:48, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello, > > On Fri, 2 Nov 2018 18:06:47 +0530, Sumit Garg wrote: > > Firmware test suite does provides efi_runtime kernel module required > > to run UEFI tests. So optionally enable this module build. > > > > Signed-off-by: Sumit Garg <sumit.garg@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" > > + depends on BR2_PACKAGE_FWTS > > + depends on BR2_LINUX_KERNEL > > + help > > + Firmware Test Suite (FWTS) also provides EFI runtime kernel > > + module required to run UEFI tests. > > In addition to the other comments being made: when there is a "depends > on BR2_LINUX_KERNEL", we typically need add something like this: > > comment "efi-runtime module needs a Linux kernel to be built" > depends on !BR2_LINUX_KERNEL > Sure will add this comment. Regards, Sumit > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-05 4:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-02 12:36 [Buildroot] [PATCH 1/1] fwts: Enable optional efi_runtime kernel module build Sumit Garg 2018-11-02 13:57 ` Erico Nunes 2018-11-02 14:19 ` Yann E. MORIN 2018-11-05 4:15 ` Sumit Garg 2018-11-02 18:18 ` Thomas Petazzoni 2018-11-05 4:16 ` Sumit Garg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox