[PATCH 1/3] kconfig: doc: fix a typo in the note about 'imply'

[PATCH 1/3] kconfig: doc: fix a typo in the note about 'imply'

[Masahiro Yamada]

This sentence does not make sense due to a typo. Fix it.

Fixes: def2fbffe62c ("kconfig: allow symbols implied by y to become m")
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 Documentation/kbuild/kconfig-language.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index 555c2f839969..86be5b857cc4 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -184,7 +184,7 @@ applicable everywhere (see syntax).
   ability to hook into a secondary subsystem while allowing the user to
   configure that subsystem out without also having to unset these drivers.
 
-  Note: If the combination of FOO=y and BAR=m causes a link error,
+  Note: If the combination of FOO=y and BAZ=m causes a link error,
   you can guard the function call with IS_REACHABLE()::
 
 	foo_init()
-- 
2.40.1

[PATCH 2/3] kconfig: doc: document behavior of 'select' and 'imply' followed by 'if'

[Masahiro Yamada]

Documentation/kbuild/kconfig-language.rst explains the behavior of
'select' as follows:

  reverse dependencies can be used to force a lower limit of
  another symbol. The value of the current menu symbol is used as the
  minimal value <symbol> can be set to.

This is not true when the 'select' property is followed by 'if'.

[Test Code]

    config MODULES
            def_bool y
            modules

    config A
            def_tristate y
            select C if B

    config B
            def_tristate m

    config C
            tristate

[Result]

    CONFIG_MODULES=y
    CONFIG_A=y
    CONFIG_B=m
    CONFIG_C=m

If "the value of A is used as the minimal value C can be set to",
C must be 'y'.

The actual behavior is "C is selected by (A && B)". The lower limit of
C is downgraded due to B being 'm'.

I have always thought this behavior was odd, and this ha arisen several
times in the mailing list.

I do not know whether it is a bug or intended behavior. Anyway, it is
not feasible to change it now because many Kconfig files rely on this
behavior. The same applies to 'imply'.

Document this (but reserve the possibility for a future change).

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

 Documentation/kbuild/kconfig-language.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index 86be5b857cc4..1fb3f5e6193c 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -150,6 +150,12 @@ applicable everywhere (see syntax).
 	That will limit the usefulness but on the other hand avoid
 	the illegal configurations all over.
 
+	If "select" <symbol> is followed by "if" <expr>, <symbol> will be
+	selected by the logical AND of the value of the current menu symbol
+	and <expr>. This means, the lower limit can be downgraded due to the
+	presence of "if" <expr>. This behavior may seem weird, but we rely on
+	it. (The future of this behavior is undecided.)
+
 - weak reverse dependencies: "imply" <symbol> ["if" <expr>]
 
   This is similar to "select" as it enforces a lower limit on another
@@ -202,6 +208,10 @@ applicable everywhere (see syntax).
 	imply BAR
 	imply BAZ
 
+  Note: If "imply" <symbol> is followed by "if" <expr>, the default of <symbol>
+  will be the logical AND of the value of the current menu symbol and <expr>.
+  (The future of this behavior is undecided.)
+
 - limiting menu display: "visible if" <expr>
 
   This attribute is only applicable to menu blocks, if the condition is
-- 
2.40.1

Re: [PATCH 2/3] kconfig: doc: document behavior of 'select' and 'imply' followed by 'if'

[Randy Dunlap]

On 6/3/24 9:19 AM, Masahiro Yamada wrote:
> Documentation/kbuild/kconfig-language.rst explains the behavior of
> 'select' as follows:
> 
>   reverse dependencies can be used to force a lower limit of
>   another symbol. The value of the current menu symbol is used as the
>   minimal value <symbol> can be set to.
> 
> This is not true when the 'select' property is followed by 'if'.
> 
> [Test Code]
> 
>     config MODULES
>             def_bool y
>             modules
> 
>     config A
>             def_tristate y
>             select C if B
> 
>     config B
>             def_tristate m
> 
>     config C
>             tristate
> 
> [Result]
> 
>     CONFIG_MODULES=y
>     CONFIG_A=y
>     CONFIG_B=m
>     CONFIG_C=m
> 
> If "the value of A is used as the minimal value C can be set to",
> C must be 'y'.
> 
> The actual behavior is "C is selected by (A && B)". The lower limit of
> C is downgraded due to B being 'm'.
> 
> I have always thought this behavior was odd, and this ha arisen several

                                                        has

