input: JOYSTICK_XPAD build fix

x86.git testing found the following build failure in -git:

ERROR: "led_classdev_register" [drivers/input/joystick/xpad.ko] undefined!
ERROR: "led_classdev_unregister" [drivers/input/joystick/xpad.ko] undefined!

which triggers with the following config:

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

the reason is dependency on NEW_LEDS that was not spelled out in the Kconfig entry of JOYSTICK_XPAD.

Quoted 6 lines

which triggers with the following config:

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

the reason is dependency on NEW_LEDS that was not spelled out in the Kconfig entry of JOYSTICK_XPAD.

Xpad can be compiled without LED support so this dependancy is incorrect. JOYSTICK_XPAD_LEDS has proper dependancy on LEDS_CLASS, the rest is Kconfig breakage.

Quoted 10 lines · Dmitry Torokhov

Quoted 6 lines

which triggers with the following config:

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

the reason is dependency on NEW_LEDS that was not spelled out in the Kconfig entry of JOYSTICK_XPAD.

Xpad can be compiled without LED support so this dependancy is incorrect. JOYSTICK_XPAD_LEDS has proper dependancy on LEDS_CLASS, the rest is Kconfig breakage.

no, you are wrong, read the current Kconfig rules again. If the user can create a .config that does not build, it is driver breakage. It always was, and has been in the past 15 years.

Kconfig might be extended to make dependencies easier to manage for developers but until that is implemented you have to craft your driver's dependencies with the current tools in a way that doesnt break the build.

Ingo

This breakage with your .config is *not* caused by an input bug as you wrongly claim (I'll send a correct fix after some testing).

Quoted 2 lines · Adrian Bunk

This breakage with your .config is *not* caused by an input bug as you wrongly claim (I'll send a correct fix after some testing).

I have no opinion about where the bug is (input or leds or elsewhere) - my only opinion is that the kernel must not stay build-broken - and the discussion with Dmitry was about that.

I'm not interested in trivial patches as those issues are much more efficiently handled by the person who maintains that code (i.e. Dmitry) and who intimately knows the dependencies and expectations of that code. That's why i sent the bugreport and patch to Dmitry.

Ingo

The bug is in arch/x86/Kconfig .

Caused by commit 4cf31841762954ad2868156ccba94d798a16630f (x86:
mach-rdc321x Kconfig fix).

Quoted 4 lines · Adrian Bunk

The bug is in arch/x86/Kconfig .

Caused by commit 4cf31841762954ad2868156ccba94d798a16630f (x86:
mach-rdc321x Kconfig fix).

ah, indeed - Florian Cc:-ed. Preliminary quick hack below.

Ingo

--------------->
Subject: rdc: leds hack
From: Ingo Molnar <[email protected]>
Date: Thu May 01 03:46:22 CEST 2008

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/Kconfig |    1 +
 1 file changed, 1 insertion(+)

Index: linux/arch/x86/Kconfig
===================================================================

I would really appreciate it if you could send the error message and the .config but not quick kconfig patches that are often wrong and that you try to push through the maintainers as you tried here.

Quoted 3 lines · Adrian Bunk

I would really appreciate it if you could send the error message and the .config but not quick kconfig patches that are often wrong and that you try to push through the maintainers as you tried here.

hey, sorry about invading your turf of trivial patches ;) I dont see it as a problem that the thought process and the initial patch is incomplete and ad-hoc. My preference is to work with people out in the open, even on trivial issues. Dmitry is a capable maintainer who understands his code very well and he'll resist me if i'm full of it. Just like i resisted you when you were full of it. That's what maintainers do, their job is to know their code.

And, occasionally, as in this case, i might end up being faced with a bug in the code i maintain ;)

Ingo

Quoted 16 lines · Ingo Molnar

Quoted 4 lines · Adrian Bunk

I would really appreciate it if you could send the error message and the .config but not quick kconfig patches that are often wrong and that you try to push through the maintainers as you tried here.

hey, sorry about invading your turf of trivial patches ;) I dont see it as a problem that the thought process and the initial patch is incomplete and ad-hoc. My preference is to work with people out in the open, even on trivial issues. Dmitry is a capable maintainer who understands his code very well and he'll resist me if i'm full of it. Just like i resisted you when you were full of it. That's what maintainers do, their job is to know their code.

Quoted 2 lines

