[RFC,1/2] Kconfig: Introduce "uses" keyword
diff mbox series

Message ID 20200417011146.83973-1-saeedm@mellanox.com
State New
Headers show
Series
  • [RFC,1/2] Kconfig: Introduce "uses" keyword
Related show

Commit Message

Saeed Mahameed April 17, 2020, 1:11 a.m. UTC
Due to the changes to the semantics of imply keyword [1], which now
doesn't force any config options to the implied configs any more.

A module (FOO) that has a weak dependency on some other modules (BAR)
is now broken if it was using imply to force dependency restrictions.
e.g.: FOO needs BAR to be reachable, especially when FOO=y and BAR=m.
Which might now introduce build/link errors.

There are two options to solve this:
1. use IS_REACHABLE(BAR), everywhere BAR is referenced inside FOO.
2. in FOO's Kconfig add: depends on (BAR || !BAR)

The first option is not desirable, and will leave the user confused when
setting FOO=y and BAR=m, FOO will never reach BAR even though both are
compiled.

The 2nd one is the preferred approach, and will guarantee BAR is always
reachable by FOO if both are compiled. But, (BAR || !BAR) is really
confusing for those who don't really get how kconfig tristate arithmetics
work.

To solve this and hide this weird expression and to avoid repetition
across the tree, we introduce new keyword "uses" to the Kconfig options
family.

uses BAR:
Equivalent to: depends on symbol || !symbol
Semantically it means, if FOO is enabled (y/m) and has the option:
uses BAR, make sure it can reach/use BAR when possible.

For example: if FOO=y and BAR=m, FOO will be forced to m.

[1] https://lore.kernel.org/linux-doc/20200302062340.21453-1-masahiroy@kernel.org/

Link: https://lkml.org/lkml/2020/4/8/839
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/kbuild/kconfig-language.rst | 10 ++++++++++
 scripts/kconfig/expr.h                    |  1 +
 scripts/kconfig/lexer.l                   |  1 +
 scripts/kconfig/menu.c                    |  4 +++-
 scripts/kconfig/parser.y                  | 15 +++++++++++++++
 scripts/kconfig/symbol.c                  |  2 ++
 6 files changed, 32 insertions(+), 1 deletion(-)

Comments

Jani Nikula April 17, 2020, 6:23 a.m. UTC | #1
On Thu, 16 Apr 2020, Saeed Mahameed <saeedm@mellanox.com> wrote:
> Due to the changes to the semantics of imply keyword [1], which now
> doesn't force any config options to the implied configs any more.
>
> A module (FOO) that has a weak dependency on some other modules (BAR)
> is now broken if it was using imply to force dependency restrictions.
> e.g.: FOO needs BAR to be reachable, especially when FOO=y and BAR=m.
> Which might now introduce build/link errors.
>
> There are two options to solve this:
> 1. use IS_REACHABLE(BAR), everywhere BAR is referenced inside FOO.
> 2. in FOO's Kconfig add: depends on (BAR || !BAR)
>
> The first option is not desirable, and will leave the user confused when
> setting FOO=y and BAR=m, FOO will never reach BAR even though both are
> compiled.
>
> The 2nd one is the preferred approach, and will guarantee BAR is always
> reachable by FOO if both are compiled. But, (BAR || !BAR) is really
> confusing for those who don't really get how kconfig tristate arithmetics
> work.
>
> To solve this and hide this weird expression and to avoid repetition
> across the tree, we introduce new keyword "uses" to the Kconfig options
> family.
>
> uses BAR:
> Equivalent to: depends on symbol || !symbol
> Semantically it means, if FOO is enabled (y/m) and has the option:
> uses BAR, make sure it can reach/use BAR when possible.
>
> For example: if FOO=y and BAR=m, FOO will be forced to m.

Thanks for doing this. I think *something* needs to be done to help
people grasp the "depends on FOO || FOO=n" construct; I've seen many
experienced stumble on this, it's not a rookie mistake.

I suggested "uses" as a keyword, but I'm not hung up on it.

Grepping some Kconfigs a problem I realized with *any* new keyword is
that (FOO || FOO=n) or (FOO || !FOO) is a construct that can be part of
a larger depends on.

For example,

drivers/net/ethernet/broadcom/Kconfig:  depends on PCI && (IPV6 || IPV6=n)

Which means that would have to split up to two. Not ideal, but doable. I
did not find any (FOO || FOO=n) || BAR which would not work with a new
keyword.

An alternative approach that I thought of is adding a lower level
expression to tackle this? "FOO=optional" would expand to (FOO || FOO=n)
anywhere. I have no clue how hard this would be to implement.

For example:

	depends on FOO=optional
=>	
	depends on (FOO || FOO=n)

and:

	depends on FOO=optional || BAR
=>
	depends on (FOO || FOO=n) || BAR


The "optional" keyword is of course open for bikeshedding, but the key
part here I think is that the "depends on" remains, and should be
obvious. And also the =optional ties better to the actual symbol being
depended on.

Thoughts?

BR,
Jani.