> times in the mailing list.
> 
> I do not know whether it is a bug or intended behavior. Anyway, it is
> not feasible to change it now because many Kconfig files rely on this
> behavior. The same applies to 'imply'.
> 
> Document this (but reserve the possibility for a future change).
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

> ---
> 
>  Documentation/kbuild/kconfig-language.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index 86be5b857cc4..1fb3f5e6193c 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -150,6 +150,12 @@ applicable everywhere (see syntax).
>  	That will limit the usefulness but on the other hand avoid
>  	the illegal configurations all over.
>  
> +	If "select" <symbol> is followed by "if" <expr>, <symbol> will be
> +	selected by the logical AND of the value of the current menu symbol
> +	and <expr>. This means, the lower limit can be downgraded due to the
> +	presence of "if" <expr>. This behavior may seem weird, but we rely on
> +	it. (The future of this behavior is undecided.)
> +
>  - weak reverse dependencies: "imply" <symbol> ["if" <expr>]
>  
>    This is similar to "select" as it enforces a lower limit on another
> @@ -202,6 +208,10 @@ applicable everywhere (see syntax).
>  	imply BAR
>  	imply BAZ
>  
> +  Note: If "imply" <symbol> is followed by "if" <expr>, the default of <symbol>
> +  will be the logical AND of the value of the current menu symbol and <expr>.
> +  (The future of this behavior is undecided.)
> +
>  - limiting menu display: "visible if" <expr>
>  
>    This attribute is only applicable to menu blocks, if the condition is

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

[PATCH 3/3] kconfig: remove wrong expr_trans_bool()

[Masahiro Yamada]

expr_trans_bool() performs an incorrect transformation.

[Test Code]

    config MODULES
            def_bool y
            modules

    config A
            def_bool y
            select C if B != n

    config B
            def_tristate m

    config C
            tristate

[Result]

    CONFIG_MODULES=y
    CONFIG_A=y
    CONFIG_B=m
    CONFIG_C=m

This result is incorrect because CONFIG_C=y is expected.

Documentation/kbuild/kconfig-language.rst clearly explains the function
of the '!=' operator:

  (3) If the values of both symbols are equal, it returns 'n',
      otherwise 'y'.

Therefore, the statement:

    select C if A != n

should be equivalent to:

    select C if y

Hence, the symbol C should be selected by 'y' instead of 'm'.

The comment block of expr_trans_bool() correctly explains its intention:

  * bool FOO!=n => FOO
    ^^^^

If FOO is bool, FOO!=n can be simplified into FOO. This is correct.

However, the actual code performs this transformation when FOO is
tristate.

    if (e->left.sym->type == S_TRISTATE) {
                             ^^^^^^^^^^

While, it can be fixed to S_BOOLEAN, there is no point in doing so
because expr_tranform() already transforms FOO!=n to FOO when FOO is
bool. (see the "case E_UNEQUAL" part)

expr_trans_bool() is wrong and unnecessary.

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

 scripts/kconfig/expr.c | 29 -----------------------------
 scripts/kconfig/expr.h |  1 -
 scripts/kconfig/menu.c |  2 --
 3 files changed, 32 deletions(-)

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 4d95fce5f9a7..fcc190b67b6f 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -396,35 +396,6 @@ static struct expr *expr_eliminate_yn(struct expr *e)
 	return e;
 }
 
