Re: [PATCH 1/3] kconfig: remove tristate choice support

From: Nicolas Schier
Date: Wed Jun 05 2024 - 16:04:38 EST

On Sun, Jun 02, 2024 at 09:54:14PM +0900 Masahiro Yamada wrote:
> I previously submitted a fix for a bug in the choice feature [1], where
> I mentioned, "Another (much cleaner) approach would be to remove the
> tristate choice support entirely".
>
> There are more issues in the tristate choice feature. For example, you
> can observe a couple of bugs in the following test code.
>
> [Test Code]
>
> config MODULES
> def_bool y
> modules
>
> choice
> prompt "tristate choice"
> default A
>
> config A
> tristate "A"
>
> config B
> tristate "B"
>
> endchoice
>
> [Bug 1] the 'default' property is not correctly processed
>
> 'make alldefconfig' produces:
>
> CONFIG_MODULES=y
> # CONFIG_A is not set
> # CONFIG_B is not set
>
> However, the correct output should be:
>
> CONFIG_MODULES=y
> CONFIG_A=y
> # CONFIG_B is not set
>
> The unit test file, scripts/kconfig/tests/choice/alldef_expected_config,
> is wrong as well.
>
> [Bug 2] choice members never get 'y' with randconfig
>
> For the test code above, the following combinations are possible:
>
> A B
> (1) y n
> (2) n y
> (3) m m
> (4) m n
> (5) n m
> (6) n n
>
> 'make randconfig' never produces (1) or (2).
>
> These bugs are fixable, but a more critical problem is the lack of a
> sensible syntax to specify the default for the tristate choice.
> The default for the choice must be one of the choice members, which
> cannot specify any of the patterns (3) through (6) above.
>
> In addition, I have never seen it being used in a useful way.
>
> The following commits removed unnecessary use of tristate choices:
>
> - df8df5e4bc37 ("usb: get rid of 'choice' for legacy gadget drivers")
> - bfb57ef0544a ("rapidio: remove choice for enumeration")
>
> This commit removes the tristate choice support entirely, which allows
> me to delete a lot of code, making further refactoring easier.
>
> This includes the revert of commit fa64e5f6a35e ("kconfig/symbol.c:
> handle choice_values that depend on 'm' symbols"). It was suspicious
> because it did not address the root cause but introduced inconsistency
> in visibility between choice members and other symbols.
>
> [1]: https://lore.kernel.org/linux-kbuild/20240427104231.2728905-1-masahiroy@xxxxxxxxxx/T/#m0a1bb6992581462ceca861b409bb33cb8fd7dbae
>
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> ---
>
> Documentation/kbuild/kconfig-language.rst | 11 +---
> scripts/kconfig/conf.c | 56 ++-----------------
> scripts/kconfig/confdata.c | 17 +-----
> scripts/kconfig/gconf.c | 5 +-
> scripts/kconfig/mconf.c | 28 +++-------
> scripts/kconfig/menu.c | 27 ---------
> scripts/kconfig/nconf.c | 28 ++--------
> scripts/kconfig/parser.y | 23 +++++---
> scripts/kconfig/qconf.cc | 2 +-
> scripts/kconfig/symbol.c | 24 +-------
> scripts/kconfig/tests/choice/Kconfig | 17 ------
> scripts/kconfig/tests/choice/__init__.py | 10 ----
> .../tests/choice/alldef_expected_config | 3 -
> .../tests/choice/allmod_expected_config | 3 -
> .../tests/choice/allno_expected_config | 3 -
> .../tests/choice/allyes_expected_config | 3 -
> .../tests/choice/oldask0_expected_stdout | 4 --
> scripts/kconfig/tests/choice/oldask1_config | 1 -
> .../tests/choice/oldask1_expected_stdout | 9 ---
> .../tests/choice_value_with_m_dep/Kconfig | 21 -------
> .../tests/choice_value_with_m_dep/__init__.py | 16 ------
> .../tests/choice_value_with_m_dep/config | 2 -
> .../choice_value_with_m_dep/expected_config | 3 -
> .../choice_value_with_m_dep/expected_stdout | 4 --
> scripts/kconfig/tests/inter_choice/Kconfig | 25 ---------
> .../kconfig/tests/inter_choice/__init__.py | 15 -----
> scripts/kconfig/tests/inter_choice/defconfig | 1 -
> .../tests/inter_choice/expected_config | 4 --
> 28 files changed, 42 insertions(+), 323 deletions(-)
> delete mode 100644 scripts/kconfig/tests/choice/oldask1_config
> delete mode 100644 scripts/kconfig/tests/choice/oldask1_expected_stdout
> delete mode 100644 scripts/kconfig/tests/choice_value_with_m_dep/Kconfig
> delete mode 100644 scripts/kconfig/tests/choice_value_with_m_dep/__init__.py
> delete mode 100644 scripts/kconfig/tests/choice_value_with_m_dep/config
> delete mode 100644 scripts/kconfig/tests/choice_value_with_m_dep/expected_config
> delete mode 100644 scripts/kconfig/tests/choice_value_with_m_dep/expected_stdout
> delete mode 100644 scripts/kconfig/tests/inter_choice/Kconfig
> delete mode 100644 scripts/kconfig/tests/inter_choice/__init__.py
> delete mode 100644 scripts/kconfig/tests/inter_choice/defconfig
> delete mode 100644 scripts/kconfig/tests/inter_choice/expected_config
>
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index 555c2f839969..dc50b5b12577 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -399,16 +399,9 @@ choices::
> "endchoice"
>
> This defines a choice group and accepts any of the above attributes as
> -options. A choice can only be of type bool or tristate. If no type is
> -specified for a choice, its type will be determined by the type of
> -the first choice element in the group or remain unknown if none of the
> -choice elements have a type specified, as well.
> +options.
>
> -While a boolean choice only allows a single config entry to be
> -selected, a tristate choice also allows any number of config entries
> -to be set to 'm'. This can be used if multiple drivers for a single
> -hardware exists and only a single driver can be compiled/loaded into
> -the kernel, but all drivers can be compiled as modules.
> +A choice only allows a single config entry to be selected.

oh, I wasn't aware of that multi-selectable 'm' feature. I think it's really
good to get rid of this, thanks!


Patch looks good to me.

Reviewed-by: Nicolas Schier <nicolas@xxxxxxxxx>



Kind regards,
Nicolas

Attachment: signature.asc
Description: PGP signature