x86.git testing found this build failure in upstream -git:

 drivers/built-in.o: In function `apanel_detach_client':
 apanel.c:(.text+0x3027af): undefined reference to `led_classdev_unregister'
 drivers/built-in.o: In function `apanel_probe':
 apanel.c:(.text+0x302ac9): undefined reference to `led_classdev_register'

with this config:

 http://redhat.com/~mingo/misc/config-Wed_Apr_30_20_22_20_CEST_2008.bad

this driver has a missing CONFIG_NEW_LEDS dependency.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/input/misc/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/input/misc/Kconfig
===================================================================
--- linux.orig/drivers/input/misc/Kconfig
+++ linux/drivers/input/misc/Kconfig
@@ -43,7 +43,7 @@ config INPUT_M68K_BEEP
 
 config INPUT_APANEL
 	tristate "Fujitsu Lifebook Application Panel buttons"
-	depends on X86 && I2C && LEDS_CLASS
+	depends on X86 && I2C && LEDS_CLASS && NEW_LEDS
 	select INPUT_POLLDEV
 	select CHECK_SIGNATURE
 	help
--

[ message continues ]

Thank you Ingo. Isn't this a Kconfig issue though? I know that Roman
is not willing to fix SELECT to evaluate the whole dependency chain,

-- 
Dmitry
--

[ message continues ]

yes and no. It can be fixed in Kconfig but i'm not going to flame yet 
another person about long-existing unfixed infrastructure bugs ;-)

The current (many years old) rule is to fix things up as much as we can 
and never expose build errors, because 1) randconfig testing is way too 
useful and because 2) users can stumble across that .config combination 
as well and get rightfully grumpy at us about breaking their build. Most 
of the thousands of drivers are fine - it's those with more complex 
Kconfig dependencies that have trouble.

	Ingo
--

[ message continues ]

Hi,


You don't have to, because this simply is no infrastructure bug...
Don't blame Kconfig for abusing select for something it was never 
intendend for. There is no bug!

In this case there is now a mixture of depends and selects on leds. First 
decide whether led support should be automatically included or the driver 
is only available when led support is enabled. In the first case it's 
probably better to create LEDS_SUPPORT, which then again selects 
everything necessary.

bye, Roman
--

[ message continues ]

That is all fine and dandy except that apanel does not select LEDs, it

The decision is made, the driver depends on LEDs except that it is not
working.

-- 
Dmitry
--

[ message continues ]

I thought the many years old rule for Linux kernel is to fix the
issues properly and not turn kernel into patchwork of crappy code.
If we want to fix the randconfi issue I'd rather revert commit
66242f7ec531953fbc2f4040c5ffe1f1ffe6c5c9 for now (they only thing it
did was beautify menuconfig look) and wait for Kconfig to be fixed.
It is not driver's task to track through all dependencies.

-- 
Dmitry
--

[ message continues ]

yep ... just dont export our internal crappiness to testers and users.

i'm not suggesting this is your fault in any way - but nevertheless many 
other subsystems have to deal with the same Kconfig issues and they 
manage to limp along.

	Ingo
--

[ message continues ]

So is there objections to reverting the commit above? This will
fix all drivers that are now depend on LEDS_CLASS bit not specify

I believe I see a steady stream of breakage for leds dependencies
from all subsystems.

-- 
Dmitry
--

[ message continues ]

they limp along by adding "depends on NEW_LEDS". 99% of the users will 
use some pre-cooked distro kernel where all these options are turned on, 
so the flattening and coupling of the dependencies is not a real issue 
in practice.

	Ingo
--

[ message continues ]

You still did not answer to the main question - do you think we should
revert the commit that actually introduced breakage in the sense that
anything depending on LEDS_CLASS should also add NEW_LEDS dependancy?
That will take care of the problem (as far as LEDs are concerned) for
_all_ subsystems and drivers at once.

-- 
Dmitry
--

[ message continues ]

Hi,


Well, that would mean reverting the complete led subsystem, since the 
menuconfig patch you pointed at, isn't really the problem (did you 
actually try to revert that patch?).

Anyway, the actual issue are all the drivers that select LEDS_CLASS
without selecting NEW_LEDS.

The patch below fixes the dependency problem without adding a lot of new 
selects by using LEDS_CORE, which is selected by LEDS_CLASS/LEDS_TRIGGERS 
as needed.
I removed the comments as they were a little redundant and I removed the 
selects of NEW_LEDS as these are not needed anymore.

Signed-off-by: Roman Zippel <zippel@linux-m68k.org>

---
 drivers/hwmon/Kconfig      |    1 -
 drivers/input/misc/Kconfig |    1 -
 drivers/leds/Kconfig       |   16 +++++++---------
 drivers/leds/Makefile      |    2 +-
 drivers/macintosh/Kconfig  |    1 -
 drivers/misc/Kconfig       |    3 ---
 net/mac80211/Kconfig       |    1 -
 7 files changed, 8 insertions(+), 17 deletions(-)

Index: linux-2.6/drivers/leds/Kconfig
===================================================================
--- linux-2.6.orig/drivers/leds/Kconfig
+++ linux-2.6/drivers/leds/Kconfig
@@ -1,3 +1,6 @@
+config LEDS_CORE
+	def_bool n
+
 menuconfig NEW_LEDS
 	bool "LED Support"
 	help
@@ -7,16 +10,13 @@ menuconfig NEW_LEDS
 	  This is not related to standard keyboard LEDs which are controlled
 	  via the input system.
 
-if NEW_LEDS
-
 config LEDS_CLASS
-	tristate "LED Class Support"
+	tristate "LED Driver Support" if NEW_LEDS
+	select LEDS_CORE
 	help
 	  This option enables the led sysfs class in /sys/class/leds.  You'll
 	  need this to do anything useful with LEDs.  If unsure, say N.
 
-comment "LED drivers"
-
 config LEDS_ATMEL_PWM
 	tristate "LED Support using Atmel PWM outputs"
 	depends on LEDS_CLASS && ATMEL_PWM
@@ -147,10 +147,9 @@ config LEDS_CLEVO_MAIL
 	  To compile this driver as a module, choose M here: the
 	  module will be called leds-clevo-mail.
 
-comment "LED Triggers"
-
 config ...
[ message continues ]

Hi Roman,


Yes, I did. The revert fixed the apanel driver which does not select


The patch appears to fix both problems at once (select and depends on + if),
I like it. Could we please have it applied to mainline, please?

BTW, does this mean that all 'if'-guarded symbols should be converted to
this style?

-- 
Dmitry
--

[ message continues ]

No, because all symbols inside an "if FOO" block just means they
have a "depends on FOO". So other than saving some typing, it
does nothing. Well, perhaps it _ensures_ that all objects are
properly put _into_ the menuconfig. Some people get this wrong
and use "depends on FOO" inconsistently, leading to options
"falling" out of the menu, i.e. reappear at the parent menu.
--

[ message continues ]

do you mean this one:

| commit 66242f7ec531953fbc2f4040c5ffe1f1ffe6c5c9
| Author: Jan Engelhardt <jengelh@gmx.de>
| Date:   Thu May 10 10:44:11 2007 +0100
|
|    leds: Use menuconfig objects II - LED
|
|    Change Kconfig objects from "menu, config" into "menuconfig" so
|    that the user can disable the whole feature without having to
|    enter the menu first.

?

that split option definitely looks a bit weird, and the sprinkling of 
LEDS_CLASS+NEW_LEDS dependencies to all affected drivers feels broken as 
well. I'm all for fixing it right and can test any patch.

	Ingo
--

[ message continues ]