Re: linux-2.4.6-pre6: numerous dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

Re: linux-2.4.6-pre6: numerous dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Adam J. Richter]

I am following up my own message here.

>>> = Adam Richter
>>  = Alan Cox
>   = Adam Richter

>>> 	Argh!  I just accidentally sent and older version of my
>>> patch.  Here is the current version.  Sorry about that.

>>This just breaks stuff

>>> +for var in $(cat arch/*/config.in |
>>> +	     egrep -w -v '^[ 	]*int' |
>>> +             tr '   $"' '\n\n\n' |
>>> +	     egrep '^CONFIG_[A-Z0-9_]*$' |
>>> +	     sort -u) ; do
>>> +	define_bool "$var" "n"
>>> +done
>>> +set -f
>>>  

>>You've changed the entire semantics of dep_tristate by doing this

>	Please provide a real example.

	Although I am curious about what you had in mind about
dep_tristate, I no longer need to see this example to see a problem
with my patch.  My patch breaks detection of "new" variables in
arch/*/config.in by "make oldconfig."

	So, I guess something like Keith Owens's patch would be the
way to go, with some additional definitions (CONFIG_AGP, CONFIG_PCI,
CONFIG_ISA, CONFIG_EISA, CONFIG_PCMCIA, and possibly others).  I am
not sure which other conditionals might also be incorrectly ignored by
some instances of dep_xxx.  Below, I have included a list of the 52
CONFIG_* variables that are used as arguments to dep_xxx in 2.4.6-pre6
and appear in arch/*/config.in.

Adam J. Richter     __     ______________   4880 Stevens Creek Blvd, Suite 104
adam@yggdrasil.com     \ /                  San Jose, California 95129-1034
+1 408 261-6630         | g g d r a s i l   United States of America
fax +1 408 261-6631      "Free Software For The Rest Of Us."

CONFIG_ACPI
CONFIG_AGP
CONFIG_AMIGA
CONFIG_ARCH_ACORN
CONFIG_ARCH_ARCA5K
CONFIG_ARCH_CLPS711X
CONFIG_ARCH_FOOTBRIDGE
CONFIG_ARCH_NETWINDER
CONFIG_ARCH_SA1100
CONFIG_ATARI
CONFIG_ATM
CONFIG_BLK_DEV_IDE
CONFIG_BLK_DEV_LOOP
CONFIG_BLK_DEV_RAM
CONFIG_BUSMOUSE
CONFIG_CPU_26
CONFIG_CPU_32
CONFIG_DEBUG_LL
CONFIG_DEVFS_FS
CONFIG_DRM
CONFIG_EISA
CONFIG_EXPERIMENTAL
CONFIG_FOOTBRIDGE
CONFIG_GVPIOEXT
CONFIG_IDE
CONFIG_IEEE1394
CONFIG_IEEE1394_OHCI1394
CONFIG_INPUT
CONFIG_ISA
CONFIG_ISDN
CONFIG_MAC
CONFIG_MCA
CONFIG_MD
CONFIG_MODULES
CONFIG_NET
CONFIG_PARPORT
CONFIG_PCI
CONFIG_PCMCIA
CONFIG_PM
CONFIG_PPP
CONFIG_PROC_FS
CONFIG_SA1100_ASSABET
CONFIG_SBUS
CONFIG_SCSI
CONFIG_SERIAL
CONFIG_SGI
CONFIG_SOUND
CONFIG_SPARC64
CONFIG_UNIX98_PTYS
CONFIG_VIDEO_DEV
CONFIG_X86
CONFIG_ZORRO

Re: linux-2.4.6-pre6: numerous dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Russell King]

On Sat, Jun 30, 2001 at 07:57:10AM -0700, Adam J. Richter wrote:
> 	So, I guess something like Keith Owens's patch would be the
> way to go, with some additional definitions (CONFIG_AGP, CONFIG_PCI,
> CONFIG_ISA, CONFIG_EISA, CONFIG_PCMCIA, and possibly others).  I am
> not sure which other conditionals might also be incorrectly ignored by
> some instances of dep_xxx.  Below, I have included a list of the 52
> CONFIG_* variables that are used as arguments to dep_xxx in 2.4.6-pre6
> and appear in arch/*/config.in.

I have confirmed that Keith Owens patch doesn't work with xconfig - you
can't select any option which has been define_bool'd to 'n'.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4.6p6: numerous dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Riley Williams]

