1

kconfig: remove tristate choice support

I previously submitted a fix for a bug in the choice feature [1], where
I mentioned, "Another (much cleaner) approach would be to remove the
tristate choice support entirely".

There are more issues in the tristate choice feature. For example, you
can observe a couple of bugs in the following test code.

[Test Code]

    config MODULES
            def_bool y
            modules

    choice
            prompt "tristate choice"
            default A

    config A
            tristate "A"

    config B
            tristate "B"

    endchoice

Bug 1: the 'default' property is not correctly processed

'make alldefconfig' produces:

    CONFIG_MODULES=y
    # CONFIG_A is not set
    # CONFIG_B is not set

However, the correct output should be:

    CONFIG_MODULES=y
    CONFIG_A=y
    # CONFIG_B is not set

The unit test file, scripts/kconfig/tests/choice/alldef_expected_config,
is wrong as well.

Bug 2: choice members never get 'y' with randconfig

For the test code above, the following combinations are possible:

               A    B
        (1)    y    n
        (2)    n    y
        (3)    m    m
        (4)    m    n
        (5)    n    m
        (6)    n    n

'make randconfig' never produces (1) or (2).

These bugs are fixable, but a more critical problem is the lack of a
sensible syntax to specify the default for the tristate choice.
The default for the choice must be one of the choice members, which
cannot specify any of the patterns (3) through (6) above.

In addition, I have never seen it being used in a useful way.

The following commits removed unnecessary use of tristate choices:

 - df8df5e4bc ("usb: get rid of 'choice' for legacy gadget drivers")
 - bfb57ef054 ("rapidio: remove choice for enumeration")

This commit removes the tristate choice support entirely, which allows
me to delete a lot of code, making further refactoring easier.