And, occasionally, as in this case, i might end up being faced with a bug in the code i maintain ;)

You completely miss my point.

You wrongly (and loudly) blamed Dmitry for something you broke yourself.

|| Kconfig might be extended to make dependencies easier to manage for 
|| developers but until that is implemented you have to craft your 
|| driver's dependencies with the current tools in a way that doesnt 
|| break the build.

and that's exactly what happens with Roman's patch: a Kconfig subsystem design bug (its inability to properly propagate the dependencies of select's) is worked around in the driver space: by the LEDS_CORE driver config introduction and no user-visible.

Quoted 17 lines · Adrian Bunk

Quoted 16 lines · Ingo Molnar

Quoted 4 lines · Adrian Bunk

I would really appreciate it if you could send the error message and the .config but not quick kconfig patches that are often wrong and that you try to push through the maintainers as you tried here.

hey, sorry about invading your turf of trivial patches ;) I dont see it as a problem that the thought process and the initial patch is incomplete and ad-hoc. My preference is to work with people out in the open, even on trivial issues. Dmitry is a capable maintainer who understands his code very well and he'll resist me if i'm full of it. Just like i resisted you when you were full of it. That's what maintainers do, their job is to know their code.

Quoted 7 lines

Quoted 2 lines

And, occasionally, as in this case, i might end up being faced with a bug in the code i maintain ;)

You completely miss my point.

You wrongly (and loudly) blamed Dmitry for something you broke yourself.

i didnt. Read what i wrote:

|| no, you are wrong, read the current Kconfig rules again. If the user 
|| can create a .config that does not build, it is driver breakage. It 
|| always was, and has been in the past 15 years.
||
|| Kconfig might be extended to make dependencies easier to manage for 
|| developers but until that is implemented you have to craft your 
|| driver's dependencies with the current tools in a way that doesnt 
|| break the build.