>
> [1] https://lore.kernel.org/linux-doc/20200302062340.21453-1-masahiroy@kernel.org/
>
> Link: https://lkml.org/lkml/2020/4/8/839
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: linux-kbuild@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  Documentation/kbuild/kconfig-language.rst | 10 ++++++++++
>  scripts/kconfig/expr.h                    |  1 +
>  scripts/kconfig/lexer.l                   |  1 +
>  scripts/kconfig/menu.c                    |  4 +++-
>  scripts/kconfig/parser.y                  | 15 +++++++++++++++
>  scripts/kconfig/symbol.c                  |  2 ++
>  6 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index a1601ec3317b..8db8c2d80794 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -130,6 +130,16 @@ applicable everywhere (see syntax).
>  	bool "foo"
>  	default y
>  
> +- uses dependencies: "uses" <symbol>
> +
> +  Equivalent to: depends on symbol || !symbol
> +  Semantically it means, if FOO is enabled (y/m) and has the option:
> +  uses BAR, make sure it can reach/use BAR when possible.
> +  For example: if FOO=y and BAR=m, FOO will be forced to m.
> +
> +  Note:
> +      To understand how (symbol || !symbol) is actually computed, please see `Menu dependencies`_
> +
>  - reverse dependencies: "select" <symbol> ["if" <expr>]
>  
>    While normal dependencies reduce the upper limit of a symbol (see
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index 5c3443692f34..face672fb4b4 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -185,6 +185,7 @@ enum prop_type {
>  	P_CHOICE,   /* choice value */
>  	P_SELECT,   /* select BAR */
>  	P_IMPLY,    /* imply BAR */
> +	P_USES,     /* uses BAR */
>  	P_RANGE,    /* range 7..100 (for a symbol) */
>  	P_SYMBOL,   /* where a symbol is defined */
>  };
> diff --git a/scripts/kconfig/lexer.l b/scripts/kconfig/lexer.l
> index 6354c905b006..c6a0017b10d4 100644
> --- a/scripts/kconfig/lexer.l
> +++ b/scripts/kconfig/lexer.l
> @@ -102,6 +102,7 @@ n	[A-Za-z0-9_-]
>  "default"		return T_DEFAULT;
>  "defconfig_list"	return T_DEFCONFIG_LIST;
>  "depends"		return T_DEPENDS;
> +"uses"			return T_USES;
>  "endchoice"		return T_ENDCHOICE;
>  "endif"			return T_ENDIF;
>  "endmenu"		return T_ENDMENU;
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index e436ba44c9c5..e26161b31a11 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -274,7 +274,9 @@ static void sym_check_prop(struct symbol *sym)
>  			break;
>  		case P_SELECT:
>  		case P_IMPLY:
> -			use = prop->type == P_SELECT ? "select" : "imply";
> +		case P_USES:
> +			use = prop->type == P_SELECT ? "select" :
> +				prop->type == P_IMPLY ? "imply" : "uses";
>  			sym2 = prop_get_symbol(prop);
>  			if (sym->type != S_BOOLEAN && sym->type != S_TRISTATE)
>  				prop_warn(prop,
> diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y
> index 708b6c4b13ca..c5e9abb49d29 100644
> --- a/scripts/kconfig/parser.y
> +++ b/scripts/kconfig/parser.y
> @@ -57,6 +57,7 @@ static struct menu *current_menu, *current_entry;
>  %token T_DEF_BOOL
>  %token T_DEF_TRISTATE
>  %token T_DEPENDS
> +%token T_USES
>  %token T_ENDCHOICE
>  %token T_ENDIF
>  %token T_ENDMENU
> @@ -169,6 +170,7 @@ config_option_list:
>  	  /* empty */
>  	| config_option_list config_option
>  	| config_option_list depends
> +	| config_option_list uses
>  	| config_option_list help
>  ;
>  
> @@ -261,6 +263,7 @@ choice_option_list:
>  	  /* empty */
>  	| choice_option_list choice_option
>  	| choice_option_list depends
> +	| choice_option_list uses
>  	| choice_option_list help
>  ;
>  
> @@ -360,6 +363,7 @@ menu_option_list:
>  	  /* empty */
>  	| menu_option_list visible
>  	| menu_option_list depends
> +	| menu_option_list uses
>  ;
>  
>  source_stmt: T_SOURCE T_WORD_QUOTE T_EOL
> @@ -384,6 +388,7 @@ comment_stmt: comment comment_option_list
>  comment_option_list:
>  	  /* empty */
>  	| comment_option_list depends
> +	| comment_option_list uses
>  ;
>  
>  /* help option */
> @@ -418,6 +423,16 @@ depends: T_DEPENDS T_ON expr T_EOL
>  	printd(DEBUG_PARSE, "%s:%d:depends on\n", zconf_curname(), zconf_lineno());
>  };
>  
> +/* uses symbol: depends on symbol || !symbol */
> +uses: T_USES symbol T_EOL
> +{
> +	struct expr *symexpr = expr_alloc_symbol($2);
> +
> +	menu_add_dep(expr_alloc_two(E_OR, symexpr, expr_alloc_one(E_NOT, symexpr)));
> +	printd(DEBUG_PARSE, "%s:%d: uses: depends on %s || ! %s\n",
> +	       zconf_curname(), zconf_lineno(), $2->name, $2->name);
> +};
> +
>  /* visibility option */
>  visible: T_VISIBLE if_expr T_EOL
>  {
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 3dc81397d003..422f7ea47722 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -1295,6 +1295,8 @@ const char *prop_get_type_name(enum prop_type type)
>  		return "choice";
>  	case P_SELECT:
>  		return "select";
> +	case P_USES:
> +		return "uses";
>  	case P_IMPLY:
>  		return "imply";
>  	case P_RANGE:
Arnd Bergmann April 17, 2020, 10:24 a.m. UTC | #2
On Fri, Apr 17, 2020 at 3:12 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> Due to the changes to the semantics of imply keyword [1], which now
> doesn't force any config options to the implied configs any more.
>
> A module (FOO) that has a weak dependency on some other modules (BAR)
> is now broken if it was using imply to force dependency restrictions.
> e.g.: FOO needs BAR to be reachable, especially when FOO=y and BAR=m.
> Which might now introduce build/link errors.
>
> There are two options to solve this:
> 1. use IS_REACHABLE(BAR), everywhere BAR is referenced inside FOO.
> 2. in FOO's Kconfig add: depends on (BAR || !BAR)
>
> The first option is not desirable, and will leave the user confused when
> setting FOO=y and BAR=m, FOO will never reach BAR even though both are
> compiled.
>
> The 2nd one is the preferred approach, and will guarantee BAR is always
> reachable by FOO if both are compiled. But, (BAR || !BAR) is really
> confusing for those who don't really get how kconfig tristate arithmetics
> work.
>
> To solve this and hide this weird expression and to avoid repetition
> across the tree, we introduce new keyword "uses" to the Kconfig options
> family.
>
> uses BAR:
> Equivalent to: depends on symbol || !symbol
> Semantically it means, if FOO is enabled (y/m) and has the option:
> uses BAR, make sure it can reach/use BAR when possible.
>
> For example: if FOO=y and BAR=m, FOO will be forced to m.
>
> [1] https://lore.kernel.org/linux-doc/20200302062340.21453-1-masahiroy@kernel.org/

Thanks a lot for getting this done. I've tried it out on my randconfig
build tree
and can confirm that this works together with your second patch to address the
specific MLX5 problem.

I also tried out replacing all other instances of 'depends on FOO ||
!FOO', using
this oneline script:

git ls-files | grep Kconfig |  xargs sed -i
's:depends.on.\([A-Z0-9_a-z]\+\) || \(\1 \?= \?n\|!\1\):uses \1:'

Unfortunately, this immediately crashes with:

$ make -skj30
how to free type 0?
double free or corruption (fasttop)
make[6]: *** [/git/arm-soc/scripts/kconfig/Makefile:71: olddefconfig]
Aborted (core dumped)
make[5]: *** [/git/arm-soc/Makefile:587: olddefconfig] Error 2
make[4]: *** [/git/arm-soc/scripts/kconfig/Makefile:95:
allrandom.config] Error 2
make[3]: *** [/git/arm-soc/Makefile:587: allrandom.config] Error 2
make[2]: *** [Makefile:180: sub-make] Error 2
make[2]: Target 'allrandom.config' not remade because of errors.
make[1]: *** [makefile:127: allrandom.config] Error 2

It's probably easy to fix, but I did not look any deeper into the bug.

       Arnd
Saeed Mahameed April 17, 2020, 11:35 a.m. UTC | #3
On Fri, 2020-04-17 at 12:24 +0200, Arnd Bergmann wrote:
> On Fri, Apr 17, 2020 at 3:12 AM Saeed Mahameed <saeedm@mellanox.com>
> wrote:
> > Due to the changes to the semantics of imply keyword [1], which now
> > doesn't force any config options to the implied configs any more.
> > 
> > A module (FOO) that has a weak dependency on some other modules
> > (BAR)
> > is now broken if it was using imply to force dependency
> > restrictions.
> > e.g.: FOO needs BAR to be reachable, especially when FOO=y and
> > BAR=m.
> > Which might now introduce build/link errors.
> > 
> > There are two options to solve this:
> > 1. use IS_REACHABLE(BAR), everywhere BAR is referenced inside FOO.
> > 2. in FOO's Kconfig add: depends on (BAR || !BAR)
> > 
> > The first option is not desirable, and will leave the user confused
> > when
> > setting FOO=y and BAR=m, FOO will never reach BAR even though both
> > are
> > compiled.
> > 
> > The 2nd one is the preferred approach, and will guarantee BAR is
> > always
> > reachable by FOO if both are compiled. But, (BAR || !BAR) is really
> > confusing for those who don't really get how kconfig tristate
> > arithmetics
> > work.
> > 
> > To solve this and hide this weird expression and to avoid
> > repetition
> > across the tree, we introduce new keyword "uses" to the Kconfig
> > options
> > family.
> > 
> > uses BAR:
> > Equivalent to: depends on symbol || !symbol
> > Semantically it means, if FOO is enabled (y/m) and has the option:
> > uses BAR, make sure it can reach/use BAR when possible.
> > 
> > For example: if FOO=y and BAR=m, FOO will be forced to m.
> > 
> > [1] 
> > https://lore.kernel.org/linux-doc/20200302062340.21453-1-masahiroy@kernel.org/
> 
> Thanks a lot for getting this done. I've tried it out on my
> randconfig
> build tree
> and can confirm that this works together with your second patch to
> address the
> specific MLX5 problem.
> 
> I also tried out replacing all other instances of 'depends on FOO ||
> !FOO', using
> this oneline script:
> 
> git ls-files | grep Kconfig |  xargs sed -i
> 's:depends.on.\([A-Z0-9_a-z]\+\) || \(\1 \?= \?n\|!\1\):uses \1:'
> 
> Unfortunately, this immediately crashes with:
> 
> $ make -skj30
> how to free type 0?
> double free or corruption (fasttop)
> make[6]: *** [/git/arm-soc/scripts/kconfig/Makefile:71: olddefconfig]
> Aborted (core dumped)
> make[5]: *** [/git/arm-soc/Makefile:587: olddefconfig] Error 2
> make[4]: *** [/git/arm-soc/scripts/kconfig/Makefile:95:
> allrandom.config] Error 2
> make[3]: *** [/git/arm-soc/Makefile:587: allrandom.config] Error 2
> make[2]: *** [Makefile:180: sub-make] Error 2
> make[2]: Target 'allrandom.config' not remade because of errors.
> make[1]: *** [makefile:127: allrandom.config] Error 2
> 

> It's probably easy to fix, but I did not look any deeper into the
> bug.
> 

Ahh, I know what it is, i am allocating only one expression for the two
symbols (FOO || !FOO) .. in the rule action in parser.y, i must
allocate two individual instances per each of the FOO appearances .. 

something like:

struct expr *symexpr1 = expr_alloc_symbol($2);
struct expr *symexpr2 = expr_alloc_symbol($2);
	
menu_add_dep(expr_alloc_two(E_OR, symexpr1, expr_alloc_one(E_NOT,
symexpr2)));


Thanks Arnd for testing this ! I will test this and send V2 later.
Jason Gunthorpe April 17, 2020, 12:28 p.m. UTC | #4
On Fri, Apr 17, 2020 at 09:23:59AM +0300, Jani Nikula wrote:

> Which means that would have to split up to two. Not ideal, but
> doable.

Why is this not ideal?

I think the one per line is easier to maintain (eg for merge
conflicts) and easier to read than a giant && expression.

I would not complicate things further by extending the boolean
language..

Jason
Jani Nikula April 17, 2020, 2:01 p.m. UTC | #5
On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Fri, Apr 17, 2020 at 09:23:59AM +0300, Jani Nikula wrote:
>
>> Which means that would have to split up to two. Not ideal, but
>> doable.
>
> Why is this not ideal?
>
> I think the one per line is easier to maintain (eg for merge
> conflicts) and easier to read than a giant && expression.
>
> I would not complicate things further by extending the boolean
> language..

Fair enough. I only found one instance where the patch at hand does not
cut it:

drivers/hwmon/Kconfig:  depends on !OF || IIO=n || IIO

That can of course be left as it is.

As to the bikeshedding topic, I think I'm now leaning towards Andrzej's
suggestion:

	optionally depends on FOO

in [1]. But I reserve my right to change my mind. ;)

BR,
Jani.


[1] http://lore.kernel.org/r/01f964ae-9c32-7531-1f07-2687616b6a71@samsung.com
Jason Gunthorpe April 17, 2020, 2:07 p.m. UTC | #6
On Fri, Apr 17, 2020 at 05:01:18PM +0300, Jani Nikula wrote:
> On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Fri, Apr 17, 2020 at 09:23:59AM +0300, Jani Nikula wrote:
> >
> >> Which means that would have to split up to two. Not ideal, but
> >> doable.
> >
> > Why is this not ideal?
> >
> > I think the one per line is easier to maintain (eg for merge
> > conflicts) and easier to read than a giant && expression.
> >
> > I would not complicate things further by extending the boolean
> > language..
> 
> Fair enough. I only found one instance where the patch at hand does not
> cut it:
> 
> drivers/hwmon/Kconfig:  depends on !OF || IIO=n || IIO

Ideally this constraint would be expressed as:

   optionally depends on OF && IIO

And if the expression is n then IIO is not prevented from being y.

Ie the code is just doing:

#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_IIO)

Jason
Masahiro Yamada April 18, 2020, 7 p.m. UTC | #7
On Fri, Apr 17, 2020 at 10:12 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> Due to the changes to the semantics of imply keyword [1], which now
> doesn't force any config options to the implied configs any more.
>
> A module (FOO) that has a weak dependency on some other modules (BAR)
> is now broken if it was using imply to force dependency restrictions.
> e.g.: FOO needs BAR to be reachable, especially when FOO=y and BAR=m.
> Which might now introduce build/link errors.
>
> There are two options to solve this:
> 1. use IS_REACHABLE(BAR), everywhere BAR is referenced inside FOO.
> 2. in FOO's Kconfig add: depends on (BAR || !BAR)
>
> The first option is not desirable, and will leave the user confused when
> setting FOO=y and BAR=m, FOO will never reach BAR even though both are
> compiled.
>
> The 2nd one is the preferred approach, and will guarantee BAR is always
> reachable by FOO if both are compiled. But, (BAR || !BAR) is really
> confusing for those who don't really get how kconfig tristate arithmetics
> work.
>
> To solve this and hide this weird expression and to avoid repetition
> across the tree, we introduce new keyword "uses" to the Kconfig options
> family.
>
> uses BAR:
> Equivalent to: depends on symbol || !symbol
> Semantically it means, if FOO is enabled (y/m) and has the option:
> uses BAR, make sure it can reach/use BAR when possible.
>
> For example: if FOO=y and BAR=m, FOO will be forced to m.
>
> [1] https://lore.kernel.org/linux-doc/20200302062340.21453-1-masahiroy@kernel.org/
>
> Link: https://lkml.org/lkml/2020/4/8/839
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: linux-kbuild@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---


I am not convinced with this patch.

This patch adds another way to do the same thing.
It is true that it _hides_ the problems, and
makes the _surface_  cleaner at best,
but the internal will be more complicated.

(FOO || !FOO) is difficult to understand, but
the behavior of "uses FOO" is as difficult to grasp.

People would wonder, "what 'uses FOO' means?",
then they would find the explanation in kconfig-language.rst:

  "Equivalent to: depends on symbol || !symbol
  Semantically it means, if FOO is enabled (y/m) and has the option:
  uses BAR, make sure it can reach/use BAR when possible."

To understand this correctly, people must study
the arithmetic of (symbol || !symbol) anyway.

I do not want to extend Kconfig for the iffy syntax sugar.


(symbol || !symbol) is horrible.
But, I am also scared to see people would think 'uses symbol'
is the right thing to do, and start using it liberally
all over the place.





--
Best Regards

Masahiro Yamada
Nicolas Pitre April 18, 2020, 7:11 p.m. UTC | #8
On Sun, 19 Apr 2020, Masahiro Yamada wrote:

> (FOO || !FOO) is difficult to understand, but
> the behavior of "uses FOO" is as difficult to grasp.

Can't this be expressed as the following instead:

	depends on FOO if FOO

That would be a little clearer.


Nicolas
Masahiro Yamada April 18, 2020, 8:07 p.m. UTC | #9
On Sun, Apr 19, 2020 at 4:11 AM Nicolas Pitre <nico@fluxnic.net> wrote:
>
> On Sun, 19 Apr 2020, Masahiro Yamada wrote:
>
> > (FOO || !FOO) is difficult to understand, but
> > the behavior of "uses FOO" is as difficult to grasp.
>
> Can't this be expressed as the following instead:
>
>         depends on FOO if FOO
>
> That would be a little clearer.
>
>
> Nicolas



'depends on' does not take the 'if <expr>'

'depends on A if B' is the syntax sugar of
'depends on (A || !B), right ?

I do not know how clearer it would make things.

depends on (m || FOO != m)
is another equivalent, but we are always
talking about a matter of expression.


How important is it to stick to
depends on (FOO || !FOO)
or its equivalents?


If a driver wants to use the feature FOO
in most usecases, 'depends on FOO' is sensible.

If FOO is just optional, you can get rid of the dependency,
and IS_REACHABLE() will do logically correct things.


I do not think IS_REACHABLE() is too bad,
but if it is confusing, we can add one more
option to make it explicit.



config DRIVER_X
       tristate "driver x"

config DRIVER_X_USES_FOO
       bool "use FOO from driver X"
       depends on DRIVER_X
       depends on DRIVER_X <= FOO
       help
         DRIVER_X works without FOO, but
         Using FOO will provide better usability.
         Say Y if you want to make driver X use FOO.



Of course,

      if (IS_ENABLED(CONFIG_DRIVER_X_USES_FOO))
               foo_init();

works like

      if (IS_REACHABLE(CONFIG_FOO))
                foo_init();


At lease, it will eliminate a question like
"I loaded the module FOO, I swear.
But my built-in driver X still would not use FOO, why?"
Jani Nikula April 20, 2020, 8:43 a.m. UTC | #10
On Sun, 19 Apr 2020, Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Sun, Apr 19, 2020 at 4:11 AM Nicolas Pitre <nico@fluxnic.net> wrote:
>>
>> On Sun, 19 Apr 2020, Masahiro Yamada wrote:
>>
>> > (FOO || !FOO) is difficult to understand, but
>> > the behavior of "uses FOO" is as difficult to grasp.
>>
>> Can't this be expressed as the following instead:
>>
>>         depends on FOO if FOO
>>
>> That would be a little clearer.
>>
>>
>> Nicolas
>
>
>
> 'depends on' does not take the 'if <expr>'
>
> 'depends on A if B' is the syntax sugar of
> 'depends on (A || !B), right ?
>
> I do not know how clearer it would make things.
>
> depends on (m || FOO != m)
> is another equivalent, but we are always
> talking about a matter of expression.
>
>
> How important is it to stick to
> depends on (FOO || !FOO)
> or its equivalents?
>
>
> If a driver wants to use the feature FOO
> in most usecases, 'depends on FOO' is sensible.
>
> If FOO is just optional, you can get rid of the dependency,
> and IS_REACHABLE() will do logically correct things.

If by logically correct you mean the kernel builds, you're
right. However the proliferation of IS_REACHABLE() is making the kernel
config *harder* to understand. User enables FOO=m and expects BAR to use
it, however if BAR=y it silently gets ignored. I have and I will oppose
adding IS_REACHABLE() usage to i915 because it's just silently accepting
configurations that should be flagged and forbidden at kconfig stage.

> I do not think IS_REACHABLE() is too bad,
> but if it is confusing, we can add one more
> option to make it explicit.
>
>
>
> config DRIVER_X
>        tristate "driver x"
>
> config DRIVER_X_USES_FOO
>        bool "use FOO from driver X"
>        depends on DRIVER_X
>        depends on DRIVER_X <= FOO
>        help
>          DRIVER_X works without FOO, but
>          Using FOO will provide better usability.
>          Say Y if you want to make driver X use FOO.
>
>
>
> Of course,
>
>       if (IS_ENABLED(CONFIG_DRIVER_X_USES_FOO))
>                foo_init();
>
> works like
>
>       if (IS_REACHABLE(CONFIG_FOO))
>                 foo_init();
>
>
> At lease, it will eliminate a question like
> "I loaded the module FOO, I swear.
> But my built-in driver X still would not use FOO, why?"

Please let's not make that a more widespread problem than it already
is. I have yet to hear *one* good rationale for allowing that in the
first place. And if that pops up, you can make it work by using
IS_REACHABLE() *without* the depends, simply by checking if the module
is there.

Most use cases increasingly solved by IS_REACHABLE() should use the
"depends on FOO || FOO=n" construct, but the problem is that's not
widely understood. I'd like to have another keyword for people to
copy-paste into their Kconfigs.

In another mail I suggested

	optionally depends on FOO

might be a better alternative than "uses".


BR,
Jani.
Jason Gunthorpe April 20, 2020, 1:53 p.m. UTC | #11
On Sun, Apr 19, 2020 at 04:00:43AM +0900, Masahiro Yamada wrote:

> People would wonder, "what 'uses FOO' means?",
> then they would find the explanation in kconfig-language.rst:
> 
>   "Equivalent to: depends on symbol || !symbol
>   Semantically it means, if FOO is enabled (y/m) and has the option:
>   uses BAR, make sure it can reach/use BAR when possible."
> 
> To understand this correctly, people must study
> the arithmetic of (symbol || !symbol) anyway.

I think people will just cargo-cult copy it and not think too hard
about how kconfig works.

The descriptions in kconfig-language.rst can be improved to better
guide C people using kconfig without entirely understanding
it. Something like:

 BAR depends on FOO // BAR selects FOO: BAR requires functionality from
 FOO

 BAR uses FOO: BAR optionally consumes functionality from FOO using
 IS_ENABLED

 BAR implies FOO: BAR optionally consumes functionality from FOO using
 IS_REACHABLE

Now someone adding IS_ENABLED or IS_REACHABLE checks to C code knows
exactly what to put in the kconfig.

Jason
Jakub Kicinski April 20, 2020, 6:42 p.m. UTC | #12
On Mon, 20 Apr 2020 11:43:13 +0300 Jani Nikula wrote:
> On Sun, 19 Apr 2020, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > On Sun, Apr 19, 2020 at 4:11 AM Nicolas Pitre <nico@fluxnic.net> wrote:  
> >>
> >> On Sun, 19 Apr 2020, Masahiro Yamada wrote:
> >>  
> >> > (FOO || !FOO) is difficult to understand, but
> >> > the behavior of "uses FOO" is as difficult to grasp.  
> >>
> >> Can't this be expressed as the following instead:
> >>
> >>         depends on FOO if FOO
> >>
> >> That would be a little clearer.
> >>
> >>
> >> Nicolas  
> >
> > 'depends on' does not take the 'if <expr>'
> >
> > 'depends on A if B' is the syntax sugar of
> > 'depends on (A || !B), right ?
> >
> > I do not know how clearer it would make things.
> >
> > depends on (m || FOO != m)
> > is another equivalent, but we are always
> > talking about a matter of expression.
> >
> >
> > How important is it to stick to
> > depends on (FOO || !FOO)
> > or its equivalents?
> >
> >
> > If a driver wants to use the feature FOO
> > in most usecases, 'depends on FOO' is sensible.
> >
> > If FOO is just optional, you can get rid of the dependency,
> > and IS_REACHABLE() will do logically correct things.  
> 
> If by logically correct you mean the kernel builds, you're
> right. However the proliferation of IS_REACHABLE() is making the kernel
> config *harder* to understand. User enables FOO=m and expects BAR to use
> it, however if BAR=y it silently gets ignored. I have and I will oppose
> adding IS_REACHABLE() usage to i915 because it's just silently accepting
> configurations that should be flagged and forbidden at kconfig stage.

+1

I wholeheartedly agree. In case of Ethernet drivers having higher
layers of the stack not able to communicate with drivers is just 
broken IMHO.
Saeed Mahameed April 21, 2020, 4:24 a.m. UTC | #13
On Mon, 2020-04-20 at 11:43 +0300, Jani Nikula wrote:
> On Sun, 19 Apr 2020, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > On Sun, Apr 19, 2020 at 4:11 AM Nicolas Pitre <nico@fluxnic.net>
> > wrote:
> > > On Sun, 19 Apr 2020, Masahiro Yamada wrote:
> > > 
> > > > (FOO || !FOO) is difficult to understand, but
> > > > the behavior of "uses FOO" is as difficult to grasp.
> > > 
> > > Can't this be expressed as the following instead:
> > > 
> > >         depends on FOO if FOO
> > > 
> > > That would be a little clearer.
> > > 
> > > 
> > > Nicolas
> > 
> > 
> > 'depends on' does not take the 'if <expr>'
> > 
> > 'depends on A if B' is the syntax sugar of
> > 'depends on (A || !B), right ?
> > 
> > I do not know how clearer it would make things.
> > 
> > depends on (m || FOO != m)
> > is another equivalent, but we are always
> > talking about a matter of expression.
> > 
> > 
> > How important is it to stick to
> > depends on (FOO || !FOO)
> > or its equivalents?
> > 
> > 
> > If a driver wants to use the feature FOO
> > in most usecases, 'depends on FOO' is sensible.
> > 
> > If FOO is just optional, you can get rid of the dependency,
> > and IS_REACHABLE() will do logically correct things.
> 
> If by logically correct you mean the kernel builds, you're
> right. However the proliferation of IS_REACHABLE() is making the
> kernel
> config *harder* to understand. User enables FOO=m and expects BAR to
> use
> it, however if BAR=y it silently gets ignored. I have and I will
> oppose
> adding IS_REACHABLE() usage to i915 because it's just silently
> accepting
> configurations that should be flagged and forbidden at kconfig stage.
> 
> > I do not think IS_REACHABLE() is too bad,
> > but if it is confusing, we can add one more
> > option to make it explicit.
> > 
> > 
> > 
> > config DRIVER_X
> >        tristate "driver x"
> > 
> > config DRIVER_X_USES_FOO
> >        bool "use FOO from driver X"
> >        depends on DRIVER_X
> >        depends on DRIVER_X <= FOO
> >        help
> >          DRIVER_X works without FOO, but
> >          Using FOO will provide better usability.
> >          Say Y if you want to make driver X use FOO.
> > 
> > 
> > 
> > Of course,
> > 
> >       if (IS_ENABLED(CONFIG_DRIVER_X_USES_FOO))
> >                foo_init();
> > 
> > works like
> > 
> >       if (IS_REACHABLE(CONFIG_FOO))
> >                 foo_init();
> > 
> > 
> > At lease, it will eliminate a question like
> > "I loaded the module FOO, I swear.
> > But my built-in driver X still would not use FOO, why?"
> 

and duplicate this all over just to avoid new keyword.


> Please let's not make that a more widespread problem than it already
> is. I have yet to hear *one* good rationale for allowing that in the
> first place. And if that pops up, you can make it work by using
> IS_REACHABLE() *without* the depends, simply by checking if the
> module
> is there.
> 
> Most use cases increasingly solved by IS_REACHABLE() should use the
> "depends on FOO || FOO=n" construct, but the problem is that's not
> widely understood. I'd like to have another keyword for people to
> copy-paste into their Kconfigs.
> 

+1 

do all C developers know how the C compiler works ? of course not !
Same goes here, there is a demand for a new keyword, so people will
avoid copy and pate and can use the kconfig language in a higher
simplified level.

I just did a quick grep to find out how really people use depend on:

# All usage of depends on 
$ git ls-files | grep Kconfig | xargs grep -E "depends\s+on" | wc -l
15071

# simple single symbol expression usage 
$ git ls-files | grep Kconfig | xargs grep -E "depends\s+on\s+[A-Za-z0-
9_]+\s*$" | wc -l
8889