Note:
This includes the revert of commit fa64e5f6a3 ("kconfig/symbol.c:
handle choice_values that depend on 'm' symbols"). It was suspicious
because it did not address the root cause but introduced inconsistency
in visibility between choice members and other symbols.

[1]: https://lore.kernel.org/linux-kbuild/20240427104231.2728905-1-masahiroy@kernel.org/T/#m0a1bb6992581462ceca861b409bb33cb8fd7dbae

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>
This commit is contained in:
Masahiro Yamada 2024-06-02 21:54:14 +09:00
parent 03638aaa79
commit fde192511b
28 changed files with 44 additions and 324 deletions

View File

@ -409,16 +409,9 @@ choices::
"endchoice"
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.
options.
While a boolean choice only allows a single config entry to be
selected, a tristate choice also allows any number of config entries
to be set to 'm'. This can be used if multiple drivers for a single
hardware exists and only a single driver can be compiled/loaded into
the kernel, but all drivers can be compiled as modules.
A choice only allows a single config entry to be selected.
comment::

View File

@ -114,21 +114,13 @@ static void set_randconfig_seed(void)
srand(seed);
}
static bool randomize_choice_values(struct symbol *csym)
static void randomize_choice_values(struct symbol *csym)
{
struct property *prop;
struct symbol *sym;
struct expr *e;
int cnt, def;
/*
* If choice is mod then we may have more items selected
* and if no then no-one.
* In both cases stop.
*/
if (csym->curr.tri != yes)
return false;
prop = sym_get_choice_prop(csym);
/* count entries in choice block */
@ -157,8 +149,6 @@ static bool randomize_choice_values(struct symbol *csym)
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
csym->flags &= ~SYMBOL_VALID;
return true;
}
enum conf_def_mode {
@ -269,15 +259,6 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode)
sym_clear_all_valid();
/*
* We have different type of choice blocks.
* If curr.tri equals to mod then we can select several
* choice symbols in one block.
* In this case we do nothing.
* If curr.tri equals yes then only one symbol can be
* selected in a choice block and we set it to yes,
* and the rest to no.
*/
if (mode != def_random) {
for_all_symbols(csym) {
if ((sym_is_choice(csym) && !sym_has_value(csym)) ||
@ -292,11 +273,10 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode)
sym_calc_value(csym);
if (mode == def_random)
has_changed |= randomize_choice_values(csym);
else {
randomize_choice_values(csym);
else
set_all_choice_values(csym);
has_changed = true;
}
has_changed = true;
}
return has_changed;
@ -454,27 +434,6 @@ static void conf_choice(struct menu *menu)
sym = menu->sym;
is_new = !sym_has_value(sym);
if (sym_is_changeable(sym)) {
conf_sym(menu);
sym_calc_value(sym);
switch (sym_get_tristate_value(sym)) {
case no:
case mod:
return;
case yes:
break;
}
} else {
switch (sym_get_tristate_value(sym)) {
case no:
return;
case mod:
printf("%*s%s\n", indent - 1, "", menu_get_prompt(menu));
return;
case yes:
break;
}
}
while (1) {
int cnt, def;
@ -596,9 +555,7 @@ static void conf(struct menu *menu)
if (sym_is_choice(sym)) {
conf_choice(menu);
if (sym->curr.tri != mod)
return;
goto conf_childs;
return;
}
switch (sym->type) {
@ -631,8 +588,7 @@ static void check_conf(struct menu *menu)
sym = menu->sym;
if (sym && !sym_has_value(sym) &&
(sym_is_changeable(sym) ||
(sym_is_choice(sym) && sym_get_tristate_value(sym) == yes))) {
(sym_is_changeable(sym) || sym_is_choice(sym))) {
switch (input_mode) {
case listnewconfig:

View File

@ -462,22 +462,12 @@ load:
if (sym && sym_is_choice_value(sym)) {
struct symbol *cs = prop_get_symbol(sym_get_choice_prop(sym));
switch (sym->def[def].tri) {
case no:
break;
case mod:
if (cs->def[def].tri == yes) {
conf_warning("%s creates inconsistent choice state", sym->name);
cs->flags &= ~def_flags;
}
break;
case yes:
if (sym->def[def].tri == yes) {
if (cs->def[def].tri != no)
conf_warning("override: %s changes choice state", sym->name);
cs->def[def].val = sym;
break;
cs->def[def].tri = yes;
}
cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
}
}
free(line);
@ -806,8 +796,7 @@ int conf_write_defconfig(const char *filename)
ds = sym_choice_default(choice->sym);
if (sym == ds) {
if ((sym->type == S_BOOLEAN) &&
sym_get_tristate_value(sym) == yes)
if (sym_get_tristate_value(sym) == yes)
continue;
}
}

View File

@ -1067,10 +1067,8 @@ static gchar **fill_row(struct menu *menu)
row[COL_VALUE] =
g_strdup(menu_get_prompt(def_menu));
if (sym_get_type(sym) == S_BOOLEAN) {
row[COL_BTNVIS] = GINT_TO_POINTER(FALSE);
return row;
}
row[COL_BTNVIS] = GINT_TO_POINTER(FALSE);
return row;
}
if (sym->flags & SYMBOL_CHOICEVAL)
row[COL_BTNRAD] = GINT_TO_POINTER(TRUE);

View File

@ -523,28 +523,14 @@ static void build_conf(struct menu *menu)
def_menu = child;
}
val = sym_get_tristate_value(sym);
if (sym_is_changeable(sym)) {
switch (val) {
case yes: ch = '*'; break;
case mod: ch = 'M'; break;
default: ch = ' '; break;
}
item_make("<%c>", ch);
item_set_tag('t');
item_set_data(menu);
} else {
item_make(" ");
item_set_tag(def_menu ? 't' : ':');
item_set_data(menu);
}
item_make(" ");
item_set_tag(def_menu ? 't' : ':');
item_set_data(menu);
item_add_str("%*c%s", indent + 1, ' ', menu_get_prompt(menu));
if (val == yes) {
if (def_menu)
item_add_str(" (%s) --->", menu_get_prompt(def_menu));
return;
}
if (def_menu)
item_add_str(" (%s) --->", menu_get_prompt(def_menu));
return;
} else {
if (menu == current_menu) {
item_make("---%*c%s", indent + 1, ' ', menu_get_prompt(menu));
@ -814,7 +800,7 @@ static void conf(struct menu *menu, struct menu *active_menu)
conf(submenu, NULL);
break;
case 't':
if (sym_is_choice(sym) && sym_get_tristate_value(sym) == yes)
if (sym_is_choice(sym))
conf_choice(submenu);
else if (submenu->prompt->type == P_MENU)
conf(submenu, NULL);

View File

@ -321,17 +321,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
@ -503,22 +492,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
menu->sym && !sym_is_choice_value(menu->sym)) {
current_entry = menu;
menu->sym->flags |= SYMBOL_CHOICEVAL;
/* Non-tristate choice values of tristate choices must
* depend on the choice being set to Y. The choice
* values' dependencies were propagated to their
* properties above, so the change here must be re-
* propagated.
*/
if (sym->type == S_TRISTATE && menu->sym->type != S_TRISTATE) {
basedep = expr_alloc_comp(E_EQUAL, sym, &symbol_yes);
menu->dep = expr_alloc_and(basedep, menu->dep);
for (prop = menu->sym->prop; prop; prop = prop->next) {
if (prop->menu != menu)
continue;
prop->visible.expr = expr_alloc_and(expr_copy(basedep),
prop->visible.expr);
}
}
menu_add_symbol(P_CHOICE, sym, NULL);
prop = sym_get_choice_prop(sym);
for (ep = &prop->expr; *ep; ep = &(*ep)->left.expr)
@ -578,13 +551,13 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
/*
* For choices, add a reverse dependency (corresponding to a select) of
* '<visibility> && m'. This prevents the user from setting the choice
* '<visibility> && y'. This prevents the user from setting the choice
* mode to 'n' when the choice is visible.
*/
if (sym && sym_is_choice(sym) && parent->prompt) {
sym->rev_dep.expr = expr_alloc_or(sym->rev_dep.expr,
expr_alloc_and(parent->prompt->visible.expr,
expr_alloc_symbol(&symbol_mod)));
expr_alloc_symbol(&symbol_yes)));
}
}

