LinuxLists.cc - [GIT] kbuild: kconfig changes

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On Wed, Aug 4, 2010 at 5:51 AM, Michal Marek <[email protected]> wrote:
>
> this is the kconfig part of kbuild. We have four new *config targets:
> * oldnoconfig: set all new options to 'n'
> * listnewconfig: list all unset config options
> * alldefconfig: set all options to their defaults specified in Kconfig
> files
> * savedefconfig: write a defconfig file with only the differences from
> an alldefconfig (aka minimal defconfig)
>
> Kconfig also warns when a select statement selects a symbol with unmet
> dependencies (which typically results in a broken config). Li Zefan did
> quite some usability fixes to the visual config interfaces.

Hmm. This seems to make "make oldconfig" a _lot_ more verbose than it
used to be. In a very annoying way.

I'm used to this (v2.6.35 "make oldconfig" with no changes):

[torvalds@i5 linux]$ make oldconfig
scripts/kconfig/conf -o arch/x86/Kconfig
#
# configuration written to .config
#
[torvalds@i5 linux]$

but now it prints _everything_. The old "oldconfig" only printed
things out when there was something to be asked about.

This is a regression.

Linus

2010-08-06 01:26:47

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On 08/05/2010 04:33 PM, Linus Torvalds wrote:
> On Wed, Aug 4, 2010 at 5:51 AM, Michal Marek<[email protected]> wrote:
>>
>> this is the kconfig part of kbuild. We have four new *config targets:
>> * oldnoconfig: set all new options to 'n'
>> * listnewconfig: list all unset config options
>> * alldefconfig: set all options to their defaults specified in Kconfig
>> files
>> * savedefconfig: write a defconfig file with only the differences from
>> an alldefconfig (aka minimal defconfig)
>>
>> Kconfig also warns when a select statement selects a symbol with unmet
>> dependencies (which typically results in a broken config). Li Zefan did
>> quite some usability fixes to the visual config interfaces.
>
> Hmm. This seems to make "make oldconfig" a _lot_ more verbose than it
> used to be. In a very annoying way.
>
> I'm used to this (v2.6.35 "make oldconfig" with no changes):
>
> [torvalds@i5 linux]$ make oldconfig
> scripts/kconfig/conf -o arch/x86/Kconfig
> #
> # configuration written to .config
> #
> [torvalds@i5 linux]$
>
> but now it prints _everything_. The old "oldconfig" only printed
> things out when there was something to be asked about.
>
> This is a regression.
>
> Linus
>

With what I submitted I did not test make oldconfig, only make
menuconfig due to gcc 4.6.0 giving me a slew of warnings.
From looking at make oldconfig I do see a more detailed explanation of
what each option is(last I remember it was more of a line with y/n)but
then again you could be seeing something completly different.

keep in mind not sure if this is with kconfig merge or not due to me
seeing make oldconfig react the same way with 2.6.35-rc6-00191-ga2dccdb
and the current(pulled a few minuetes ago)
but look kind of differently with 2.6.34(but could be wrong due to not
using make oldconfig that much).
Now if this is with what I submitted then I'll have a look at it and see
if I can get this fixed.

Justin P. Mattock

2010-08-06 02:10:05

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On Thu, Aug 5, 2010 at 6:27 PM, Justin P. Mattock
<[email protected]> wrote:
>
> With what I submitted I did not test make oldconfig, only make menuconfig
> due to gcc 4.6.0 giving me a slew of warnings.
> From looking at make oldconfig I do see a more detailed explanation of what
> each option is(last I remember it was more of a line with y/n)but then again
> you could be seeing something completly different.

Just test what I described: do a "make oldconfig" (do it twice, if
the first one needs to actually get some input). Do it on 2.6.35, and
then on the current tree. Look at the _huge_ difference in output.

> Now if this is with what I submitted then I'll have a look at it and see if I can get this fixed.

I just did a quick git bisect. It's introduced by commit 4062f1a4c030
("kconfig: use long options in conf") by Sam Ravnborg. Apparently that
thing is totally buggy, and doesn't just change the option names, but
actively breaks them.

Linus

