defconfig's shouldn't set CONFIG_BROKEN=y

defconfig's shouldn't set CONFIG_BROKEN=y.

---

 arch/arm/configs/bast_defconfig       |    3 +--
 arch/arm/configs/collie_defconfig     |    3 +--
 arch/arm/configs/s3c2410_defconfig    |    3 +--
 arch/ia64/configs/sim_defconfig       |    3 +--
 arch/ia64/configs/zx1_defconfig       |    3 +--
 arch/m32r/mappi/defconfig.smp         |    3 +--
 arch/m32r/mappi/defconfig.up          |    3 +--
 arch/m32r/mappi3/defconfig.smp        |    3 +--
 arch/parisc/configs/712_defconfig     |    3 +--
 arch/parisc/configs/a500_defconfig    |    3 +--
 arch/parisc/configs/c3000_defconfig   |    3 +--
 arch/ppc/configs/mpc86x_ads_defconfig |    3 +--
 arch/ppc/configs/mpc885ads_defconfig  |    3 +--
 arch/ppc/configs/rpxcllf_defconfig    |    3 +--
 arch/ppc/configs/rpxlite_defconfig    |    3 +--
 arch/sh/configs/hp680_defconfig       |    3 +--
 arch/sh/configs/sh03_defconfig        |    3 +--
 arch/sh/configs/systemh_defconfig     |    3 +--
 18 files changed, 18 insertions(+), 36 deletions(-)

@@ -14,8 +14,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 
 #

@@ -13,8 +13,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 CONFIG_INIT_ENV_ARG_LIMIT2

@@ -13,8 +13,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_INIT_ENV_ARG_LIMIT2
 

@@ -6,9 +6,8 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
+CONFIG_CLEAN_COMPILE=y
 # CONFIG_STANDALONE is not set
-CONFIG_BROKEN=y
 CONFIG_BROKEN_ON_SMP=y
 
 #

@@ -8,8 +8,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 CONFIG_INIT_ENV_ARG_LIMIT2

@@ -13,8 +13,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 CONFIG_INIT_ENV_ARG_LIMIT2

@@ -13,8 +13,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 CONFIG_INIT_ENV_ARG_LIMIT2

@@ -13,8 +13,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 CONFIG_INIT_ENV_ARG_LIMIT2

@@ -16,8 +16,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_INIT_ENV_ARG_LIMIT2
 

@@ -16,8 +16,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 CONFIG_INIT_ENV_ARG_LIMIT2

@@ -16,8 +16,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_INIT_ENV_ARG_LIMIT2
 

@@ -17,8 +17,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_INIT_ENV_ARG_LIMIT2
 

@@ -17,8 +17,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_INIT_ENV_ARG_LIMIT2
 

@@ -15,8 +15,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 
 #

@@ -15,8 +15,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 
 #

@@ -14,8 +14,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 
 #

@@ -14,8 +14,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 

@@ -14,8 +14,7 @@
 # Code maturity level options
 #
 CONFIG_EXPERIMENTAL=y
-# CONFIG_CLEAN_COMPILE is not set
-CONFIG_BROKEN=y
+CONFIG_CLEAN_COMPILE=y
 CONFIG_BROKEN_ON_SMP=y
 CONFIG_LOCK_KERNEL=y
 

NACK. This changes other configuration options in addition, for example in collie_defconfig:

-CONFIG_MTD_OBSOLETE_CHIPS=y
-# CONFIG_MTD_AMDSTD is not set
-CONFIG_MTD_SHARP=y
-# CONFIG_MTD_JEDEC is not set

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

It's a problem introduced by your patch because the resulting defconfig file becomes _wrong_ by your change, and other changes in the defconfig are thereby hidden.

If you change any options in a defconfig file, and they're obviously not leaf options, you should check what impact they have on other options by running it through an "oldconfig" cycle. That's what I just did with this script:

#!/bin/sh -e
alias amake='make CROSS_COMPILE=arm-linux- ARCH=arm'
amake $1 O=../build/t >/dev/null 2>&1
mv ../build/t/.config ../build/t/.config.orig
sed '/CONFIG_BROKEN/d;s,^# CONFIG_CLEAN_COMPILE is not set,CONFIG_CLEAN_COMPILE=y,' < ../build/t/.config.orig > ../build/t/.config amake oldconfig O=../build/t >/dev/null 2>&1
diff -u ../build/t/.config.orig ../build/t/.config

Hence I discovered that disabling CONFIG_BROKEN removes the above options for the collie case.

BTW, it might be worth using something like the above script for all the changes to the defconfig files in your patch so that it correctly updates these files. It will also mean that any review of it is more meaningful because we can see the full extent of your changes.

That's something which collie folk need to comment on. However, what I can say is that the collie_defconfig builds successfully today:

http://armlinux.simtec.co.uk/kautobuild/2.6.15-rc5-git1/collie_defconfig/zimage.log

so it's quite possible that the Kconfig is out of sync with reality.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

No, CONFIG_BROKEN=y in a defconfig file is a bug.

