Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y
diff mbox series

Message ID 20191002194346.GA3792@localhost.localdomain
State Superseded
Headers show
Series
  • Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y
Related show

Commit Message

Narendra.K@dell.com Oct. 2, 2019, 7:44 p.m. UTC
From: Narendra K <Narendra.K@dell.com>

For the EFI_RCI2_TABLE kconfig option, 'make oldconfig' asks the user
for input as it is a new kconfig option in kernel version 5.4. This patch
modifies the kconfig option to ask the user for input only when CONFIG_X86
or CONFIG_COMPILE_TEST is set to y.

The patch also makes EFI_RCI2_TABLE kconfig option depend on CONFIG_EFI.

Signed-off-by: Narendra K <Narendra.K@dell.com>
---
The patch is created on kernel version 5.4-rc1.

Hi Ard, I have made following changes -

- changed the prompt string from "EFI Runtime Configuration
Interface Table Version 2 Support" to "EFI RCI Table Version 2 Support"
as the string crossed 80 char limit. 

- added "depends on EFI" so that code builds only when CONFIG_EFI is
set to y.

- added 'default n' for ease of understanding though default is set to n.

 drivers/firmware/efi/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Ard Biesheuvel Oct. 9, 2019, 2:11 p.m. UTC | #1
Hello Narendra,

On Wed, 2 Oct 2019 at 21:44, <Narendra.K@dell.com> wrote:
>
> From: Narendra K <Narendra.K@dell.com>
>
> For the EFI_RCI2_TABLE kconfig option, 'make oldconfig' asks the user
> for input as it is a new kconfig option in kernel version 5.4. This patch
> modifies the kconfig option to ask the user for input only when CONFIG_X86
> or CONFIG_COMPILE_TEST is set to y.
>
> The patch also makes EFI_RCI2_TABLE kconfig option depend on CONFIG_EFI.
>
> Signed-off-by: Narendra K <Narendra.K@dell.com>
> ---
> The patch is created on kernel version 5.4-rc1.
>
> Hi Ard, I have made following changes -
>
> - changed the prompt string from "EFI Runtime Configuration
> Interface Table Version 2 Support" to "EFI RCI Table Version 2 Support"
> as the string crossed 80 char limit.
>
> - added "depends on EFI" so that code builds only when CONFIG_EFI is
> set to y.
>
> - added 'default n' for ease of understanding though default is set to n.
>

None of these changes are necessary, tbh. 'depends on EFI' is implied
by the placement of the option, and default n is indeed the default.


>  drivers/firmware/efi/Kconfig | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 178ee8106828..6e4c46e8a954 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
>           reboots.
>
>  config EFI_RCI2_TABLE
> -       bool "EFI Runtime Configuration Interface Table Version 2 Support"
> +       bool
> +       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST

You can drop the || COMPILE_TEST as well.
> +       depends on EFI
> +       default n
>         help
>           Displays the content of the Runtime Configuration Interface
>           Table version 2 on Dell EMC PowerEdge systems as a binary
> --
> 2.18.1
>
> --
> With regards,
> Narendra K
Narendra.K@dell.com Oct. 10, 2019, 5:47 p.m. UTC | #2
Hi Ard,

Thank you for the review comments. 

On Wed, Oct 09, 2019 at 04:11:04PM +0200, Ard Biesheuvel wrote:
> On Wed, 2 Oct 2019 at 21:44, <Narendra.K@dell.com> wrote:
> >
> > From: Narendra K <Narendra.K@dell.com>
> >
> > For the EFI_RCI2_TABLE kconfig option, 'make oldconfig' asks the user
> > for input as it is a new kconfig option in kernel version 5.4. This patch
> > modifies the kconfig option to ask the user for input only when CONFIG_X86
> > or CONFIG_COMPILE_TEST is set to y.
> >
> > The patch also makes EFI_RCI2_TABLE kconfig option depend on CONFIG_EFI.
> >
> > Signed-off-by: Narendra K <Narendra.K@dell.com>
> > ---
> > The patch is created on kernel version 5.4-rc1.
> >
> > Hi Ard, I have made following changes -
> >
> > - changed the prompt string from "EFI Runtime Configuration
> > Interface Table Version 2 Support" to "EFI RCI Table Version 2 Support"
> > as the string crossed 80 char limit.
> >
> > - added "depends on EFI" so that code builds only when CONFIG_EFI is
> > set to y.
> >
> > - added 'default n' for ease of understanding though default is set to n.
> >
> 
> None of these changes are necessary, tbh. 'depends on EFI' is implied
> by the placement of the option, and default n is indeed the default.

