Re: [2.6 patch] defconfig's shouldn't set CONFIG_BROKEN=y

Re: [2.6 patch] defconfig's shouldn't set CONFIG_BROKEN=y

[Adrian Bunk]

On Sun, Dec 11, 2005 at 07:21:10PM +0000, Russell King wrote:
> On Sun, Dec 11, 2005 at 07:52:12PM +0100, Adrian Bunk wrote:
> > defconfig's shouldn't set CONFIG_BROKEN=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

That's not a problem introduced by my patch.

Either the depency of MTD_OBSOLETE_CHIPS on BROKEN is correct (in which 
case CONFIG_MTD_OBSOLETE_CHIPS=y wouldn't bring you anything), or the 
dependency on BROKEN is not correct and should be corrected.

David, can you comment on this issue?

> Russell King

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

[RFC: 2.6 patch] no longer mark MTD_OBSOLETE_CHIPS as BROKEN and remove broken MTD_OBSOLETE_CHIPS drivers

[Adrian Bunk]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

On Mon, Dec 12, 2005 at 10:38:15AM +0100, David Woodhouse wrote:
> On Sun, 2005-12-11 at 20:31 +0100, Adrian Bunk wrote:
> > Either the depency of MTD_OBSOLETE_CHIPS on BROKEN is correct (in
> > which case CONFIG_MTD_OBSOLETE_CHIPS=y wouldn't bring you anything),
> > or the dependency on BROKEN is not correct and should be corrected.
> > 
> > David, can you comment on this issue?
> 
> 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.
>...

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.

> dwmw2

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.


Signed-off-by: Adrian Bunk <bunk@stusta.de>

<--  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(-)


[-- Attachment #2: patch-MTD_OBSOLETE_CHIPS.gz --]
[-- Type: application/octet-stream, Size: 19885 bytes --]

Re: [2.6 patch] defconfig's shouldn't set CONFIG_BROKEN=y

[Adrian Bunk]

On Sun, Dec 11, 2005 at 07:44:37PM +0000, Russell King wrote:
> On Sun, Dec 11, 2005 at 08:31:18PM +0100, Adrian Bunk wrote:
> > On Sun, Dec 11, 2005 at 07:21:10PM +0000, Russell King wrote:
> > > On Sun, Dec 11, 2005 at 07:52:12PM +0100, Adrian Bunk wrote:
> > > > defconfig's shouldn't set CONFIG_BROKEN=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
> > 
> > That's not a problem introduced by my patch.
> 
> 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.
>...


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 ...


> Russell King

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

Re: [2.6 patch] defconfig's shouldn't set CONFIG_BROKEN=y

[Adrian Bunk]

On Tue, Dec 13, 2005 at 02:34:04PM +0100, Simon Richter wrote:

> Hi,

Hi Simon,

> Adrian Bunk wrote:
> 
> >>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.
> >>...
> 
> >No, CONFIG_BROKEN=y in a defconfig file is a bug.
> 
> 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.

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.

>    Simon

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

Re: [2.6 patch] defconfig's shouldn't set CONFIG_BROKEN=y

[Russell King]

On Tue, Dec 13, 2005 at 03:00:01PM +0100, Adrian Bunk wrote:
> 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.

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

[2.6 patch] don't allow users to set CONFIG_BROKEN=y

[Adrian Bunk]

On Tue, Dec 13, 2005 at 05:31:12PM +0000, Russell King wrote:
> On Tue, Dec 13, 2005 at 03:00:01PM +0100, Adrian Bunk wrote:
> > 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.
> 
> Adrian,

Hi Russell,

> 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.

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".

> 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.

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.

> Thanks.

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.


Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.15-rc5-mm2-full/init/Kconfig.old	2005-12-13 18:48:40.000000000 +0100
+++ linux-2.6.15-rc5-mm2-full/init/Kconfig	2005-12-13 18:48:52.000000000 +0100
@@ -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

Re: [2.6 patch] don't allow users to set CONFIG_BROKEN=y

[Geert Uytterhoeven]

On Tue, 13 Dec 2005, Adrian Bunk wrote:
> 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.
                      ^^^^
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 -- geert@linux-m68k.org

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

Re: [2.6 patch] don't allow users to set CONFIG_BROKEN=y

[Adrian Bunk]

On Tue, Dec 13, 2005 at 07:28:41PM +0100, Geert Uytterhoeven wrote:
> On Tue, 13 Dec 2005, Adrian Bunk wrote:
> > 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.
>                       ^^^^
> OK, a user, not an expert. Let's assume users don't enable EXPERIMENTAL.

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

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

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.

> Gr{oetje,eeting}s,
> 
> 						Geert

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

Re: [2.6 patch] don't allow users to set CONFIG_BROKEN=y

[Jesper Juhl]

On 12/13/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, 13 Dec 2005, Adrian Bunk wrote:
> > 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.
>                       ^^^^
> 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.
>
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 <jesper.juhl@gmail.com>
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

Re: [2.6 patch] don't allow users to set CONFIG_BROKEN=y

[Russell King]

On Tue, Dec 13, 2005 at 07:05:52PM +0100, Adrian Bunk wrote:
> On Tue, Dec 13, 2005 at 05:31:12PM +0000, Russell King wrote:
> > 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.
> 
> if a working platform configuration configuration requires 
> CONFIG_BROKEN=y, the problem is a bug that should be fixed properly.

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

Re: [2.6 patch] don't allow users to set CONFIG_BROKEN=y

[Adrian Bunk]

On Tue, Dec 13, 2005 at 08:01:06PM +0000, Russell King wrote:
> On Tue, Dec 13, 2005 at 07:05:52PM +0100, Adrian Bunk wrote:
> > On Tue, Dec 13, 2005 at 05:31:12PM +0000, Russell King wrote:
> > > 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.
> > 
> > if a working platform configuration configuration requires 
> > CONFIG_BROKEN=y, the problem is a bug that should be fixed properly.
> 
> 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.

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?

> 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.

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?

> 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?

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?

> Russell King

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

Re: [2.6 patch] don't allow users to set CONFIG_BROKEN=y

[Russell King]

On Tue, Dec 13, 2005 at 09:19:20PM +0100, Adrian Bunk wrote:
> On Tue, Dec 13, 2005 at 08:01:06PM +0000, Russell King wrote:
> > 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?
> 
> The point is that I haven't yet heard any good reason for 
> CONFIG_BROKEN=y in a defconfig.

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