Re: [PATCH] kconfig: fix missing choice values in auto.conf

[Joonas KylmÃlÃ]

Hi,

thanks a lot for this patch! I tested this and it fixes what is says in
the commit message:

Tested-by: Joonas KylmÃlà <joonas.kylmala@xxxxxx>

Masahiro Yamada:
> Since commit 00c864f8903d ("kconfig: allow all config targets to write
> auto.conf if missing"), Kconfig creates include/config/auto.conf in the
> defconfig stage when it is missing.
> 
> Joonas KylmÃlà reported incorrect auto.conf generation under some
> circumstances.
> 
> Apply the following diff:
> 
> | --- a/arch/arm/configs/imx_v6_v7_defconfig
> | +++ b/arch/arm/configs/imx_v6_v7_defconfig
> | @@ -345,14 +345,7 @@ CONFIG_USB_CONFIGFS_F_MIDI=y
> |  CONFIG_USB_CONFIGFS_F_HID=y
> |  CONFIG_USB_CONFIGFS_F_UVC=y
> |  CONFIG_USB_CONFIGFS_F_PRINTER=y
> | -CONFIG_USB_ZERO=m
> | -CONFIG_USB_AUDIO=m
> | -CONFIG_USB_ETH=m
> | -CONFIG_USB_G_NCM=m
> | -CONFIG_USB_GADGETFS=m
> | -CONFIG_USB_FUNCTIONFS=m
> | -CONFIG_USB_MASS_STORAGE=m
> | -CONFIG_USB_G_SERIAL=m
> | +CONFIG_USB_FUNCTIONFS=y
> |  CONFIG_MMC=y
> |  CONFIG_MMC_SDHCI=y
> |  CONFIG_MMC_SDHCI_PLTFM=y
> 
> And then, run:
> 
> $ make ARCH=arm mrproper imx_v6_v7_defconfig
> 
> CONFIG_USB_FUNCTIONFS=y is correctly contained in the .config, but not
> in the auto.conf.
> 
> Please note drivers/usb/gadget/legacy/Kconfig is included from a choice
> block in drivers/usb/gadget/Kconfig. So USB_FUNCTIONFS is a choice value.
> 
> This is probably a similar situation described in commit beaaddb62540
> ("kconfig: tests: test defconfig when two choices interact").
> 
> When sym_calc_choice() is called, the choice symbol forgets the
> SYMBOL_DEF_USER unless all of its choice values are explicitly set by
> the user.
> 
> The choice symbol is given just one chance to recall it because
> set_all_choice_values() is called if SYMBOL_NEED_SET_CHOICE_VALUES
> is set.
> 
> When sym_calc_choice() is called again, the choice symbol forgets it
> forever, since SYMBOL_NEED_SET_CHOICE_VALUES is a one-time aid.
> Hence, we cannot call sym_clear_all_valid() again and again.
> 
> It is crazy to set and clear internal flags. However, we cannot simply
> get rid of "sym->flags &= flags | ~SYMBOL_DEF_USER;" Doing so would
> re-introduce the problem solved by commit 5d09598d488f ("kconfig: fix
> new choices being skipped upon config update").
> 
> To work around the issue, conf_write_autoconf() stopped calling
> sym_clear_all_valid().
> 
> conf_write() must be changed accordingly. Currently, it clears
> SYMBOL_WRITE after the symbol is written into the .config file. This
> is needed to prevent it from writing the same symbol multiple times in
> case the symbol is declared in two or more locations. I added the new
> flag SYMBOL_WRITTEN, to track the symbols that have been written.
> 
> Anyway, this is a cheesy workaround in order to suppress the issue
> as far as defconfig is concerned.
> 
> Handling of choices is totally broken. sym_clear_all_valid() is called
> every time a user touches a symbol from the GUI interface. To reproduce
> it, just add a new symbol drivers/usb/gadget/legacy/Kconfig, then touch
> around unrelated symbols from menuconfig. USB_FUNCTIONFS will disappear
> from the .config file.
> 
> I added the Fixes tag since it is more fatal than before. But, this
> has been broken since long long time before, and still it is.
> We should take a closer look to fix this correctly somehow.
> 
> Fixes: 00c864f8903d ("kconfig: allow all config targets to write auto.conf if missing")
> Cc: linux-stable <stable@xxxxxxxxxxxxxxx> # 4.19+
> Reported-by: Joonas KylmÃlà <joonas.kylmala@xxxxxx>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> ---
> 
>  scripts/kconfig/confdata.c | 7 +++----
>  scripts/kconfig/expr.h     | 1 +
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index cbb6efa4a5a6..e0972b255aac 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -895,7 +895,8 @@ int conf_write(const char *name)
>  				     "# %s\n"
>  				     "#\n", str);
>  			need_newline = false;
> -		} else if (!(sym->flags & SYMBOL_CHOICE)) {
> +		} else if (!(sym->flags & SYMBOL_CHOICE) &&
> +			   !(sym->flags & SYMBOL_WRITTEN)) {
>  			sym_calc_value(sym);
>  			if (!(sym->flags & SYMBOL_WRITE))
>  				goto next;
> @@ -903,7 +904,7 @@ int conf_write(const char *name)
>  				fprintf(out, "\n");
>  				need_newline = false;
>  			}
> -			sym->flags &= ~SYMBOL_WRITE;
> +			sym->flags |= SYMBOL_WRITTEN;
>  			conf_write_symbol(out, sym, &kconfig_printer_cb, NULL);
>  		}
>  
> @@ -1063,8 +1064,6 @@ int conf_write_autoconf(int overwrite)
>  	if (!overwrite && is_present(autoconf_name))
>  		return 0;
>  
> -	sym_clear_all_valid();
> -
>  	conf_write_dep("include/config/auto.conf.cmd");
>  
>  	if (conf_touch_deps())
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index 8dde65bc3165..017843c9a4f4 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -141,6 +141,7 @@ struct symbol {
>  #define SYMBOL_OPTIONAL   0x0100  /* choice is optional - values can be 'n' */
>  #define SYMBOL_WRITE      0x0200  /* write symbol to file (KCONFIG_CONFIG) */
>  #define SYMBOL_CHANGED    0x0400  /* ? */
> +#define SYMBOL_WRITTEN    0x0800  /* track info to avoid double-write to .config */
>  #define SYMBOL_NO_WRITE   0x1000  /* Symbol for internal use only; it will not be written */
>  #define SYMBOL_CHECKED    0x2000  /* used during dependency checking */
>  #define SYMBOL_WARNED     0x8000  /* warning has been issued */
>