View File

@ -825,30 +825,13 @@ static void build_conf(struct menu *menu)
}
val = sym_get_tristate_value(sym);
if (sym_is_changeable(sym)) {
switch (val) {
case yes:
ch = '*';
break;
case mod:
ch = 'M';
break;
default:
ch = ' ';
break;
}
item_make(menu, 't', "<%c>", ch);
} else {
item_make(menu, def_menu ? 't' : ':', " ");
}
item_make(menu, def_menu ? 't' : ':', " ");
item_add_str("%*c%s", indent + 1,
' ', menu_get_prompt(menu));
if (val == yes) {
if (def_menu)
item_add_str(" (%s) --->", menu_get_prompt(def_menu));
return;
}
if (def_menu)
item_add_str(" (%s) --->", menu_get_prompt(def_menu));
return;
} else {
if (menu == current_menu) {
item_make(menu, ':',
@ -1191,8 +1174,7 @@ static void selected_conf(struct menu *menu, struct menu *active_menu)
conf(submenu);
break;
case 't':
if (sym_is_choice(sym) &&
sym_get_tristate_value(sym) == yes)
if (sym_is_choice(sym))
conf_choice(submenu);
else if (submenu->prompt &&
submenu->prompt->type == P_MENU)

View File

@ -90,7 +90,7 @@ static bool inside_choice = false;
%type <symbol> nonconst_symbol
%type <symbol> symbol
%type <type> type logic_type default
%type <type> type default
%type <expr> expr
%type <expr> if_expr
%type <string> end
@ -153,6 +153,12 @@ config_stmt: config_entry_start config_option_list
current_entry->filename, current_entry->lineno);
yynerrs++;
}
if (current_entry->sym->type != S_BOOLEAN) {
fprintf(stderr, "%s:%d: error: choice member must be bool\n",
current_entry->filename, current_entry->lineno);
yynerrs++;
}
}
printd(DEBUG_PARSE, "%s:%d:endconfig\n", cur_filename, cur_lineno);
@ -235,6 +241,8 @@ choice: T_CHOICE T_EOL
menu_add_entry(sym);
menu_add_expr(P_CHOICE, NULL, NULL);
menu_set_type(S_BOOLEAN);
printd(DEBUG_PARSE, "%s:%d:choice\n", cur_filename, cur_lineno);
};
@ -277,10 +285,10 @@ choice_option: T_PROMPT T_WORD_QUOTE if_expr T_EOL
printd(DEBUG_PARSE, "%s:%d:prompt\n", cur_filename, cur_lineno);
};
choice_option: logic_type prompt_stmt_opt T_EOL
choice_option: T_BOOL T_WORD_QUOTE if_expr T_EOL
{
menu_set_type($1);
printd(DEBUG_PARSE, "%s:%d:type(%u)\n", cur_filename, cur_lineno, $1);
menu_add_prompt(P_PROMPT, $2, $3);
printd(DEBUG_PARSE, "%s:%d:bool\n", cur_filename, cur_lineno);
};
choice_option: T_DEFAULT nonconst_symbol if_expr T_EOL
@ -290,15 +298,12 @@ choice_option: T_DEFAULT nonconst_symbol if_expr T_EOL
};
type:
logic_type
T_BOOL { $$ = S_BOOLEAN; }
| T_TRISTATE { $$ = S_TRISTATE; }
| T_INT { $$ = S_INT; }
| T_HEX { $$ = S_HEX; }
| T_STRING { $$ = S_STRING; }
logic_type:
T_BOOL { $$ = S_BOOLEAN; }
| T_TRISTATE { $$ = S_TRISTATE; }
default:
T_DEFAULT { $$ = S_UNKNOWN; }
| T_DEF_BOOL { $$ = S_BOOLEAN; }