Hi Russell, Adam.

 >> So, I guess something like Keith Owens's patch would be the way
 >> to go, with some additional definitions (CONFIG_AGP, CONFIG_PCI,
 >> CONFIG_ISA, CONFIG_EISA, CONFIG_PCMCIA, and possibly others).
 >> I am not sure which other conditionals might also be incorrectly
 >> ignored by some instances of dep_xxx.  Below, I have included a
 >> list of the 52 CONFIG_* variables that are used as arguments to
 >> dep_xxx in 2.4.6-pre6 and appear in arch/*/config.in.

 > I have confirmed that Keith Owens patch doesn't work with
 > xconfig - you can't select any option which has been
 > define_bool'd to 'n'.

I've followed this thread with interest, and think I've followed both
sides of the argument, so can I summarise:

 1. Adam's point is that there are dep_* statements in the config
    setup that have been used to say that a particular option is
    dependant upon a particular architecture, but this doesn't work.

 2. Russell's point is that Adam's point is correct, but the patch
    that Adam submitted can't be used as it breaks other things.

 3. MY understanding of the situation is that ALL architecture
    specific config lines are EXPECTED to be in the arch/*/config.in
    files, where they will only even be seen when the relevant
    architecture is being compiled for.

As a result of this, I would summarise this discussion as saying that
there is a bug in the kernel config scripts in that some options that
should be located in the architecture-specific config files are in the
all-architecture config files instead.

Best wishes from Riley.

Re: linux-2.4.6-pre6: numerous dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Keith Owens]

On Sat, 30 Jun 2001 16:01:01 +0100, 
Russell King <rmk@arm.linux.org.uk> wrote:
>I have confirmed that Keith Owens patch doesn't work with xconfig - you
>can't select any option which has been define_bool'd to 'n'.

My patch only define_bool's the arch variables.  None of those are
selectable.

Re: 2.4.6p6: numerous dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Keith Owens]

On Sat, 30 Jun 2001 21:36:26 +0100 (BST), 
Riley Williams <rhw@MemAlpha.CX> wrote:
> 1. Adam's point is that there are dep_* statements in the config
>    setup that have been used to say that a particular option is
>    dependant upon a particular architecture, but this doesn't work.
>
> 3. MY understanding of the situation is that ALL architecture
>    specific config lines are EXPECTED to be in the arch/*/config.in
>    files, where they will only even be seen when the relevant
>    architecture is being compiled for.
>
>As a result of this, I would summarise this discussion as saying that
>there is a bug in the kernel config scripts in that some options that
>should be located in the architecture-specific config files are in the
>all-architecture config files instead.

(1) and (3) are correct but your conclusion is not.  The problem is

  dep_tristate CONFIG_some_driver $CONFIG_some_arch

where the intention is to allow the driver only if some_arch is set.
When you compile for some_other_arch, CONFIG_some_arch is undefined.
dep_tristate treats undefined variables as "don't care" and we cannot
fix that without changing bash or a major rewrite of the shell scripts.
There are two solutions, either change all such lines to

  if [ "$CONFIG_some_arch" = "y" ];then
    tristate CONFIG_some_driver
  fi

or

  define_bool CONFIG_some_arch n

for all architectures at the start, followed by turning on the one that
is required.

Lots of if statements are messy and they will not prevent somebody
adding new options with exactly the same problem.  Explicitly setting
all but one arch variable to 'n' results in cleaner config scripts and
new arch dependent driver settings will automatically work.

[PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Riley Williams]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4360 bytes --]

Hi Keith.

I don't agree with your analysis in the first block quoted below, but
that's not relevant as I feel there's a better solution than either of
the ones proposed so far. See below for details...

 >> 1. Adam's point is that there are dep_* statements in the config
 >>    setup that have been used to say that a particular option is
 >>    dependant upon a particular architecture, but this doesn't work.
 >>
 >> 3. MY understanding of the situation is that ALL architecture
 >>    specific config lines are EXPECTED to be in the arch/*/config.in
 >>    files, where they will only even be seen when the relevant
 >>    architecture is being compiled for.
 >>
 >> As a result of this, I would summarise this discussion as saying
 >> that there is a bug in the kernel config scripts in that some
 >> options that should be located in the architecture-specific
 >> config files are in the all-architecture config files instead.

 > (1) and (3) are correct but your conclusion is not. The problem is
 >
 >   dep_tristate CONFIG_some_driver $CONFIG_some_arch
 >
 > where the intention is to allow the driver only if some_arch is
 > set.