I will drop the changes in the next version of the patch.

> 
> 
> >  drivers/firmware/efi/Kconfig | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index 178ee8106828..6e4c46e8a954 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
> >           reboots.
> >
> >  config EFI_RCI2_TABLE
> > -       bool "EFI Runtime Configuration Interface Table Version 2 Support"
> > +       bool
> > +       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST
> 
> You can drop the || COMPILE_TEST as well.

I will drop this part of the change in the next version of the patch.
Geert Uytterhoeven Oct. 10, 2019, 6:50 p.m. UTC | #3
Hi Narendra,

On Thu, Oct 10, 2019 at 7:47 PM <Narendra.K@dell.com> wrote:
> On Wed, Oct 09, 2019 at 04:11:04PM +0200, Ard Biesheuvel wrote:
> > On Wed, 2 Oct 2019 at 21:44, <Narendra.K@dell.com> wrote:
> > >
> > > From: Narendra K <Narendra.K@dell.com>
> > >
> > > For the EFI_RCI2_TABLE kconfig option, 'make oldconfig' asks the user
> > > for input as it is a new kconfig option in kernel version 5.4. This patch
> > > modifies the kconfig option to ask the user for input only when CONFIG_X86
> > > or CONFIG_COMPILE_TEST is set to y.
> > >
> > > The patch also makes EFI_RCI2_TABLE kconfig option depend on CONFIG_EFI.
> > >
> > > Signed-off-by: Narendra K <Narendra.K@dell.com>

> > >  drivers/firmware/efi/Kconfig | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > index 178ee8106828..6e4c46e8a954 100644
> > > --- a/drivers/firmware/efi/Kconfig
> > > +++ b/drivers/firmware/efi/Kconfig
> > > @@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
> > >           reboots.
> > >
> > >  config EFI_RCI2_TABLE
> > > -       bool "EFI Runtime Configuration Interface Table Version 2 Support"
> > > +       bool
> > > +       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST

Why the split of bool and prompt?
Why not simply add a single line "depends on X86 || COMPILE_TEST"?

> >
> > You can drop the || COMPILE_TEST as well.
>
> I will drop this part of the change in the next version of the patch.

Why drop that part? Isn't it good to have more compile test coverage?

Gr{oetje,eeting}s,

                        Geert
Narendra.K@dell.com Oct. 11, 2019, 9:43 a.m. UTC | #4
Hi Geert,

On Thu, Oct 10, 2019 at 08:50:45PM +0200, Geert Uytterhoeven wrote:
[...]
> > > >  drivers/firmware/efi/Kconfig | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > > index 178ee8106828..6e4c46e8a954 100644
> > > > --- a/drivers/firmware/efi/Kconfig
> > > > +++ b/drivers/firmware/efi/Kconfig
> > > > @@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
> > > >           reboots.
> > > >
> > > >  config EFI_RCI2_TABLE
> > > > -       bool "EFI Runtime Configuration Interface Table Version 2 Support"
> > > > +       bool
> > > > +       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST
> 
> Why the split of bool and prompt?
> Why not simply add a single line "depends on X86 || COMPILE_TEST"?

It is because of the findings shared in [1]. Please let me know your
thoughts on the findings.

> 
> > >
> > > You can drop the || COMPILE_TEST as well.
> >
> > I will drop this part of the change in the next version of the patch.
> 
> Why drop that part? Isn't it good to have more compile test coverage?

It is per the suggestion in the previous review comment. 

Ard, please share your thought here. I could add the || COMPILE_TEST.

