[PATCH 1/2] kconfig: allow symbols implied by y to become m

[PATCH 1/2] kconfig: allow symbols implied by y to become m

[Masahiro Yamada]

The 'imply' keyword restricts a symbol to y or n, excluding m
when it is implied by y. This is the original behavior since
commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword").

However, the author of the 'imply' keyword, Nicolas Pitre, stated
that the 'imply' keyword should not impose any restrictions. [1]

I agree, and want to get rid of this tricky behavior.

[1]: https://lkml.org/lkml/2020/2/19/714

Suggested-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 Documentation/kbuild/kconfig-language.rst | 12 +++++++++++-
 scripts/kconfig/symbol.c                  |  5 +----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index d0111dd26410..d4d988aea679 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -173,7 +173,7 @@ applicable everywhere (see syntax).
 	===		===		=============	==============
 	n		y		n		N/m/y
 	m		y		m		M/y/n
-	y		y		y		Y/n
+	y		y		y		Y/m/n
 	y		n		*		N
 	===		===		=============	==============
 
@@ -181,6 +181,16 @@ 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,
+  you can guard the function call with IS_REACHABLE()::
+
+	foo_init()
+	{
+		if (IS_REACHABLE(CONFIG_BAZ))
+			baz_register(&foo);
+		...
+	}
+
 - limiting menu display: "visible if" <expr>
 
   This attribute is only applicable to menu blocks, if the condition is
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 8d38b700b314..b101ef3c377a 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -401,8 +401,7 @@ void sym_calc_value(struct symbol *sym)
 				sym_warn_unmet_dep(sym);
 			newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri);
 		}
-		if (newval.tri == mod &&
-		    (sym_get_type(sym) == S_BOOLEAN || sym->implied.tri == yes))
+		if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN)
 			newval.tri = yes;
 		break;
 	case S_STRING:
@@ -484,8 +483,6 @@ bool sym_tristate_within_range(struct symbol *sym, tristate val)
 		return false;
 	if (sym->visible <= sym->rev_dep.tri)
 		return false;
-	if (sym->implied.tri == yes && val == mod)
-		return false;
 	if (sym_is_choice_value(sym) && sym->visible == yes)
 		return val == yes;
 	return val >= sym->rev_dep.tri && val <= sym->visible;
-- 
2.17.1

[PATCH 2/2] kconfig: make 'imply' obey the direct dependency

[Masahiro Yamada]

The 'imply' statement may create unmet direct dependency when the
implied symbol depends on m.

[Test Code]

  config FOO
          tristate "foo"
          imply BAZ

  config BAZ
          tristate "baz"
          depends on BAR

  config BAR
          def_tristate m

  config MODULES
          def_bool y
          option modules

If you set FOO=y, BAZ is also promoted to y, which results in the
following .config file:

  CONFIG_FOO=y
  CONFIG_BAZ=y
  CONFIG_BAR=m
  CONFIG_MODULES=y

This does not meet the dependency 'BAZ depends on BAR'.

Unlike 'select', what is worse, Kconfig never shows the
'WARNING: unmet direct dependencies detected for ...' for this case.

Because 'imply' is considered to be weaker than 'depends on', Kconfig
should take the direct dependency into account.

For clarification, describe this case in kconfig-language.rst too.

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

 Documentation/kbuild/kconfig-language.rst | 7 +++++--
 scripts/kconfig/symbol.c                  | 4 +++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index d4d988aea679..68719e78ff85 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -159,11 +159,11 @@ applicable everywhere (see syntax).
   Given the following example::
 
     config FOO
-	tristate
+	tristate "foo"
 	imply BAZ
 
     config BAZ
-	tristate
+	tristate "baz"
 	depends on BAR
 
   The following values are possible:
@@ -174,6 +174,9 @@ applicable everywhere (see syntax).
 	n		y		n		N/m/y
 	m		y		m		M/y/n
 	y		y		y		Y/m/n