almost 60%.. 

people really like simple things especially for the tools they are
using "like kconfig", no one really wants to understand how it really
work under the hood if it is a one time thing that you need to setup
for your kernel project, unless it is really necessary ..

I wonder how many of those 8889 cases wanted a weak dependency but
couldn't figure out how to do it ? 

Users of depends on FOO || !FOO

$ git ls-files | grep Kconfig | xargs grep -E \
  "depends\s+on\s+([A-Za-z0-9_]+)\s*\|\|\s*(\!\s*\1|\1\s*=\s*n)" \
 | wc -l

156

a new keyword is required :) .. 


> In another mail I suggested
> 
> 	optionally depends on FOO
> 
> might be a better alternative than "uses".
> 
> 

how about just:
      optional FOO

It is clear and easy to document .. 


> BR,
> Jani.
>
Nicolas Pitre April 21, 2020, 1:58 p.m. UTC | #14
On Tue, 21 Apr 2020, Saeed Mahameed wrote:

> I wonder how many of those 8889 cases wanted a weak dependency but
> couldn't figure out how to do it ? 
> 
> Users of depends on FOO || !FOO
> 
> $ git ls-files | grep Kconfig | xargs grep -E \
>   "depends\s+on\s+([A-Za-z0-9_]+)\s*\|\|\s*(\!\s*\1|\1\s*=\s*n)" \
>  | wc -l
> 
> 156
> 
> a new keyword is required :) .. 
> 
> 
> > In another mail I suggested
> > 
> > 	optionally depends on FOO
> > 
> > might be a better alternative than "uses".
> > 
> > 
> 
> how about just:
>       optional FOO
> 
> It is clear and easy to document .. 

