* [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 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 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 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.