+	n		m		n		N/m
+	m		m		m		M/n
+	y		m		n		M/n
 	y		n		*		N
 	===		===		=============	==============
 
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index b101ef3c377a..3dc81397d003 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -221,7 +221,7 @@ static void sym_calc_visibility(struct symbol *sym)
 		sym_set_changed(sym);
 	}
 	tri = no;
-	if (sym->implied.expr && sym->dir_dep.tri != no)
+	if (sym->implied.expr)
 		tri = expr_calc_value(sym->implied.expr);
 	if (tri == mod && sym_get_type(sym) == S_BOOLEAN)
 		tri = yes;
@@ -394,6 +394,8 @@ void sym_calc_value(struct symbol *sym)
 				if (sym->implied.tri != no) {
 					sym->flags |= SYMBOL_WRITE;
 					newval.tri = EXPR_OR(newval.tri, sym->implied.tri);
+					newval.tri = EXPR_AND(newval.tri,
+							      sym->dir_dep.tri);
 				}
 			}
 		calc_newval:
-- 
2.17.1

Re: [PATCH 2/2] kconfig: make 'imply' obey the direct dependency

[Nicolas Pitre]

On Mon, 2 Mar 2020, Masahiro Yamada wrote:

> The 'imply' statement may create unmet direct dependency when the
> implied symbol depends on m.
> 
> [Test Code]
> 
>   config FOO
>           tristate "foo"
>           imply BAZ
> 
>   config BAZ
>           tristate "baz"
>           depends on BAR
> 
>   config BAR
>           def_tristate m
> 
>   config MODULES
>           def_bool y
>           option modules
> 
> If you set FOO=y, BAZ is also promoted to y, which results in the
> following .config file:
> 
>   CONFIG_FOO=y
>   CONFIG_BAZ=y
>   CONFIG_BAR=m
>   CONFIG_MODULES=y
> 
> This does not meet the dependency 'BAZ depends on BAR'.
> 
> Unlike 'select', what is worse, Kconfig never shows the
> 'WARNING: unmet direct dependencies detected for ...' for this case.
> 
> Because 'imply' is considered to be weaker than 'depends on', Kconfig
> should take the direct dependency into account.
> 
> For clarification, describe this case in kconfig-language.rst too.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> ---
> 
>  Documentation/kbuild/kconfig-language.rst | 7 +++++--
>  scripts/kconfig/symbol.c                  | 4 +++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index d4d988aea679..68719e78ff85 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -159,11 +159,11 @@ applicable everywhere (see syntax).
>    Given the following example::
>  
>      config FOO
> -	tristate
> +	tristate "foo"
>  	imply BAZ
>  
>      config BAZ
> -	tristate
> +	tristate "baz"
>  	depends on BAR
>  
>    The following values are possible:
> @@ -174,6 +174,9 @@ applicable everywhere (see syntax).
>  	n		y		n		N/m/y
>  	m		y		m		M/y/n
>  	y		y		y		Y/m/n
> +	n		m		n		N/m
> +	m		m		m		M/n
> +	y		m		n		M/n
>  	y		n		*		N
>  	===		===		=============	==============
>  
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index b101ef3c377a..3dc81397d003 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -221,7 +221,7 @@ static void sym_calc_visibility(struct symbol *sym)
>  		sym_set_changed(sym);
>  	}
>  	tri = no;
> -	if (sym->implied.expr && sym->dir_dep.tri != no)
> +	if (sym->implied.expr)
>  		tri = expr_calc_value(sym->implied.expr);
>  	if (tri == mod && sym_get_type(sym) == S_BOOLEAN)
>  		tri = yes;
> @@ -394,6 +394,8 @@ void sym_calc_value(struct symbol *sym)
>  				if (sym->implied.tri != no) {
>  					sym->flags |= SYMBOL_WRITE;
>  					newval.tri = EXPR_OR(newval.tri, sym->implied.tri);
> +					newval.tri = EXPR_AND(newval.tri,
> +							      sym->dir_dep.tri);
>  				}
>  			}
>  		calc_newval:
> -- 
> 2.17.1
> 
> 

Re: [PATCH 2/2] kconfig: make 'imply' obey the direct dependency