2010-08-06 02:54:14

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On 08/05/2010 07:08 PM, Linus Torvalds wrote:
> On Thu, Aug 5, 2010 at 6:27 PM, Justin P. Mattock
> <[email protected]> wrote:
>>
>> With what I submitted I did not test make oldconfig, only make menuconfig
>> due to gcc 4.6.0 giving me a slew of warnings.
>> From looking at make oldconfig I do see a more detailed explanation of what
>> each option is(last I remember it was more of a line with y/n)but then again
>> you could be seeing something completly different.
>
> Just test what I described: do a "make oldconfig" (do it twice, if
> the first one needs to actually get some input). Do it on 2.6.35, and
> then on the current tree. Look at the _huge_ difference in output.
>

yeah I see what your hitting, just experienced what you posted

make oldconfig
scripts/kconfig/conf -o arch/x86/Kconfig
#
# configuration written to .config
#

(just sits there)

seemed to not want to go away.(doing a make menuconfig got
make oldconfig to wake up of whatever it was in) as for reproducing
(even though already bisected) seems once make oldconfig starts working
properly, hitting the above doesn't happen as easily.

>> Now if this is with what I submitted then I'll have a look at it and see if I can get this fixed.
>
> I just did a quick git bisect. It's introduced by commit 4062f1a4c030
> ("kconfig: use long options in conf") by Sam Ravnborg. Apparently that
> thing is totally buggy, and doesn't just change the option names, but
> actively breaks them.
>
> Linus
>

cool..

Justin P. Mattock

2010-08-06 05:13:59

[permalink] [raw]

Subject: [PATCH] kconfig: fix make oldconfig

[PATCH] kconfig: fix make oldconfig

Linus wrote:
This seems to make "make oldconfig" a _lot_ more verbose than it
used to be. In a very annoying way.

I just did a quick git bisect. It's introduced by commit 4062f1a4c030
("kconfig: use long options in conf") by Sam Ravnborg. Apparently that
thing is totally buggy, and doesn't just change the option names, but
actively breaks them.

The old behaviour (from years ago) were reintroduced by accident.
Fix this so we are back to the version that are silent
if there is nothing to ask about.

Reported-by: Linus Torvalds <[email protected]>
Signed-off-by: Sam Ravnborg <[email protected]>
---

Sorry for this regression. Dunno how I missed it.
I guess I only tested "silentoldconfig".

Following patch seems obviously correct but as I am on the way
out of the door I could not do much testing.

Sam

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 010600e..274f271 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -599,12 +599,12 @@ int main(int ac, char **av)
break;
case savedefconfig:
break;
- case oldconfig:
case oldaskconfig:
rootEntry = &rootmenu;
conf(&rootmenu);
input_mode = silentoldconfig;
/* fall through */
+ case oldconfig:
case listnewconfig:
case oldnoconfig:
case silentoldconfig:

2010-08-06 06:01:45

[permalink] [raw]

Subject: Re: [PATCH] kconfig: fix make oldconfig

> [PATCH] kconfig: fix make oldconfig
>
> Linus wrote:
> This seems to make "make oldconfig" a _lot_ more verbose than it
> used to be. In a very annoying way.
>
> I just did a quick git bisect. It's introduced by commit 4062f1a4c030
> ("kconfig: use long options in conf") by Sam Ravnborg. Apparently that
> thing is totally buggy, and doesn't just change the option names, but
> actively breaks them.
>
> The old behaviour (from years ago) were reintroduced by accident.
> Fix this so we are back to the version that are silent
> if there is nothing to ask about.
>
> Reported-by: Linus Torvalds<[email protected]>
> Signed-off-by: Sam Ravnborg<[email protected]>
> ---
>
> Sorry for this regression. Dunno how I missed it.
> I guess I only tested "silentoldconfig".
>
> Following patch seems obviously correct but as I am on the way
> out of the door I could not do much testing.
>
> Sam
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 010600e..274f271 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -599,12 +599,12 @@ int main(int ac, char **av)
> break;
> case savedefconfig:
> break;
> - case oldconfig:
> case oldaskconfig:
> rootEntry =&rootmenu;
> conf(&rootmenu);
> input_mode = silentoldconfig;
> /* fall through */
> + case oldconfig:
> case listnewconfig:
> case oldnoconfig:
> case silentoldconfig:
>

Id like to be more useful with really hitting this, seems I was for a
few moments, then something in there tripped and caused oldconfig to
behave.. anyways only real evidence I have of hitting this is an fpaste
of my shell output http://fpaste.org/LFew/

