Received-SPF: pass (google.com: domain of linux-kernel+bounces-203700-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240602125416.976336-1-masahiroy@kernel.org>
In-Reply-To: <20240602125416.976336-1-masahiroy@kernel.org>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Thu, 6 Jun 2024 15:57:54 +0900
Message-ID: <CAK7LNAT_rH1ZcL2uhwRgVRLM58rvNMYuEMikdBr2ize-XLTHuw@mail.gmail.com>
Subject: Re: [PATCH 1/3] kconfig: remove tristate choice support
To: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>, 
	Nathan Chancellor <nathan@kernel.org>, Nicolas Schier <nicolas@fjasle.eu>, linux-doc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Jun 2, 2024 at 9:54=E2=80=AFPM Masahiro Yamada <masahiroy@kernel.or=
g> 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=3Dy
>     # CONFIG_A is not set
>     # CONFIG_B is not set
>
> However, the correct output should be:
>
>     CONFIG_MODULES=3Dy
>     CONFIG_A=3Dy
>     # 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-masahi=
roy@kernel.org/T/#m0a1bb6992581462ceca861b409bb33cb8fd7dbae
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---


I will fix a couple of mistakes.


> diff --git a/scripts/kconfig/gconf.c b/scripts/kconfig/gconf.c
> index baa1c512de3c..217f85ea0910 100644
> --- a/scripts/kconfig/gconf.c
> +++ b/scripts/kconfig/gconf.c
> @@ -1067,10 +1067,7 @@ static gchar **fill_row(struct menu *menu)
>                         row[COL_VALUE] =3D
>                             g_strdup(menu_get_prompt(def_menu));
>
> -               if (sym_get_type(sym) =3D=3D S_BOOLEAN) {
> -                       row[COL_BTNVIS] =3D GINT_TO_POINTER(FALSE);
> -                       return row;
> -               }
> +               return row;


I accidentally dropped

  row[COL_BTNVIS] =3D GINT_TO_POINTER(FALSE);


I will restore it.

The intention is to drop the if-condition that
is always met.


> @@ -479,7 +461,7 @@ void sym_calc_value(struct symbol *sym)
>         }
>
>         sym->curr =3D newval;
> -       if (sym_is_choice(sym) && newval.tri =3D=3D yes)
> +       if (sym_is_choice(sym))
>                 sym->curr.val =3D sym_calc_choice(sym);
>         sym_validate_range(sym);
>


I will keep "&& newval.tri =3D=3D yes" here.


When a choice block is hidden by 'depends on',
there is no need to call sym_calc_choice().


--=20
Best Regards
Masahiro Yamada