Can I suggest you re-read your comment and the points you quoted and
said are correct. As a DIRECT logical consequence of "(1) and (3) are
correct" (as you phrased it), we get the conclusion that any config
lines that depend on "if some arch is set" (as you phrased it) are in
the architecture-specific config files. This leads DIRECTLY to the
conclusion that ANY lines dependant on a particular architecture that
are not in the architecture-specific config files are a bug in the
kernel config scripts.

=======================================================================

Personally, I would suggest that the bug in this case is the actual
assumption that (3) is true, and the correct course of action is to
remove that assumption as it leads directly to the spaghetti in the
configuration system that we currently have.

 > When you compile for some_other_arch, CONFIG_some_arch is
 > undefined. dep_tristate treats undefined variables as "don't
 > care"...

Agreed - and whoever did the config lines where the dependancy is an
arch variable made the mistake of misunderstanding that.

 > ...and we cannot fix that without changing bash or a major
 > rewrite of the shell scripts.

Why is either needed? As I see it, the cure is to define a pair of new
statement types in the config language, as follows:

 1. dep_arch_bool CONFIG_var CONFIG_arch [CONFIG_other_var...]

    This states that CONFIG_var is a boolean var that depends both on
    CONFIG_arch being specifically "y" and on CONFIG_other_var being
    such as to satisfy dep_bool as currently.

 2. dep_arch_tristate CONFIG_var CONFIG_arch [CONFIG_other_var...]

    This is the tristate version of dep_arch_bool as expected.

When we've done that, ALL of the problem lines can be trivially
converted from dep_ to dep_arch_ and the problem vanishes. As a free
bonus, the need for the arch-specific config files vanishes as well,
and each option can be documented as to what architectures it is valid
for. This thus removes a possible source of bugs from the equation.

The enclosed patch (against 2.4.5 raw) adds these two statements to
both the `make config` and `make menuconfig` scripts. I don't
understand TCL/TK so can't add them to the `make xconfig` script, and
as I also don't understand PERL, I can't add it to `make checkconfig`
so somebody else will need to deal with those two scripts.

 > There are two solutions, either change all such lines to
 >
 >   if [ "$CONFIG_some_arch" = "y" ];then
 >     tristate CONFIG_some_driver
 >   fi
 >
 > or
 >
 >   define_bool CONFIG_some_arch n
 >
 > for all architectures at the start, followed by turning on the
 > one that is required.

Both are messy, and best described as kludges. I wouldn't support
either.

 > Lots of if statements are messy and they will not prevent
 > somebody adding new options with exactly the same problem.

Agreed.

 > Explicitly setting all but one arch variable to 'n' results in
 > cleaner config scripts and new arch dependent driver settings
 > will automatically work.

Until somebody adds a new port and fails to upgrade any one of the
various files doing this. That's an open recipe for bugs IMHO, and
should be avoided at all costs.

Best wishes from Riley.

[-- Attachment #2: Configure and Menuconfig patch --]
[-- Type: APPLICATION/x-gzip, Size: 1137 bytes --]

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Keith Owens]

On Mon, 2 Jul 2001 00:04:22 +0100 (BST), 
Riley Williams <rhw@MemAlpha.CX> wrote:
>Can I suggest you re-read your comment and the points you quoted and
>said are correct. As a DIRECT logical consequence of "(1) and (3) are
>correct" (as you phrased it), we get the conclusion that any config
>lines that depend on "if some arch is set" (as you phrased it) are in
>the architecture-specific config files. This leads DIRECTLY to the
>conclusion that ANY lines dependant on a particular architecture that
>are not in the architecture-specific config files are a bug in the
>kernel config scripts.

No.  We want network drivers to be in the network menu, SCSI drivers in
the SCSI menu etc.  Some of those drivers are restricted to some
architectures but putting them in the arch config splits the logical
flow of selection and duplicates text.  Look at the repeated SCSI code
in some of the arch configs.

> 2. dep_arch_tristate CONFIG_var CONFIG_arch [CONFIG_other_var...]

dep_arch_tristate '  AM79C961A support' CONFIG_ARM_AM79C961A $CONFIG_ARCH_ACORN $CONFIG_NET_ETHERNET

