Received-SPF: pass (google.com: domain of linux-kernel+bounces-203145-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 5 Jun 2024 22:03:03 +0200
From: Nicolas Schier <nicolas@fjasle.eu>
To: Masahiro Yamada <masahiroy@kernel.org>
Cc: linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
 Jonathan Corbet <corbet@lwn.net>,
 Nathan Chancellor <nathan@kernel.org>, linux-doc@vger.kernel.org
Subject: Re: [PATCH 1/3] kconfig: remove tristate choice support
Message-ID: <ZmDEd9KpCvnLBobT@fjasle.eu>
References: <20240602125416.976336-1-masahiroy@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature"; boundary="z5IY1njxa83vktCR"
Content-Disposition: inline
In-Reply-To: <20240602125416.976336-1-masahiroy@kernel.org>
Feedback-ID: 1174286m:1174286a9YXZ7r:1174286sjF3jDkxxc


--z5IY1njxa83vktCR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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".
>=20
> There are more issues in the tristate choice feature. For example, you
> can observe a couple of bugs in the following test code.
>=20
> [Test Code]
>=20
>     config MODULES
>             def_bool y
>             modules
>=20
>     choice
>             prompt "tristate choice"
>             default A
>=20
>     config A
>             tristate "A"
>=20
>     config B
>             tristate "B"
>=20
>     endchoice
>=20
> [Bug 1] the 'default' property is not correctly processed
>=20
> 'make alldefconfig' produces:
>=20
>     CONFIG_MODULES=3Dy
>     # CONFIG_A is not set
>     # CONFIG_B is not set
>=20
> However, the correct output should be:
>=20
>     CONFIG_MODULES=3Dy
>     CONFIG_A=3Dy
>     # CONFIG_B is not set
>=20
> The unit test file, scripts/kconfig/tests/choice/alldef_expected_config,
> is wrong as well.
>=20
> [Bug 2] choice members never get 'y' with randconfig
>=20
> For the test code above, the following combinations are possible:
>=20
>                A    B
>         (1)    y    n
>         (2)    n    y
>         (3)    m    m
>         (4)    m    n
>         (5)    n    m
>         (6)    n    n
>=20
> 'make randconfig' never produces (1) or (2).
>=20
> 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.
>=20
> In addition, I have never seen it being used in a useful way.
>=20
> The following commits removed unnecessary use of tristate choices:
>=20
>  - df8df5e4bc37 ("usb: get rid of 'choice' for legacy gadget drivers")
>  - bfb57ef0544a ("rapidio: remove choice for enumeration")
>=20
> This commit removes the tristate choice support entirely, which allows
> me to delete a lot of code, making further refactoring easier.
>=20
> 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.
>=20
> [1]: https://lore.kernel.org/linux-kbuild/20240427104231.2728905-1-masahi=
roy@kernel.org/T/#m0a1bb6992581462ceca861b409bb33cb8fd7dbae
>=20
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>=20
>  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/expecte=
d_config
>  delete mode 100644 scripts/kconfig/tests/choice_value_with_m_dep/expecte=
d_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
>=20
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kb=
uild/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"
> =20
>  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.
> =20
> -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 real=
ly
good to get rid of this, thanks!


Patch looks good to me.

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>


Kind regards,
Nicolas


--z5IY1njxa83vktCR
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEh0E3p4c3JKeBvsLGB1IKcBYmEmkFAmZgxHMACgkQB1IKcBYm
EmndVQ/+I/hxjhcRiNRPAJ9qRVyn/3ZBwjUOjOMTPNkGV4aLvtPtnMRB9u6WpyoJ
Rk6URdeJXpDIP8OpqexJsXCxR0gaPL2ejHaHU/TbuKab7c+SKn6/xl9FWqUdWRxu
H9pZ76HLFddKIjGag9GhwyKABeyCS34KxPRiBCFsnjIpyA5fu3DHZhAgOn1CeNWM
4bFAKsq6RjyCqQt7oEEnMTnIfy6yDLuUi9sCByHLAN/CN4ABea2jElTH1gANuglD
u78qOY/iDBY/crm7xJsVQ3W9/yL/ekOBfsPyydwsG0HnhpxaMCFlQ9shFoJsHM2o
CqyaX84nqOFizbP4UHDstJUtiXlnLJIn9P91pjZ7AIrvS24KX6avFSihD5kTwjlr
5YeTynEpzae0OPCIW6SoGFBBTrP35FJj7MeccjTSndnAKKPAN524sZHvElbEGr55
1xQeRioQb1oqIGP/UvXMwReCD6Bev+8JmpbZwxRgrgb1G7VguFeJwbmIPef7ctwi
1yhx+wpntv6bc9cIc4MwaQLuAsK2HPT+KPl+PuoH/iKFv+kPI+buM75NXJuep8x1
kUsCPvDvYeuITGfsV7ICF/qWQxLtI0NsWVvMBDwqBzLf3GwQxwyhy5a+m51V5bet
zVf4QZbZZGTTgWhxyIxLHOU9MbUdHfKuQrZTjiBwyPzen5Ii0YE=
=cH/K
-----END PGP SIGNATURE-----

--z5IY1njxa83vktCR--