[Geert Uytterhoeven]

Hi Yamada-san,

On Mon, Mar 2, 2020 at 7:24 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> The 'imply' statement may create unmet direct dependency when the
> implied symbol depends on m.
>
> [Test Code]
>
>   config FOO
>           tristate "foo"
>           imply BAZ
>
>   config BAZ
>           tristate "baz"
>           depends on BAR
>
>   config BAR
>           def_tristate m
>
>   config MODULES
>           def_bool y
>           option modules
>
> If you set FOO=y, BAZ is also promoted to y, which results in the
> following .config file:
>
>   CONFIG_FOO=y
>   CONFIG_BAZ=y
>   CONFIG_BAR=m
>   CONFIG_MODULES=y
>
> This does not meet the dependency 'BAZ depends on BAR'.
>
> Unlike 'select', what is worse, Kconfig never shows the
> 'WARNING: unmet direct dependencies detected for ...' for this case.
>
> Because 'imply' is considered to be weaker than 'depends on', Kconfig
> should take the direct dependency into account.
>
> For clarification, describe this case in kconfig-language.rst too.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

This fixes some issue with "imply SND_SOC_WCD934X".
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 1/2] kconfig: allow symbols implied by y to become m

[Nicolas Pitre]

On Mon, 2 Mar 2020, Masahiro Yamada wrote:

> The 'imply' keyword restricts a symbol to y or n, excluding m
> when it is implied by y. This is the original behavior since
> commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword").
> 
> However, the author of the 'imply' keyword, Nicolas Pitre, stated
> that the 'imply' keyword should not impose any restrictions. [1]
> 
> I agree, and want to get rid of this tricky behavior.
> 
> [1]: https://lkml.org/lkml/2020/2/19/714
> 
> Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Acked-by: Nicolas Pitre <nico@fluxnic.net>

In addition to the IS_REACHABLE() note, it might be a good idea to 
suggest adding an "imply" reference to the dependency if the feature 
provided by BAZ is highli desirable, e.g.:

config FOO
	tristate
	imply BAR
	imply BAZ


> ---
> 
>  Documentation/kbuild/kconfig-language.rst | 12 +++++++++++-
>  scripts/kconfig/symbol.c                  |  5 +----
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index d0111dd26410..d4d988aea679 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -173,7 +173,7 @@ applicable everywhere (see syntax).
>  	===		===		=============	==============
>  	n		y		n		N/m/y
>  	m		y		m		M/y/n
> -	y		y		y		Y/n
> +	y		y		y		Y/m/n
>  	y		n		*		N
>  	===		===		=============	==============
>  
> @@ -181,6 +181,16 @@ 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,
> +  you can guard the function call with IS_REACHABLE()::
> +
> +	foo_init()
> +	{
> +		if (IS_REACHABLE(CONFIG_BAZ))
> +			baz_register(&foo);
> +		...
> +	}
> +
>  - limiting menu display: "visible if" <expr>
>  
>    This attribute is only applicable to menu blocks, if the condition is
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 8d38b700b314..b101ef3c377a 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -401,8 +401,7 @@ void sym_calc_value(struct symbol *sym)
>  				sym_warn_unmet_dep(sym);
>  			newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri);
>  		}
> -		if (newval.tri == mod &&
> -		    (sym_get_type(sym) == S_BOOLEAN || sym->implied.tri == yes))
> +		if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN)
>  			newval.tri = yes;
>  		break;
>  	case S_STRING:
> @@ -484,8 +483,6 @@ bool sym_tristate_within_range(struct symbol *sym, tristate val)
>  		return false;
>  	if (sym->visible <= sym->rev_dep.tri)
>  		return false;
> -	if (sym->implied.tri == yes && val == mod)
> -		return false;
>  	if (sym_is_choice_value(sym) && sym->visible == yes)
>  		return val == yes;
>  	return val >= sym->rev_dep.tri && val <= sym->visible;
> -- 
> 2.17.1
> 
> 

Re: [PATCH 1/2] kconfig: allow symbols implied by y to become m

