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

PATCH In review [email protected] 3 participants 9 messages Oct 2, 2019 – Oct 13, 2019 EFI

lore

Share & tools

lore

[email protected]

author

2019-10-02 19:44 UTC #0 lore raw

PATCH 1/1 Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y

From: Narendra K <[email protected]>

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 <[email protected]>

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

drivers/firmware/efi/Kconfig +4 -1

index 178ee8106828..6e4c46e8a954 100644
@@ -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

Signature · 5 lines

2.18.1

-- 
With regards,
Narendra K

Ard Biesheuvel

2019-10-09 14:11 UTC #1 lore raw

Hello Narendra,

Quoted 16 lines · On Wed, 2 Oct 2019 at 21:44, <[email protected]> wrote:

From: Narendra K <[email protected]>
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 <[email protected]>
---
The patch is created on kernel version 5.4-rc1.
Hi Ard, I have made following changes -

Quoted 9 lines

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

Quoted 8 lines

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

drivers/firmware/efi/Kconfig +0 -0

index 178ee8106828..6e4c46e8a954 100644

drivers/firmware/efi/Kconfig config RESET_ATTACK_MITIGATION · line 185

Quoted 6 lines

drivers/firmware/efi/Kconfig +2 -1

@@ -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.

Quoted 11 lines

drivers/firmware/efi/Kconfig +2 -1

+       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

[email protected]

author

2019-10-10 17:47 UTC #2 lore raw

Hi Ard,

Thank you for the review comments.

Quoted 21 lines · Ard Biesheuvel

Quoted 20 lines · On Wed, 2 Oct 2019 at 21:44, <[email protected]> wrote:
From: Narendra K <[email protected]>
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 <[email protected]>
---
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.

Quoted 8 lines

Quoted 5 lines
- 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.

Quoted 12 lines


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

drivers/firmware/efi/Kconfig +0 -0

index 178ee8106828..6e4c46e8a954 100644
@@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
          reboots.

Quoted 6 lines

drivers/firmware/efi/Kconfig config RESET_ATTACK_MITIGATION · line 185
Quoted 4 lines
drivers/firmware/efi/Kconfig +2 -1
 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.

Signature · 2 lines

With regards,
Narendra K

Geert Uytterhoeven

2019-10-10 18:50 UTC #3 lore raw

Hi Narendra,

Quoted 24 lines · On Thu, Oct 10, 2019 at 7:47 PM <[email protected]> wrote:

Quoted 23 lines · Ard Biesheuvel
Quoted 22 lines · On Wed, 2 Oct 2019 at 21:44, <[email protected]> wrote:
From: Narendra K <[email protected]>
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 <[email protected]>
 drivers/firmware/efi/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/firmware/efi/Kconfig +0 -0
index 178ee8106828..6e4c46e8a954 100644
@@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
          reboots.
 

Quoted 4 lines

Quoted 4 lines
drivers/firmware/efi/Kconfig config RESET_ATTACK_MITIGATION · line 185
Quoted 4 lines
drivers/firmware/efi/Kconfig +2 -1
 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"?

Quoted 4 lines

Quoted 2 lines
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

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

[email protected]

author

2019-10-11 09:43 UTC #4 lore raw

Hi Geert,

On Thu, Oct 10, 2019 at 08:50:45PM +0200, Geert Uytterhoeven wrote:
[...]

Quoted 10 lines

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

drivers/firmware/efi/Kconfig +0 -0

index 178ee8106828..6e4c46e8a954 100644
@@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
          reboots.

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
drivers/firmware/efi/Kconfig config RESET_ATTACK_MITIGATION · line 185
Quoted 4 lines
drivers/firmware/efi/Kconfig +2 -1
 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.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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/[email protected]/T/#mebff9ba48499808f59b33b2daef2d94e006296d8

Signature · 2 lines

With regards,
Narendra K

Geert Uytterhoeven

2019-10-11 10:01 UTC #5 lore raw

Hi Narendra,

Quoted 20 lines · On Fri, Oct 11, 2019 at 11:43 AM <[email protected]> wrote:

On Thu, Oct 10, 2019 at 08:50:45PM +0200, Geert Uytterhoeven wrote:
[...]
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
 drivers/firmware/efi/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/firmware/efi/Kconfig +0 -0
index 178ee8106828..6e4c46e8a954 100644
@@ -181,7 +181,10 @@ config RESET_ATTACK_MITIGATION
          reboots.
 
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
drivers/firmware/efi/Kconfig config RESET_ATTACK_MITIGATION · line 185
Quoted 4 lines
drivers/firmware/efi/Kconfig +2 -1
 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"?

Quoted 2 lines

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>".

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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.

Quoted 2 lines

[1] Re: [PATCH 4/5] efi: Export Runtime Configuration Interface table to sysfs
https://lore.kernel.org/linux-efi/[email protected]/T/#mebff9ba48499808f59b33b2daef2d94e006296d8

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

[email protected]

author

2019-10-11 12:55 UTC #6 lore raw

Quoted 4 lines · Geert Uytterhoeven

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/firmware/efi/Kconfig +2 -1
-       bool "EFI Runtime Configuration Interface Table Version 2 Support"
+       bool
+       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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,

Quoted 8 lines

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)
Quoted 4 lines
"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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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/[email protected]/T/#m0d73b4fa0dd7ece5038e4c9580dcc4e2ce5bd63c

[2] Re: [PATCH 4/5] efi: Export Runtime Configuration Interface table to sysfs
https://lore.kernel.org/linux-efi/[email protected]/T/#mf45a9bc1861c71f110c800a53c60f0be65c68ec7

Signature · 2 lines

With regards,
Narendra K

Geert Uytterhoeven

2019-10-12 17:06 UTC #7 lore raw

Hi Narendra,

Quoted 25 lines · On Fri, Oct 11, 2019 at 2:55 PM <[email protected]> wrote:

Quoted 4 lines · Geert Uytterhoeven
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(quoted diff) +2 -1
-       bool "EFI Runtime Configuration Interface Table Version 2 Support"
+       bool
+       prompt "EFI RCI Table Version 2 Support" if X86 || COMPILE_TEST
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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,
Quoted 8 lines
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)
Quoted 4 lines
"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".

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 4 lines

Quoted 2 lines
Quoted 1 line
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.

Quoted 5 lines

Quoted 2 lines
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

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

[email protected]

author

2019-10-13 19:08 UTC #8 lore raw

Geert,

On Sat, Oct 12, 2019 at 07:04:15PM +0200, Geert Uytterhoeven wrote:
[...]

Quoted 11 lines

Quoted 7 lines
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.

Quoted 10 lines

Quoted 6 lines
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.

Quoted 19 lines

Quoted 9 lines
Quoted 4 lines
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.
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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 ?

Signature · 2 lines

With regards,
Narendra K