The 'imply' statement may create unmet direct dependency when the
implied symbol depends on m.
[Test Code]
config FOO
tristate "foo"
imply BAZ
config BAZ
tristate "baz"
depends on BAR
config BAR
def_tristate m
config MODULES
def_bool y
option modules
If you set FOO=y, BAZ is also promoted to y, which results in the
following .config file:
CONFIG_FOO=y
CONFIG_BAZ=y
CONFIG_BAR=m
CONFIG_MODULES=y
This ignores the dependency "BAZ depends on BAR".
Unlike 'select', what is worse, Kconfig never shows the
"WARNING: unmet direct dependencies detected for ..." for this case.
Because 'imply' should be weaker than 'depends on', Kconfig should
take the direct dependency into account.
Describe this case in Documentation/kbuild/kconfig-language.rst for
clarification.
Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
a symbol implied by y is restricted to y or n, excluding m.
As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
possible value is BAZ=n.
Having said that, this case was probably "We don't care" at that time
because Kconfig did not handle 'depends on m' correctly until
commit f622f8279581 ("kconfig: warn unmet direct dependency of tristate
symbols selected by y") fixed it.
Backporting this to 4.19+ will probably be fine. If you care this
problem on 4.14.x, you need to backport f622f8279581 as well.
Signed-off-by: Masahiro Yamada <[email protected]>
---
Documentation/kbuild/kconfig-language.rst | 7 +++++--
scripts/kconfig/symbol.c | 17 +++++++++++++----
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index d0111dd26410..9141f03ff744 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -159,11 +159,11 @@ applicable everywhere (see syntax).
Given the following example::
config FOO
- tristate
+ tristate "foo"
imply BAZ
config BAZ
- tristate
+ tristate "baz"
depends on BAR
The following values are possible:
@@ -174,6 +174,9 @@ applicable everywhere (see syntax).
n y n N/m/y
m y m M/y/n
y y y Y/n
+ n m n N/m
+ m m m M/n
+ y m n N
y n * N
=== === ============= ==============
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 8d38b700b314..6100e6ead8ad 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -221,7 +221,7 @@ static void sym_calc_visibility(struct symbol *sym)
sym_set_changed(sym);
}
tri = no;
- if (sym->implied.expr && sym->dir_dep.tri != no)
+ if (sym->implied.expr)
tri = expr_calc_value(sym->implied.expr);
if (tri == mod && sym_get_type(sym) == S_BOOLEAN)
tri = yes;
@@ -393,7 +393,17 @@ void sym_calc_value(struct symbol *sym)
}
if (sym->implied.tri != no) {
sym->flags |= SYMBOL_WRITE;
- newval.tri = EXPR_OR(newval.tri, sym->implied.tri);
+ newval.tri = EXPR_OR(newval.tri,
+ sym->implied.tri);
+ newval.tri = EXPR_AND(newval.tri,
+ sym->dir_dep.tri);
+
+ if (newval.tri == mod && sym->implied.tri == yes) {
+ if (sym->dir_dep.tri == yes)
+ newval.tri = yes;
+ else
+ newval.tri = no;
+ }
}
}
calc_newval:
@@ -401,8 +411,7 @@ void sym_calc_value(struct symbol *sym)
sym_warn_unmet_dep(sym);
newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri);
}
- if (newval.tri == mod &&
- (sym_get_type(sym) == S_BOOLEAN || sym->implied.tri == yes))
+ if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN)
newval.tri = yes;
break;
case S_STRING:
--
2.17.1
Hi Yamada-san,
On Wed, Feb 19, 2020 at 8:51 AM Masahiro Yamada <[email protected]> wrote:
> The 'imply' statement may create unmet direct dependency when the
> implied symbol depends on m.
>
> [Test Code]
>
> config FOO
> tristate "foo"
> imply BAZ
>
> config BAZ
> tristate "baz"
> depends on BAR
>
> config BAR
> def_tristate m
>
> config MODULES
> def_bool y
> option modules
>
> If you set FOO=y, BAZ is also promoted to y, which results in the
> following .config file:
>
> CONFIG_FOO=y
> CONFIG_BAZ=y
> CONFIG_BAR=m
> CONFIG_MODULES=y
>
> This ignores the dependency "BAZ depends on BAR".
>
> Unlike 'select', what is worse, Kconfig never shows the
> "WARNING: unmet direct dependencies detected for ..." for this case.
>
> Because 'imply' should be weaker than 'depends on', Kconfig should
> take the direct dependency into account.
>
> Describe this case in Documentation/kbuild/kconfig-language.rst for
> clarification.
>
> Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
> a symbol implied by y is restricted to y or n, excluding m.
>
> As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> possible value is BAZ=n.
>
> Having said that, this case was probably "We don't care" at that time
> because Kconfig did not handle 'depends on m' correctly until
> commit f622f8279581 ("kconfig: warn unmet direct dependency of tristate
> symbols selected by y") fixed it.
>
> Backporting this to 4.19+ will probably be fine. If you care this
> problem on 4.14.x, you need to backport f622f8279581 as well.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Thanks a lot! This fixes the build issues in
https://lore.kernel.org/alsa-devel/CAMuHMdW8SvDgQJyenTtEm4Xn2Ma6PK9pfwKR2_gn60t2AqNWXg@mail.gmail.com/
Tested-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Feb 19, 2020 at 5:42 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Yamada-san,
>
> On Wed, Feb 19, 2020 at 8:51 AM Masahiro Yamada <[email protected]> wrote:
> > The 'imply' statement may create unmet direct dependency when the
> > implied symbol depends on m.
> >
> > [Test Code]
> >
> > config FOO
> > tristate "foo"
> > imply BAZ
> >
> > config BAZ
> > tristate "baz"
> > depends on BAR
> >
> > config BAR
> > def_tristate m
> >
> > config MODULES
> > def_bool y
> > option modules
> >
> > If you set FOO=y, BAZ is also promoted to y, which results in the
> > following .config file:
> >
> > CONFIG_FOO=y
> > CONFIG_BAZ=y
> > CONFIG_BAR=m
> > CONFIG_MODULES=y
> >
> > This ignores the dependency "BAZ depends on BAR".
> >
> > Unlike 'select', what is worse, Kconfig never shows the
> > "WARNING: unmet direct dependencies detected for ..." for this case.
> >
> > Because 'imply' should be weaker than 'depends on', Kconfig should
> > take the direct dependency into account.
> >
> > Describe this case in Documentation/kbuild/kconfig-language.rst for
> > clarification.
> >
> > Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
> > a symbol implied by y is restricted to y or n, excluding m.
> >
> > As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> > by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> > possible value is BAZ=n.
> >
> > Having said that, this case was probably "We don't care" at that time
> > because Kconfig did not handle 'depends on m' correctly until
> > commit f622f8279581 ("kconfig: warn unmet direct dependency of tristate
> > symbols selected by y") fixed it.
> >
> > Backporting this to 4.19+ will probably be fine. If you care this
> > problem on 4.14.x, you need to backport f622f8279581 as well.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
>
> Thanks a lot! This fixes the build issues in
> https://lore.kernel.org/alsa-devel/CAMuHMdW8SvDgQJyenTtEm4Xn2Ma6PK9pfwKR2_gn60t2AqNWXg@mail.gmail.com/
>
> Tested-by: Geert Uytterhoeven <[email protected]>
>
Even if this patch fixes the build issues,
the result may not be what you expect.
The very subtle part of 'imply' is that
symbols implied by 'y' cannot be 'm'.
When CONFIG_SND_SOC_ALL_CODECS=y,
the select'ed drivers were previously able to be 'm'.
After ea00d95200d02, drivers that were previously
'm' are now 'n'.
Probably, it shrunk the test-coverage.
I'd recommend to compare the .config file.
--
Best Regards
Masahiro Yamada
Hi Yamada-san,
On Wed, Feb 19, 2020 at 10:24 AM Masahiro Yamada <[email protected]> wrote:
> On Wed, Feb 19, 2020 at 5:42 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Wed, Feb 19, 2020 at 8:51 AM Masahiro Yamada <[email protected]> wrote:
> > > The 'imply' statement may create unmet direct dependency when the
> > > implied symbol depends on m.
> > >
> > > [Test Code]
> > >
> > > config FOO
> > > tristate "foo"
> > > imply BAZ
> > >
> > > config BAZ
> > > tristate "baz"
> > > depends on BAR
> > >
> > > config BAR
> > > def_tristate m
> > >
> > > config MODULES
> > > def_bool y
> > > option modules
> > >
> > > If you set FOO=y, BAZ is also promoted to y, which results in the
> > > following .config file:
> > >
> > > CONFIG_FOO=y
> > > CONFIG_BAZ=y
> > > CONFIG_BAR=m
> > > CONFIG_MODULES=y
> > >
> > > This ignores the dependency "BAZ depends on BAR".
> > >
> > > Unlike 'select', what is worse, Kconfig never shows the
> > > "WARNING: unmet direct dependencies detected for ..." for this case.
> > >
> > > Because 'imply' should be weaker than 'depends on', Kconfig should
> > > take the direct dependency into account.
> > >
> > > Describe this case in Documentation/kbuild/kconfig-language.rst for
> > > clarification.
> > >
> > > Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
> > > a symbol implied by y is restricted to y or n, excluding m.
> > >
> > > As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> > > by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> > > possible value is BAZ=n.
> > >
> > > Having said that, this case was probably "We don't care" at that time
> > > because Kconfig did not handle 'depends on m' correctly until
> > > commit f622f8279581 ("kconfig: warn unmet direct dependency of tristate
> > > symbols selected by y") fixed it.
> > >
> > > Backporting this to 4.19+ will probably be fine. If you care this
> > > problem on 4.14.x, you need to backport f622f8279581 as well.
> > >
> > > Signed-off-by: Masahiro Yamada <[email protected]>
> >
> > Thanks a lot! This fixes the build issues in
> > https://lore.kernel.org/alsa-devel/CAMuHMdW8SvDgQJyenTtEm4Xn2Ma6PK9pfwKR2_gn60t2AqNWXg@mail.gmail.com/
> >
> > Tested-by: Geert Uytterhoeven <[email protected]>
>
> Even if this patch fixes the build issues,
> the result may not be what you expect.
>
> The very subtle part of 'imply' is that
> symbols implied by 'y' cannot be 'm'.
>
> When CONFIG_SND_SOC_ALL_CODECS=y,
> the select'ed drivers were previously able to be 'm'.
>
> After ea00d95200d02, drivers that were previously
> 'm' are now 'n'.
>
> Probably, it shrunk the test-coverage.
>
> I'd recommend to compare the .config file.
I always do, when running make oldconfig ;-)
I only see expected changes from y to m.
I don't see any unexpected changes from m to n.
I do see a few unexpected "CONFIG_<foo> is not set" that are no longer
suppressed.
--- .config.orig 2020-02-19 09:26:41.411812042 +0100
+++ .config 2020-02-19 09:28:05.143822097 +0100
@@ -1193,7 +1193,7 @@
CONFIG_REGMAP=y
CONFIG_REGMAP_AC97=y
CONFIG_REGMAP_I2C=y
-CONFIG_REGMAP_SLIMBUS=y
+CONFIG_REGMAP_SLIMBUS=m
CONFIG_REGMAP_SPI=y
CONFIG_REGMAP_MMIO=y
CONFIG_REGMAP_IRQ=y
@@ -5235,7 +5235,7 @@
CONFIG_SND_SOC_UDA1334=y
CONFIG_SND_SOC_UDA134X=y
CONFIG_SND_SOC_UDA1380=y
CONFIG_SND_SOC_WCD934X=m
CONFIG_SND_SOC_WL1273=y
CONFIG_SND_SOC_WM0010=y
@@ -5017,6 +5017,7 @@
CONFIG_SND_SOC_ARIZONA=y
CONFIG_SND_SOC_WM_HUBS=y
CONFIG_SND_SOC_WM_ADSP=y
+# CONFIG_SND_SOC_AB8500_CODEC is not set
CONFIG_SND_SOC_AC97_CODEC=y
CONFIG_SND_SOC_AD1836=y
CONFIG_SND_SOC_AD193X=y
@@ -5085,6 +5086,7 @@
CONFIG_SND_SOC_CS4341=y
CONFIG_SND_SOC_CS4349=y
CONFIG_SND_SOC_CS47L15=y
+# CONFIG_SND_SOC_CS47L24 is not set
CONFIG_SND_SOC_CS47L35=y
CONFIG_SND_SOC_CS47L85=y
CONFIG_SND_SOC_CS47L90=y
@@ -5117,6 +5119,7 @@
CONFIG_SND_SOC_INNO_RK3036=y
CONFIG_SND_SOC_ISABELLE=y
CONFIG_SND_SOC_LM49453=y
+# CONFIG_SND_SOC_LOCHNAGAR_SC is not set
CONFIG_SND_SOC_MADERA=y
CONFIG_SND_SOC_MAX98088=y
CONFIG_SND_SOC_MAX98090=y
@@ -5228,11 +5231,12 @@
CONFIG_SND_SOC_TSCS42XX=y
CONFIG_SND_SOC_TSCS454=y
CONFIG_SND_SOC_TWL4030=y
+# CONFIG_SND_SOC_TWL6040 is not set
CONFIG_SND_SOC_UDA1334=y
CONFIG_SND_SOC_UDA134X=y
CONFIG_SND_SOC_UDA1380=y
-CONFIG_SND_SOC_WCD9335=y
-CONFIG_SND_SOC_WCD934X=y
+CONFIG_SND_SOC_WCD9335=m
+CONFIG_SND_SOC_WCD934X=m
CONFIG_SND_SOC_WL1273=y
CONFIG_SND_SOC_WM0010=y
CONFIG_SND_SOC_WM1250_EV1=y
@@ -5241,6 +5245,8 @@
CONFIG_SND_SOC_WM5100=y
CONFIG_SND_SOC_WM5102=m
CONFIG_SND_SOC_WM5110=y
+# CONFIG_SND_SOC_WM8350 is not set
+# CONFIG_SND_SOC_WM8400 is not set
CONFIG_SND_SOC_WM8510=y
CONFIG_SND_SOC_WM8523=y
CONFIG_SND_SOC_WM8524=y
@@ -5293,7 +5299,7 @@
CONFIG_SND_SOC_MAX9768=y
CONFIG_SND_SOC_MAX9877=y
# CONFIG_SND_SOC_DUMMY is not set
-CONFIG_SND_SOC_MC13783=y
+CONFIG_SND_SOC_MC13783=m
CONFIG_SND_SOC_ML26124=y
CONFIG_SND_SOC_MT6351=y
CONFIG_SND_SOC_MT6358=y
@@ -5899,6 +5905,7 @@
# CONFIG_COMMON_CLK_MT8516 is not set
# end of Clock driver for MediaTek SoC
+# CONFIG_COMMON_CLK_AXG_AUDIO is not set
CONFIG_QCOM_GDSC=y
CONFIG_COMMON_CLK_QCOM=m
CONFIG_QCOM_A53PLL=m
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 19 Feb 2020, Masahiro Yamada wrote:
> The 'imply' statement may create unmet direct dependency when the
> implied symbol depends on m.
>
> [Test Code]
>
> config FOO
> tristate "foo"
> imply BAZ
>
> config BAZ
> tristate "baz"
> depends on BAR
>
> config BAR
> def_tristate m
>
> config MODULES
> def_bool y
> option modules
>
> If you set FOO=y, BAZ is also promoted to y, which results in the
> following .config file:
>
> CONFIG_FOO=y
> CONFIG_BAZ=y
> CONFIG_BAR=m
> CONFIG_MODULES=y
>
> This ignores the dependency "BAZ depends on BAR".
>
> Unlike 'select', what is worse, Kconfig never shows the
> "WARNING: unmet direct dependencies detected for ..." for this case.
>
> Because 'imply' should be weaker than 'depends on', Kconfig should
> take the direct dependency into account.
>
> Describe this case in Documentation/kbuild/kconfig-language.rst for
> clarification.
>
> Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
> a symbol implied by y is restricted to y or n, excluding m.
>
> As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> possible value is BAZ=n.
I don't think this is right. The imply keyword provide influence over
another symbol but it should not impose any restrictions. If BAR=m then
BAZ should still be allowed to be m or n.
> @@ -174,6 +174,9 @@ applicable everywhere (see syntax).
> n y n N/m/y
> m y m M/y/n
> y y y Y/n
> + n m n N/m
> + m m m M/n
> + y m n N
Here the last line shoule be y m n N/m.
Generally speaking, the code enabled by FOO may rely on functionalities
provided by BAZ only when BAZ >= FOO. This is accomplished with
IS_REACHABLE():
foo_init()
{
if (IS_REACHABLE(CONFIG_BAZ))
baz_register(&foo);
...
}
So if FOO=y and BAZ=m then IS_REACHABLE(CONFIG_BAZ) will be false. Maybe
adding a note to that effect linked to the "y m n N/m" line in the table
would be a good idea.
Nicolas
Hi Nicolas,
On Thu, Feb 20, 2020 at 1:16 AM Nicolas Pitre <[email protected]> wrote:
>
> On Wed, 19 Feb 2020, Masahiro Yamada wrote:
>
> > The 'imply' statement may create unmet direct dependency when the
> > implied symbol depends on m.
> >
> > [Test Code]
> >
> > config FOO
> > tristate "foo"
> > imply BAZ
> >
> > config BAZ
> > tristate "baz"
> > depends on BAR
> >
> > config BAR
> > def_tristate m
> >
> > config MODULES
> > def_bool y
> > option modules
> >
> > If you set FOO=y, BAZ is also promoted to y, which results in the
> > following .config file:
> >
> > CONFIG_FOO=y
> > CONFIG_BAZ=y
> > CONFIG_BAR=m
> > CONFIG_MODULES=y
> >
> > This ignores the dependency "BAZ depends on BAR".
> >
> > Unlike 'select', what is worse, Kconfig never shows the
> > "WARNING: unmet direct dependencies detected for ..." for this case.
> >
> > Because 'imply' should be weaker than 'depends on', Kconfig should
> > take the direct dependency into account.
> >
> > Describe this case in Documentation/kbuild/kconfig-language.rst for
> > clarification.
> >
> > Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
> > a symbol implied by y is restricted to y or n, excluding m.
> >
> > As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> > by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> > possible value is BAZ=n.
>
> I don't think this is right. The imply keyword provide influence over
> another symbol but it should not impose any restrictions. If BAR=m then
> BAZ should still be allowed to be m or n.
>
> > @@ -174,6 +174,9 @@ applicable everywhere (see syntax).
> > n y n N/m/y
> > m y m M/y/n
> > y y y Y/n
> > + n m n N/m
> > + m m m M/n
> > + y m n N
>
> Here the last line shoule be y m n N/m.
>
> Generally speaking, the code enabled by FOO may rely on functionalities
> provided by BAZ only when BAZ >= FOO. This is accomplished with
> IS_REACHABLE():
>
> foo_init()
> {
> if (IS_REACHABLE(CONFIG_BAZ))
> baz_register(&foo);
> ...
> }
>
> So if FOO=y and BAZ=m then IS_REACHABLE(CONFIG_BAZ) will be false. Maybe
> adding a note to that effect linked to the "y m n N/m" line in the table
> would be a good idea.
>
I also thought so.
I agree IS_REACHABLE() is much saner approach.
So, do you agree to change the current behavior
as follows?
index d0111dd26410..47dbfd1ee003 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -173,7 +173,7 @@ applicable everywhere (see syntax).
=== === ============= ==============
n y n N/m/y
m y m M/y/n
- y y y Y/n
+ y y y Y/m/n
y n * N
=== === ============= ==============
--
Best Regards
Masahiro Yamada
On Thu, 20 Feb 2020, Masahiro Yamada wrote:
> Hi Nicolas,
>
> On Thu, Feb 20, 2020 at 1:16 AM Nicolas Pitre <[email protected]> wrote:
> >
> > On Wed, 19 Feb 2020, Masahiro Yamada wrote:
> >
> > > As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> > > by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> > > possible value is BAZ=n.
> >
> > I don't think this is right. The imply keyword provide influence over
> > another symbol but it should not impose any restrictions. If BAR=m then
> > BAZ should still be allowed to be m or n.
> >
> > > @@ -174,6 +174,9 @@ applicable everywhere (see syntax).
> > > n y n N/m/y
> > > m y m M/y/n
> > > y y y Y/n
> > > + n m n N/m
> > > + m m m M/n
> > > + y m n N
> >
> > Here the last line shoule be y m n N/m.
> >
> > Generally speaking, the code enabled by FOO may rely on functionalities
> > provided by BAZ only when BAZ >= FOO. This is accomplished with
> > IS_REACHABLE():
> >
> > foo_init()
> > {
> > if (IS_REACHABLE(CONFIG_BAZ))
> > baz_register(&foo);
> > ...
> > }
> >
> > So if FOO=y and BAZ=m then IS_REACHABLE(CONFIG_BAZ) will be false. Maybe
> > adding a note to that effect linked to the "y m n N/m" line in the table
> > would be a good idea.
> >
>
> I also thought so.
>
> I agree IS_REACHABLE() is much saner approach.
>
> So, do you agree to change the current behavior
> as follows?
>
>
> index d0111dd26410..47dbfd1ee003 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -173,7 +173,7 @@ applicable everywhere (see syntax).
> === === ============= ==============
> n y n N/m/y
> m y m M/y/n
> - y y y Y/n
> + y y y Y/m/n
> y n * N
> === === ============= ==============
>
Yes. That should have been the case all along.
Nicolas
Hi Geert,
On Wed, Feb 19, 2020 at 6:54 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Yamada-san,
>
> On Wed, Feb 19, 2020 at 10:24 AM Masahiro Yamada <[email protected]> wrote:
> > On Wed, Feb 19, 2020 at 5:42 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Wed, Feb 19, 2020 at 8:51 AM Masahiro Yamada <[email protected]> wrote:
> > > > The 'imply' statement may create unmet direct dependency when the
> > > > implied symbol depends on m.
> > > >
> > > > [Test Code]
> > > >
> > > > config FOO
> > > > tristate "foo"
> > > > imply BAZ
> > > >
> > > > config BAZ
> > > > tristate "baz"
> > > > depends on BAR
> > > >
> > > > config BAR
> > > > def_tristate m
> > > >
> > > > config MODULES
> > > > def_bool y
> > > > option modules
> > > >
> > > > If you set FOO=y, BAZ is also promoted to y, which results in the
> > > > following .config file:
> > > >
> > > > CONFIG_FOO=y
> > > > CONFIG_BAZ=y
> > > > CONFIG_BAR=m
> > > > CONFIG_MODULES=y
> > > >
> > > > This ignores the dependency "BAZ depends on BAR".
> > > >
> > > > Unlike 'select', what is worse, Kconfig never shows the
> > > > "WARNING: unmet direct dependencies detected for ..." for this case.
> > > >
> > > > Because 'imply' should be weaker than 'depends on', Kconfig should
> > > > take the direct dependency into account.
> > > >
> > > > Describe this case in Documentation/kbuild/kconfig-language.rst for
> > > > clarification.
> > > >
> > > > Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
> > > > a symbol implied by y is restricted to y or n, excluding m.
> > > >
> > > > As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> > > > by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> > > > possible value is BAZ=n.
> > > >
> > > > Having said that, this case was probably "We don't care" at that time
> > > > because Kconfig did not handle 'depends on m' correctly until
> > > > commit f622f8279581 ("kconfig: warn unmet direct dependency of tristate
> > > > symbols selected by y") fixed it.
> > > >
> > > > Backporting this to 4.19+ will probably be fine. If you care this
> > > > problem on 4.14.x, you need to backport f622f8279581 as well.
> > > >
> > > > Signed-off-by: Masahiro Yamada <[email protected]>
> > >
> > > Thanks a lot! This fixes the build issues in
> > > https://lore.kernel.org/alsa-devel/CAMuHMdW8SvDgQJyenTtEm4Xn2Ma6PK9pfwKR2_gn60t2AqNWXg@mail.gmail.com/
> > >
> > > Tested-by: Geert Uytterhoeven <[email protected]>
> >
> > Even if this patch fixes the build issues,
> > the result may not be what you expect.
> >
> > The very subtle part of 'imply' is that
> > symbols implied by 'y' cannot be 'm'.
> >
> > When CONFIG_SND_SOC_ALL_CODECS=y,
> > the select'ed drivers were previously able to be 'm'.
> >
> > After ea00d95200d02, drivers that were previously
> > 'm' are now 'n'.
> >
> > Probably, it shrunk the test-coverage.
> >
> > I'd recommend to compare the .config file.
>
> I always do, when running make oldconfig ;-)
>
> I only see expected changes from y to m.
> I don't see any unexpected changes from m to n.
This is because you used oldconfig.
The 'imply' keyword defines the default
of symbols _without_ user-defined values.
oldconfig loads the .config file,
which contains the user-defined values.
When you test the 'imply' keyword,
you must start Kconfig from a pristine state
(e.g. 'make defconfig')
Compare the difference between v5.6-rc4 and
v5.6-rc4 + the following two patches.
https://lore.kernel.org/patchwork/patch/1190445/
https://lore.kernel.org/patchwork/patch/1196514/
[How to reproduce]
Tweak x86_64_defconfig as follows, then
compare the result of 'make defconfig'.
masahiro@pug:~/ref/linux$ git diff
diff --git a/arch/x86/configs/x86_64_defconfig
b/arch/x86/configs/x86_64_defconfig
index 0b9654c7a05c..e102d75d3707 100644
--- a/arch/x86/configs/x86_64_defconfig
+++ b/arch/x86/configs/x86_64_defconfig
@@ -194,7 +194,7 @@ CONFIG_WATCHDOG=y
CONFIG_AGP=y
CONFIG_AGP_AMD64=y
CONFIG_AGP_INTEL=y
-CONFIG_DRM=y
+# CONFIG_DRM is not set
CONFIG_DRM_I915=y
CONFIG_FB_MODE_HELPERS=y
CONFIG_FB_TILEBLITTING=y
@@ -296,3 +296,11 @@ CONFIG_SECURITY_SELINUX_DISABLE=y
CONFIG_EFI_STUB=y
CONFIG_EFI_MIXED=y
CONFIG_ACPI_BGRT=y
+CONFIG_EXPERT=y
+# CONFIG_TTY is not set
+CONFIG_INPUT=m
+CONFIG_I2C=y
+CONFIG_SPI=y
+CONFIG_COMPILE_TEST=y
+CONFIG_SND_SOC=y
+CONFIG_SND_SOC_ALL_CODECS=y
masahiro@pug:~/ref/linux$ git checkout v5.6-rc4
M arch/x86/configs/x86_64_defconfig
Note: checking out 'v5.6-rc4'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:
git checkout -b <new-branch-name>
HEAD is now at 98d54f81e36b Linux 5.6-rc4
masahiro@pug:~/ref/linux$ git log --oneline -1
98d54f81e36b (HEAD, tag: v5.6-rc4, origin/master, origin/HEAD, master)
Linux 5.6-rc4
masahiro@pug:~/ref/linux$ make -s O=before defconfig
M arch/x86/configs/x86_64_defconfig
Previous HEAD position was 98d54f81e36b Linux 5.6-rc4
Switched to branch 'geert'
masahiro@pug:~/ref/linux$ git log --oneline -3
231be9523aee (HEAD -> geert) kconfig: make 'imply' obey the direct dependency
4132db11c62e ASoC: Use imply for SND_SOC_ALL_CODECS
98d54f81e36b (tag: v5.6-rc4, origin/master, origin/HEAD, master) Linux 5.6-rc4
masahiro@pug:~/ref/linux$ make -s O=after defconfig
masahiro@pug:~/ref/linux$ ./scripts/diffconfig before/.config after/.config
SND_SOC_CS42L52 m -> n
SND_SOC_CS42L56 m -> n
SND_SOC_MT6351 n -> y
SND_SOC_MT6358 n -> y
SND_SOC_WM8962 m -> n
+SND_SOC_88PM860X n
+SND_SOC_ARIZONA y
+SND_SOC_CROS_EC_CODEC n
+SND_SOC_CS47L15 y
+SND_SOC_CS47L24 y
+SND_SOC_CS47L35 y
+SND_SOC_CS47L85 y
+SND_SOC_CS47L90 y
+SND_SOC_CS47L92 y
+SND_SOC_CX20442 n
+SND_SOC_LOCHNAGAR_SC n
+SND_SOC_MADERA y
+SND_SOC_MC13783 y
+SND_SOC_RT1308_SDW n
+SND_SOC_RT700_SDW n
+SND_SOC_RT711_SDW n
+SND_SOC_RT715_SDW n
+SND_SOC_SI476X y
+SND_SOC_TWL4030 n
+SND_SOC_TWL6040 n
+SND_SOC_WCD9335 n
+SND_SOC_WCD934X n
+SND_SOC_WL1273 y
+SND_SOC_WM5102 y
+SND_SOC_WM5110 y
+SND_SOC_WM8350 n
+SND_SOC_WM8400 n
+SND_SOC_WM8994 y
+SND_SOC_WM8997 y
+SND_SOC_WM8998 y
+SND_SOC_WSA881X n
SND_SOC_CS42L52, SND_SOC_CS42L56 and SND_SOC_WM8962
were previously m, but now disabled.
--
Best Regards
Masahiro Yamada
On Mon, Mar 02, 2020 at 05:21:51PM +0900, Masahiro Yamada wrote:
> On Wed, Feb 19, 2020 at 6:54 PM Geert Uytterhoeven <[email protected]> wrote:
> > I always do, when running make oldconfig ;-)
> > I only see expected changes from y to m.
> > I don't see any unexpected changes from m to n.
> This is because you used oldconfig.
> The 'imply' keyword defines the default
> of symbols _without_ user-defined values.
This is going to make the behaviour of imply statements a bit
inconsistent which doesn't seem ideal though it's *probably* fine for
the SND_SOC_ALL_CODECS case.