View File

@ -147,7 +147,7 @@ void ConfigItem::updateMenu(void)
expr = sym_get_tristate_value(sym);
switch (expr) {
case yes:
if (sym_is_choice_value(sym) && type == S_BOOLEAN)
if (sym_is_choice_value(sym))
setIcon(promptColIdx, choiceYesIcon);
else
setIcon(promptColIdx, symbolYesIcon);

View File

@ -44,12 +44,8 @@ enum symbol_type sym_get_type(struct symbol *sym)
{
enum symbol_type type = sym->type;
if (type == S_TRISTATE) {
if (sym_is_choice_value(sym) && sym->visible == yes)
type = S_BOOLEAN;
else if (modules_val == no)
type = S_BOOLEAN;
}
if (type == S_TRISTATE && modules_val == no)
type = S_BOOLEAN;
return type;
}
@ -208,26 +204,12 @@ static void sym_set_all_changed(void)
static void sym_calc_visibility(struct symbol *sym)
{
struct property *prop;
struct symbol *choice_sym = NULL;
tristate tri;
/* any prompt visible? */
tri = no;
if (sym_is_choice_value(sym))
choice_sym = prop_get_symbol(sym_get_choice_prop(sym));
for_all_prompts(sym, prop) {
prop->visible.tri = expr_calc_value(prop->visible.expr);
/*
* Tristate choice_values with visibility 'mod' are
* not visible if the corresponding choice's value is
* 'yes'.
*/
if (choice_sym && sym->type == S_TRISTATE &&
prop->visible.tri == mod && choice_sym->curr.tri == yes)
prop->visible.tri = no;
tri = EXPR_OR(tri, prop->visible.tri);
}
if (tri == mod && (sym->type != S_TRISTATE || modules_val == no))

View File

@ -1,10 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
config MODULES
bool "Enable loadable module support"
modules
default y
choice
prompt "boolean choice"
default BOOL_CHOICE1
@ -16,15 +11,3 @@ config BOOL_CHOICE1
bool "choice 1"
endchoice
choice
prompt "tristate choice"
default TRI_CHOICE1
config TRI_CHOICE0
tristate "choice 0"
config TRI_CHOICE1
tristate "choice 1"
endchoice

View File

@ -1,11 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
"""
Basic choice tests.
The handling of 'choice' is a bit complicated part in Kconfig.
The behavior of 'y' choice is intuitive. If choice values are tristate,
the choice can be 'm' where each value can be enabled independently.
"""
@ -14,11 +9,6 @@ def test_oldask0(conf):
assert conf.stdout_contains('oldask0_expected_stdout')
def test_oldask1(conf):
assert conf.oldaskconfig('oldask1_config') == 0
assert conf.stdout_contains('oldask1_expected_stdout')
def test_allyes(conf):
assert conf.allyesconfig() == 0
assert conf.config_contains('allyes_expected_config')

View File

@ -1,5 +1,2 @@
CONFIG_MODULES=y
# CONFIG_BOOL_CHOICE0 is not set
CONFIG_BOOL_CHOICE1=y
# CONFIG_TRI_CHOICE0 is not set
# CONFIG_TRI_CHOICE1 is not set

View File

@ -1,5 +1,2 @@
CONFIG_MODULES=y
# CONFIG_BOOL_CHOICE0 is not set
CONFIG_BOOL_CHOICE1=y
CONFIG_TRI_CHOICE0=m
CONFIG_TRI_CHOICE1=m

View File

@ -1,5 +1,2 @@
# CONFIG_MODULES is not set
# CONFIG_BOOL_CHOICE0 is not set
CONFIG_BOOL_CHOICE1=y
# CONFIG_TRI_CHOICE0 is not set
CONFIG_TRI_CHOICE1=y

View File

@ -1,5 +1,2 @@
CONFIG_MODULES=y
# CONFIG_BOOL_CHOICE0 is not set
CONFIG_BOOL_CHOICE1=y
# CONFIG_TRI_CHOICE0 is not set
CONFIG_TRI_CHOICE1=y

View File

@ -1,8 +1,4 @@
Enable loadable module support (MODULES) [Y/n/?] (NEW)
boolean choice
1. choice 0 (BOOL_CHOICE0) (NEW)
> 2. choice 1 (BOOL_CHOICE1) (NEW)
choice[1-2?]:
tristate choice [M/y/?] (NEW)
choice 0 (TRI_CHOICE0) [N/m/?] (NEW)
choice 1 (TRI_CHOICE1) [N/m/?] (NEW)

View File

@ -1 +0,0 @@
# CONFIG_MODULES is not set

View File

@ -1,9 +0,0 @@
Enable loadable module support (MODULES) [N/y/?]
boolean choice
1. choice 0 (BOOL_CHOICE0) (NEW)
> 2. choice 1 (BOOL_CHOICE1) (NEW)
choice[1-2?]:
tristate choice
1. choice 0 (TRI_CHOICE0) (NEW)
> 2. choice 1 (TRI_CHOICE1) (NEW)
choice[1-2?]:

View File

@ -1,21 +0,0 @@
# SPDX-License-Identifier: GPL-2.0
config MODULES
def_bool y
modules
config DEP
tristate
default m
choice
prompt "Tristate Choice"
config CHOICE0
tristate "Choice 0"
config CHOICE1
tristate "Choice 1"
depends on DEP
endchoice

View File

@ -1,16 +0,0 @@
# SPDX-License-Identifier: GPL-2.0
"""
Hide tristate choice values with mod dependency in y choice.
If tristate choice values depend on symbols set to 'm', they should be
hidden when the choice containing them is changed from 'm' to 'y'
(i.e. exclusive choice).
Related Linux commit: fa64e5f6a35efd5e77d639125d973077ca506074
"""
def test(conf):
assert conf.oldaskconfig('config', 'y') == 0
assert conf.config_contains('expected_config')
assert conf.stdout_contains('expected_stdout')

View File

@ -1,2 +0,0 @@
CONFIG_CHOICE0=m
CONFIG_CHOICE1=m

View File

@ -1,3 +0,0 @@
CONFIG_MODULES=y
CONFIG_DEP=m
CONFIG_CHOICE0=y

View File

@ -1,4 +0,0 @@
Tristate Choice [M/y/?] y
Tristate Choice
> 1. Choice 0 (CHOICE0)
choice[1]: 1

View File

@ -1,25 +0,0 @@
# SPDX-License-Identifier: GPL-2.0
config MODULES
def_bool y
modules
choice
prompt "Choice"
config CHOICE_VAL0
tristate "Choice 0"
config CHOIVE_VAL1
tristate "Choice 1"
endchoice
choice
prompt "Another choice"
depends on CHOICE_VAL0
config DUMMY
bool "dummy"
endchoice

View File

@ -1,15 +0,0 @@
# SPDX-License-Identifier: GPL-2.0
"""
Do not affect user-assigned choice value by another choice.
Handling of state flags for choices is complecated. In old days,
the defconfig result of a choice could be affected by another choice
if those choices interact by 'depends on', 'select', etc.
Related Linux commit: fbe98bb9ed3dae23e320c6b113e35f129538d14a
"""
def test(conf):
assert conf.defconfig('defconfig') == 0
assert conf.config_contains('expected_config')

View File

@ -1 +0,0 @@
CONFIG_CHOICE_VAL0=y

View File

@ -1,4 +0,0 @@
CONFIG_MODULES=y
CONFIG_CHOICE_VAL0=y
# CONFIG_CHOIVE_VAL1 is not set
CONFIG_DUMMY=y