[1]  Re: [PATCH 4/5] efi: Export Runtime Configuration Interface table to sysfs
https://lore.kernel.org/linux-efi/20190812150452.27983-1-ard.biesheuvel@linaro.org/T/#mebff9ba48499808f59b33b2daef2d94e006296d8
Geert Uytterhoeven Oct. 11, 2019, 10:01 a.m. UTC | #5
Hi Narendra,

On Fri, Oct 11, 2019 at 11:43 AM <Narendra.K@dell.com> wrote:
> On Thu, Oct 10, 2019 at 08:50:45PM +0200, Geert Uytterhoeven wrote:
> [...]
> > > > >  drivers/firmware/efi/Kconfig | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > > > index 178ee8106828..6e4c46e8a954 100644
> > > > > --- a/drivers/firmware/efi/Kconfig
> > > > > +++ b/drivers/firmware/efi/Kconfig
> > > > > @@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
> > > > >           reboots.
> > > > >
> > > > >  config EFI_RCI2_TABLE
> > > > > -       bool "EFI Runtime Configuration Interface Table Version 2 Support"
> > > > > +       bool
> > > > > +       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST
> >
> > Why the split of bool and prompt?
> > Why not simply add a single line "depends on X86 || COMPILE_TEST"?
>
> It is because of the findings shared in [1]. Please let me know your
> thoughts on the findings.

So you want to prevent the user from seeing a prompt for an option he may
or may not need to enable, when running "make oldconfig"?

One common approach is to let the Kconfig symbol for the platform (not for
all of X86!) select EFI_RCI2_TABLE.
That way it will be enabled automatically when needed.

Another approach is to not force the option on, but guide the user towards
enabling it, by adding "default y if <platform_symbol>".

> > > > You can drop the || COMPILE_TEST as well.
> > >
> > > I will drop this part of the change in the next version of the patch.
> >
> > Why drop that part? Isn't it good to have more compile test coverage?
>
> It is per the suggestion in the previous review comment.
>
> Ard, please share your thought here. I could add the || COMPILE_TEST.

Without the "|| COMPILE_TEST", you cannot enable compile-testing of
the driver on non-x86 platforms with EFI.

> [1]  Re: [PATCH 4/5] efi: Export Runtime Configuration Interface table to sysfs
> https://lore.kernel.org/linux-efi/20190812150452.27983-1-ard.biesheuvel@linaro.org/T/#mebff9ba48499808f59b33b2daef2d94e006296d8

Gr{oetje,eeting}s,

                        Geert
Narendra.K@dell.com Oct. 11, 2019, 12:55 p.m. UTC | #6
On Fri, Oct 11, 2019 at 12:01:25PM +0200, Geert Uytterhoeven wrote:
> > > > > > -       bool "EFI Runtime Configuration Interface Table Version 2 Support"
> > > > > > +       bool
> > > > > > +       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST
> > >
> > > Why the split of bool and prompt?
> > > Why not simply add a single line "depends on X86 || COMPILE_TEST"?
> >
> > It is because of the findings shared in [1]. Please let me know your
> > thoughts on the findings.
> 
> So you want to prevent the user from seeing a prompt for an option he may
> or may not need to enable, when running "make oldconfig"?

Geert,

> The code in question is entirely architecture agnostic, and defaults
> to 'n', so I am not convinced this is needed. (It came up in the
> review as well)

>> "make oldconfig" still asks me the question on e.g. arm64, where it is
>> irrelevant, until arm64 variants of the hardware show up.

>> So IMHO it should have "depends on X86 || COMPILE_TEST".

From the discussion in [1] and [2](pasted a part of it above), my understanding
of the issue you reported is that 'make oldconfig' asks the user a question for arm64
though the EFI_RCI2_TABLE is not relevant for arm64. From the tests,
it seemed like adding "depends on X86 || COMPILE_TEST" does not fix the
issue, splitting bool into bool + prompt fixes it.

Please let me know if I am missing any detail in the issue you reported.  

With the way EFI_RCI2_TABLE is currently defined, my understanding is
that 'make oldconfig' does not set the EFI_RCI2_TABLE to 'y' by default
on arm64, but it asks the user the question. User has to say 'y' if he
wants it to be set to 'y', else by default 'n' is set. This behavior is
as expected. 

