[PATCH 0/5] kconfig: fix conditional prompt for choice and recursive error messages

[PATCH 0/5] kconfig: fix conditional prompt for choice and recursive error messages

[Masahiro Yamada]

Masahiro Yamada (5):
  treewide: change conditional prompt for choices to 'depends on'
  kconfig: fix conditional prompt behavior for choice
  kconfig: improve error message for dependency between choice members
  kconfig: improve error message for recursive dependency in choice
  kconfig: refactor error messages in sym_check_print_recursive()

 arch/arm/Kconfig         |  6 ++++--
 arch/arm64/Kconfig       |  3 ++-
 arch/mips/Kconfig        |  6 ++++--
 arch/powerpc/Kconfig     |  3 ++-
 arch/riscv/Kconfig       |  3 ++-
 fs/jffs2/Kconfig         |  3 ++-
 scripts/kconfig/menu.c   | 38 +++-----------------------------------
 scripts/kconfig/symbol.c | 27 +++++++++++----------------
 8 files changed, 30 insertions(+), 59 deletions(-)

-- 
2.43.0

[PATCH 1/5] treewide: change conditional prompt for choices to 'depends on'

[Masahiro Yamada]

While Documentation/kbuild/kconfig-language.rst provides a brief
explanation, there are recurring confusions regarding the usage of a
prompt followed by 'if <expr>'. This conditional controls _only_ the
prompt.

A typical usage is as follows:

    menuconfig BLOCK
            bool "Enable the block layer" if EXPERT
            default y

When EXPERT=n, the prompt is hidden, but this config entry is still
active, and BLOCK is set to its default value 'y'. This is reasonable
because you are likely want to enable the block device support. When
EXPERT=y, the prompt is shown, allowing you to toggle BLOCK.

Please note that it is different from 'depends on EXPERT', which would
disable the entire config entry.

However, this conditional prompt has never worked for a choice.

The following two work in the same way: when EXPERT is disabled, the
choice block is entirely disabled.

[Test Code 1]

    choice
            prompt "choose" if EXPERT

    config A
            bool "A"

    config B
            bool "B"

    endchoice

[Test Code 2]

    choice
            prompt "choose"
            depends on EXPERT

    config A
            bool "A"

    config B
            bool "B"

    endchoice

I believe the first case should hide only the prompt, but still produce
the default of the choice block:

   CONFIG_A=y
   # CONFIG_B is not set

The next commit will change (fix) the behavior of the conditional prompt
in choice blocks.

I see several choice blocks wrongly using a conditional prompt, where
'depends on' makes more sense.

To preserve the current behavior, this commit converts such misuses.

I did not touch the following hunk in arch/x86/Kconfig:

    choice
            prompt "Memory split" if EXPERT
            default VMSPLIT_3G

This is truly the correct use of the conditional prompt; when EXPERT=n,
this choice block should silently select the reasonable VMSPLIT_3G,
although the resulting PAGE_OFFSET will not be affected anyway.

Presumably, the one in fs/jffs2/Kconfig is also correct, but I converted
it to 'depends on' to avoid any potential behavioral change.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 arch/arm/Kconfig     | 6 ++++--
 arch/arm64/Kconfig   | 3 ++-
 arch/mips/Kconfig    | 6 ++++--
 arch/powerpc/Kconfig | 3 ++-
 arch/riscv/Kconfig   | 3 ++-
 fs/jffs2/Kconfig     | 3 ++-
 6 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ee5115252aac..a5bf65b06c53 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1482,7 +1482,8 @@ config ARM_ATAG_DTB_COMPAT
 	  from the ATAG list and store it at run time into the appended DTB.
 
 choice
-	prompt "Kernel command line type" if ARM_ATAG_DTB_COMPAT
+	prompt "Kernel command line type"
+	depends on ARM_ATAG_DTB_COMPAT
 	default ARM_ATAG_DTB_COMPAT_CMDLINE_FROM_BOOTLOADER
 
 config ARM_ATAG_DTB_COMPAT_CMDLINE_FROM_BOOTLOADER
