[patch, -git] input: CONFIG_INPUT_APANEL build fix

[patch, -git] input: CONFIG_INPUT_APANEL build fix

[Ingo Molnar]

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Dmitry Torokhov]

On Wed, Apr 30, 2008 at 08:54:16PM +0200, Ingo Molnar wrote:
> 
> 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.

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,
but in this case it is "depens on" that does not evaluate properly...

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

-- 
Dmitry

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Ingo Molnar]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Wed, Apr 30, 2008 at 08:54:16PM +0200, Ingo Molnar wrote:
> > 
> > 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.
> 
> 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, 
> but in this case it is "depens on" that does not evaluate properly...

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Roman Zippel]

Hi,

On Wed, 30 Apr 2008, Ingo Molnar wrote:

> > 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, 
> > but in this case it is "depens on" that does not evaluate properly...
> 
> 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 ;-)

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Dmitry Torokhov]

On Wed, Apr 30, 2008 at 10:56:05PM +0200, Roman Zippel wrote:
> Hi,
> 
> On Wed, 30 Apr 2008, Ingo Molnar wrote:
> 
> > > 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, 
> > > but in this case it is "depens on" that does not evaluate properly...
> > 
> > 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 ;-)
> 
> 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!
>

That is all fine and dandy except that apanel does not select LEDs, it
depends on LEDS_CLASS.
 
> 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.
>

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

-- 
Dmitry

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Dmitry Torokhov]

On Wed, Apr 30, 2008 at 10:20:13PM +0200, Ingo Molnar wrote:
> 
> * Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Wed, Apr 30, 2008 at 08:54:16PM +0200, Ingo Molnar wrote:
> > > 
> > > 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.
> > 
> > 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, 
> > but in this case it is "depens on" that does not evaluate properly...
> 
> 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.
> 

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Ingo Molnar]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

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

correct, except if that would mean breakage for users.

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

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Dmitry Torokhov]

On Wed, Apr 30, 2008 at 11:20:29PM +0200, Ingo Molnar wrote:
> 
> * Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > 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.
> 
> correct, except if that would mean breakage for users.
> 
> > 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.
> 
> yep ... just dont export our internal crappiness to testers and users.
> 

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

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

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

-- 
Dmitry

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Ingo Molnar]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> > 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.
> 
> I believe I see a steady stream of breakage for leds dependencies from 
> all subsystems.

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Dmitry Torokhov]

On Thu, May 01, 2008 at 12:38:29AM +0200, Ingo Molnar wrote:
> 
> * Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > > 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.
> > 
> > I believe I see a steady stream of breakage for leds dependencies from 
> > all subsystems.
> 
> 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.

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Roman Zippel]

Hi,

On Wed, 30 Apr 2008, Dmitry Torokhov wrote:

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

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 LEDS_TRIGGERS
-	bool "LED Trigger support"
+	bool "LED Trigger support" if NEW_LEDS
+	select LEDS_CORE
 	help
 	  This option enables trigger support for the leds class.
 	  These triggers allow kernel events to drive the LEDs and can
@@ -190,4 +189,3 @@ config LEDS_TRIGGER_DEFAULT_ON
 	  This allows LEDs to be initialised in the ON state.
 	  If unsure, say Y.
 
-endif # NEW_LEDS
Index: linux-2.6/drivers/leds/Makefile
===================================================================
--- linux-2.6.orig/drivers/leds/Makefile
+++ linux-2.6/drivers/leds/Makefile
@@ -1,6 +1,6 @@
 
 # LED Core
-obj-$(CONFIG_NEW_LEDS)			+= led-core.o
+obj-$(CONFIG_LEDS_CORE)			+= led-core.o
 obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
 
Index: linux-2.6/drivers/hwmon/Kconfig
===================================================================
--- linux-2.6.orig/drivers/hwmon/Kconfig
+++ linux-2.6/drivers/hwmon/Kconfig
@@ -763,7 +763,6 @@ config SENSORS_HDAPS
 config SENSORS_APPLESMC
 	tristate "Apple SMC (Motion sensor, light sensor, keyboard backlight)"
 	depends on INPUT && X86