Anyways applied your patch below, make oldconfig worked as is no:

make oldconfig
scripts/kconfig/conf -o arch/x86/Kconfig
#
# configuration written to .config
#

that I was hitting for some reason or another then all of a sudden
disappeared.

Justin P. Mattock

2010-08-06 10:23:21

by Michal Marek

[permalink] [raw]

Subject: Re: [PATCH] kconfig: fix make oldconfig

On 6.8.2010 07:13, Sam Ravnborg wrote:
> [PATCH] kconfig: fix make oldconfig
>
> Linus wrote:
> This seems to make "make oldconfig" a _lot_ more verbose than it
> used to be. In a very annoying way.
>
> I just did a quick git bisect. It's introduced by commit 4062f1a4c030
> ("kconfig: use long options in conf") by Sam Ravnborg. Apparently that
> thing is totally buggy, and doesn't just change the option names, but
> actively breaks them.
>
> The old behaviour (from years ago) were reintroduced by accident.
> Fix this so we are back to the version that are silent
> if there is nothing to ask about.
>
> Reported-by: Linus Torvalds <[email protected]>
> Signed-off-by: Sam Ravnborg <[email protected]>
> ---
>
> Sorry for this regression. Dunno how I missed it.
> I guess I only tested "silentoldconfig".
>
> Following patch seems obviously correct but as I am on the way
> out of the door I could not do much testing.
>
> Sam
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 010600e..274f271 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -599,12 +599,12 @@ int main(int ac, char **av)
> break;
> case savedefconfig:
> break;
> - case oldconfig:
> case oldaskconfig:
> rootEntry = &rootmenu;
> conf(&rootmenu);
> input_mode = silentoldconfig;
> /* fall through */
> + case oldconfig:
> case listnewconfig:
> case oldnoconfig:
> case silentoldconfig:

Reviewed-by: Michal Marek <[email protected]>

The problem is that in the previous code, input_mode could never be set
to 'ask_new' ('oldconfig' in the new code), both oldconfig and
silentoldconfig set the variable to 'ask_silent'. Therefore, the 'case
ask_new:' label had no effect here. The fix is correct, both oldconfig
and silentoldconfig jump to the same place again.

Linus, will you apply this directly or should I apply it to the kbuild
tree and send you a pull request?

Michal

2010-08-06 16:20:07

[permalink] [raw]

Subject: Re: [PATCH] kconfig: fix make oldconfig

On Fri, Aug 6, 2010 at 3:21 AM, Michal Marek <[email protected]> wrote:
>
> Linus, will you apply this directly or should I apply it to the kbuild
> tree and send you a pull request?

I took it directly, since I spend a lot of time doing "make oldconfig
; make -j16" which is why I found this (after pulling, I tend verify
that at least nothing broke my _own_ configuration)

Linus

2010-08-06 17:52:21

[permalink] [raw]

Subject: Re: [PATCH] kconfig: fix make oldconfig

On Fri, Aug 06, 2010 at 09:19:23AM -0700, Linus Torvalds wrote:
> On Fri, Aug 6, 2010 at 3:21 AM, Michal Marek <[email protected]> wrote:
> >
> > Linus, will you apply this directly or should I apply it to the kbuild
> > tree and send you a pull request?
>
> I took it directly, since I spend a lot of time doing "make oldconfig
> ; make -j16" which is why I found this (after pulling, I tend verify
> that at least nothing broke my _own_ configuration)

Hmm, I wonder why you call oldconfig explicitly?

A plain "make -j16" executes "silentoldconfig" if there
is any changes in a Kconfig* file or in .config.
Just double checked and it works as I expected.

So you are asked if there is any new options anyway even
if you skip your "oldconfig" step.