@@ -1511,7 +1512,8 @@ config CMDLINE
 	  memory size and the root device (e.g., mem=64M root=/dev/nfs).
 
 choice
-	prompt "Kernel command line type" if CMDLINE != ""
+	prompt "Kernel command line type"
+	depends on CMDLINE != ""
 	default CMDLINE_FROM_BOOTLOADER
 
 config CMDLINE_FROM_BOOTLOADER
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5d91259ee7b5..c87d16b12e9b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2302,7 +2302,8 @@ config CMDLINE
 	  root device (e.g. root=/dev/nfs).
 
 choice
-	prompt "Kernel command line type" if CMDLINE != ""
+	prompt "Kernel command line type"
+	depends on CMDLINE != ""
 	default CMDLINE_FROM_BOOTLOADER
 	help
 	  Choose how the kernel will handle the provided default kernel
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index f1aa1bf11166..8cbc23f0c1a7 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -2924,7 +2924,8 @@ config BUILTIN_DTB
 	bool
 
 choice
-	prompt "Kernel appended dtb support" if USE_OF
+	prompt "Kernel appended dtb support"
+	depends on USE_OF
 	default MIPS_NO_APPENDED_DTB
 
 	config MIPS_NO_APPENDED_DTB
@@ -2965,7 +2966,8 @@ choice
 endchoice
 
 choice
-	prompt "Kernel command line type" if !CMDLINE_OVERRIDE
+	prompt "Kernel command line type"
+	depends on !CMDLINE_OVERRIDE
 	default MIPS_CMDLINE_FROM_DTB if USE_OF && !ATH79 && !MACH_INGENIC && \
 					 !MACH_LOONGSON64 && !MIPS_MALTA && \
 					 !CAVIUM_OCTEON_SOC
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c88c6d46a5bc..68e35b33e123 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -965,7 +965,8 @@ config CMDLINE
 	  most cases you will need to specify the root device here.
 
 choice
-	prompt "Kernel command line type" if CMDLINE != ""
+	prompt "Kernel command line type"
+	depends on CMDLINE != ""
 	default CMDLINE_FROM_BOOTLOADER
 
 config CMDLINE_FROM_BOOTLOADER
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 0525ee2d63c7..48b7faf62d0b 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -914,7 +914,8 @@ config CMDLINE
 	  line here and choose how the kernel should use it later on.
 
 choice
-	prompt "Built-in command line usage" if CMDLINE != ""
+	prompt "Built-in command line usage"
+	depends on CMDLINE != ""
 	default CMDLINE_FALLBACK
 	help
 	  Choose how the kernel will handle the provided built-in command
diff --git a/fs/jffs2/Kconfig b/fs/jffs2/Kconfig
index 7c96bc107218..560187d61562 100644
--- a/fs/jffs2/Kconfig
+++ b/fs/jffs2/Kconfig
@@ -151,8 +151,9 @@ config JFFS2_RUBIN
 	  RUBINMIPS and DYNRUBIN compressors. Say 'N' if unsure.
 
 choice
-	prompt "JFFS2 default compression mode" if JFFS2_COMPRESSION_OPTIONS
+	prompt "JFFS2 default compression mode"
 	default JFFS2_CMODE_PRIORITY
+	depends on JFFS2_COMPRESSION_OPTIONS
 	depends on JFFS2_FS
 	help
 	  You can set here the default compression mode of JFFS2 from
-- 
2.43.0

[PATCH 2/5] kconfig: fix conditional prompt behavior for choice

[Masahiro Yamada]

When a prompt is followed by "if <expr>", the symbol is configurable
when the if-conditional evaluates to true.

A typical usage is as follows:

    menuconfig BLOCK
            bool "Enable the block layer" if EXPERT
            default y

When EXPERT=n, the prompt is hidden, but this config entry is still
active, and BLOCK is set to its default value 'y'. When EXPERT=y, the
prompt is shown, making BLOCK a user-configurable option.