and that's exactly what happens with Roman's patch: a Kconfig subsystem design bug (its inability to properly propagate the dependencies of select's) is worked around in the driver space: by the LEDS_CORE driver config introduction and no user-visible.

Roman's patch is obviously cleaner than my hack (i just fixed a single instantiation of the problem, while he changed the LEDS driver dependency structure), but it's still a workaround for a Kconfig subsystem bug and the same problem could reoccur elsewhere. It could hit anytime dual dependencies are introduced in a driver accidentally.

As Sam said it, fixing that Kconfig design bug would be "nice" - but unfortunately the Kconfig subsystem is not actively developed anymore.

Would you like to volunteer for that? It would be a _very_ useful contribution. One such fix could avoid hundreds or even thousands of trivial problems in the future - it could avoid having to make hundreds or thousands of trivial patches in the future.

Ingo

Quoted 19 lines

Quoted 2 lines

You wrongly (and loudly) blamed Dmitry for something you broke yourself.

i didnt. Read what i wrote:

|| no, you are wrong, read the current Kconfig rules again. If the user 
|| can create a .config that does not build, it is driver breakage. It 
|| always was, and has been in the past 15 years.
||
|| Kconfig might be extended to make dependencies easier to manage for 
|| developers but until that is implemented you have to craft your 
|| driver's dependencies with the current tools in a way that doesnt 
|| break the build.

and that's exactly what happens with Roman's patch: a Kconfig subsystem design bug (its inability to properly propagate the dependencies of select's) is worked around in the driver space: by the LEDS_CORE driver config introduction and no user-visible.

Quoted 5 lines

Roman's patch is obviously cleaner than my hack (i just fixed a single instantiation of the problem, while he changed the LEDS driver dependency structure), but it's still a workaround for a Kconfig subsystem bug and the same problem could reoccur elsewhere. It could hit anytime dual dependencies are introduced in a driver accidentally.

Ingo, perhaps it helps if I put it in caps:

YOU TRIED TO PUSH AN INPUT PATCH FOR AN X86 BUG.

Roman's patch is better than adding a select, but your patch would have added the select in the completely wrong subsystem.

Why can't you admit the patch you tried to push was wrong?

Quoted 3 lines

As Sam said it, fixing that Kconfig design bug would be "nice" - but unfortunately the Kconfig subsystem is not actively developed anymore.

Roman is still active.

* Adrian Bunk <[email protected]> wrote:
...

Quoted 5 lines

Quoted 3 lines

As Sam said it, fixing that Kconfig design bug would be "nice" - but unfortunately the Kconfig subsystem is not actively developed anymore.

Roman is still active.

great, does this mean we'll see fixes for select's misbehavior, along the lines of Sam's suggestions?

at minimum a warning needs to be emitted by the kconfig tool if such incomplete selects are used. I've stopped counting the number of times such issues have broken the build and have held up kernel development.

All the information is already in the Kconfig files for the kconfig tool/subsystem to make an intelligent decision. It's just not fully used, and the burden of fixing these problems is pushed back to the developers who create the Kconfig files.

Ingo

Quoted 7 lines · Ingo Molnar

* Adrian Bunk <[email protected]> wrote:
...

Quoted 4 lines

Quoted 3 lines

As Sam said it, fixing that Kconfig design bug would be "nice" - but unfortunately the Kconfig subsystem is not actively developed anymore.

Quoted 4 lines

Quoted 1 line

Roman is still active.

great, does this mean we'll see fixes for select's misbehavior, along the lines of Sam's suggestions?

In the case of the problem here it would have turned one problem into another, and Roman's patch is the correct solution no matter whether you change the semantics of select or not.

Quoted 8 lines · Adrian Bunk

Quoted 7 lines · Ingo Molnar

* Adrian Bunk <[email protected]> wrote:
...

Quoted 4 lines

Quoted 3 lines

As Sam said it, fixing that Kconfig design bug would be "nice" - but unfortunately the Kconfig subsystem is not actively developed anymore.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Roman is still active.

great, does this mean we'll see fixes for select's misbehavior, along the lines of Sam's suggestions?

In the case of the problem here it would have turned one problem into another, and Roman's patch is the correct solution no matter whether you change the semantics of select or not.

Hi Adrian
It would be nice to know what was wrong with my suggestion. You have done your share of kconfig patches so you have a good grip on the problems we face.
So any input on how we can improve kconfig so we can actually provide what people often expects or requests or need would be nice.

I have so far not done any hacking on the core of the backend of kconfig but if I one day find more than one hour in row where I can do some kernel stuff I may actually try it out.
And then it would be nice to have a sketch ready how to solve the issues.

I would though request you to start a new mail thread for this and include both [email protected] and Roman Zippel in the loop.

Sam

Quoted 12 lines

Hi Adrian
It would be nice to know what was wrong with my suggestion. You have done your share of kconfig patches so you have a good grip on the problems we face.
So any input on how we can improve kconfig so we can actually provide what people often expects or requests or need would be nice.

I have so far not done any hacking on the core of the backend of kconfig but if I one day find more than one hour in row where I can do some kernel stuff I may actually try it out.
And then it would be nice to have a sketch ready how to solve the issues.

Quoted 2 lines

I would though request you to start a new mail thread for this and include both [email protected] and Roman Zippel in the loop.

If you've sent a suggestion that included a description of the exact semantics you want to implement I must have missed it.

Can you send me a pointer or a description of the semantics? I'll then have a look.

Quoted 18 lines

Quoted 7 lines

Hi Adrian
It would be nice to know what was wrong with my suggestion. You have done your share of kconfig patches so you have a good grip on the problems we face.
So any input on how we can improve kconfig so we can actually provide what people often expects or requests or need would be nice.

Quoted 7 lines

I have so far not done any hacking on the core of the backend of kconfig but if I one day find more than one hour in row where I can do some kernel stuff I may actually try it out.
And then it would be nice to have a sketch ready how to solve the issues.

I would though request you to start a new mail thread for this and include both [email protected] and Roman Zippel in the loop.

If you've sent a suggestion that included a description of the exact semantics you want to implement I must have missed it.

Quoted 2 lines

Can you send me a pointer or a description of the semantics? I'll then have a look.

I took the original idea and added a few more details. Please see mail named "http://lkml.org/lkml/2008/5/4/18"
http://lkml.org/lkml/2008/5/4/18

Sam

Quoted 4 lines

at minimum a warning needs to be emitted by the kconfig tool if such incomplete selects are used. I've stopped counting the number of times such issues have broken the build and have held up kernel development.

It might held up your randconfig compiles.

Actual kernel development isn't much affected.

Quoted 5 lines · Adrian Bunk

Quoted 4 lines

at minimum a warning needs to be emitted by the kconfig tool if such incomplete selects are used. I've stopped counting the number of times such issues have broken the build and have held up kernel development.

Quoted 3 lines

It might held up your randconfig compiles.

Actual kernel development isn't much affected.

uhm, you are quite wrong - countless times have people been bitten by select's breakages in the past, and not via randconfig. That's the main reason why select use in Kconfig was not encouraged for a long time.

Select does make sense in some situations but it's a double-edged sword:
kconfig does not warn at all about the situations where it's "unsafe" to use it - while it has all the information in the Kconfig files to emit that warning. Instead we get build breakages not visible when an incorrect select is added, but much later, if someone happens to stumble on the wrong kind of .config. That is obviously harmful.

From an UI perspective we often want to select options that have

My larger point is that this kconfig tool bug breeds a constant stream of avoidable breakages, which causes lost manpower and causes a stream of trivial patches hindering maintainers all around the tree. Because every such trivial patch has to be reviewed, tested, it clogs the commit logs, etc.

So the more trivial patches we _avoid_ having to do in the future, the better. I'm not sure why you are even arguing against this this rather simple point - your arguments are rather hard to understand. Wouldnt you be happier if a whole category of trivial breakages was avoided and if you didnt have to deal with and waste your time on that category of trivial patches anymore?

Most of the time reoccuring trivial patches are an indicator of some deeper structural problem - as in this case.

Ingo

Quoted 13 lines · Ingo Molnar

My larger point is that this kconfig tool bug breeds a constant stream of avoidable breakages, which causes lost manpower and causes a stream of trivial patches hindering maintainers all around the tree. Because every such trivial patch has to be reviewed, tested, it clogs the commit logs, etc.

So the more trivial patches we _avoid_ having to do in the future, the better. I'm not sure why you are even arguing against this this rather simple point - your arguments are rather hard to understand. Wouldnt you be happier if a whole category of trivial breakages was avoided and if you didnt have to deal with and waste your time on that category of trivial patches anymore?

Quoted 2 lines

Most of the time reoccuring trivial patches are an indicator of some deeper structural problem - as in this case.

Your conclusions are based on an assumption that isn't true.

"trivial patches" are the patches you send.

But they are often bogus.

Fixing these issues properly often requires a deeper understanding of both kconfig and the dependencies of the underlying code.

Quoted 20 lines · Adrian Bunk

Quoted 13 lines · Ingo Molnar

My larger point is that this kconfig tool bug breeds a constant stream of avoidable breakages, which causes lost manpower and causes a stream of trivial patches hindering maintainers all around the tree. Because every such trivial patch has to be reviewed, tested, it clogs the commit logs, etc.

So the more trivial patches we _avoid_ having to do in the future, the better. I'm not sure why you are even arguing against this this rather simple point - your arguments are rather hard to understand. Wouldnt you be happier if a whole category of trivial breakages was avoided and if you didnt have to deal with and waste your time on that category of trivial patches anymore?

Quoted 2 lines

Most of the time reoccuring trivial patches are an indicator of some deeper structural problem - as in this case.

Your conclusions are based on an assumption that isn't true.

Quoted 6 lines

"trivial patches" are the patches you send.

But they are often bogus.

Fixing these issues properly often requires a deeper understanding of both kconfig and the dependencies of the underlying code.

I suspect that Ingo is however correct

- although a *proper* fix of one of
these bugs requires human-intelligence to figure out what's *really* intended, the kconfig program *does* have enough information available to issue a a clear warning:

"Yo doodz - I don't know *what* you intended here, but this SELECT is just waiting to sink its teeth into somebody's posterior. You might want to fix it somehow before somebody needs rabies shots..."
...

Quoted 3 lines

at minimum a warning needs to be emitted by the kconfig tool if such incomplete selects are used. I've stopped counting the number of times such issues have broken the build and have held up kernel development.

It might held up your randconfig compiles.

Actual kernel development isn't much affected.

Before you started your randconfig builds and sending (often buggy) kconfig patches like crazy there wasn't a problem (and Toralf's randconfig builds already catched these problems in the past).

Quoted 7 lines · Adrian Bunk

Quoted 3 lines

at minimum a warning needs to be emitted by the kconfig tool if such incomplete selects are used. I've stopped counting the number of times such issues have broken the build and have held up kernel development.

It might held up your randconfig compiles.

Actual kernel development isn't much affected.

Interesting. You confidently define how kernel development works and what affects it and what not.

I just caught a build wreckage via randconfig builds on a patch which was sent through Andrew to x86 for merging into mainline.

So you think that we should stop sharing with the wider kernel community the fixes that result out of our automated randconfig builds and should instead wait for Joe User or another kernel developer to trip into some subtle missing Kconfig dependency, just because a fix might not be the right one and might need another iteration before it can go upstream?

You are wrong as usual.

I for one prefer getting a (even wrong) patch which points to the problem instead of some (often grumpy) "this broke" message.

Quoted 3 lines

Before you started your randconfig builds and sending (often buggy) kconfig patches like crazy there wasn't a problem (and Toralf's randconfig builds already catched these problems in the past).