I don't dispute your argument for having a new keyword. But the most 
difficult part as Arnd said is to find it. You cannot pretend that 
"optional FOO" is clear when it actually imposes a restriction when 
FOO=m. Try to justify to people why they cannot select y because of this 
"optional" thing.


Nicolas
Saeed Mahameed April 21, 2020, 4:30 p.m. UTC | #15
On Tue, 2020-04-21 at 09:58 -0400, Nicolas Pitre wrote:
> On Tue, 21 Apr 2020, Saeed Mahameed wrote:
> 
> > I wonder how many of those 8889 cases wanted a weak dependency but
> > couldn't figure out how to do it ? 
> > 
> > Users of depends on FOO || !FOO
> > 
> > $ git ls-files | grep Kconfig | xargs grep -E \
> >   "depends\s+on\s+([A-Za-z0-9_]+)\s*\|\|\s*(\!\s*\1|\1\s*=\s*n)" \
> >  | wc -l
> > 
> > 156
> > 
> > a new keyword is required :) .. 
> > 
> > 
> > > In another mail I suggested
> > > 
> > > 	optionally depends on FOO
> > > 
> > > might be a better alternative than "uses".
> > > 
> > > 
> > 
> > how about just:
> >       optional FOO
> > 
> > It is clear and easy to document .. 
> 
> I don't dispute your argument for having a new keyword. But the most 
> difficult part as Arnd said is to find it. You cannot pretend that 