-/*
- * bool FOO!=n => FOO
- */
-struct expr *expr_trans_bool(struct expr *e)
-{
-	if (!e)
-		return NULL;
-	switch (e->type) {
-	case E_AND:
-	case E_OR:
-	case E_NOT:
-		e->left.expr = expr_trans_bool(e->left.expr);
-		e->right.expr = expr_trans_bool(e->right.expr);
-		break;
-	case E_UNEQUAL:
-		// FOO!=n -> FOO
-		if (e->left.sym->type == S_TRISTATE) {
-			if (e->right.sym == &symbol_no) {
-				e->type = E_SYMBOL;
-				e->right.sym = NULL;
-			}
-		}
-		break;
-	default:
-		;
-	}
-	return e;
-}
-
 /*
  * e1 || e2 -> ?
  */
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index fa50fc45622e..7c0c242318bc 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -284,7 +284,6 @@ void expr_free(struct expr *e);
 void expr_eliminate_eq(struct expr **ep1, struct expr **ep2);
 int expr_eq(struct expr *e1, struct expr *e2);
 tristate expr_calc_value(struct expr *e);
-struct expr *expr_trans_bool(struct expr *e);
 struct expr *expr_eliminate_dups(struct expr *e);
 struct expr *expr_transform(struct expr *e);
 int expr_contains_symbol(struct expr *dep, struct symbol *sym);
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 53151c5a6028..eef9b63cdf11 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -398,8 +398,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
 				dep = expr_transform(dep);
 				dep = expr_alloc_and(expr_copy(basedep), dep);
 				dep = expr_eliminate_dups(dep);