The difference between oldconfig and silentoldconfig these days
are just that silentoldconfig updates include/config/*
which oldconfig does not.

It could be as simple as old habits.
Just curious to know if there is something I have missed.

Sam

2010-08-06 18:10:46

[permalink] [raw]

Subject: Re: [PATCH] kconfig: fix make oldconfig

On Fri, Aug 6, 2010 at 10:52 AM, Sam Ravnborg <[email protected]> wrote:
>
> Hmm, I wonder why you call oldconfig explicitly?
>
> A plain "make -j16" executes "silentoldconfig" if there
> is any changes in a Kconfig* file or in .config.
> Just double checked and it works as I expected.
>
> So you are asked if there is any new options anyway even
> if you skip your "oldconfig" step.

Try this:

git clean -dqfx
make -j16 > ../makes

It doesn't work, because "make silentconfig" will say

***
*** You have not yet configured your kernel!
*** (missing kernel config file ".config")
***
*** Please run some configurator (e.g. "make oldconfig" or
*** "make menuconfig" or "make xconfig").
***

which is why I always run "make oldconfig".

Sure, I could do it only when I need to, but quite frankly, it's much
easier to just always do the thing that works, rather than try
something that doesn't work, do something else, and then re-try the
thing that can fail.

Linus

2010-08-06 19:51:59

[permalink] [raw]

Subject: Re: [PATCH] kconfig: fix make oldconfig

On 08/06/2010 11:09 AM, Linus Torvalds wrote:
> On Fri, Aug 6, 2010 at 10:52 AM, Sam Ravnborg<[email protected]> wrote:
>>
>> Hmm, I wonder why you call oldconfig explicitly?
>>
>> A plain "make -j16" executes "silentoldconfig" if there
>> is any changes in a Kconfig* file or in .config.
>> Just double checked and it works as I expected.
>>
>> So you are asked if there is any new options anyway even
>> if you skip your "oldconfig" step.
>
> Try this:
>
> git clean -dqfx
> make -j16> ../makes
>
> It doesn't work, because "make silentconfig" will say
>
> ***
> *** You have not yet configured your kernel!
> *** (missing kernel config file ".config")
> ***
> *** Please run some configurator (e.g. "make oldconfig" or
> *** "make menuconfig" or "make xconfig").
> ***
>
> which is why I always run "make oldconfig".
>
> Sure, I could do it only when I need to, but quite frankly, it's much
> easier to just always do the thing that works, rather than try
> something that doesn't work, do something else, and then re-try the
> thing that can fail.
>
> Linus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

the above command does break over here. using the make oldconfig command
seems to be working after I did the make menuconfig.

Now before doing all of this I did make a copy of the entire kernel for
a new system Im building(clfs) so I went into that tree and did a make
oldconfig and(luckily) hit the non responsive oldconfig thing that Linus
had originally posted.

here is a strace of when make oldconfig was not working(with git log at
the top)

http://fpaste.org/317B/

and strace of a git pull today and make oldconfig does not crap out and
starts asking me y/n options:

http://fpaste.org/QWWn/

hope this helps with debugging and such.

Justin P. Mattock

2010-08-06 23:19:23

by Arve Hjønnevåg

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On Wed, Aug 4, 2010 at 5:51 AM, Michal Marek <[email protected]> wrote:
> Hi Linus,
>
> this is the kconfig part of kbuild. We have four new *config targets:
> * oldnoconfig: set all new options to 'n'
> * listnewconfig: list all unset config options
> * alldefconfig: set all options to their defaults specified in Kconfig
> ?files
> * savedefconfig: write a defconfig file with only the differences from
> ?an alldefconfig (aka minimal defconfig)
>
> Kconfig also warns when a select statement selects a symbol with unmet
> dependencies (which typically results in a broken config). Li Zefan did
> quite some usability fixes to the visual config interfaces.
>
> Michal
>
> The following changes since commit 9fe6206f400646a2322096b56c59891d530e8d51:
>
> ?Linux 2.6.35 (2010-08-01 15:11:14 -0700)
>
> are available in the git repository at:
> ?git://git.kernel.org/pub/scm/linux/kernel/git/mmarek/kbuild-2.6.git kconfig
>
> Aristeu Rozanski (1):
> ? ? ?kconfig: introduce nonint_oldconfig and loose_nonint_oldconfig
>
> Catalin Marinas (1):
> ? ? ?kbuild: Warn on selecting symbols with unmet direct dependencies
>
> Jan Beulich (1):
> ? ? ?kconfig: Don't write invisible choice values

This change prevents some the minimal defconfig options from working.
Specifically our usb gadget drivers do not get set. Reverting this
part of the changes fixes my problem, but I don't know what other side
effects it may have:

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index e95718f..1797226 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -261,18 +261,14 @@ struct symbol *sym_choice_default(struct symbol *sym)
static struct symbol *sym_calc_choice(struct symbol *sym)
{
struct symbol *def_sym;
- struct property *prop;
- struct expr *e;
-
- /* first calculate all choice values' visibilities */
- prop = sym_get_choice_prop(sym);
- expr_list_for_each_sym(prop->expr, e, def_sym)
- sym_calc_visibility(def_sym);