fails your code.  $CONFIG_ARCH_ACORN is undefined, $CONFIG_NET_ETHERNET
is 'y'.  dep_arch_tristate is invoked with 3 (not 4 parameters), text,
CONFIG_ARM_AM79C961A and 'y'.  The intervening undefined variable
disappears in shell scripts.

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Riley Williams]

Hi Keith.

 >> Can I suggest you re-read your comment and the points you quoted
 >> and said are correct. As a DIRECT logical consequence of "(1)
 >> and (3) are correct" (as you phrased it), we get the conclusion
 >> that any config lines that depend on "if some arch is set" (as
 >> you phrased it) are in the architecture-specific config files.
 >> This leads DIRECTLY to the conclusion that ANY lines dependant
 >> on a particular architecture that are not in the
 >> architecture-specific config files are a bug in the kernel
 >> config scripts.

 > No.  We want network drivers to be in the network menu, SCSI
 > drivers in the SCSI menu etc.  Some of those drivers are
 > restricted to some architectures but putting them in the arch
 > config splits the logical flow of selection and duplicates text.
 > Look at the repeated SCSI code in some of the arch configs.

That's a direct consequence of the policy quoted in paragraph (3) and
NOT an error in the logic. It's why I'm against the current policy.

 >> 2. dep_arch_tristate CONFIG_var CONFIG_arch [CONFIG_other_var...]

 > dep_arch_tristate ' AM79C961A support' CONFIG_ARM_AM79C961A \
 >	$CONFIG_ARCH_ACORN $CONFIG_NET_ETHERNET

 > fails your code.  $CONFIG_ARCH_ACORN is undefined,
 > $CONFIG_NET_ETHERNET is 'y'.  dep_arch_tristate is invoked with
 > 3 (not 4 parameters), text, CONFIG_ARM_AM79C961A and 'y'.  The
 > intervening undefined variable disappears in shell scripts.

You're not thinking clearly. Try the following simple fix to that:

 Q> dep_arch_tristate '  AM79C961A support' CONFIG_ARM_AM79C961A \
 Q>	"$CONFIG_ARCH_ACORN" $CONFIG_NET_ETHERNET

That adds only two extra characters, neither conspicuous, and PASSES
my code.

Best wishes from Riley.

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Keith Owens]

On Mon, 2 Jul 2001 08:16:50 +0100 (BST), 
Riley Williams <rhw@MemAlpha.CX> wrote:
> Q> dep_arch_tristate '  AM79C961A support' CONFIG_ARM_AM79C961A \
> Q>	"$CONFIG_ARCH_ACORN" $CONFIG_NET_ETHERNET
>
>That adds only two extra characters, neither conspicuous, and PASSES
>my code.

It relies on everybody writing new dep_arch_... rules to remember the
quotes.  You are just introducing another source of human error.  That
is how this mess occurred, no automatic validation of input.

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Riley Williams]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1125 bytes --]

Hi Keith.

 >> Q> dep_arch_tristate '  AM79C961A support' CONFIG_ARM_AM79C961A \
 >> Q>	"$CONFIG_ARCH_ACORN" $CONFIG_NET_ETHERNET

 >> That adds only two extra characters, neither conspicuous, and
 >> PASSES my code.

 > It relies on everybody writing new dep_arch_... rules to
 > remember the quotes.  You are just introducing another source of
 > human error.  That is how this mess occurred, no automatic
 > validation of input.

OK then, a simple change to the patch I submitted, and the following
instead...

 Q> dep_arch_tristate '  AM79C961A support' CONFIG_ARM_AM79C961A \
 Q>	ACORN $CONFIG_NET_ETHERNET

The enclosed patch implements this syntax, and CHECKS that the syntax
stated has been supplied, for both `make config` and `make menuconfig`
and I would imagine the same would be easily added to `make xconfig`
and `make checkconfig` as well by those fluent in the relevant
languages.

Note specifically that it requires that the CONFIG_ARCH_ part of the
name of the variable is NOT provided, and as a result requires that
config variables naming the architecture begin with that string.

Best wishes from Riley.

[-- Attachment #2: Patch v2 --]
[-- Type: APPLICATION/x-gzip, Size: 1555 bytes --]

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Russell King]

On Mon, Jul 02, 2001 at 09:25:50AM +0100, Riley Williams wrote:
>  Q> dep_arch_tristate '  AM79C961A support' CONFIG_ARM_AM79C961A \
>  Q>	ACORN $CONFIG_NET_ETHERNET