kconfig-language.rst  ?

> "optional FOO" is clear when it actually imposes a restriction when 
> FOO=m. Try to justify to people why they cannot select y because of
> this 
> "optional" thing.
> 

Then let's use "uses" it is more assertive. Documentation will cover
any vague anything about it .. 

> 
> Nicolas
Nicolas Pitre April 21, 2020, 6:23 p.m. UTC | #16
On Tue, 21 Apr 2020, Saeed Mahameed wrote:

> On Tue, 2020-04-21 at 09:58 -0400, Nicolas Pitre wrote:
> > On Tue, 21 Apr 2020, Saeed Mahameed wrote:
> > 
> > > I wonder how many of those 8889 cases wanted a weak dependency but
> > > couldn't figure out how to do it ? 
> > > 
> > > Users of depends on FOO || !FOO
> > > 
> > > $ git ls-files | grep Kconfig | xargs grep -E \
> > >   "depends\s+on\s+([A-Za-z0-9_]+)\s*\|\|\s*(\!\s*\1|\1\s*=\s*n)" \
> > >  | wc -l
> > > 
> > > 156
> > > 
> > > a new keyword is required :) .. 
> > > 
> > > 
> > > > In another mail I suggested
> > > > 
> > > > 	optionally depends on FOO
> > > > 
> > > > might be a better alternative than "uses".
> > > > 
> > > > 
> > > 
> > > how about just:
> > >       optional FOO
> > > 
> > > It is clear and easy to document .. 
> > 
> > I don't dispute your argument for having a new keyword. But the most 
> > difficult part as Arnd said is to find it. You cannot pretend that 
> 
> kconfig-language.rst  ?
> 
> > "optional FOO" is clear when it actually imposes a restriction when 
> > FOO=m. Try to justify to people why they cannot select y because of
> > this 
> > "optional" thing.
> > 
> 
> Then let's use "uses" it is more assertive. Documentation will cover
> any vague anything about it .. 

It uses what? And why can't I configure this with "uses FOO" when FOO=m?
That's not any clearer. And saying that "this is weird but it is 
described in the documentation" is not good enough. We must make things 
clear in the first place.

This is really a conditional dependency. That's all this is about.
So why not simply making it so rather than fooling ourselves? All that 
is required is an extension that would allow:

	depends on (expression) if (expression)

This construct should be obvious even without reading the doc, is 
already used extensively for other things already, and is flexible 
enough to cover all sort of cases in addition to this particular one.


Nicolas
Jani Nikula April 22, 2020, 8:51 a.m. UTC | #17
On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 21 Apr 2020, Saeed Mahameed wrote:
>
>> On Tue, 2020-04-21 at 09:58 -0400, Nicolas Pitre wrote:
>> > On Tue, 21 Apr 2020, Saeed Mahameed wrote:
>> > 
>> > > I wonder how many of those 8889 cases wanted a weak dependency but
>> > > couldn't figure out how to do it ? 
>> > > 
>> > > Users of depends on FOO || !FOO
>> > > 
>> > > $ git ls-files | grep Kconfig | xargs grep -E \
>> > >   "depends\s+on\s+([A-Za-z0-9_]+)\s*\|\|\s*(\!\s*\1|\1\s*=\s*n)" \
>> > >  | wc -l
>> > > 
>> > > 156
>> > > 
>> > > a new keyword is required :) .. 
>> > > 
>> > > 
>> > > > In another mail I suggested
>> > > > 
>> > > > 	optionally depends on FOO
>> > > > 
>> > > > might be a better alternative than "uses".
>> > > > 
>> > > > 
>> > > 
>> > > how about just:
>> > >       optional FOO
>> > > 
>> > > It is clear and easy to document .. 
>> > 
>> > I don't dispute your argument for having a new keyword. But the most 
>> > difficult part as Arnd said is to find it. You cannot pretend that 
>> 
>> kconfig-language.rst  ?
>> 
>> > "optional FOO" is clear when it actually imposes a restriction when 
>> > FOO=m. Try to justify to people why they cannot select y because of
>> > this 
>> > "optional" thing.
>> > 
>> 
>> Then let's use "uses" it is more assertive. Documentation will cover
>> any vague anything about it .. 
>
> It uses what? And why can't I configure this with "uses FOO" when FOO=m?
> That's not any clearer. And saying that "this is weird but it is 
> described in the documentation" is not good enough. We must make things 
> clear in the first place.
>
> This is really a conditional dependency. That's all this is about.
> So why not simply making it so rather than fooling ourselves? All that 
> is required is an extension that would allow:
>
> 	depends on (expression) if (expression)
>
> This construct should be obvious even without reading the doc, is 
> already used extensively for other things already, and is flexible 
> enough to cover all sort of cases in addition to this particular one.

Okay, you convinced me. Now you only need to convince whoever is doing
the actual work of implementing this stuff. ;)

BR,
Jani.
Nicolas Pitre April 22, 2020, 9:13 p.m. UTC | #18
On Wed, 22 Apr 2020, Jani Nikula wrote:

> On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
> > This is really a conditional dependency. That's all this is about.
> > So why not simply making it so rather than fooling ourselves? All that 
> > is required is an extension that would allow:
> >
> > 	depends on (expression) if (expression)
> >
> > This construct should be obvious even without reading the doc, is 
> > already used extensively for other things already, and is flexible 
> > enough to cover all sort of cases in addition to this particular one.
> 
> Okay, you convinced me. Now you only need to convince whoever is doing
> the actual work of implementing this stuff. ;)

What about this:

----- >8
Subject: [PATCH] kconfig: allow for conditional dependencies

This might appear to be a strange concept, but sometimes we want
a dependency to be conditionally applied. One such case is currently
expressed with:

	depends on FOO || !FOO

This pattern is strange enough to give one's pause. Given that it is
also frequent, let's make the intent more obvious with some syntaxic 
sugar by effectively making dependencies optionally conditional.
This also makes the kconfig language more uniform.

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

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index d0111dd264..0f841e0037 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -114,7 +114,7 @@ applicable everywhere (see syntax).
   This is a shorthand notation for a type definition plus a value.
   Optionally dependencies for this default value can be added with "if".
 
-- dependencies: "depends on" <expr>
+- dependencies: "depends on" <expr> ["if" <expr>]
 
   This defines a dependency for this menu entry. If multiple
   dependencies are defined, they are connected with '&&'. Dependencies
@@ -130,6 +130,16 @@ applicable everywhere (see syntax).
 	bool "foo"
 	default y
 
+  The dependency definition itself may be conditional by appending "if"
+  followed by an expression. If such expression is false (n) then this
+  dependency is ignored. One possible use case is:
+
+    config FOO
+	tristate
+	depends on BAZ if BAZ != n
+
+  meaning that FOO is constrained by the value of BAZ only when it is set.
+
 - reverse dependencies: "select" <symbol> ["if" <expr>]
 
   While normal dependencies reduce the upper limit of a symbol (see
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index d4ca829736..1a9337d1b9 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -72,7 +72,7 @@ void menu_warn(struct menu *menu, const char *fmt, ...);
 struct menu *menu_add_menu(void);
 void menu_end_menu(void);
 void menu_add_entry(struct symbol *sym);
-void menu_add_dep(struct expr *dep);
+void menu_add_dep(struct expr *dep, struct expr *cond);
 void menu_add_visibility(struct expr *dep);
 struct property *menu_add_prompt(enum prop_type type, char *prompt, struct expr *dep);
 void menu_add_expr(enum prop_type type, struct expr *expr, struct expr *dep);
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index e436ba44c9..e6b204225e 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -103,8 +103,22 @@ static struct expr *rewrite_m(struct expr *e)
 	return e;
 }
 
-void menu_add_dep(struct expr *dep)
+void menu_add_dep(struct expr *dep, struct expr *cond)
 {
+	if (cond) {
+		struct expr *cond2, *left, *right;
+		/*
+		 * We have "depends on X if Y" and we want:
+		 *	Y != n --> X
+		 *	Y == n --> y
+		 * Meaning: ((Y != n) && X) || (Y == n)
+		 */
+		cond2 = expr_copy(cond);
+		left = expr_trans_compare(cond2, E_UNEQUAL, &symbol_no);
+		left = expr_alloc_and(dep, left);
+		right = expr_trans_compare(cond, E_EQUAL, &symbol_no);
+		dep = expr_alloc_or(left, right);
+	}
 	current_entry->dep = expr_alloc_and(current_entry->dep, dep);
 }
 
diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y
index 708b6c4b13..4161207da2 100644
--- a/scripts/kconfig/parser.y
+++ b/scripts/kconfig/parser.y
@@ -316,7 +316,7 @@ if_entry: T_IF expr T_EOL
 {
 	printd(DEBUG_PARSE, "%s:%d:if\n", zconf_curname(), zconf_lineno());
 	menu_add_entry(NULL);
-	menu_add_dep($2);
+	menu_add_dep($2, NULL);
 	$$ = menu_add_menu();
 };
 
@@ -412,9 +412,9 @@ help: help_start T_HELPTEXT
 
 /* depends option */
 
-depends: T_DEPENDS T_ON expr T_EOL
+depends: T_DEPENDS T_ON expr if_expr T_EOL
 {
-	menu_add_dep($3);
+	menu_add_dep($3, $4);
 	printd(DEBUG_PARSE, "%s:%d:depends on\n", zconf_curname(), zconf_lineno());
 };
Randy Dunlap April 22, 2020, 10:37 p.m. UTC | #19
On 4/22/20 2:13 PM, Nicolas Pitre wrote:
> On Wed, 22 Apr 2020, Jani Nikula wrote:
> 
>> On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
>>> This is really a conditional dependency. That's all this is about.
>>> So why not simply making it so rather than fooling ourselves? All that 
>>> is required is an extension that would allow:
>>>
>>> 	depends on (expression) if (expression)
>>>
>>> This construct should be obvious even without reading the doc, is 
>>> already used extensively for other things already, and is flexible 
>>> enough to cover all sort of cases in addition to this particular one.
>>
>> Okay, you convinced me. Now you only need to convince whoever is doing
>> the actual work of implementing this stuff. ;)
> 
> What about this:
> 
> ----- >8
> Subject: [PATCH] kconfig: allow for conditional dependencies
> 
> This might appear to be a strange concept, but sometimes we want
> a dependency to be conditionally applied. One such case is currently
> expressed with:
> 
> 	depends on FOO || !FOO
> 
> This pattern is strange enough to give one's pause. Given that it is
> also frequent, let's make the intent more obvious with some syntaxic 
> sugar by effectively making dependencies optionally conditional.
> This also makes the kconfig language more uniform.
> 
> Signed-off-by: Nicolas Pitre <nico@fluxnic.net>

Hi,

If we must do something here, I prefer this one.

Nicolas, would you do another example, specifically for
CRAMFS_MTD in fs/cramfs/Kconfig, please?

thanks.
Nicolas Pitre April 23, 2020, 3:01 p.m. UTC | #20
On Wed, 22 Apr 2020, Randy Dunlap wrote:

> On 4/22/20 2:13 PM, Nicolas Pitre wrote:
> > On Wed, 22 Apr 2020, Jani Nikula wrote:
> > 
> >> On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
> >>> This is really a conditional dependency. That's all this is about.
> >>> So why not simply making it so rather than fooling ourselves? All that 
> >>> is required is an extension that would allow:
> >>>
> >>> 	depends on (expression) if (expression)
> >>>
> >>> This construct should be obvious even without reading the doc, is 
> >>> already used extensively for other things already, and is flexible 
> >>> enough to cover all sort of cases in addition to this particular one.
> >>
> >> Okay, you convinced me. Now you only need to convince whoever is doing
> >> the actual work of implementing this stuff. ;)
> > 
> > What about this:
> > 
> > ----- >8
> > Subject: [PATCH] kconfig: allow for conditional dependencies
> > 
> > This might appear to be a strange concept, but sometimes we want
> > a dependency to be conditionally applied. One such case is currently
> > expressed with:
> > 
> > 	depends on FOO || !FOO
> > 
> > This pattern is strange enough to give one's pause. Given that it is
> > also frequent, let's make the intent more obvious with some syntaxic 
> > sugar by effectively making dependencies optionally conditional.
> > This also makes the kconfig language more uniform.
> > 
> > Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> 
> Hi,
> 
> If we must do something here, I prefer this one.
> 
> Nicolas, would you do another example, specifically for
> CRAMFS_MTD in fs/cramfs/Kconfig, please?

I don't see how that one can be helped. The MTD dependency is not 
optional.


Nicolas
Jason Gunthorpe April 23, 2020, 3:05 p.m. UTC | #21
On Thu, Apr 23, 2020 at 11:01:40AM -0400, Nicolas Pitre wrote:
> On Wed, 22 Apr 2020, Randy Dunlap wrote:
> 
> > On 4/22/20 2:13 PM, Nicolas Pitre wrote:
> > > On Wed, 22 Apr 2020, Jani Nikula wrote:
> > > 
> > >> On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
> > >>> This is really a conditional dependency. That's all this is about.
> > >>> So why not simply making it so rather than fooling ourselves? All that 
> > >>> is required is an extension that would allow:
> > >>>
> > >>> 	depends on (expression) if (expression)
> > >>>
> > >>> This construct should be obvious even without reading the doc, is 
> > >>> already used extensively for other things already, and is flexible 
> > >>> enough to cover all sort of cases in addition to this particular one.
> > >>
> > >> Okay, you convinced me. Now you only need to convince whoever is doing
> > >> the actual work of implementing this stuff. ;)
> > > 
> > > What about this:
> > > 
> > > Subject: [PATCH] kconfig: allow for conditional dependencies
> > > 
> > > This might appear to be a strange concept, but sometimes we want
> > > a dependency to be conditionally applied. One such case is currently
> > > expressed with:
> > > 
> > > 	depends on FOO || !FOO
> > > 
> > > This pattern is strange enough to give one's pause. Given that it is
> > > also frequent, let's make the intent more obvious with some syntaxic 
> > > sugar by effectively making dependencies optionally conditional.
> > > This also makes the kconfig language more uniform.
> > > 
> > > Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> > 
> > Hi,
> > 
> > If we must do something here, I prefer this one.
> > 
> > Nicolas, would you do another example, specifically for
> > CRAMFS_MTD in fs/cramfs/Kconfig, please?
> 
> I don't see how that one can be helped. The MTD dependency is not 
> optional.