/* is the user choice visible? */
def_sym = sym->def[S_DEF_USER].val;
- if (def_sym && def_sym->visible != no)
- return def_sym;
+ if (def_sym) {
+ sym_calc_visibility(def_sym);
+ if (def_sym->visible != no)
+ return def_sym;
+ }

def_sym = sym_choice_default(sym);

--
Arve Hj?nnev?g

2010-08-07 04:01:10

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

>
> This change prevents some the minimal defconfig options from working.
> Specifically our usb gadget drivers do not get set.

Can you help me reproduce this?

I have found an issue with choice values in combination with
tristate logic that fails. I hope this is something similar.

Sam

2010-08-07 04:43:28

by Arve Hjønnevåg

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On Fri, Aug 6, 2010 at 9:01 PM, Sam Ravnborg <[email protected]> wrote:
>>
>> This change prevents some the minimal defconfig options from working.
>> Specifically our usb gadget drivers do not get set.
>
> Can you help me reproduce this?
>
> I have found an issue with choice values in combination with
> tristate logic that fails. I hope this is something similar.
>

It is probably the same problem. The gadget driver that was not set is
not buildable as a module (it is not in the mainline kernel). If I
select another gadget driver instead it just gets changed to build as
a module instead.

If you create a file, arch/arm/configs/test_defconfig with the following:
CONFIG_MODULES=y
CONFIG_USB_GADGET=y
CONFIG_USB_MASS_STORAGE=y

then "make test_defconfig" results in .config having:
CONFIG_USB_MASS_STORAGE=m

(at least if you are set up to compile for arm)

--
Arve Hj?nnev?g

2010-08-08 15:57:33

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

2010-08-10 14:07:05

by Michal Marek

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On 8.8.2010 17:57, Sam Ravnborg wrote:
> On Fri, Aug 06, 2010 at 09:43:24PM -0700, Arve Hj?nnev?g wrote:
>> On Fri, Aug 6, 2010 at 9:01 PM, Sam Ravnborg <[email protected]> wrote:
>>>>
>>>> This change prevents some the minimal defconfig options from working.
>>>> Specifically our usb gadget drivers do not get set.
>>>
>>> Can you help me reproduce this?
>>>
>>> I have found an issue with choice values in combination with
>>> tristate logic that fails. I hope this is something similar.
>>>
>>
>> It is probably the same problem. The gadget driver that was not set is
>> not buildable as a module (it is not in the mainline kernel). If I
>> select another gadget driver instead it just gets changed to build as
>> a module instead.
>>
>> If you create a file, arch/arm/configs/test_defconfig with the following:
>> CONFIG_MODULES=y
>> CONFIG_USB_GADGET=y
>> CONFIG_USB_MASS_STORAGE=y
>>
>> then "make test_defconfig" results in .config having:
>> CONFIG_USB_MASS_STORAGE=m
>>
>> (at least if you are set up to compile for arm)
>
> Thanks Arve.
>
> I have it reproduced now with a simple Kconfig:
>
> $ cat Kconfig
> config M
> def_bool y
> option modules
>
> choice
> prompt "choice list"
>
> config A
> tristate "a"
>
> config B
> tristate "b"
>
> endchoice
>
> $cat defconfig
> CONFIG_M=y
> CONFIG_A=y
> # CONFIG_B is not set
>
>
> If I do:
>
> $scripts/kconfig/conf --defconfig=defconfig Kconfig
>
> with the above input the resulting .config is OK.
>
> But If I drop the line:
>
> # CONFIG_B is not set
>
> in the defconfig file then I end with CONFIG_A set to m.
> And this is not as expected - I cannot see why it should matter
> if we specify the value of B or not.
>
> What we see here is that savedefconfig trigger a bug in the
> other part of kconfig - a bug which was not exposed before.
>
> The reason why your patch cured it was that we then no
> longer triggered the bug (at least I guess so I did not look to close).
>
> I will look into this as time permits. I assume the fix is simple
> when I find the reason.

I'm looking into it now, but understanding the kconfig internals is not
easy...

