[PATCH 0/2] kconfig: fixes for savedefconfig

[PATCH 0/2] kconfig: fixes for savedefconfig

[Sam Ravnborg]

While tranislating all remaining defconfigs in
the kernel to the minimal format I encountered
an issue where the resulting .config was not
equal to the original.

This was tracked down to missing info in the saved defconfig file.

Arve reported another problem with tristate choice that was
reproduceable with a small config sample.

The followng patches fixes both issues.
There is no outstanding issue with respect to savedefconfig.

	Sam

Sam Ravnborg (2):
      kconfig: fix savedefconfig for tristate choices
      kconfig: fix tristate choice with minimal config

 scripts/kconfig/confdata.c |  109 ++++++++++++++++++++++++++++----------------
 1 files changed, 70 insertions(+), 39 deletions(-)

[PATCH 1/2] kconfig: fix savedefconfig for tristate choices

[Sam Ravnborg]

savedefconfig failed to save choice symbols equal to 'y'
for tristate choices.
This resulted in this value being lost.

In particular is fixes an issue where

	make ARCH=avr32 atngw100_defconfig
	make ARCH=avr32 savedefconfig
	cp defconfig arch/avr32/configs/atngw100_defconfig
	make ARCH=avr32 atngw100_defconfig
	diff -u .config .config.old

failed to produce an identical .config.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/kconfig/confdata.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index f81f263..e5d66e4 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -497,7 +497,7 @@ int conf_write_defconfig(const char *filename)
 			/*
 			 * If symbol is a choice value and equals to the
 			 * default for a choice - skip.
-			 * But only if value equal to "y".
+			 * But only if value is bool and equal to "y" .
 			 */
 			if (sym_is_choice_value(sym)) {
 				struct symbol *cs;
@@ -506,9 +506,8 @@ int conf_write_defconfig(const char *filename)
 				cs = prop_get_symbol(sym_get_choice_prop(sym));
 				ds = sym_choice_default(cs);
 				if (sym == ds) {
-					if ((sym->type == S_BOOLEAN ||
-					sym->type == S_TRISTATE) &&
-					sym_get_tristate_value(sym) == yes)
+					if ((sym->type == S_BOOLEAN) &&
+					    sym_get_tristate_value(sym) == yes)
 						goto next_menu;
 				}
 			}
-- 
1.6.0.6

[PATCH 2/2] kconfig: fix tristate choice with minimal config

[Sam Ravnborg]

If a minimal config did not specify the value
of all choice values, the resulting configuration
could have wrong values.

Consider following example:
config M
        def_bool y
        option modules
choice
        prompt "choice list"
config A
        tristate "a"
config B
	tristate "b"
endchoice

With a defconfig like this:
CONFIG_M=y
CONFIG_A=y

The resulting configuration would have

    CONFIG_A=m

which was unexpected.

The problem was not not all choice values were set and thus
kconfig calculated a wrong value.

The fix is to set all choice values when we
read a defconfig files.

conf_set_all_new_symbols() is refactored such that
random choice values are now handled by a dedicated function.
And new choice values are set by set_all_choice_values().

This was not the minimal fix, but the fix that resulted
in the most readable code.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reported-by: Arve Hjønnevåg <arve@android.com>
Tested-by: Arve Hjønnevåg <arve@android.com>
---
 scripts/kconfig/confdata.c |  102 +++++++++++++++++++++++++++++---------------
 1 files changed, 67 insertions(+), 35 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index e5d66e4..ac13f0f 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -918,13 +918,73 @@ void conf_set_changed_callback(void (*fn)(void))
 	conf_changed_callback = fn;
 }
 