-				if (menu->sym && menu->sym->type != S_TRISTATE)
-					dep = expr_trans_bool(dep);
 				prop->visible.expr = dep;
 
 				/*
-- 
2.40.1

Re: [PATCH 3/3] kconfig: remove wrong expr_trans_bool()

[Randy Dunlap]

On 6/3/24 9:19 AM, Masahiro Yamada wrote:
> expr_trans_bool() performs an incorrect transformation.
> 
> [Test Code]
> 
>     config MODULES
>             def_bool y
>             modules
> 
>     config A
>             def_bool y
>             select C if B != n
> 
>     config B
>             def_tristate m
> 
>     config C
>             tristate
> 
> [Result]
> 
>     CONFIG_MODULES=y
>     CONFIG_A=y
>     CONFIG_B=m
>     CONFIG_C=m
> 
> This result is incorrect because CONFIG_C=y is expected.
> 
> Documentation/kbuild/kconfig-language.rst clearly explains the function
> of the '!=' operator:
> 
>   (3) If the values of both symbols are equal, it returns 'n',
>       otherwise 'y'.
> 
> Therefore, the statement:
> 
>     select C if A != n
> 
> should be equivalent to:
> 
>     select C if y
> 
> Hence, the symbol C should be selected by 'y' instead of 'm'.
> 
> The comment block of expr_trans_bool() correctly explains its intention:
> 
>   * bool FOO!=n => FOO
>     ^^^^
> 
> If FOO is bool, FOO!=n can be simplified into FOO. This is correct.
> 
> However, the actual code performs this transformation when FOO is
> tristate.
> 
>     if (e->left.sym->type == S_TRISTATE) {
>                              ^^^^^^^^^^
> 
> While, it can be fixed to S_BOOLEAN, there is no point in doing so

  While it can

> because expr_tranform() already transforms FOO!=n to FOO when FOO is
> bool. (see the "case E_UNEQUAL" part)
> 
> expr_trans_bool() is wrong and unnecessary.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Acked-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

> ---
> 
>  scripts/kconfig/expr.c | 29 -----------------------------
>  scripts/kconfig/expr.h |  1 -
>  scripts/kconfig/menu.c |  2 --
>  3 files changed, 32 deletions(-)
> 
> diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
> index 4d95fce5f9a7..fcc190b67b6f 100644
> --- a/scripts/kconfig/expr.c
> +++ b/scripts/kconfig/expr.c
> @@ -396,35 +396,6 @@ static struct expr *expr_eliminate_yn(struct expr *e)
>  	return e;
>  }
>  
> -/*
> - * bool FOO!=n => FOO
> - */
> -struct expr *expr_trans_bool(struct expr *e)
> -{
> -	if (!e)
> -		return NULL;
> -	switch (e->type) {
> -	case E_AND:
> -	case E_OR:
> -	case E_NOT:
> -		e->left.expr = expr_trans_bool(e->left.expr);
> -		e->right.expr = expr_trans_bool(e->right.expr);
> -		break;
> -	case E_UNEQUAL:
> -		// FOO!=n -> FOO
> -		if (e->left.sym->type == S_TRISTATE) {
> -			if (e->right.sym == &symbol_no) {
> -				e->type = E_SYMBOL;
> -				e->right.sym = NULL;
> -			}
> -		}
> -		break;
> -	default:
> -		;
> -	}
> -	return e;
> -}
> -
>  /*
>   * e1 || e2 -> ?
>   */
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index fa50fc45622e..7c0c242318bc 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -284,7 +284,6 @@ void expr_free(struct expr *e);
>  void expr_eliminate_eq(struct expr **ep1, struct expr **ep2);
>  int expr_eq(struct expr *e1, struct expr *e2);
>  tristate expr_calc_value(struct expr *e);
> -struct expr *expr_trans_bool(struct expr *e);
>  struct expr *expr_eliminate_dups(struct expr *e);
>  struct expr *expr_transform(struct expr *e);
>  int expr_contains_symbol(struct expr *dep, struct symbol *sym);
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 53151c5a6028..eef9b63cdf11 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -398,8 +398,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
>  				dep = expr_transform(dep);
>  				dep = expr_alloc_and(expr_copy(basedep), dep);
>  				dep = expr_eliminate_dups(dep);
> -				if (menu->sym && menu->sym->type != S_TRISTATE)
> -					dep = expr_trans_bool(dep);
>  				prop->visible.expr = dep;
>  
>  				/*

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

Re: [PATCH 3/3] kconfig: remove wrong expr_trans_bool()

[Masahiro Yamada]

On Tue, Jun 4, 2024 at 1:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> expr_trans_bool() performs an incorrect transformation.
>
> [Test Code]
>
>     config MODULES
>             def_bool y
>             modules
>
>     config A
>             def_bool y
>             select C if B != n
>
>     config B
>             def_tristate m
>
>     config C
>             tristate
>
> [Result]
>
>     CONFIG_MODULES=y
>     CONFIG_A=y
>     CONFIG_B=m
>     CONFIG_C=m
>
> This result is incorrect because CONFIG_C=y is expected.
>
> Documentation/kbuild/kconfig-language.rst clearly explains the function
> of the '!=' operator:
>
>   (3) If the values of both symbols are equal, it returns 'n',
>       otherwise 'y'.
>
> Therefore, the statement:
>
>     select C if A != n

This is wrong.

I meant this:


      select C if B != n






-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/3] kconfig: doc: fix a typo in the note about 'imply'

[Randy Dunlap]

On 6/3/24 9:19 AM, Masahiro Yamada wrote:
> This sentence does not make sense due to a typo. Fix it.
> 
> Fixes: def2fbffe62c ("kconfig: allow symbols implied by y to become m")
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

> ---
> 
>  Documentation/kbuild/kconfig-language.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index 555c2f839969..86be5b857cc4 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -184,7 +184,7 @@ applicable everywhere (see syntax).
>    ability to hook into a secondary subsystem while allowing the user to
>    configure that subsystem out without also having to unset these drivers.
>  
> -  Note: If the combination of FOO=y and BAR=m causes a link error,
> +  Note: If the combination of FOO=y and BAZ=m causes a link error,
>    you can guard the function call with IS_REACHABLE()::
>  
>  	foo_init()

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html