2008-10-15 04:13:57

by Adrian Bunk

[permalink] [raw]
Subject: usbhid_set_leds problems

With CONFIG_USB_HID=n I'm getting the following build error:

<-- snip -->

...
LD .tmp_vmlinux1
drivers/built-in.o: In function `bright_probe':
drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
drivers/built-in.o: In function `dell_probe':
drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
drivers/built-in.o: In function `lg_probe':
drivers/hid/hid-lg.c:252: undefined reference to `usbhid_set_leds'
drivers/hid/hid-lg.c:252: more undefined references to `usbhid_set_leds' follow
...
make[1]: *** [.tmp_vmlinux1] Error 1

<-- snip -->


And I'm not sure commit 6edfa8dc33803a49ad936ead9840e453bee6ca3b
(HID: move reset leds quirk) was a good idea at all since I don't
see cases like HID_DELL=y, CONFIG_USB_HID=m could now work at all.


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


2008-10-15 05:06:15

by Adrian Bunk

[permalink] [raw]
Subject: Re: usbhid_set_leds problems

On Wed, Oct 15, 2008 at 07:13:46AM +0300, Adrian Bunk wrote:
> With CONFIG_USB_HID=n I'm getting the following build error:
>
> <-- snip -->
>
> ...
> LD .tmp_vmlinux1
> drivers/built-in.o: In function `bright_probe':
> drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> drivers/built-in.o: In function `dell_probe':
> drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> drivers/built-in.o: In function `lg_probe':
> drivers/hid/hid-lg.c:252: undefined reference to `usbhid_set_leds'
> drivers/hid/hid-lg.c:252: more undefined references to `usbhid_set_leds' follow
> ...
> make[1]: *** [.tmp_vmlinux1] Error 1
>
> <-- snip -->
>
>
> And I'm not sure commit 6edfa8dc33803a49ad936ead9840e453bee6ca3b
> (HID: move reset leds quirk) was a good idea at all since I don't
> see cases like HID_DELL=y, CONFIG_USB_HID=m could now work at all.

Scrap that mail, I shouldn't send patches before the first cup of tea...

I just found the actual problem.

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

2008-10-15 07:13:43

by Jiri Kosina

[permalink] [raw]
Subject: Re: usbhid_set_leds problems

On Wed, 15 Oct 2008, Adrian Bunk wrote:

> > LD .tmp_vmlinux1
> > drivers/built-in.o: In function `bright_probe':
> > drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> > drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> > drivers/built-in.o: In function `dell_probe':
> > drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> > drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> > drivers/built-in.o: In function `lg_probe':
> > drivers/hid/hid-lg.c:252: undefined reference to `usbhid_set_leds'
> > drivers/hid/hid-lg.c:252: more undefined references to `usbhid_set_leds' follow
> > ...
> > make[1]: *** [.tmp_vmlinux1] Error 1
> > And I'm not sure commit 6edfa8dc33803a49ad936ead9840e453bee6ca3b
> > (HID: move reset leds quirk) was a good idea at all since I don't
> > see cases like HID_DELL=y, CONFIG_USB_HID=m could now work at all.
> Scrap that mail, I shouldn't send patches before the first cup of tea...
> I just found the actual problem.

Hmm, so what was the culprit actually? The build error shouldn't be there.

Thanks,

--
Jiri Kosina
SUSE Labs

2008-10-15 07:30:27

by Adrian Bunk

[permalink] [raw]
Subject: [2.6 patch] HID: fix default building of all quirky devices

Commit 9be7bbd54df3c9c393ccd19acc49f90c517d1291
(HID: build drivers for all quirky devices by default)
was wrong in that it select'ed the drivers, causing
build errors like the following with CONFIG_USB_HID=n:

<-- snip -->

...
LD .tmp_vmlinux1
drivers/built-in.o: In function `bright_probe':
drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
drivers/built-in.o: In function `dell_probe':
drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
drivers/built-in.o: In function `lg_probe':
drivers/hid/hid-lg.c:252: undefined reference to `usbhid_set_leds'
drivers/hid/hid-lg.c:252: more undefined references to `usbhid_set_leds' follow
drivers/built-in.o: In function `sony_set_operational':
drivers/hid/hid-sony.c:42: undefined reference to `usb_control_msg'
drivers/hid/hid-sony.c:42: undefined reference to `usb_control_msg'
make[1]: *** [.tmp_vmlinux1] Error 1

<-- snip -->


Implement it in a better way.


Signed-off-by: Adrian Bunk <[email protected]>

---

drivers/hid/Kconfig | 91 +++++++++++++++++---------------------------
1 file changed, 36 insertions(+), 55 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index da64108..eec9b73 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -17,25 +17,6 @@ config HID
tristate "Generic HID support"
depends on INPUT
default y
- select HID_A4TECH if !EMBEDDED
- select HID_APPLE if !EMBEDDED
- select HID_BELKIN if !EMBEDDED
- select HID_BRIGHT if !EMBEDDED
- select HID_CHERRY if !EMBEDDED
- select HID_CHICONY if !EMBEDDED
- select HID_CYPRESS if !EMBEDDED
- select HID_DELL if !EMBEDDED
- select HID_EZKEY if !EMBEDDED
- select HID_GYRATION if !EMBEDDED
- select HID_LOGITECH if !EMBEDDED
- select HID_MICROSOFT if !EMBEDDED
- select HID_MONTEREY if !EMBEDDED
- select HID_PANTHERLORD if !EMBEDDED
- select HID_PETALYNX if !EMBEDDED
- select HID_SAMSUNG if !EMBEDDED
- select HID_SONY if !EMBEDDED
- select HID_SUNPLUS if !EMBEDDED
-
---help---
A human interface device (HID) is a type of computer device that
interacts directly with and takes input from humans. The term "HID"
@@ -102,15 +83,15 @@ config HID_COMPAT
If unsure, say Y.

config HID_A4TECH
- tristate "A4 tech"
- default m
+ tristate "A4 tech" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for A4 tech X5 and WOP-35 / Trust 450L mice.

config HID_APPLE
- tristate "Apple"
- default m
+ tristate "Apple" if EMBEDDED
+ default (USB_HID || BT_HIDP)
depends on (USB_HID || BT_HIDP)
---help---
Support for some Apple devices which less or more break
@@ -123,64 +104,64 @@ config HID_APPLE
If unsure, say M.

config HID_BELKIN
- tristate "Belkin"
- default m
+ tristate "Belkin" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Belkin Flip KVM and Wireless keyboard.

config HID_BRIGHT
- tristate "Bright"
- default m
+ tristate "Bright" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Bright ABNT-2 keyboard.

config HID_CHERRY
- tristate "Cherry"
- default m
+ tristate "Cherry" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Cherry Cymotion.

config HID_CHICONY
- tristate "Chicony"
- default m
+ tristate "Chicony" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Chicony Tactical pad.

config HID_CYPRESS
- tristate "Cypress"
- default m
+ tristate "Cypress" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Cypress mouse and barcodes.

config HID_DELL
- tristate "Dell"
- default m
+ tristate "Dell" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Dell W7658.

config HID_EZKEY
- tristate "Ezkey"
- default m
+ tristate "Ezkey" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Ezkey mouse and barcodes.

config HID_GYRATION
- tristate "Gyration"
- default m
+ tristate "Gyration" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Gyration remote.

config HID_LOGITECH
- tristate "Logitech"
- default m
+ tristate "Logitech" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for some Logitech devices which breaks less or more
@@ -211,23 +192,23 @@ config LOGIRUMBLEPAD2_FF
Rumblepad 2 devices.

config HID_MICROSOFT
- tristate "Microsoft"
- default m
+ tristate "Microsoft" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for some Microsoft devices which breaks less or more
HID specification.

config HID_MONTEREY
- tristate "Monterey"
- default m
+ tristate "Monterey" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Monterey Genius KB29E.

config HID_PANTHERLORD
- tristate "Pantherlord devices support"
- default m
+ tristate "Pantherlord devices support" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for PantherLord/GreenAsia based device support.
@@ -242,29 +223,29 @@ config PANTHERLORD_FF
or adapter and want to enable force feedback support for it.

config HID_PETALYNX
- tristate "Petalynx"
- default m
+ tristate "Petalynx" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Petalynx Maxter remote.

config HID_SAMSUNG
- tristate "Samsung"
- default m
+ tristate "Samsung" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Samsung IR remote.

config HID_SONY
- tristate "Sony"
- default m
+ tristate "Sony" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Sony PS3 controller.

config HID_SUNPLUS
- tristate "Sunplus"
- default m
+ tristate "Sunplus" if EMBEDDED
+ default USB_HID
depends on USB_HID
---help---
Support for Sunplus WDesktop input device.

2008-10-15 07:43:29

by Jiri Kosina

[permalink] [raw]
Subject: Re: [2.6 patch] HID: fix default building of all quirky devices

On Wed, 15 Oct 2008, Adrian Bunk wrote:

> Commit 9be7bbd54df3c9c393ccd19acc49f90c517d1291
> (HID: build drivers for all quirky devices by default)
> was wrong in that it select'ed the drivers, causing
> build errors like the following with CONFIG_USB_HID=n:
> <-- snip -->
>
> ...
> LD .tmp_vmlinux1
> drivers/built-in.o: In function `bright_probe':
> drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> drivers/built-in.o: In function `dell_probe':
> drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> drivers/built-in.o: In function `lg_probe':
> drivers/hid/hid-lg.c:252: undefined reference to `usbhid_set_leds'
> drivers/hid/hid-lg.c:252: more undefined references to `usbhid_set_leds' follow
> drivers/built-in.o: In function `sony_set_operational':
> drivers/hid/hid-sony.c:42: undefined reference to `usb_control_msg'
> drivers/hid/hid-sony.c:42: undefined reference to `usb_control_msg'
> make[1]: *** [.tmp_vmlinux1] Error 1

Hmm ... I am not really sure, isn't this a bug in Kconfig?

What we have right now:

- CONFIG_HID selects all the individual quirk-drivers automatically
- the individual quirk drivers have dependency on CONFIG_USB_HID

Therefore I'd expect when CONFIG_HID is selected and CONFIG_USB_HID
deselected the individual drivers not to get built (due to their
dependency on unset option).

But currently what happens is when I de-select CONFIG_USB_HID in
menuconfig, the configuration options for individual drivers vanish from
the menuconfig menu (which is expected, they depend on unset option), but
after saving .config they are still there. Is this how it is expected to
behave? (i.e. select having preference over dependency).

Thanks,

--
Jiri Kosina
SUSE Labs

2008-10-15 10:21:31

by Adrian Bunk

[permalink] [raw]
Subject: Re: [2.6 patch] HID: fix default building of all quirky devices

On Wed, Oct 15, 2008 at 09:43:18AM +0200, Jiri Kosina wrote:
> On Wed, 15 Oct 2008, Adrian Bunk wrote:
>
> > Commit 9be7bbd54df3c9c393ccd19acc49f90c517d1291
> > (HID: build drivers for all quirky devices by default)
> > was wrong in that it select'ed the drivers, causing
> > build errors like the following with CONFIG_USB_HID=n:
> > <-- snip -->
> >
> > ...
> > LD .tmp_vmlinux1
> > drivers/built-in.o: In function `bright_probe':
> > drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> > drivers/hid/hid-bright.c:38: undefined reference to `usbhid_set_leds'
> > drivers/built-in.o: In function `dell_probe':
> > drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> > drivers/hid/hid-dell.c:41: undefined reference to `usbhid_set_leds'
> > drivers/built-in.o: In function `lg_probe':
> > drivers/hid/hid-lg.c:252: undefined reference to `usbhid_set_leds'
> > drivers/hid/hid-lg.c:252: more undefined references to `usbhid_set_leds' follow
> > drivers/built-in.o: In function `sony_set_operational':
> > drivers/hid/hid-sony.c:42: undefined reference to `usb_control_msg'
> > drivers/hid/hid-sony.c:42: undefined reference to `usb_control_msg'
> > make[1]: *** [.tmp_vmlinux1] Error 1
>
> Hmm ... I am not really sure, isn't this a bug in Kconfig?

No.

> What we have right now:
>
> - CONFIG_HID selects all the individual quirk-drivers automatically
> - the individual quirk drivers have dependency on CONFIG_USB_HID
>
> Therefore I'd expect when CONFIG_HID is selected and CONFIG_USB_HID
> deselected the individual drivers not to get built (due to their
> dependency on unset option).

Different people have different expectations.

Many kernel developers seem to wrongly assume kconfig was something
trivial and everything that does not work as expected was a bug in
kconfig. But it is not, and there is no magic bullet like changing
how select behaves in some way that will suddenly solve all problems.

> But currently what happens is when I de-select CONFIG_USB_HID in
> menuconfig, the configuration options for individual drivers vanish from
> the menuconfig menu (which is expected, they depend on unset option), but
> after saving .config they are still there. Is this how it is expected to
> behave? (i.e. select having preference over dependency).

Select ignores dependencies on the selected options.

That's the documented behavior.

> Thanks,

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

2008-10-15 17:15:34

by Linus Torvalds

[permalink] [raw]
Subject: Re: [2.6 patch] HID: fix default building of all quirky devices



On Wed, 15 Oct 2008, Jiri Kosina wrote:
>
> Hmm ... I am not really sure, isn't this a bug in Kconfig?

Not really, more of a misfeature.

That said, even if we were to consider it a bug, Adrian's patch is
obviously the better way to do it. You shouldn't do non-local dependencies
(have one config option select fifty other ones), when the local ones are
clearer and more readable (have one config option just describe its *own*
dependencies).

Don't get me wrong - select is very useful, but not for "should I ask this
question or not". The point to use select is when you have some library or
other common infrastructure that isn't worth a question of its own (eg
"Why the hell would the user want to care whether he needs CRC32
routines?"), and some code says "I will need this infrastructure" by just
saying "select CRC32" to let the build system know that it needs that
particular piece of functionality.

In other words, "select" is kind of a "depends on", but for things that it
is insane to ask. It would be totally _idiotic_ to ask a user "do you want
to have CRC32 routines in the kernel?" and then based on that say "ok, you
didn't ask for CRC32, so now you cannot use the AX88796 network driver".

See? THAT is what "select" is for. And when you use select, the way we do
things now, you have to select everything you need. You cannot assume that
it will recursively select whatever it needs.

And notice how you mis-use select. That's not how to disable a question. A
question gets disabled by just doing an "if xyz" on the question itself,
like Adrian did.

Linus

2008-10-15 17:19:47

by Linus Torvalds

[permalink] [raw]
Subject: Re: [2.6 patch] HID: fix default building of all quirky devices



On Wed, 15 Oct 2008, Linus Torvalds wrote:
>
> And notice how you mis-use select. That's not how to disable a question. A
> question gets disabled by just doing an "if xyz" on the question itself,
> like Adrian did.

Side note: Adrian too has a very odd and non-obvious way of doing this.

His patch did config entries like

config HID_BRIGHT
tristate "Bright" if EMBEDDED
default USB_HID
depends on USB_HID

which is a really odd way to express this. The much more natural one is

config HID_BRIGHT
tristate "Bright" if EMBEDDED
depends on USB_HID
default y

since there's no point in saying "default USB_HID" + "depends on USB_HID",
and that's just confusing. Since it depends on USB_HID, the "default y"
will automatically degrade to whatever USB_HID was set to.

Linus

2008-10-15 17:32:58

by Adrian Bunk

[permalink] [raw]
Subject: Re: [2.6 patch] HID: fix default building of all quirky devices

On Wed, Oct 15, 2008 at 10:17:27AM -0700, Linus Torvalds wrote:
>
>
> On Wed, 15 Oct 2008, Linus Torvalds wrote:
> >
> > And notice how you mis-use select. That's not how to disable a question. A
> > question gets disabled by just doing an "if xyz" on the question itself,
> > like Adrian did.
>
> Side note: Adrian too has a very odd and non-obvious way of doing this.
>
> His patch did config entries like
>
> config HID_BRIGHT
> tristate "Bright" if EMBEDDED
> default USB_HID
> depends on USB_HID
>
> which is a really odd way to express this. The much more natural one is
>
> config HID_BRIGHT
> tristate "Bright" if EMBEDDED
> depends on USB_HID
> default y
>
> since there's no point in saying "default USB_HID" + "depends on USB_HID",
> and that's just confusing. Since it depends on USB_HID, the "default y"
> will automatically degrade to whatever USB_HID was set to.

For me the more obvious way was what I did, but I agree that your
solution looks better.

> Linus

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

2008-10-15 17:44:49

by Linus Torvalds

[permalink] [raw]
Subject: Re: [2.6 patch] HID: fix default building of all quirky devices



On Wed, 15 Oct 2008, Adrian Bunk wrote:
>
> For me the more obvious way was what I did, but I agree that your
> solution looks better.

I guess the "more obvious" part simply depends on what you're used to. The
"default y" + "depends on XYZ" thing is a rather common model, so I find
it to not just be shorter, but also "more obvious", exactly because I've
seen it before.

Of course, the behaviour of 'default y' is much simpler (and _much_ more
common!) for a bool than for a tristate, and the rules for tristate logic
in general are obviously often a bit non-intuitive considering that we're
almost always used to the binary kind, and then the fact that Kconfgi uses
ternary logic has caused no end of hiccups.

Linus

2008-10-15 22:54:35

by Jiri Kosina

[permalink] [raw]
Subject: Re: [2.6 patch] HID: fix default building of all quirky devices

On Wed, 15 Oct 2008, Linus Torvalds wrote:

> > Hmm ... I am not really sure, isn't this a bug in Kconfig?
> Not really, more of a misfeature.
> That said, even if we were to consider it a bug, Adrian's patch is
> obviously the better way to do it. You shouldn't do non-local dependencies
> (have one config option select fifty other ones), when the local ones are
> clearer and more readable (have one config option just describe its *own*
> dependencies).

I agree.

And even though I really think this is at least a misfeature, it's indeed
properly documented in Documentation/kbuild/kconfig-language.txt, so I am
really the only one to blame here.

On Wed, 15 Oct 2008, Linus Torvalds wrote:

> Side note: Adrian too has a very odd and non-obvious way of doing this.
>
> His patch did config entries like
>
> config HID_BRIGHT
> tristate "Bright" if EMBEDDED
> default USB_HID
> depends on USB_HID
>
> which is a really odd way to express this. The much more natural one is
>
> config HID_BRIGHT
> tristate "Bright" if EMBEDDED
> depends on USB_HID
> default y
>
> since there's no point in saying "default USB_HID" + "depends on USB_HID",
> and that's just confusing. Since it depends on USB_HID, the "default y"
> will automatically degrade to whatever USB_HID was set to.

Hmm, yes, looks slightly better.

I see you haven't pulled Adrian's fix from my tree yet. So please drop my
latest pull request, I will do it the "depens on + default y" way and send
you a new one shortly. There have been some more VID/PID additions
accumulated in the meantime anyway, so I'll send them too within a new
pull request.

--
Jiri Kosina

2008-10-16 08:07:05

by Geert Uytterhoeven

[permalink] [raw]
Subject: select validation (was: Re: [2.6 patch] HID: fix default building of all quirky devices)

On Wed, 15 Oct 2008, Linus Torvalds wrote:
> On Wed, 15 Oct 2008, Jiri Kosina wrote:
> > Hmm ... I am not really sure, isn't this a bug in Kconfig?
>
> Not really, more of a misfeature.
>
> That said, even if we were to consider it a bug, Adrian's patch is
> obviously the better way to do it. You shouldn't do non-local dependencies
> (have one config option select fifty other ones), when the local ones are
> clearer and more readable (have one config option just describe its *own*
> dependencies).
>
> Don't get me wrong - select is very useful, but not for "should I ask this
> question or not". The point to use select is when you have some library or
> other common infrastructure that isn't worth a question of its own (eg
> "Why the hell would the user want to care whether he needs CRC32
> routines?"), and some code says "I will need this infrastructure" by just
> saying "select CRC32" to let the build system know that it needs that
> particular piece of functionality.
>
> In other words, "select" is kind of a "depends on", but for things that it
> is insane to ask. It would be totally _idiotic_ to ask a user "do you want
> to have CRC32 routines in the kernel?" and then based on that say "ok, you
> didn't ask for CRC32, so now you cannot use the AX88796 network driver".
>
> See? THAT is what "select" is for. And when you use select, the way we do
> things now, you have to select everything you need. You cannot assume that
> it will recursively select whatever it needs.
>
> And notice how you mis-use select. That's not how to disable a question. A
> question gets disabled by just doing an "if xyz" on the question itself,
> like Adrian did.

Would it be possible for kconfig to check for invalid usage of select?

Like you should not use select to enable something that has
dependencies. However, that would fail in case both the selector option
and the selected option depend on the same.

And doing it dynamically (you should not use select to enable something that
has dependencies that are (currently) not fulfilled) wouldn't find all
incorrect usages.

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

2008-10-16 18:48:58

by Linus Torvalds

[permalink] [raw]
Subject: Re: select validation (was: Re: [2.6 patch] HID: fix default building of all quirky devices)



On Thu, 16 Oct 2008, Geert Uytterhoeven wrote:
>
> Would it be possible for kconfig to check for invalid usage of select?
>
> Like you should not use select to enable something that has
> dependencies. However, that would fail in case both the selector option
> and the selected option depend on the same.

Well, at least in theory you may actually want to select something that
has dependencies, even if you don't want to select the dependencies.

For example, some feature may be enabled by default on some architecture
or with some config. Example:

bool SUPPORT_FEATURE
default y
depends on EXPERIMENTAL
depends on !EMBEDDED
depends on X86

which is just another way of saying

bool SUPPORT_FEATURE
default X86 && !EMBEDDED && EXPERIMENTAL

and it's still possible that some code wants to do a

select SUPPORT_FEATURE

because the dependency isn't a _code_ dependency, it's a default-value
dependency.

Now, you could say that then you should use that second version (ie the
"default X86 && !EMBEDDED && EXPERIMENTAL" version), but the thing is,
"depends on" is actually a more powerful and can result in more readable
setup (because you can have multiple "depends on" lines and they are all
logically anded together.

Do we do that? I dunno. But I wouldn't be surprised if we do.

Linus

2008-10-16 19:31:24

by Adrian Bunk

[permalink] [raw]
Subject: Re: select validation (was: Re: [2.6 patch] HID: fix default building of all quirky devices)

On Thu, Oct 16, 2008 at 11:47:55AM -0700, Linus Torvalds wrote:
>
>
> On Thu, 16 Oct 2008, Geert Uytterhoeven wrote:
> >
> > Would it be possible for kconfig to check for invalid usage of select?
> >
> > Like you should not use select to enable something that has
> > dependencies. However, that would fail in case both the selector option
> > and the selected option depend on the same.
>
> Well, at least in theory you may actually want to select something that
> has dependencies, even if you don't want to select the dependencies.
>
> For example, some feature may be enabled by default on some architecture
> or with some config. Example:
>
> bool SUPPORT_FEATURE
> default y
> depends on EXPERIMENTAL
> depends on !EMBEDDED
> depends on X86
>
> which is just another way of saying
>
> bool SUPPORT_FEATURE
> default X86 && !EMBEDDED && EXPERIMENTAL

Such constructs without a prompt are for a different usecase than what
you have in mind, there wouldn't be EXPERIMENTAL or EMBEDDED in the
dependencies, this is used for stuff like

config X86_HT
bool
depends on SMP
depends on (X86_32 && !X86_VOYAGER) || X86_64
default y

And that's usually not the kind of symbol that should get selected.


In reality your example would be:

config SUPPORT_FEATURE
bool "support feature" if EMBEDDED
default y
depends on X86 && EXPERIMENTAL


> and it's still possible that some code wants to do a
>
> select SUPPORT_FEATURE
>
> because the dependency isn't a _code_ dependency, it's a default-value
> dependency.
>...

I'd be more concerned about dependencies that are used to make the
kconfig UI better.

Dependencies on options like NETDEV_1000 are not code dependencies.

> Linus

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