Could it be done as 

config MTD
   depends on CRAMFS if CRAMFS_MTD

?

Jason
Nicolas Pitre April 23, 2020, 3:11 p.m. UTC | #22
On Thu, 23 Apr 2020, Jason Gunthorpe wrote:

> On Thu, Apr 23, 2020 at 11:01:40AM -0400, Nicolas Pitre wrote:
> > On Wed, 22 Apr 2020, Randy Dunlap wrote:
> > 
> > > On 4/22/20 2:13 PM, Nicolas Pitre wrote:
> > > > On Wed, 22 Apr 2020, Jani Nikula wrote:
> > > > 
> > > >> On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
> > > >>> This is really a conditional dependency. That's all this is about.
> > > >>> So why not simply making it so rather than fooling ourselves? All that 
> > > >>> is required is an extension that would allow:
> > > >>>
> > > >>> 	depends on (expression) if (expression)
> > > >>>
> > > >>> This construct should be obvious even without reading the doc, is 
> > > >>> already used extensively for other things already, and is flexible 
> > > >>> enough to cover all sort of cases in addition to this particular one.
> > > >>
> > > >> Okay, you convinced me. Now you only need to convince whoever is doing
> > > >> the actual work of implementing this stuff. ;)
> > > > 
> > > > What about this:
> > > > 
> > > > Subject: [PATCH] kconfig: allow for conditional dependencies
> > > > 
> > > > This might appear to be a strange concept, but sometimes we want
> > > > a dependency to be conditionally applied. One such case is currently
> > > > expressed with:
> > > > 
> > > > 	depends on FOO || !FOO
> > > > 
> > > > This pattern is strange enough to give one's pause. Given that it is
> > > > also frequent, let's make the intent more obvious with some syntaxic 
> > > > sugar by effectively making dependencies optionally conditional.
> > > > This also makes the kconfig language more uniform.
> > > > 
> > > > Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> > > 
> > > Hi,
> > > 
> > > If we must do something here, I prefer this one.
> > > 
> > > Nicolas, would you do another example, specifically for
> > > CRAMFS_MTD in fs/cramfs/Kconfig, please?
> > 
> > I don't see how that one can be helped. The MTD dependency is not 
> > optional.
> 
> Could it be done as 
> 
> config MTD
>    depends on CRAMFS if CRAMFS_MTD
> 
> ?

No. There is no logic in restricting MTD usage based on CRAMFS or 
CRAMFS_MTD.


Nicolas
Jason Gunthorpe April 23, 2020, 3:16 p.m. UTC | #23
On Thu, Apr 23, 2020 at 11:11:46AM -0400, Nicolas Pitre wrote:
> On Thu, 23 Apr 2020, Jason Gunthorpe wrote:
> 
> > On Thu, Apr 23, 2020 at 11:01:40AM -0400, Nicolas Pitre wrote:
> > > On Wed, 22 Apr 2020, Randy Dunlap wrote:
> > > 
> > > > On 4/22/20 2:13 PM, Nicolas Pitre wrote:
> > > > > On Wed, 22 Apr 2020, Jani Nikula wrote:
> > > > > 
> > > > >> On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
> > > > >>> This is really a conditional dependency. That's all this is about.
> > > > >>> So why not simply making it so rather than fooling ourselves? All that 
> > > > >>> is required is an extension that would allow:
> > > > >>>
> > > > >>> 	depends on (expression) if (expression)
> > > > >>>
> > > > >>> This construct should be obvious even without reading the doc, is 
> > > > >>> already used extensively for other things already, and is flexible 
> > > > >>> enough to cover all sort of cases in addition to this particular one.
> > > > >>
> > > > >> Okay, you convinced me. Now you only need to convince whoever is doing
> > > > >> the actual work of implementing this stuff. ;)
> > > > > 
> > > > > What about this:
> > > > > 
> > > > > Subject: [PATCH] kconfig: allow for conditional dependencies
> > > > > 
> > > > > This might appear to be a strange concept, but sometimes we want
> > > > > a dependency to be conditionally applied. One such case is currently
> > > > > expressed with:
> > > > > 
> > > > > 	depends on FOO || !FOO
> > > > > 
> > > > > This pattern is strange enough to give one's pause. Given that it is
> > > > > also frequent, let's make the intent more obvious with some syntaxic 
> > > > > sugar by effectively making dependencies optionally conditional.
> > > > > This also makes the kconfig language more uniform.
> > > > > 
> > > > > Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> > > > 
> > > > Hi,
> > > > 
> > > > If we must do something here, I prefer this one.
> > > > 
> > > > Nicolas, would you do another example, specifically for
> > > > CRAMFS_MTD in fs/cramfs/Kconfig, please?
> > > 
> > > I don't see how that one can be helped. The MTD dependency is not 
> > > optional.
> > 
> > Could it be done as 
> > 
> > config MTD
> >    depends on CRAMFS if CRAMFS_MTD
> > 
> > ?
> 
> No. There is no logic in restricting MTD usage based on CRAMFS or 
> CRAMFS_MTD.

Ah, I got it backwards, maybe this:

config CRAMFS
   depends on MTD if CRAMFS_MTD

?

Jason
Arnd Bergmann April 23, 2020, 3:28 p.m. UTC | #24
On Thu, Apr 23, 2020 at 5:16 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Thu, Apr 23, 2020 at 11:11:46AM -0400, Nicolas Pitre wrote:

> > > > I don't see how that one can be helped. The MTD dependency is not
> > > > optional.
> > >
> > > Could it be done as
> > >
> > > config MTD
> > >    depends on CRAMFS if CRAMFS_MTD
> > >
> > > ?
> >
> > No. There is no logic in restricting MTD usage based on CRAMFS or
> > CRAMFS_MTD.
>
> Ah, I got it backwards, maybe this:
>
> config CRAMFS
>    depends on MTD if CRAMFS_MTD

I'm not sure this can work if you also have the requirement that 'CRAMFS_MTD
depends on CRAMFS': dependencies in Kconfig generally cannot have
loops in them.

       Arnd
Nicolas Pitre April 23, 2020, 3:33 p.m. UTC | #25
On Thu, 23 Apr 2020, Jason Gunthorpe wrote:

> On Thu, Apr 23, 2020 at 11:11:46AM -0400, Nicolas Pitre wrote:
> > On Thu, 23 Apr 2020, Jason Gunthorpe wrote:
> > 
> > > On Thu, Apr 23, 2020 at 11:01:40AM -0400, Nicolas Pitre wrote:
> > > > On Wed, 22 Apr 2020, Randy Dunlap wrote:
> > > > 
> > > > > On 4/22/20 2:13 PM, Nicolas Pitre wrote:
> > > > > > On Wed, 22 Apr 2020, Jani Nikula wrote:
> > > > > > 
> > > > > >> On Tue, 21 Apr 2020, Nicolas Pitre <nico@fluxnic.net> wrote:
> > > > > >>> This is really a conditional dependency. That's all this is about.
> > > > > >>> So why not simply making it so rather than fooling ourselves? All that 
> > > > > >>> is required is an extension that would allow:
> > > > > >>>
> > > > > >>> 	depends on (expression) if (expression)
> > > > > >>>
> > > > > >>> This construct should be obvious even without reading the doc, is 
> > > > > >>> already used extensively for other things already, and is flexible 
> > > > > >>> enough to cover all sort of cases in addition to this particular one.
> > > > > >>
> > > > > >> Okay, you convinced me. Now you only need to convince whoever is doing
> > > > > >> the actual work of implementing this stuff. ;)
> > > > > > 
> > > > > > What about this:
> > > > > > 
> > > > > > Subject: [PATCH] kconfig: allow for conditional dependencies
> > > > > > 
> > > > > > This might appear to be a strange concept, but sometimes we want
> > > > > > a dependency to be conditionally applied. One such case is currently
> > > > > > expressed with:
> > > > > > 
> > > > > > 	depends on FOO || !FOO
> > > > > > 
> > > > > > This pattern is strange enough to give one's pause. Given that it is
> > > > > > also frequent, let's make the intent more obvious with some syntaxic 
> > > > > > sugar by effectively making dependencies optionally conditional.
> > > > > > This also makes the kconfig language more uniform.
> > > > > > 
> > > > > > Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > If we must do something here, I prefer this one.
> > > > > 
> > > > > Nicolas, would you do another example, specifically for
> > > > > CRAMFS_MTD in fs/cramfs/Kconfig, please?
> > > > 
> > > > I don't see how that one can be helped. The MTD dependency is not 
> > > > optional.
> > > 
> > > Could it be done as 
> > > 
> > > config MTD
> > >    depends on CRAMFS if CRAMFS_MTD
> > > 
> > > ?
> > 
> > No. There is no logic in restricting MTD usage based on CRAMFS or 
> > CRAMFS_MTD.
> 
> Ah, I got it backwards, maybe this:
> 
> config CRAMFS
>    depends on MTD if CRAMFS_MTD
> 
> ?