This usage is common throughout the kernel tree, but it has never worked
within a choice block.

[Test Code]

    config EXPERT
            bool "Allow expert users to modify more options"

    choice
            prompt "Choose" if EXPERT

    config A
            bool "A"

    config B
            bool "B"

    endchoice

[Result]

    # CONFIG_EXPERT is not set

When the prompt is hidden, the choice block should produce the default
without asking for the user's preference. Hence, the output should be:

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

Removing unnecessary hacks fixes the issue.

This commit also changes the behavior of 'select' by choice members.

[Test Code 2]

    config MODULES
            def_bool y
            modules

    config DEP
            def_tristate m

    if DEP

    choice
            prompt "choose"

    config A
            bool "A"
            select C

    endchoice

    config B
            def_bool y
            select D

    endif

    config C
            tristate

    config D
            tristate

The current output is as follows:

    CONFIG_MODULES=y
    CONFIG_DEP=m
    CONFIG_A=y
    CONFIG_B=y
    CONFIG_C=y
    CONFIG_D=m

With this commit, the output will be changed as follows:

    CONFIG_MODULES=y
    CONFIG_DEP=m
    CONFIG_A=y
    CONFIG_B=y
    CONFIG_C=m
    CONFIG_D=m

CONFIG_C will be changed to 'm' because 'select C' will inherit the
dependency on DEP, which is 'm'.

This change is aligned with the behavior of 'select' outside a choice
block; 'select D' depends on DEP, therefore D is selected by (B && DEP).

Note:

With this commit, allmodconfig will set CONFIG_USB_ROLE_SWITCH to 'm'
instead of 'y'. I did not see any build regression with this change.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/menu.c   | 38 +++-----------------------------------
 scripts/kconfig/symbol.c |  2 +-
 2 files changed, 4 insertions(+), 36 deletions(-)

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 23c95e54660d..b1fbaf2ff792 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -306,7 +306,7 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
 	struct menu *menu, *last_menu;
 	struct symbol *sym;
 	struct property *prop;
-	struct expr *parentdep, *basedep, *dep, *dep2;
+	struct expr *basedep, *dep, *dep2;
 
 	sym = parent->sym;
 	if (parent->list) {
@@ -315,24 +315,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
 		 * and propagate parent dependencies before moving on.
 		 */
 
-		bool is_choice = false;
-
-		if (sym && sym_is_choice(sym))
-			is_choice = true;
-
-		if (is_choice) {
-			/*
-			 * Use the choice itself as the parent dependency of
-			 * the contained items. This turns the mode of the
-			 * choice into an upper bound on the visibility of the
-			 * choice value symbols.
-			 */
-			parentdep = expr_alloc_symbol(sym);
-		} else {
-			/* Menu node for 'menu', 'if' */
-			parentdep = parent->dep;
-		}
-
 		/* For each child menu node... */
 		for (menu = parent->list; menu; menu = menu->next) {
 			/*
@@ -341,7 +323,7 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
 			 */
 			basedep = rewrite_m(menu->dep);
 			basedep = expr_transform(basedep);
-			basedep = expr_alloc_and(expr_copy(parentdep), basedep);
+			basedep = expr_alloc_and(expr_copy(parent->dep), basedep);
 			basedep = expr_eliminate_dups(basedep);
 			menu->dep = basedep;
 
@@ -405,15 +387,12 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
 			}
 		}
 
-		if (is_choice)
-			expr_free(parentdep);
-
 		/*
 		 * Recursively process children in the same fashion before
 		 * moving on
 		 */
 		for (menu = parent->list; menu; menu = menu->next)