+static void randomize_choice_values(struct symbol *csym)
+{
+	struct property *prop;
+	struct symbol *sym;
+	struct expr *e;
+	int cnt, def;
 
-void conf_set_all_new_symbols(enum conf_def_mode mode)
+	/*
+	 * If choice is mod then we may have more items slected
+	 * and if no then no-one.
+	 * In both cases stop.
+	 */
+	if (csym->curr.tri != yes)
+		return;
+
+	prop = sym_get_choice_prop(csym);
+
+	/* count entries in choice block */
+	cnt = 0;
+	expr_list_for_each_sym(prop->expr, e, sym)
+		cnt++;
+
+	/*
+	 * find a random value and set it to yes,
+	 * set the rest to no so we have only one set
+	 */
+	def = (rand() % cnt);
+
+	cnt = 0;
+	expr_list_for_each_sym(prop->expr, e, sym) {
+		if (def == cnt++) {
+			sym->def[S_DEF_USER].tri = yes;
+			csym->def[S_DEF_USER].val = sym;
+		}
+		else {
+			sym->def[S_DEF_USER].tri = no;
+		}
+	}
+	csym->flags |= SYMBOL_DEF_USER;
+	/* clear VALID to get value calculated */
+	csym->flags &= ~(SYMBOL_VALID);
+}
+
+static void set_all_choice_values(struct symbol *csym)
 {
-	struct symbol *sym, *csym;
 	struct property *prop;
+	struct symbol *sym;
 	struct expr *e;
-	int i, cnt, def;
+
+	prop = sym_get_choice_prop(csym);
+
+	/*
+	 * Set all non-assinged choice values to no
+	 */
+	expr_list_for_each_sym(prop->expr, e, sym) {
+		if (!sym_has_value(sym))
+			sym->def[S_DEF_USER].tri = no;
+	}
+	csym->flags |= SYMBOL_DEF_USER;
+	/* clear VALID to get value calculated */
+	csym->flags &= ~(SYMBOL_VALID);
+}
+
+void conf_set_all_new_symbols(enum conf_def_mode mode)
+{
+	struct symbol *sym, *csym;
+	int i, cnt;
 
 	for_all_symbols(i, sym) {
 		if (sym_has_value(sym))
@@ -960,8 +1020,6 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
 
 	sym_clear_all_valid();
 
-	if (mode != def_random)
-		return;
 	/*
 	 * We have different type of choice blocks.
 	 * If curr.tri equal to mod then we can select several
@@ -976,35 +1034,9 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
 			continue;
 
 		sym_calc_value(csym);
-
-		if (csym->curr.tri != yes)
-			continue;
-
-		prop = sym_get_choice_prop(csym);
-
-		/* count entries in choice block */
-		cnt = 0;
-		expr_list_for_each_sym(prop->expr, e, sym)
-			cnt++;
-
-		/*
-		 * find a random value and set it to yes,
-		 * set the rest to no so we have only one set
-		 */
-		def = (rand() % cnt);
-
-		cnt = 0;
-		expr_list_for_each_sym(prop->expr, e, sym) {
-			if (def == cnt++) {
-				sym->def[S_DEF_USER].tri = yes;
-				csym->def[S_DEF_USER].val = sym;
-			}
-			else {
-				sym->def[S_DEF_USER].tri = no;
-			}
-		}
-		csym->flags |= SYMBOL_DEF_USER;
-		/* clear VALID to get value calculated */
-		csym->flags &= ~(SYMBOL_VALID);
+		if (mode == def_random)
+			randomize_choice_values(csym);
+		else
+			set_all_choice_values(csym);
 	}
 }
-- 
1.6.0.6

Re: [PATCH 0/2] kconfig: fixes for savedefconfig

[Michal Marek]

On 12.8.2010 09:10, Sam Ravnborg wrote:
> While tranislating all remaining defconfigs in
> the kernel to the minimal format I encountered
> an issue where the resulting .config was not
> equal to the original.
> 
> This was tracked down to missing info in the saved defconfig file.
> 
> Arve reported another problem with tristate choice that was
> reproduceable with a small config sample.
> 
> The followng patches fixes both issues.
> There is no outstanding issue with respect to savedefconfig.

Applied, thanks a lot Sam!

Michal

Re: [PATCH 0/2] kconfig: fixes for savedefconfig

[Sam Ravnborg]

On Thu, Aug 12, 2010 at 11:12:41AM +0200, Michal Marek wrote:
> On 12.8.2010 09:10, Sam Ravnborg wrote:
> > While tranislating all remaining defconfigs in
> > the kernel to the minimal format I encountered
> > an issue where the resulting .config was not
> > equal to the original.
> > 
> > This was tracked down to missing info in the saved defconfig file.
> > 
> > Arve reported another problem with tristate choice that was
> > reproduceable with a small config sample.
> > 
> > The followng patches fixes both issues.
> > There is no outstanding issue with respect to savedefconfig.
> 
> Applied, thanks a lot Sam!

I would like to see these patches hit mainline soonish.
I have a small shellscript that will convert remaining defconfigs to
the minimal format and like it to be run just before after -rc1.
But these patches is needed before I can ask Linus to run the script.

	Sam