diff mbox

[6/8] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG

Message ID 422c809f03f043d0950d8362214818e956a9daee.1366841993.git.yann.morin.1998@free.fr (mailing list archive)
State New, archived
Headers show

Commit Message

Yann E. MORIN April 24, 2013, 10:29 p.m. UTC 
From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
is specified.

For example, given those two files (Thomas' test-case):

    ---8<--- Config.test.in
    config OPTIONA
        bool "Option A"

    choice
        prompt "This is a choice"

    config CHOICE_OPTIONA
        bool "Choice Option A"

    config CHOICE_OPTIONB
        bool "Choice Option B"

    endchoice

    config OPTIONB
        bool "Option B"
    ---8<--- Config.test.in

    ---8<--- config.defaults
    CONFIG_OPTIONA=y
    ---8<--- config.defaults

And running:
    ./scripts/kconfig/conf --randconfig Config.test.in

does properly randomise the two choice symbols (and the two booleans).

However, running:
    KCONFIG_ALLCONFIG=config.defaults \
    ./scripts/kconfig/conf --randconfig Config.test.in

does *not* reandomise the two choice entries, and only CHOICE_OPTIONA
will ever be selected. (OPTIONA will always be set (expected), and
OPTIONB will be be properly randomised (expected).)

This patch defers setting that a choice has a value until a symbol for
that choice is indeed set, so that choices are properly randomised when
KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.

Also, as a side-efect, this patch fixes the following case:

    ---8<---
    choice
    config OPTION_A
        bool "Option A"
    config OPTION_B
        bool "Option B"
    config OPTION_C
        bool "Option C"
    endchoice
    ---8<---

which could previously generate such .config files:

    ---8<---                            ---8<---
    CONFIG_OPTION_A=y                   CONFIG_OPTION_A=y
    CONFIG_OPTION_B=y                   # CONFIG_OPTION_B is not set
    # CONFIG_OPTION_C is not set        CONFIG_OPTION_C=y
    ---8<---                            ---8<---

Ie., the first entry in a choice is always set, plus zero or one of
the other options may be set.

This patch ensures that only one option may be set for a choice.

Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Arnaud Lacombe <lacombar@gmail.com>

---
Changes v2 -> v3
  - ensure only one symbol is set in a choice

Changes v1 -> v2:
  - further postpone setting that a choice has a value until
    one is indeed set
  - do not print symbols that are part of an invisible choice
---
 scripts/kconfig/confdata.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Arnd Bergmann April 26, 2013, 12:05 p.m. UTC | #1 
On Thursday 25 April 2013 00:29:53 Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
> 
> For example, given those two files (Thomas' test-case):

This patch has made it into linux-next today, and while I agree with
the basic idea, it seems to cause regressions for me.

For these ARM defconfigs, 'make' always asks about USB_GADGET now
after doing any of these defconfigs:

am200epdkit_defconfig
at91_dt_defconfig
at91rm9200_defconfig
at91sam9260_defconfig
at91sam9261_defconfig
at91sam9263_defconfig
at91sam9g20_defconfig
at91sam9g45_defconfig
corgi_defconfig
ezx_defconfig
h5000_defconfig
imote2_defconfig
kzm9g_defconfig
lpc32xx_defconfig
lubbock_defconfig
mackerel_defconfig
magician_defconfig
mini2440_defconfig
msm_defconfig
omap1_defconfig
prima2_defconfig
sama5_defconfig
tct_hammer_defconfig

~/arm-soc$ make at91_dt_defconfig
make[1]: Entering directory `/git/arm-soc'
  GEN     /git/arm-soc/obj-tmp/Makefile
  HOSTCC  scripts/kconfig/zconf.tab.o
  HOSTLD  scripts/kconfig/conf
#
# configuration written to .config
#
make[1]: Leaving directory `/git/arm-soc'
~/arm-soc$ make -sj40
make[1]: Entering directory `/git/arm-soc'
  GEN     /git/arm-soc/obj-tmp/Makefile
scripts/kconfig/conf --silentoldconfig Kconfig
*
* Restart config...
*
*
* USB Gadget Support
*
USB Gadget Support (USB_GADGET) [Y/n/m/?] y
  Debugging messages (DEVELOPMENT) (USB_GADGET_DEBUG) [N/y/?] n
  Debugging information files (DEVELOPMENT) (USB_GADGET_DEBUG_FILES) [N/y/?] n
  Debugging information files in debugfs (DEVELOPMENT) (USB_GADGET_DEBUG_FS) [N/y/?] n
  Maximum VBUS Power usage (2-500 mA) (USB_GADGET_VBUS_DRAW) [2] 2
  Number of storage pipeline buffers (USB_GADGET_STORAGE_NUM_BUFFERS) [2] 2
  USB Gadget Drivers [M/y/?] (NEW) 

I'm taking Greg on Cc here so he's aware of the problem and knows it's
not caused by any of the USB Kconfig changes that I and others submitted
recently. It took me a while to confirm that it's just your patch and
not mine that caused the problem.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yann E. MORIN April 26, 2013, 12:11 p.m. UTC | #2 
Arnd, All,

On Fri, Apr 26, 2013 at 02:05:37PM +0200, Arnd Bergmann wrote:
> On Thursday 25 April 2013 00:29:53 Yann E. MORIN wrote:
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > 
> > Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> > is specified.
> > 
> > For example, given those two files (Thomas' test-case):
> 
> This patch has made it into linux-next today, and while I agree with
> the basic idea, it seems to cause regressions for me.

Yes, this has already been noticed. Thanks for the report.

So far, we have a plan B (which is to revert that patch). I'll work on
plan A when I'm back home after work (GMT+2 here, so in about 5h from
now).

Sorry for the annoyance...

Regards,
Yann E. MORIN.
Yann E. MORIN April 26, 2013, 10:09 p.m. UTC | #3 
On Fri, Apr 26, 2013 at 02:05:37PM +0200, Arnd Bergmann wrote:
> On Thursday 25 April 2013 00:29:53 Yann E. MORIN wrote:
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > 
> > Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> > is specified.
> > 
> > For example, given those two files (Thomas' test-case):
> 
> This patch has made it into linux-next today, and while I agree with
> the basic idea, it seems to cause regressions for me.
> 
> For these ARM defconfigs, 'make' always asks about USB_GADGET now

OK, I threw in the towel on that one for tonight.
I'll send a patch to revert this changeset, in the coming minutes.

It will have to be revisited later on, and hopefully we can come to a
proper solution during the pre-rc1 window, or at worst, early during the
-rc phase (as I'm on holidays the next two weeks, I should find sometime
to gather what remains of my courage to look at that again).

Really sorry for the inconvenience... :-(

Regards,
Yann E. MORIN.
Arnd Bergmann April 27, 2013, 9:01 p.m. UTC | #4 
On Saturday 27 April 2013, Yann E. MORIN wrote:
> 
> On Fri, Apr 26, 2013 at 02:05:37PM +0200, Arnd Bergmann wrote:
> > On Thursday 25 April 2013 00:29:53 Yann E. MORIN wrote:
> > > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > > 
> > > Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> > > is specified.
> > > 
> > > For example, given those two files (Thomas' test-case):
> > 
> > This patch has made it into linux-next today, and while I agree with
> > the basic idea, it seems to cause regressions for me.
> > 
> > For these ARM defconfigs, 'make' always asks about USB_GADGET now
> 
> OK, I threw in the towel on that one for tonight.
> I'll send a patch to revert this changeset, in the coming minutes.
> 
> It will have to be revisited later on, and hopefully we can come to a
> proper solution during the pre-rc1 window, or at worst, early during the
> -rc phase (as I'm on holidays the next two weeks, I should find sometime
> to gather what remains of my courage to look at that again).

Ok, fair enough. For my own testing, I've actually started relying on the
behavior that is meant to be changed by the patch, even though I realize
that  we should really randomize choice statements.

It's easy enough for me to change my script to always select
CONFIG_ARCH_MUTLIPLATFORM for ARM testing in the future.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yann E. MORIN April 27, 2013, 10:31 p.m. UTC | #5 
Arnd, All,

On Sat, Apr 27, 2013 at 11:01:00PM +0200, Arnd Bergmann wrote:
> On Saturday 27 April 2013, Yann E. MORIN wrote:
> > 
> > On Fri, Apr 26, 2013 at 02:05:37PM +0200, Arnd Bergmann wrote:
> > > On Thursday 25 April 2013 00:29:53 Yann E. MORIN wrote:
> > > > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > > > 
> > > > Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> > > > is specified.
> > > > 
> > > > For example, given those two files (Thomas' test-case):
> > > 
> > > This patch has made it into linux-next today, and while I agree with
> > > the basic idea, it seems to cause regressions for me.
> > > 
> > > For these ARM defconfigs, 'make' always asks about USB_GADGET now
> > 
> > OK, I threw in the towel on that one for tonight.
> > I'll send a patch to revert this changeset, in the coming minutes.
> > 
> > It will have to be revisited later on, and hopefully we can come to a
> > proper solution during the pre-rc1 window, or at worst, early during the
> > -rc phase (as I'm on holidays the next two weeks, I should find sometime
> > to gather what remains of my courage to look at that again).
> 
> Ok, fair enough. For my own testing, I've actually started relying on the
> behavior that is meant to be changed by the patch, even though I realize
> that  we should really randomize choice statements.
> 
> It's easy enough for me to change my script to always select
> CONFIG_ARCH_MUTLIPLATFORM for ARM testing in the future.

Fact is, choices are properly randomised, unless KCONFIG_ALLCONFIG is
passed.

So, if you currently rely on choices not being randomised, it means
you're using KCONFIG_ALLCONFIG [*] so you can set CONFIG_ARCH_MULTIPLATFORM
in the defconfig you use.

Which should not change your workflow by much, I believe.

[*] If not, then we have another issue. Can you confirm you're already
using KCONFIG_ALLCONFIG? If not, can you explain how you handle
CONFIG_ARCH_MULTIPLATFORM always being set?

Regards,
Yann E. MORIN.
Arnd Bergmann April 28, 2013, 1:05 a.m. UTC | #6 
On Sunday 28 April 2013, Yann E. MORIN wrote:
> Fact is, choices are properly randomised, unless KCONFIG_ALLCONFIG is
> passed.
> 
> So, if you currently rely on choices not being randomised, it means
> you're using KCONFIG_ALLCONFIG [*] so you can set CONFIG_ARCH_MULTIPLATFORM
> in the defconfig you use.

Yes, you are right, I can easily do this.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 2e35d4b..8927480 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -288,8 +288,6 @@  load:
 	for_all_symbols(i, sym) {
 		sym->flags |= SYMBOL_CHANGED;
 		sym->flags &= ~(def_flags|SYMBOL_VALID);
-		if (sym_is_choice(sym))
-			sym->flags |= def_flags;
 		switch (sym->type) {
 		case S_INT:
 		case S_HEX:
@@ -379,13 +377,13 @@  setsym:
 			case mod:
 				if (cs->def[def].tri == yes) {
 					conf_warning("%s creates inconsistent choice state", sym->name);
-					cs->flags &= ~def_flags;
 				}
 				break;
 			case yes:
 				if (cs->def[def].tri != no)
 					conf_warning("override: %s changes choice state", sym->name);
 				cs->def[def].val = sym;
+				cs->flags |= def_flags;
 				break;
 			}
 			cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
@@ -791,6 +789,8 @@  int conf_write(const char *name)
 			sym_calc_value(sym);
 			if (!(sym->flags & SYMBOL_WRITE))
 				goto next;
+			if (sym_is_choice_value(sym) && !menu_is_visible(menu->parent))
+				goto next;
 			sym->flags &= ~SYMBOL_WRITE;
 
 			conf_write_symbol(out, sym, &kconfig_printer_cb, NULL);
@@ -1077,6 +1077,7 @@  static void randomize_choice_values(struct symbol *csym)
 		else {
 			sym->def[S_DEF_USER].tri = no;
 		}
+		sym->flags &= ~(SYMBOL_VALID);
 	}
 	csym->flags |= SYMBOL_DEF_USER;
 	/* clear VALID to get value calculated */