Michal

2010-08-10 14:25:27

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

> >
> > The reason why your patch cured it was that we then no
> > longer triggered the bug (at least I guess so I did not look to close).
> >
> > I will look into this as time permits. I assume the fix is simple
> > when I find the reason.
>
> I'm looking into it now, but understanding the kconfig internals is not
> easy...
Nope...

What I have understood so far...

If we have a choice then the choice is represented by a symbol.
symbol->def[S_DEF_USER] is set to the selected symbol.
If we read all the choice values then we set:
symbol->flags |= SYMBOL_DEF_USER

If we have a choice where we do not have all choice_values
which is the case with the minimal defconfig then the
missing choice_value results in that SYMBOL_DEF_USER
is not set.
[See confdata:conf_read after the sym_ok label]

Later in symbol.c:sym_calc_value the visibility
is set to mod if we do not have all values.

And therefore the coice value is set to m not y.

I will look a bit more at it tonight.

Sam

2010-08-11 19:51:36

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On Fri, Aug 06, 2010 at 09:43:24PM -0700, Arve Hj?nnev?g wrote:
> On Fri, Aug 6, 2010 at 9:01 PM, Sam Ravnborg <[email protected]> wrote:
> >>
> >> This change prevents some the minimal defconfig options from working.
> >> Specifically our usb gadget drivers do not get set.
> >
> > Can you help me reproduce this?
> >
> > I have found an issue with choice values in combination with
> > tristate logic that fails. I hope this is something similar.
> >
>
> It is probably the same problem. The gadget driver that was not set is
> not buildable as a module (it is not in the mainline kernel). If I
> select another gadget driver instead it just gets changed to build as
> a module instead.
>
> If you create a file, arch/arm/configs/test_defconfig with the following:
> CONFIG_MODULES=y
> CONFIG_USB_GADGET=y
> CONFIG_USB_MASS_STORAGE=y
>
> then "make test_defconfig" results in .config having:
> CONFIG_USB_MASS_STORAGE=m
>
> (at least if you are set up to compile for arm)

Arve, can you please try the attached patch and check if this
fixes the issue you see.
At least I confirmed that it fixed the minimal test-case I used.

Note: the patch chenges how we interpret a minimal config,
it does not change the content of a minimal config.

Unfortunately the bug I had discovered is another bug.
If I do:
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

then the diff shows an unexpected difference.

I will continue of that issue.
This is mainly to inform you that we have at least one
additional issue with "savedefconfig".

Sam

>From 6f36cdab76c4ee26bc39c38a2895cac5166e253b Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <[email protected]>
Date: Wed, 11 Aug 2010 21:40:23 +0200
Subject: [PATCH] kconfig: fix tristate choice with minimal config

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 <[email protected]>
Cc: Arve Hj?nnev?g <[email protected]>
---
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 f81f263..23d4fa6 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -919,13 +919,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))
@@ -961,8 +1021,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
@@ -977,35 +1035,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

2010-08-11 20:34:57

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

>
> Unfortunately the bug I had discovered is another bug.
> If I do:
> 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
>
> then the diff shows an unexpected difference.
>
> I will continue of that issue.
Fixed it - see patch below.

@Michal - I will do a proper patch submission if Arve
can confirm that the problem he saw has been fixed
with these patches.

Sam

>From d93f83bbd650499c146f29a8a4de2b1f98dfbefe Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <[email protected]>
Date: Wed, 11 Aug 2010 22:29:05 +0200
Subject: [PATCH] kconfig: fix savedefconfig for tristate choices

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 <[email protected]>
---
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

2010-08-11 23:39:41

by Arve Hjønnevåg

[permalink] [raw]

Subject: Re: [GIT] kbuild: kconfig changes

On Wed, Aug 11, 2010 at 1:34 PM, Sam Ravnborg <[email protected]> wrote:
>>
>> Unfortunately the bug I had discovered is another bug.
>> If I do:
>> ? ? ? 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
>>
>> then the diff shows an unexpected difference.
>>
>> I will continue of that issue.
> Fixed it - see patch below.
>
> @Michal - I will do a proper patch submission if Arve
> can confirm that the problem he saw has been fixed
> with these patches.
>

Your first patch fixes the problem I saw.

--
Arve Hj?nnev?g

2010-08-12 03:45:09