Either the defconfig doesn't use BROKEN code in which case it's simply wrong, or it's a wrong workaround (as in the CONFIG_MTD_SHARP case) for a wrong BROKEN dependency.

And it's a dangerous workaround:

Consider e.g. that "both marked as obsolete and BROKEN" are the best candidates for "remove obsolete code" cleanups - and there goes your driver to /dev/null ...

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Hi,

Indeed, but that's not the point. A defconfig file should be the result of running one of the various configuration targets; yours are hand-patched. If you run the defconfig target, it will copy the config file and run oldconfig, thus resulting in a different configuration file (because options may now be gone and hence disabled) than what was in the defconfig, and thus people may come to the wrong conclusion that if a driver is enabled in a defconfig file, it will be built.

Simon

Hi Simon,

defconfig files are virtually never a configuration for the kernel they are shipped with since they aren't updated every time some configuration option is changed.

Consider a defconfig with CONFIG_BROKEN=n, and a driver that is enabled in this defconfig gets for some reason marked as broken in the Kconfig file - this will give exactly the same result as the one you describe.

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Adrian,

The defconfig files in arch/arm/configs are for platform configurations and are provided by the platform maintainers as a _working_ configuration for their platform. They're not "defconfigs". They got called "defconfigs" as a result of the kbuild "cleanups". Please don't confuse them as such.

If, in order to have a working platform configuration, they deem that CONFIG_BROKEN must be enabled, then that's the way it is.

Therefore, I request that either you leave the ARM platform configurations well alone, or follow the advice I've given so that we can _properly_ assess the impact of your changes.

Thanks.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

                         ^^^^^^^

         ^^^^^^

Still funny...

So either one of them is lying...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

They might be broken in other situations. However, if you look at the latest build at:

http://armlinux.simtec.co.uk/kautobuild/

you'll notice that all, even the ones with CONFIG_BROKEN build successfully. Without any bug reports to the contary, we must assume that the configuration files supplied by the folk who developed the support for the platform are correct and working.

Therefore, CONFIG_BROKEN may have been added to configuration options which don't work for some particular small corner cases.

This brings on to another subject. If we mark something broken we should say _why_ we're doing so, especially if it is non-obvious. That seems to be the case here - if these drivers are broken, it's non-obvious why they're broken.

So, all in all, CONFIG_BROKEN is a broken idea in itself!

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

The bug in this case was the (implicit) BROKEN dependency of MTD_SHARP.

Such corner cases could easily be handled using

	depends on (BROKEN || SA1100_COLLIE)

Or in other cases wie have

	depends on (BROKEN || !64BIT)

If it works its not BROKEN, and we can express this.

The vast majority of drivers depending on BROKEN simply don't compile.

How many examples besides MTD_SHARP can you name where you have problems to determine why something is marked as BROKEN?

The idea behind BROKEN is to not offer drivers where we know that they don't compile or will for sure not work to users.

The ARM case that people are using the supplied defconfig's more or less unchanged is a big exception.

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Hi Russell,

if a working platform configuration configuration requires CONFIG_BROKEN=y, the problem is a bug that should be fixed properly.

We are talking about a class of bugs that can usually be easily fixed if reported - so why aren't they reported?

The MTD_SHARP case is a good example, because otherwise I might have soon sent a patch that would have removed this driver with the rationale "both marked as obsolete and BROKEN can clearly be removed".

Unless someone can tell me a valid case for enabling BROKEN that does both create a working configuration and not hide real issues it seems the approch below might be the way to go.

Yes, you might dislike this at the first sight.

But if you consider that although this might result in a short-term breakage of some configurations, this will also result in proper bug reports and fixing of the wrong BROKEN dependency bugs, I hope you agree that this will actually improve the situation.

cu
Adrian

<-- snip -->

Do not allow people to create configurations with CONFIG_BROKEN=y.

The sole reason for CONFIG_BROKEN=y would be if you are working on fixing a broken driver, but in this case editing the Kconfig file is trivial.

Never ever should a user enable CONFIG_BROKEN.

@@ -31,19 +31,8 @@
 	  you say Y here, you will be offered the choice of using features or
 	  drivers that are currently considered to be in the alpha-test phase.
 
-config CLEAN_COMPILE
-	bool "Select only drivers expected to compile cleanly" if EXPERIMENTAL
-	default y
-	help
-	  Select this option if you don't even want to see the option
-	  to configure known-broken drivers.
-
-	  If unsure, say Y
-
 config BROKEN
 	bool
-	depends on !CLEAN_COMPILE
-	default y
 
 config BROKEN_ON_SMP
 	bool

                      ^^^^

OK, a user, not an expert. Let's assume users don't enable EXPERIMENTAL.

But I'd like to at least have the possibility to enable broken drivers, even if it's just for compile regression tests.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Let's assume users don't have any hardware (e.g. several SATA drivers) that requires EXPERIMENTAL...

If a kernel developer really wants to enable BROKEN drivers, my patch still gives them the possibility to do so with a trivial edit of init/Kconfig.

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