FYI, Ingo has been doing randconfig build and boot tests for a long, long time. There are dozens and dozens of (non-build) fixes in the upstream kernel that resulted from that effort - it's really valuable in practice.

So what you say is complete nonsense.

The RDC subarch commit was buggy, but that does not justify your insulting and self-righteous behaviour at all. If you think that your behaviour vs. Ingo is improving kernel development, then you are on the completely wrong track. It might earn you an entry in his killfile, but nothing else.

Thanks,

tglx

I'm not even sure the semantics of "select follows dependencies" would actually be better than what we have today.

Quoted 2 lines · Adrian Bunk

I'm not even sure the semantics of "select follows dependencies" would actually be better than what we have today.

I think it would. But how would that actually work? By displaying all relevant dependency chains and asking the user to select one?

Krzysztof Halasa

If you select LEDS_CLASS "select follows dependencies" would let inherit X86_RDC321X the dependencies of LEDS_CLASS,

And regarding "displaying all relevant dependency chains" to the user - I can't see how that would work in the more complicated cases.

Quoted 2 lines · Adrian Bunk

If you select LEDS_CLASS "select follows dependencies" would let inherit X86_RDC321X the dependencies of LEDS_CLASS,

I see, "follows" the other way around. That fixes the breakage but I guess it's not exactly what the people doing make menuconfig want. Though in absence of the following it would IMHO make sense anyway.

From a developer perspective if might be the easiest to simply remove

Quoted 2 lines

And regarding "displaying all relevant dependency chains" to the user - I can't see how that would work in the more complicated cases.

I fear it too but perhaps there is some sane way? I don't know, asking (recursively?) interactively?

Like:
QWERTY requires at least one, select (Y/M):
- ARCH_X86 && NET_ETHERNET, or
- USB4_0 && !SMP, or
- XXX

then:
NET_ETHERNET requires at lest one, select (Y/M):
- PCI128
- ISA256

then (info):
PCI128 requires PCI and it's selected automatically.

I don't know. It could be useful even with a single dependency when configuring as the module, the user could select Y or M for dependency interactively (people may want to have E1000E=m and TULIP=m but MII=y).

Not sure if still on the same planet, though :-)

Signature · 1 line

Krzysztof Halasa

FB_SGIVW requires X86_VISWS and it's selected automatically.

(which is not good if you want a kernel that runs on a PC)

Why do we have to bother users with the MII option at all?

"E1000E=m and TULIP=m but MII=y" works, but it doesn't really make sense.

Quoted 3 lines · Adrian Bunk

FB_SGIVW requires X86_VISWS and it's selected automatically.

(which is not good if you want a kernel that runs on a PC)

Not sure what do you mean, it currently seem to "depend" on X86_VISWS:

config FB_SGIVW
tristate "SGI Visual Workstation framebuffer support" depends on FB && X86_VISWS
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT

Quoted 4 lines

Why do we have to bother users with the MII option at all?

"E1000E=m and TULIP=m but MII=y" works, but it doesn't really make sense.

(Obviously it assumes both TULIP and E1000E required MII which is not exactly the case)

But it makes a perfect sense, I can have modular drivers for (few) hardware devices (just in case I want to rmmod etc) but most of the kernel may be not modular. It would be nice if the Kconfig ask me if I want to "select" the dependency Y or M, even if there is only one way to make "select" dependencies happy (not counting "Y vs "M").

Krzysztof Halasa

Unless I misunderstand your proposal you want to resolve dependencies reversely.

And if you want to only do this for select's: FB_SGIVW could also be select'ed.