-	select NEW_LEDS
 	select LEDS_CLASS
 	select INPUT_POLLDEV
 	default n
Index: linux-2.6/drivers/input/misc/Kconfig
===================================================================
--- linux-2.6.orig/drivers/input/misc/Kconfig
+++ linux-2.6/drivers/input/misc/Kconfig
@@ -81,7 +81,6 @@ config INPUT_WISTRON_BTNS
 	tristate "x86 Wistron laptop button interface"
 	depends on X86 && !X86_64
 	select INPUT_POLLDEV
-	select NEW_LEDS
 	select LEDS_CLASS
 	select CHECK_SIGNATURE
 	help
Index: linux-2.6/drivers/macintosh/Kconfig
===================================================================
--- linux-2.6.orig/drivers/macintosh/Kconfig
+++ linux-2.6/drivers/macintosh/Kconfig
@@ -90,7 +90,6 @@ config ADB_PMU
 config ADB_PMU_LED
 	bool "Support for the Power/iBook front LED"
 	depends on ADB_PMU
-	select NEW_LEDS
 	select LEDS_CLASS
 	help
 	  Support the front LED on Power/iBooks as a generic LED that can
Index: linux-2.6/drivers/misc/Kconfig
===================================================================
--- linux-2.6.orig/drivers/misc/Kconfig
+++ linux-2.6/drivers/misc/Kconfig
@@ -140,7 +140,6 @@ config ACER_WMI
 	depends on EXPERIMENTAL
 	depends on ACPI
 	depends on LEDS_CLASS
-	depends on NEW_LEDS
 	depends on BACKLIGHT_CLASS_DEVICE
 	depends on SERIO_I8042
 	select ACPI_WMI
@@ -161,7 +160,6 @@ config ASUS_LAPTOP
         depends on ACPI
 	depends on EXPERIMENTAL && !ACPI_ASUS
 	depends on LEDS_CLASS
-	depends on NEW_LEDS
 	depends on BACKLIGHT_CLASS_DEVICE
         ---help---
 	  This is the new Linux driver for Asus laptops. It may also support some
@@ -248,7 +246,6 @@ config THINKPAD_ACPI
 	select HWMON
 	select NVRAM
 	select INPUT
-	select NEW_LEDS
 	select LEDS_CLASS
 	---help---
 	  This is a driver for the IBM and Lenovo ThinkPad laptops. It adds
Index: linux-2.6/net/mac80211/Kconfig
===================================================================
--- linux-2.6.orig/net/mac80211/Kconfig
+++ linux-2.6/net/mac80211/Kconfig
@@ -74,7 +74,6 @@ config MAC80211_MESH
 config MAC80211_LEDS
 	bool "Enable LED triggers"
 	depends on MAC80211
-	select NEW_LEDS
 	select LEDS_TRIGGERS
 	---help---
 	  This option enables a few LED triggers for different

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Dmitry Torokhov]

Hi Roman,

On Thu, May 01, 2008 at 02:51:47AM +0200, Roman Zippel wrote:
> Hi,
> 
> On Wed, 30 Apr 2008, Dmitry Torokhov wrote:
> 
> > 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.
> 
> 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?).
> 

Yes, I did. The revert fixed the apanel driver which does not select
but rather "depends on" LEDS_CLASS for me.

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

This is a different issue from the original reported one.
 
> 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>
> 

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

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Jan Engelhardt]

On Thursday 2008-05-01 04:36, Dmitry Torokhov wrote:
>
>BTW, does this mean that all 'if'-guarded symbols should be converted to
>this style?
>
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.

Re: [patch, -git] input: CONFIG_INPUT_APANEL build fix

[Ingo Molnar]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Thu, May 01, 2008 at 12:38:29AM +0200, Ingo Molnar wrote:
> > 
> > * Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > > 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.
> > > 
> > > I believe I see a steady stream of breakage for leds dependencies from 
> > > all subsystems.
> > 
> > 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.
> 
> 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.

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