I agree, and it's very convenient to be able to enable it in menuconfig etc. Perhaps CONFIG_BROKEN should just be moved to kernel hacking instead...?

--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

Maybe they're only broken for a small subset of platforms, and someone added a BROKEN without properly considering whether it should be global or not?

I don't disagree with the overall notion that CONFIG_BROKEN should not be set _where_ _possible_. However, if it needs to be set to get the required options, then that's what needs to happen until such time that the above is corrected.

However - and now to the main bug bear - how can we tell what is really broken if you _just_ change the default configuration file settings for CONFIG_BROKEN? What happens is that, on review, we see a simple change. We'd assume that it has little impact, and we accept that change.

Maybe a month or two down the line, someone whines that their platform doesn't work for some reason, and it's tracked down to this and the resulting fallout from disabling CONFIG_BROKEN.

That means that the original review was _worthless_. It wasn't a review at all.

So, what I am trying to get across is the need to show the _full_ set of changes to a default configuratoin when you disable CONFIG_BROKEN, which is trivially producable if you run the script I've already posted.

You can even use that in conjunction with your present patch to produce a patch which shows _exactly_ _everything_ which changes as a result of disabling CONFIG_BROKEN. Surely giving reviewers the _full_ story is far better than half a story, and should be something that any change to the kernel strives for.

If not, what's the point of the original change?

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Where is the bug report from the person who set CONFIG_BROKEN=y in the collie defconfig that the BROKEN dependency on MTD_SHARP was wrong?

The whining is the bug report the person who set the CONFIG_BROKEN=y in the defconfig didn't send.

And things would have been even worse if I had sent a patch erasing MTD_SHARP from the kernel because code "both marked as obsolete and BROKEN can clearly be removed" and the code was therefore completely removed two months before the first person whined?

The point is that I haven't yet heard any good reason for CONFIG_BROKEN=y in a defconfig.

No, it's not a good reason if someone used it as a workaround instead of sending a bug report that would result in a fixing of the wrong BROKEN dependency.

Where is the bug report of the person setting CONFIG_BROKEN=y in the collie defconfig that the MTD_SHARP dependency on BROKEN was wrong?

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

I'm sorry, I feel like I'm beating my head against a brick wall. I have said everything that needs to be said, and made my position on this patch crystal clear.

The patch to the ARM configuration files is nacked as it stands. Please go back and rework it along the guidelines I've pointed out several times in this thread and maybe then it becomes acceptable.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

I don't see any justification for MTD_OBSOLETE_CHIPS depending on BROKEN. That option covers a few of the obsolete chip drivers which people shouldn't be using any more -- and I'm perfectly willing to believe that one or two of those don't work any more, but if that's the case then those individual drivers ought to be marked BROKEN (or just removed). We shouldn't mark MTD_OBSOLETE_CHIPS broken.

I'd like to see the collie_defconfig updated to use the appropriate CFI driver back end.

dwmw2

Two of them don't even compile any more.

The patch below removes them and all code depending on them and makes the remaining one (MTD_SHARP) compilable again.

cu
Adrian

<-- snip -->

MTD_OBSOLETE_CHIPS shouldn't be marked as BROKEN.

This patch brings the MTD_SHARP driver back into life and removes the non-compiling MTD_AMDSTD and MTD_JEDEC with everything depending on them.

<-- snip -->

Due to it's size, the patch is attached gzip'ed.

 arch/cris/arch-v10/drivers/Kconfig        |    1 
 arch/cris/arch-v10/drivers/axisflashmap.c |    3 
 arch/cris/arch-v32/drivers/Kconfig        |    1 
 arch/cris/arch-v32/drivers/axisflashmap.c |    3 
 drivers/mtd/chips/Kconfig                 |   22 
 drivers/mtd/chips/Makefile                |    2 
 drivers/mtd/chips/amd_flash.c             | 1403 ----------------------
 drivers/mtd/chips/jedec.c                 |  935 --------------
 drivers/mtd/maps/Kconfig                  |   18 
 drivers/mtd/maps/Makefile                 |    2 
 drivers/mtd/maps/octagon-5066.c           |  248 ---
 drivers/mtd/maps/vmax301.c                |  198 ---
 include/linux/mtd/jedec.h                 |   66 -
 13 files changed, 1 insertion(+), 2901 deletions(-)

As things stand, collie doesn't work with the unmodified mainline obsolete chips driver but doesn't work with CFI either. Updating the defconfig isn't really going to help one way or the other as both are broken in some way.

A patch does exist to make the obsolete chip driver work on collie and it could probably be made acceptable to mainline but I doubt David would apply it as he (understandably) wants the "obsolete" driver to die.

I'm just a spectator in this as I don't have access to hardware to work out what the problem with CFI is. I agree CFI is the way to go but until it works, every real world configuration is going to use the patched obsolete driver.

Adrian Bunk:

In this case the sharp driver didn't use to compile but does now since my patch to fix it was applied. If broken means doesn't compile, the driver does now and the broken status can be removed. It should work on the platforms it originally worked on. It is still missing some code that's needed to make it work specifically on collie but I guess that's a separate issue.

Richard