There are two types supported for the choice statement,
"bool" and "tristate".
There is no ambiguity if you explicitly specifiy the type of the
choice.
For example
choice
bool "choose"
...
endchoice
Or,
choice
tristate "choose"
...
endchoice
Those are valid code, and clear about the behavior.
However, if you see the reality in the kernel code, most of people
omit the type definition.
Usually, the choice is written like this:
choice
prompt "choose"
...
endchoice
The "prompt" does not specify the type at all.
You may wonder how Kconfig knows the choice type then.
When the choice type is not specified, Kconfig infers it from the first
entry within the choice block.
In the following, the choice type is bool because the first entry,
CONFIG_A, is bool.
choice
prompt "choose"
config A
bool "A"
config B
bool "B"
endchoice
As described in 2/2, this has a bug when "if" ... "endif" exists
within a "choice" ... "endchoice".
Of course, I can fix this bug, but the value of this feature is
questionable.
This patch set stop the type implification. Instead, make the
default type of the choice "bool".
This is reasonable because 99% of choice blocks are bool.
The only user of tristate choice is drivers/rapidio/Kconfig.
(although that choice is unneeded because it cotains a single
entry, RAPIDIO_ENUM_BASIC)
It changed it to specify "tristate" explicitly.
Masahiro Yamada (2):
rapidio: specify the type for tristate choice explicitly
kconfig: do not imply the type of choice from the first entry
Documentation/kbuild/kconfig-language.rst | 4 +---
drivers/rapidio/Kconfig | 2 +-
scripts/kconfig/menu.c | 11 -----------
scripts/kconfig/parser.y | 3 +++
scripts/kconfig/tests/choice/Kconfig | 2 +-
scripts/kconfig/tests/choice_value_with_m_dep/Kconfig | 2 +-
6 files changed, 7 insertions(+), 17 deletions(-)
--
2.40.1
If the type of choice is not specified, it is implied by the first
entry within the choice block.
In this case, the first (and only) entry, RAPIDIO_ENUM_BASIC, is
tristate, hence this choice behaves as tristate.
Kconfig will stop this implication because it has a bug, and 99% of
choice use cases are bool. In fact, this is the only instance of
tristate choice in the kernel.
Before transitioning the default choice type to 'bool', specify the
type explicitly for this case.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/rapidio/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rapidio/Kconfig b/drivers/rapidio/Kconfig
index b9f8514909bf..72b5b02492a1 100644
--- a/drivers/rapidio/Kconfig
+++ b/drivers/rapidio/Kconfig
@@ -60,7 +60,7 @@ config RAPIDIO_DEBUG
If you are unsure about this, say N here.
choice
- prompt "Enumeration method"
+ tristate "Enumeration method"
depends on RAPIDIO
default RAPIDIO_ENUM_BASIC
help
--
2.40.1
The followng two test cases are very similar, but the latter does not
work.
[test case 1]
choice
prompt "choose"
config A
bool "A"
if y
config B
bool "B"
endif
endchoice
[test case 2]
choice
prompt "choose"
if y
config A
bool "A"
config B
bool "B"
endif
endchoice
Since 'if y' is always true, both of them should be equivalent to:
choice
prompt "choose"
config A
bool "A"
config B
bool "B"
endchoice
However, the test case 2 warns:
Kconfig:1:warning: config symbol defined without type
If the type of choice is not specified, it is implied from the first
entry within the choice block.
When inferring the choice type, menu_finalize() checks only direct
children of the choice. At this point, the menu entries still exist
under the 'if' entry:
choice
\-- if y
|-- A
\-- B
Later, menu_finalize() re-parents the menu, so A and B will be lifted up
right under the choice:
choice
|-- if y
|-- A
\-- B
This is complex because menu_finalize() sets attributes, restructures
the menu tree, and checks the sanity at the same time, leading to some
bugs.
It would be possible to resolve it by setting the choice type after
re-parenting, but the current mechanism looks questionable to me.
Let's default all choices to 'bool' unless the type is specified.
This change makes sense because 99% of choice use cases are bool.
There exists only one 'tristate' choice in drivers/rapidio/Kconfig.
Another (much cleaner) approach would be to remove the tristate choice
support entirely, but I have not yet made up my mind.
Signed-off-by: Masahiro Yamada <[email protected]>
---
Documentation/kbuild/kconfig-language.rst | 4 +---
scripts/kconfig/menu.c | 11 -----------
scripts/kconfig/parser.y | 3 +++
scripts/kconfig/tests/choice/Kconfig | 2 +-
scripts/kconfig/tests/choice_value_with_m_dep/Kconfig | 2 +-
5 files changed, 6 insertions(+), 16 deletions(-)
diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index 555c2f839969..42b975b8e0cf 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -400,9 +400,7 @@ choices::
This defines a choice group and accepts any of the above attributes as
options. A choice can only be of type bool or tristate. If no type is
-specified for a choice, its type will be determined by the type of
-the first choice element in the group or remain unknown if none of the
-choice elements have a type specified, as well.
+specified for a choice, its type will be the default 'bool'.
While a boolean choice only allows a single config entry to be
selected, a tristate choice also allows any number of config entries
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index e01b9ee87c05..134ef120ad08 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -323,17 +323,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
is_choice = true;
if (is_choice) {
- if (sym->type == S_UNKNOWN) {
- /* find the first choice value to find out choice type */
- current_entry = parent;
- for (menu = parent->list; menu; menu = menu->next) {
- if (menu->sym && menu->sym->type != S_UNKNOWN) {
- menu_set_type(menu->sym->type);
- break;
- }
- }
- }
-
/*
* Use the choice itself as the parent dependency of
* the contained items. This turns the mode of the
diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y
index 613fa8c9c2d0..70ea3152d9b8 100644
--- a/scripts/kconfig/parser.y
+++ b/scripts/kconfig/parser.y
@@ -230,6 +230,9 @@ choice: T_CHOICE T_EOL
choice_entry: choice choice_option_list
{
+ if (current_entry->sym->type == S_UNKNOWN)
+ menu_set_type(S_BOOLEAN);
+
if (!current_entry->prompt) {
fprintf(stderr, "%s:%d: error: choice must have a prompt\n",
current_entry->filename, current_entry->lineno);
diff --git a/scripts/kconfig/tests/choice/Kconfig b/scripts/kconfig/tests/choice/Kconfig
index 8cdda40868a1..4dc0d3a1e089 100644
--- a/scripts/kconfig/tests/choice/Kconfig
+++ b/scripts/kconfig/tests/choice/Kconfig
@@ -18,7 +18,7 @@ config BOOL_CHOICE1
endchoice
choice
- prompt "tristate choice"
+ tristate "tristate choice"
default TRI_CHOICE1
config TRI_CHOICE0
diff --git a/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig b/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig
index bd970cec07d6..3e600c83279a 100644
--- a/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig
+++ b/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig
@@ -9,7 +9,7 @@ config DEP
default m
choice
- prompt "Tristate Choice"
+ tristate "Tristate Choice"
config CHOICE0
tristate "Choice 0"
--
2.40.1
On Sat, Apr 27, 2024 at 07:42:30PM +0900, Masahiro Yamada wrote:
> If the type of choice is not specified, it is implied by the first
> entry within the choice block.
>
> In this case, the first (and only) entry, RAPIDIO_ENUM_BASIC, is
> tristate, hence this choice behaves as tristate.
>
> Kconfig will stop this implication because it has a bug, and 99% of
> choice use cases are bool. In fact, this is the only instance of
> tristate choice in the kernel.
>
> Before transitioning the default choice type to 'bool', specify the
> type explicitly for this case.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
Reviewed-by: Nicolas Schier <[email protected]>