> 
> One common approach is to let the Kconfig symbol for the platform (not for
> all of X86!) select EFI_RCI2_TABLE.
> That way it will be enabled automatically when needed.

We did not intend to enable EFI_RCI2_TABLE option by default even on all
X86 systems from the begining. As a result, we chose to set it to 'n' by
default and added the guidance in 'help' section to say 'y' for Dell EMC
PowerEdge systems. 

> 
> Another approach is to not force the option on, but guide the user towards
> enabling it, by adding "default y if <platform_symbol>".

As mentioned above, we want to keep the default to n.

> Without the "|| COMPILE_TEST", you cannot enable compile-testing of
> the driver on non-x86 platforms with EFI.

Ok. We could keep the check. Could we make it independent of platforms
by adding 'defbool y if COMPILE_TEST' ? 

[1] Re: [PATCH 4/5] efi: Export Runtime Configuration Interface table to sysfs
https://lore.kernel.org/linux-efi/20190812150452.27983-5-ard.biesheuvel@linaro.org/T/#m0d73b4fa0dd7ece5038e4c9580dcc4e2ce5bd63c

[2] Re: [PATCH 4/5] efi: Export Runtime Configuration Interface table to sysfs
https://lore.kernel.org/linux-efi/20190812150452.27983-5-ard.biesheuvel@linaro.org/T/#mf45a9bc1861c71f110c800a53c60f0be65c68ec7
Geert Uytterhoeven Oct. 12, 2019, 5:04 p.m. UTC | #7
Hi Narendra,

On Fri, Oct 11, 2019 at 2:55 PM <Narendra.K@dell.com> wrote:
> On Fri, Oct 11, 2019 at 12:01:25PM +0200, Geert Uytterhoeven wrote:
> > > > > > > -       bool "EFI Runtime Configuration Interface Table Version 2 Support"
> > > > > > > +       bool
> > > > > > > +       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST
> > > >
> > > > Why the split of bool and prompt?
> > > > Why not simply add a single line "depends on X86 || COMPILE_TEST"?
> > >
> > > It is because of the findings shared in [1]. Please let me know your
> > > thoughts on the findings.
> >
> > So you want to prevent the user from seeing a prompt for an option he may
> > or may not need to enable, when running "make oldconfig"?
>
> Geert,
>
> > The code in question is entirely architecture agnostic, and defaults
> > to 'n', so I am not convinced this is needed. (It came up in the
> > review as well)
>
> >> "make oldconfig" still asks me the question on e.g. arm64, where it is
> >> irrelevant, until arm64 variants of the hardware show up.
>
> >> So IMHO it should have "depends on X86 || COMPILE_TEST".
>
> From the discussion in [1] and [2](pasted a part of it above), my understanding
> of the issue you reported is that 'make oldconfig' asks the user a question for arm64
> though the EFI_RCI2_TABLE is not relevant for arm64. From the tests,
> it seemed like adding "depends on X86 || COMPILE_TEST" does not fix the
> issue, splitting bool into bool + prompt fixes it.
>
> Please let me know if I am missing any detail in the issue you reported.

Adding a "depends on X86 || COMPILE_TEST" should fix the issue, as
X86 is never set on arm64, nor on any other architecture than X86.
If COMPILE_TEST=y, it's normal expected behavior to show the question.

> With the way EFI_RCI2_TABLE is currently defined, my understanding is
> that 'make oldconfig' does not set the EFI_RCI2_TABLE to 'y' by default
> on arm64, but it asks the user the question. User has to say 'y' if he
> wants it to be set to 'y', else by default 'n' is set. This behavior is
> as expected.

If the option doesn't make sense on arm64 (more broadly: on non-X86),
it should depend on X86 || COMPILE_TEST, to avoid spamming the user
with (zillions of) options that do not matter for his platform.

> > One common approach is to let the Kconfig symbol for the platform (not for
> > all of X86!) select EFI_RCI2_TABLE.
> > That way it will be enabled automatically when needed.
>
> We did not intend to enable EFI_RCI2_TABLE option by default even on all
> X86 systems from the begining. As a result, we chose to set it to 'n' by
> default and added the guidance in 'help' section to say 'y' for Dell EMC
> PowerEdge systems.