-			_menu_finalize(menu, is_choice);
+			_menu_finalize(menu, sym && sym_is_choice(sym));
 	} else if (!inside_choice && sym) {
 		/*
 		 * Automatic submenu creation. If sym is a symbol and A, B, C,
@@ -541,17 +520,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
 		sym_check_prop(sym);
 		sym->flags |= SYMBOL_WARNED;
 	}
-
-	/*
-	 * For choices, add a reverse dependency (corresponding to a select) of
-	 * '<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_yes)));
-	}
 }
 
 void menu_finalize(void)
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index e5441378c4b0..1cb8b6a22c5a 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -868,7 +868,7 @@ const char *sym_get_string_value(struct symbol *sym)
 
 bool sym_is_changeable(struct symbol *sym)
 {
-	return sym->visible > sym->rev_dep.tri;
+	return !sym_is_choice(sym) && sym->visible > sym->rev_dep.tri;
 }
 
 HASHTABLE_DEFINE(sym_hashtable, SYMBOL_HASHSIZE);
-- 
2.43.0

[PATCH 3/5] kconfig: improve error message for dependency between choice members

[Masahiro Yamada]

A choice member must not depend on another member within the same choice
block.

Kconfig detects this, but the error message is not sensible.

[Test Code]

    choice
            prompt "choose"

    config A
            bool "A"
            depends on B

    config B
            bool "B"

    endchoice

[Result]

    Kconfig:1:error: recursive dependency detected!
    Kconfig:1:      choice <choice> contains symbol A
    Kconfig:4:      symbol A is part of choice B
    Kconfig:8:      symbol B is part of choice <choice>
    For a resolution refer to Documentation/kbuild/kconfig-language.rst
    subsection "Kconfig recursive dependency limitations"

The phrase "part of choice B" is weird because B is not a choice block,
but a choice member.

To determine whether the current symbol is a part of a choice block,
sym_is_choice(next_sym) must be checked.

This commit improves the error message to:

    Kconfig:1:error: recursive dependency detected!
    Kconfig:1:      choice <choice> contains symbol A
    Kconfig:4:      symbol A symbol is visible depending on B
    Kconfig:8:      symbol B is part of choice <choice>
    For a resolution refer to Documentation/kbuild/kconfig-language.rst
    subsection "Kconfig recursive dependency limitations"

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/symbol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 1cb8b6a22c5a..0c4b2894ac4e 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -1111,7 +1111,7 @@ static void sym_check_print_recursive(struct symbol *last_sym)
 				menu->filename, menu->lineno,
 				sym->name ? sym->name : "<choice>",
 				next_sym->name ? next_sym->name : "<choice>");
-		} else if (sym_is_choice_value(sym)) {
+		} else if (sym_is_choice(next_sym)) {
 			fprintf(stderr, "%s:%d:\tsymbol %s is part of choice %s\n",
 				menu->filename, menu->lineno,
 				sym->name ? sym->name : "<choice>",
-- 
2.43.0

[PATCH 4/5] kconfig: improve error message for recursive dependency in choice

[Masahiro Yamada]

Kconfig detects recursive dependencies in a choice block, but the error
message is unclear.

[Test Code]

    choice
            prompt "choose"
            depends on A

    config A
            bool "A"

    config B
            bool "B"

    endchoice

[Result]

    Kconfig:1:error: recursive dependency detected!
    Kconfig:1:      choice <choice> contains symbol A
    Kconfig:5:      symbol A is part of choice <choice>
    For a resolution refer to Documentation/kbuild/kconfig-language.rst
    subsection "Kconfig recursive dependency limitations"

The phrase "contains symbol A" does not accurately describe the problem.
The issue is that the choice depends on A, which is a member of itself.

The first if-block does not print a sensible message. Remove it.

This commit improves the error message to:

    Kconfig:1:error: recursive dependency detected!
    Kconfig:1:      symbol <choice> symbol is visible depending on A
    Kconfig:5:      symbol A is part of choice <choice>
    For a resolution refer to Documentation/kbuild/kconfig-language.rst
    subsection "Kconfig recursive dependency limitations"

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/symbol.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 0c4b2894ac4e..787f0667836b 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -1106,12 +1106,7 @@ static void sym_check_print_recursive(struct symbol *last_sym)
 			fprintf(stderr, "%s:%d:error: recursive dependency detected!\n",
 				prop->filename, prop->lineno);
 
-		if (sym_is_choice(sym)) {
-			fprintf(stderr, "%s:%d:\tchoice %s contains symbol %s\n",
-				menu->filename, menu->lineno,
-				sym->name ? sym->name : "<choice>",
-				next_sym->name ? next_sym->name : "<choice>");
-		} else if (sym_is_choice(next_sym)) {
+		if (sym_is_choice(next_sym)) {
 			fprintf(stderr, "%s:%d:\tsymbol %s is part of choice %s\n",
 				menu->filename, menu->lineno,
 				sym->name ? sym->name : "<choice>",
-- 
2.43.0

[PATCH 5/5] kconfig: refactor error messages in sym_check_print_recursive()

[Masahiro Yamada]

Improve the error messages and clean up redundant code.

[1] remove redundant next_sym->name checks

If 'next_sym' is a choice, the first 'if' block is executed. In the
subsequent 'else if' blocks, 'next_sym" is not a choice, hence
next_sym->name is not NULL.

[2] remove redundant sym->name checks

A choice is never selected or implied by anyone because it has no name
(it is syntactically impossible). If it is, sym->name is not NULL.

[3] Show the location of choice instead of "<choice>"

"part of choice <choice>" does not convey useful information. Since a
choice has no name, it is more informative to display the file name and
line number.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/symbol.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 787f0667836b..c05d188a1857 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -1107,37 +1107,37 @@ static void sym_check_print_recursive(struct symbol *last_sym)
 				prop->filename, prop->lineno);
 
 		if (sym_is_choice(next_sym)) {
-			fprintf(stderr, "%s:%d:\tsymbol %s is part of choice %s\n",
+			choice = list_first_entry(&next_sym->menus, struct menu, link);
+
+			fprintf(stderr, "%s:%d:\tsymbol %s is part of choice block at %s:%d\n",
 				menu->filename, menu->lineno,
 				sym->name ? sym->name : "<choice>",
-				next_sym->name ? next_sym->name : "<choice>");
+				choice->filename, choice->lineno);
 		} else if (stack->expr == &sym->dir_dep.expr) {
 			fprintf(stderr, "%s:%d:\tsymbol %s depends on %s\n",
 				prop->filename, prop->lineno,
 				sym->name ? sym->name : "<choice>",
-				next_sym->name ? next_sym->name : "<choice>");
+				next_sym->name);
 		} else if (stack->expr == &sym->rev_dep.expr) {
 			fprintf(stderr, "%s:%d:\tsymbol %s is selected by %s\n",
 				prop->filename, prop->lineno,
-				sym->name ? sym->name : "<choice>",
-				next_sym->name ? next_sym->name : "<choice>");
+				sym->name, next_sym->name);
 		} else if (stack->expr == &sym->implied.expr) {
 			fprintf(stderr, "%s:%d:\tsymbol %s is implied by %s\n",
 				prop->filename, prop->lineno,
-				sym->name ? sym->name : "<choice>",
-				next_sym->name ? next_sym->name : "<choice>");
+				sym->name, next_sym->name);
 		} else if (stack->expr) {
 			fprintf(stderr, "%s:%d:\tsymbol %s %s value contains %s\n",
 				prop->filename, prop->lineno,
 				sym->name ? sym->name : "<choice>",
 				prop_get_type_name(prop->type),
-				next_sym->name ? next_sym->name : "<choice>");
+				next_sym->name);
 		} else {
 			fprintf(stderr, "%s:%d:\tsymbol %s %s is visible depending on %s\n",
 				prop->filename, prop->lineno,
 				sym->name ? sym->name : "<choice>",
 				prop_get_type_name(prop->type),
-				next_sym->name ? next_sym->name : "<choice>");
+				next_sym->name);
 		}
 	}
 
-- 
2.43.0