Before we go and create a patch for Linus to apply, please note that the
above is totally bogus and is in fact 100% wrong.  Don't create a patch
yourself.  Let me know how you propose to do it, and I will create the
patch using the correct symbols.

Also note that the majority of the machine-dependent symbols for StrongARM
platforms (of which there are around 43) start CONFIG_SA1100_*, not
CONFIG_ARCH_*.  Unfortunately, its far too late to get around this
special case (I'm not too happy that we have this special case either,
so don't whinge at me please).

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Riley Williams]

Hi Russell.

 >> Q> dep_arch_tristate '  AM79C961A support' CONFIG_ARM_AM79C961A \
 >> Q>		ACORN $CONFIG_NET_ETHERNET

 > Before we go and create a patch for Linus to apply...

Who's sent a patch to Linus? I haven't, and don't intend to do so.

 > ...please note that the above is totally bogus and is in fact
 > 100% wrong.

Please explain? I don't see the problem with the proposed syntax, and
presume you have noticed that it's a new statement type, not a
modification to an existing one?

 > Don't create a patch yourself. Let me know how you propose to do
 > it, and I will create the patch using the correct symbols.

OK, here's the proposal:

 1. Define new config statements that take as their parameters the
    following:

	1. The prompt text.

	2. The name of the config variable to set.

	3. The NAME of the config variable that defines the
	   required architecture.

	4. The VALUES of all other config variables this variable
	   depends on.

 2. Define the new `dep_arch_bool` statement to be the same as
    `dep_bool` if passed items 1, 2 and 4 when the config var
    named in item 3 has the value "y". When the config var named
    has any other value, it becomes `define_bool "$2" "n" instead.

 3. Define the new `dep_arch_tristate` similarly.

 > Also note that the majority of the machine-dependent symbols for
 > StrongARM platforms (of which there are around 43) start
 > CONFIG_SA1100_*, not CONFIG_ARCH_*.  Unfortunately, its far too
 > late to get around this special case (I'm not too happy that we
 > have this special case either, so don't whinge at me please).

First, why is it "far too late" as you put it? It won't be the first
time config vars have been renamed, and it's unlikely to be the last
either...

Secondly, that isn't a problem to this proposal. The reason for taking
the CONFIG_ARCH_ out of the syntax is to emphasise that it's the name
and not the value that goes in there, thus reducing the likelihood of
problems creeping in in the first place.

Best wishes from Riley.

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Russell King]

On Mon, Jul 02, 2001 at 01:40:25PM +0100, Riley Williams wrote:
> Who's sent a patch to Linus? I haven't, and don't intend to do so.

The way this thread is progressing, it won't take much to get there.  We've
already propagated the inaccuracies in the "example" depencencies by 3 or
more mails.  I'm just trying to stop it before someone forgets that it's
just an example.

> First, why is it "far too late" as you put it? It won't be the first
> time config vars have been renamed, and it's unlikely to be the last
> either...

I'm not going to cause disruption across the board to lots of people just
because someone wants to keep the length of the symbols down.

If you really do want to do this change, then I suggest that you get in
touch with Nicolas Pitre and discuss it with him.  When you come to a
conclusion, its not as simple as patching the kernel.  You need to
update the database at www.arm.linux.org.uk/developer/machines/ to
reflect the new symbols _at the same time_ that you change them in
everyones tree, since anyone can pull down a new copy of the symbols
from the database at any time.

IMHO, a logistical nightmare to perform.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] Re: 2.4.6p6: dep_{bool,tristate} $CONFIG_ARCH_xxx bugs

[Nicolas Pitre]

On Mon, 2 Jul 2001, Russell King wrote:

> > First, why is it "far too late" as you put it? It won't be the first
> > time config vars have been renamed, and it's unlikely to be the last
> > either...

Can we just break everything apart in 2.5 please?  Will this still be an
issue with CML2 anyway?

> I'm not going to cause disruption across the board to lots of people just
> because someone wants to keep the length of the symbols down.
>
> If you really do want to do this change, then I suggest that you get in
> touch with Nicolas Pitre and discuss it with him.  When you come to a
> conclusion, its not as simple as patching the kernel.  You need to
> update the database at www.arm.linux.org.uk/developer/machines/ to
> reflect the new symbols _at the same time_ that you change them in
> everyones tree, since anyone can pull down a new copy of the symbols
> from the database at any time.

For instance we might just keep the SA1100 out of the picture until we're
ready to make such change as "atomic" as possible for all people involved.


Nicolas