[PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Narendra.K]

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(-)

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
-- 
2.18.1

-- 
With regards,
Narendra K

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Ard Biesheuvel]

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

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Narendra.K]

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. 

-- 
With regards,
Narendra K

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Narendra.K]

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

-- 
With regards,
Narendra K

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Narendra.K]

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
-- 
With regards,
Narendra K

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

[Narendra.K]

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 ? 

-- 
With regards,
Narendra K