| Message ID | 20200417011146.83973-1-saeedm@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/2] Kconfig: Introduce "uses" keyword | expand |
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:
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
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.
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
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
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
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
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
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?"
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.
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
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.
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. >
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
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
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
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.
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()); };
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.
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
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
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
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
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
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
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
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
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:
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(-)