Good.

>> > Another approach is to not force the option on, but guide the user towards
> > enabling it, by adding "default y if <platform_symbol>".
>
> As mentioned above, we want to keep the default to n.

OK.

> > Without the "|| COMPILE_TEST", you cannot enable compile-testing of
> > the driver on non-x86 platforms with EFI.
>
> Ok. We could keep the check. Could we make it independent of platforms
> by adding 'defbool y if COMPILE_TEST' ?

Please don't do that, as it with always enable the driver if COMPILE_TEST=y,
without providing a way to opt-out for the user.

Thanks!

Gr{oetje,eeting}s,

                        Geert
Narendra.K@dell.com Oct. 13, 2019, 7:08 p.m. UTC | #8
Geert,

On Sat, Oct 12, 2019 at 07:04:15PM +0200, Geert Uytterhoeven wrote:
[...]
> > From the discussion in [1] and [2](pasted a part of it above), my understanding
> > of the issue you reported is that 'make oldconfig' asks the user a question for arm64
> > though the EFI_RCI2_TABLE is not relevant for arm64. From the tests,
> > it seemed like adding "depends on X86 || COMPILE_TEST" does not fix the
> > issue, splitting bool into bool + prompt fixes it.
> >
> > Please let me know if I am missing any detail in the issue you reported.
> 
> Adding a "depends on X86 || COMPILE_TEST" should fix the issue, as
> X86 is never set on arm64, nor on any other architecture than X86.
> If COMPILE_TEST=y, it's normal expected behavior to show the question.

Ok. Thank you for the details. 

> 
> > With the way EFI_RCI2_TABLE is currently defined, my understanding is
> > that 'make oldconfig' does not set the EFI_RCI2_TABLE to 'y' by default
> > on arm64, but it asks the user the question. User has to say 'y' if he
> > wants it to be set to 'y', else by default 'n' is set. This behavior is
> > as expected.
> 
> If the option doesn't make sense on arm64 (more broadly: on non-X86),
> it should depend on X86 || COMPILE_TEST, to avoid spamming the user
> with (zillions of) options that do not matter for his platform.

Ok.

> 
> > > One common approach is to let the Kconfig symbol for the platform (not for
> > > all of X86!) select EFI_RCI2_TABLE.
> > > That way it will be enabled automatically when needed.
> >
> > We did not intend to enable EFI_RCI2_TABLE option by default even on all
> > X86 systems from the begining. As a result, we chose to set it to 'n' by
> > default and added the guidance in 'help' section to say 'y' for Dell EMC
> > PowerEdge systems.
> 
> Good.
> 
> >> > Another approach is to not force the option on, but guide the user towards
> > > enabling it, by adding "default y if <platform_symbol>".
> >
> > As mentioned above, we want to keep the default to n.
> 
> OK.
> 
> > > Without the "|| COMPILE_TEST", you cannot enable compile-testing of
> > > the driver on non-x86 platforms with EFI.
> >
> > Ok. We could keep the check. Could we make it independent of platforms
> > by adding 'defbool y if COMPILE_TEST' ?
> 
> Please don't do that, as it with always enable the driver if COMPILE_TEST=y,
> without providing a way to opt-out for the user.

Ok.

Thank you for the inputs. 

I have submitted the patch with 'depends on X86 || COMPILE_TEST' added to 
the Kconfig file. Could you please help test it on your platform/system to 
verify if the patch works as expected ?

Patch
diff mbox series

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 178ee8106828..6e4c46e8a954 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -181,7 +181,10 @@  config RESET_ATTACK_MITIGATION
 	  reboots.
 
 config EFI_RCI2_TABLE
-	bool "EFI Runtime Configuration Interface Table Version 2 Support"
+	bool
+	prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST
+	depends on EFI
+	default n
 	help
 	  Displays the content of the Runtime Configuration Interface
 	  Table version 2 on Dell EMC PowerEdge systems as a binary