[Masahiro Yamada]

Hi Nicolas,

On Tue, Mar 3, 2020 at 12:31 AM Nicolas Pitre <nico@fluxnic.net> wrote:
>
> On Mon, 2 Mar 2020, Masahiro Yamada wrote:
>
> > The 'imply' keyword restricts a symbol to y or n, excluding m
> > when it is implied by y. This is the original behavior since
> > commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword").
> >
> > However, the author of the 'imply' keyword, Nicolas Pitre, stated
> > that the 'imply' keyword should not impose any restrictions. [1]
> >
> > I agree, and want to get rid of this tricky behavior.
> >
> > [1]: https://lkml.org/lkml/2020/2/19/714
> >
> > Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> Acked-by: Nicolas Pitre <nico@fluxnic.net>
>
> In addition to the IS_REACHABLE() note, it might be a good idea to
> suggest adding an "imply" reference to the dependency if the feature
> provided by BAZ is highli desirable, e.g.:
>
> config FOO
>         tristate
>         imply BAR
>         imply BAZ
>


OK, this info is more relevant to the 2nd patch.

I folded this following to 2/2.

diff --git a/Documentation/kbuild/kconfig-language.rst
b/Documentation/kbuild/kconfig-language.rst
index 68719e78ff85..a1601ec3317b 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -194,6 +194,14 @@ applicable everywhere (see syntax).
                ...
        }

+  Note: If the feature provided by BAZ is highly desirable for FOO,
+  FOO should imply not only BAZ, but also its dependency BAR::
+
+    config FOO
+       tristate "foo"
+       imply BAR
+       imply BAZ
+
 - limiting menu display: "visible if" <expr>

   This attribute is only applicable to menu blocks, if the condition is




Both applied.



> > ---
> >
> >  Documentation/kbuild/kconfig-language.rst | 12 +++++++++++-
> >  scripts/kconfig/symbol.c                  |  5 +----
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> > index d0111dd26410..d4d988aea679 100644
> > --- a/Documentation/kbuild/kconfig-language.rst
> > +++ b/Documentation/kbuild/kconfig-language.rst
> > @@ -173,7 +173,7 @@ applicable everywhere (see syntax).
> >       ===             ===             =============   ==============
> >       n               y               n               N/m/y
> >       m               y               m               M/y/n
> > -     y               y               y               Y/n
> > +     y               y               y               Y/m/n
> >       y               n               *               N
> >       ===             ===             =============   ==============
> >
> > @@ -181,6 +181,16 @@ 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,
> > +  you can guard the function call with IS_REACHABLE()::
> > +
> > +     foo_init()
> > +     {
> > +             if (IS_REACHABLE(CONFIG_BAZ))
> > +                     baz_register(&foo);
> > +             ...
> > +     }
> > +
> >  - limiting menu display: "visible if" <expr>
> >
> >    This attribute is only applicable to menu blocks, if the condition is
> > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> > index 8d38b700b314..b101ef3c377a 100644
> > --- a/scripts/kconfig/symbol.c
> > +++ b/scripts/kconfig/symbol.c
> > @@ -401,8 +401,7 @@ void sym_calc_value(struct symbol *sym)
> >                               sym_warn_unmet_dep(sym);
> >                       newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri);
> >               }
> > -             if (newval.tri == mod &&
> > -                 (sym_get_type(sym) == S_BOOLEAN || sym->implied.tri == yes))
> > +             if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN)
> >                       newval.tri = yes;
> >               break;
> >       case S_STRING:
> > @@ -484,8 +483,6 @@ bool sym_tristate_within_range(struct symbol *sym, tristate val)
> >               return false;
> >       if (sym->visible <= sym->rev_dep.tri)
> >               return false;
> > -     if (sym->implied.tri == yes && val == mod)
> > -             return false;
> >       if (sym_is_choice_value(sym) && sym->visible == yes)
> >               return val == yes;
> >       return val >= sym->rev_dep.tri && val <= sym->visible;
> > --
> > 2.17.1
> >
> >



-- 
Best Regards
Masahiro Yamada