Still half-backward. CRAMFS should not depend on either MTD nor 
CRAMFS_MTD.

It is CRAMFS_MTD that needs both CRAMFS and MTD. 
Furthermore CRAMFS_MTD can't be built-in if MTD is modular.


Nicolas
Jason Gunthorpe April 23, 2020, 6:30 p.m. UTC | #26
On Thu, Apr 23, 2020 at 11:33:33AM -0400, Nicolas Pitre wrote:
> > > No. There is no logic in restricting MTD usage based on CRAMFS or 
> > > CRAMFS_MTD.
> > 
> > Ah, I got it backwards, maybe this:
> > 
> > config CRAMFS
> >    depends on MTD if CRAMFS_MTD
> > 
> > ?
> 
> Still half-backward. CRAMFS should not depend on either MTD nor
> CRAMFS_MTD.

Well, I would view this the same as all the other cases.. the CRAMFS
module has an optional ability consume symbols from MTD.  Here that is
controlled by another 'CRAMFS_MTD' selection, but it should still
settle it out the same way as other cases like this - ie CRAMFS is
restricted to m if MTD is m

Arnd's point that kconfig is acyclic does kill it though :(

> It is CRAMFS_MTD that needs both CRAMFS and MTD.
> Furthermore CRAMFS_MTD can't be built-in if MTD is modular.

CRAMFS_MTD is a bool feature flag for the CRAMFS tristate - it is
CRAMFS that can't be built in if MTD is modular.

Jason
Nicolas Pitre April 23, 2020, 6:52 p.m. UTC | #27
On Thu, 23 Apr 2020, Jason Gunthorpe wrote:

> On Thu, Apr 23, 2020 at 11:33:33AM -0400, Nicolas Pitre wrote:
> > > > No. There is no logic in restricting MTD usage based on CRAMFS or 
> > > > CRAMFS_MTD.
> > > 
> > > Ah, I got it backwards, maybe this:
> > > 
> > > config CRAMFS
> > >    depends on MTD if CRAMFS_MTD
> > > 
> > > ?
> > 
> > Still half-backward. CRAMFS should not depend on either MTD nor
> > CRAMFS_MTD.
> 
> Well, I would view this the same as all the other cases.. the CRAMFS
> module has an optional ability consume symbols from MTD.  Here that is
> controlled by another 'CRAMFS_MTD' selection, but it should still
> settle it out the same way as other cases like this - ie CRAMFS is
> restricted to m if MTD is m
> 
> Arnd's point that kconfig is acyclic does kill it though :(
> 
> > It is CRAMFS_MTD that needs both CRAMFS and MTD.
> > Furthermore CRAMFS_MTD can't be built-in if MTD is modular.
> 
> CRAMFS_MTD is a bool feature flag for the CRAMFS tristate - it is
> CRAMFS that can't be built in if MTD is modular.

Not exactly. CRAMFS still can be built irrespective of MTD. It is only 
the XIP part of CRAMFS relying on MTD that is restricted.


Nicolas

Patch
diff mbox series

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index a1601ec3317b..8db8c2d80794 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -130,6 +130,16 @@  applicable everywhere (see syntax).
 	bool "foo"
 	default y
 
+- uses dependencies: "uses" <symbol>
+
+  Equivalent to: depends on symbol || !symbol
+  Semantically it means, if FOO is enabled (y/m) and has the option:
+  uses BAR, make sure it can reach/use BAR when possible.
+  For example: if FOO=y and BAR=m, FOO will be forced to m.
+
+  Note:
+      To understand how (symbol || !symbol) is actually computed, please see `Menu dependencies`_
+
 - reverse dependencies: "select" <symbol> ["if" <expr>]
 
   While normal dependencies reduce the upper limit of a symbol (see
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 5c3443692f34..face672fb4b4 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -185,6 +185,7 @@  enum prop_type {
 	P_CHOICE,   /* choice value */
 	P_SELECT,   /* select BAR */
 	P_IMPLY,    /* imply BAR */
+	P_USES,     /* uses BAR */
 	P_RANGE,    /* range 7..100 (for a symbol) */
 	P_SYMBOL,   /* where a symbol is defined */
 };
diff --git a/scripts/kconfig/lexer.l b/scripts/kconfig/lexer.l
index 6354c905b006..c6a0017b10d4 100644
--- a/scripts/kconfig/lexer.l
+++ b/scripts/kconfig/lexer.l
@@ -102,6 +102,7 @@  n	[A-Za-z0-9_-]
 "default"		return T_DEFAULT;
 "defconfig_list"	return T_DEFCONFIG_LIST;
 "depends"		return T_DEPENDS;
+"uses"			return T_USES;
 "endchoice"		return T_ENDCHOICE;
 "endif"			return T_ENDIF;
 "endmenu"		return T_ENDMENU;
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index e436ba44c9c5..e26161b31a11 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -274,7 +274,9 @@  static void sym_check_prop(struct symbol *sym)
 			break;
 		case P_SELECT:
 		case P_IMPLY:
-			use = prop->type == P_SELECT ? "select" : "imply";
+		case P_USES:
+			use = prop->type == P_SELECT ? "select" :
+				prop->type == P_IMPLY ? "imply" : "uses";
 			sym2 = prop_get_symbol(prop);
 			if (sym->type != S_BOOLEAN && sym->type != S_TRISTATE)
 				prop_warn(prop,
diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y
index 708b6c4b13ca..c5e9abb49d29 100644
--- a/scripts/kconfig/parser.y
+++ b/scripts/kconfig/parser.y
@@ -57,6 +57,7 @@  static struct menu *current_menu, *current_entry;
 %token T_DEF_BOOL
 %token T_DEF_TRISTATE
 %token T_DEPENDS
+%token T_USES
 %token T_ENDCHOICE
 %token T_ENDIF
 %token T_ENDMENU
@@ -169,6 +170,7 @@  config_option_list:
 	  /* empty */
 	| config_option_list config_option
 	| config_option_list depends
+	| config_option_list uses
 	| config_option_list help
 ;
 
@@ -261,6 +263,7 @@  choice_option_list:
 	  /* empty */
 	| choice_option_list choice_option
 	| choice_option_list depends
+	| choice_option_list uses
 	| choice_option_list help
 ;
 
@@ -360,6 +363,7 @@  menu_option_list:
 	  /* empty */
 	| menu_option_list visible
 	| menu_option_list depends
+	| menu_option_list uses
 ;
 
 source_stmt: T_SOURCE T_WORD_QUOTE T_EOL
@@ -384,6 +388,7 @@  comment_stmt: comment comment_option_list
 comment_option_list:
 	  /* empty */
 	| comment_option_list depends
+	| comment_option_list uses
 ;
 
 /* help option */
@@ -418,6 +423,16 @@  depends: T_DEPENDS T_ON expr T_EOL
 	printd(DEBUG_PARSE, "%s:%d:depends on\n", zconf_curname(), zconf_lineno());
 };
 
+/* uses symbol: depends on symbol || !symbol */
+uses: T_USES symbol T_EOL
+{
+	struct expr *symexpr = expr_alloc_symbol($2);
+
+	menu_add_dep(expr_alloc_two(E_OR, symexpr, expr_alloc_one(E_NOT, symexpr)));
+	printd(DEBUG_PARSE, "%s:%d: uses: depends on %s || ! %s\n",
+	       zconf_curname(), zconf_lineno(), $2->name, $2->name);
+};
+
 /* visibility option */
 visible: T_VISIBLE if_expr T_EOL
 {
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 3dc81397d003..422f7ea47722 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -1295,6 +1295,8 @@  const char *prop_get_type_name(enum prop_type type)
 		return "choice";
 	case P_SELECT:
 		return "select";
+	case P_USES:
+		return "uses";
 	case P_IMPLY:
 		return "imply";
 	case P_RANGE: