select FW_LOADER -> depends HOTPLUG

The contract is easy:
If you select something, you have to ensure the depenencies of the selected option are met.

This is simple.

And most people get it wrong.

The patch below adds dependencies on HOTPLUG for all options that select FW_LOADER.

diffstat output:

 drivers/bluetooth/Kconfig           |    4 ++--
 drivers/media/dvb/ttpci/Kconfig     |    2 +-
 drivers/media/dvb/ttusb-dec/Kconfig |    2 +-
 drivers/net/wireless/Kconfig        |    5 ++---
 drivers/scsi/Kconfig                |    2 +-
 5 files changed, 7 insertions(+), 8 deletions(-)

@@ -65,7 +65,7 @@
 
 config BT_HCIBCM203X
 	tristate "HCI BCM203x USB driver"
-	depends on USB
+	depends on USB && HOTPLUG
 	select FW_LOADER
 	help
 	  Bluetooth HCI BCM203x USB driver.

@@ -77,7 +77,7 @@
 
 config BT_HCIBFUSB
 	tristate "HCI BlueFRITZ! USB driver"
-	depends on USB
+	depends on USB && HOTPLUG
 	select FW_LOADER
 	help
 	  Bluetooth HCI BlueFRITZ! USB driver.

@@ -1019,7 +1019,7 @@
 
 config SCSI_IPR
 	tristate "IBM Power Linux RAID adapter support"
-	depends on PCI && SCSI && PPC
+	depends on PCI && SCSI && PPC && HOTPLUG
 	select FW_LOADER
 	---help---
 	  This driver supports the IBM Power Linux family RAID adapters.

@@ -1,6 +1,6 @@
 config DVB_TTUSB_DEC
 	tristate "Technotrend/Hauppauge USB DEC devices"
-	depends on DVB_CORE && USB
+	depends on DVB_CORE && USB && HOTPLUG
 	select FW_LOADER
 	select CRC32
 	help

@@ -1,6 +1,6 @@
 config DVB_AV7110
 	tristate "AV7110 cards"
-	depends on DVB_CORE
+	depends on DVB_CORE && HOTPLUG
 	select FW_LOADER
 	select VIDEO_DEV
 	select VIDEO_SAA7146_VV

@@ -223,7 +223,7 @@
 
 config ATMEL
       tristate "Atmel at76c50x chipset  802.11b support"
-      depends on NET_RADIO && EXPERIMENTAL
+      depends on NET_RADIO && HOTPLUG && EXPERIMENTAL
       select FW_LOADER
       select CRC32
        ---help---

@@ -293,8 +293,6 @@
 config PCMCIA_ATMEL
 	tristate "Atmel at76c502/at76c504 PCMCIA cards"
 	depends on NET_RADIO && ATMEL && PCMCIA
-	select FW_LOADER
-	select CRC32
 	---help---
 	  Enable support for PCMCIA cards containing the
 	  Atmel at76c502 and at76c504 chips.

@@ -309,6 +307,7 @@
 
 comment "Prism GT/Duette 802.11(a/b/g) PCI/Cardbus support"
 	depends on NET_RADIO && PCI
+
 config PRISM54
 	tristate 'Intersil Prism GT/Duette/Indigo PCI/Cardbus' 
 	depends on PCI && NET_RADIO && EXPERIMENTAL && HOTPLUG

I missed one more option (an option that selects FW_LOADER was itself selected...).

Updated patch below.

diffstat output:

 drivers/bluetooth/Kconfig           |    4 ++--
 drivers/media/dvb/ttpci/Kconfig     |    4 ++--
 drivers/media/dvb/ttusb-dec/Kconfig |    2 +-
 drivers/net/wireless/Kconfig        |    5 ++---
 drivers/scsi/Kconfig                |    2 +-
 5 files changed, 8 insertions(+), 9 deletions(-)

@@ -65,7 +65,7 @@
 
 config BT_HCIBCM203X
 	tristate "HCI BCM203x USB driver"
-	depends on USB
+	depends on USB && HOTPLUG
 	select FW_LOADER
 	help
 	  Bluetooth HCI BCM203x USB driver.

@@ -77,7 +77,7 @@
 
 config BT_HCIBFUSB
 	tristate "HCI BlueFRITZ! USB driver"
-	depends on USB
+	depends on USB && HOTPLUG
 	select FW_LOADER
 	help
 	  Bluetooth HCI BlueFRITZ! USB driver.

@@ -1019,7 +1019,7 @@
 
 config SCSI_IPR
 	tristate "IBM Power Linux RAID adapter support"
-	depends on PCI && SCSI && PPC
+	depends on PCI && SCSI && PPC && HOTPLUG
 	select FW_LOADER
 	---help---
 	  This driver supports the IBM Power Linux family RAID adapters.

@@ -1,6 +1,6 @@
 config DVB_TTUSB_DEC
 	tristate "Technotrend/Hauppauge USB DEC devices"
-	depends on DVB_CORE && USB
+	depends on DVB_CORE && USB && HOTPLUG
 	select FW_LOADER
 	select CRC32
 	help

@@ -223,7 +223,7 @@
 
 config ATMEL
       tristate "Atmel at76c50x chipset  802.11b support"
-      depends on NET_RADIO && EXPERIMENTAL
+      depends on NET_RADIO && HOTPLUG && EXPERIMENTAL
       select FW_LOADER
       select CRC32
        ---help---

@@ -293,8 +293,6 @@
 config PCMCIA_ATMEL
 	tristate "Atmel at76c502/at76c504 PCMCIA cards"
 	depends on NET_RADIO && ATMEL && PCMCIA
-	select FW_LOADER
-	select CRC32
 	---help---
 	  Enable support for PCMCIA cards containing the
 	  Atmel at76c502 and at76c504 chips.

@@ -309,6 +307,7 @@
 
 comment "Prism GT/Duette 802.11(a/b/g) PCI/Cardbus support"
 	depends on NET_RADIO && PCI
+
 config PRISM54
 	tristate 'Intersil Prism GT/Duette/Indigo PCI/Cardbus' 
 	depends on PCI && NET_RADIO && EXPERIMENTAL && HOTPLUG

@@ -1,6 +1,6 @@
 config DVB_AV7110
 	tristate "AV7110 cards"
-	depends on DVB_CORE
+	depends on DVB_CORE && HOTPLUG
 	select FW_LOADER
 	select VIDEO_DEV
 	select VIDEO_SAA7146_VV

@@ -91,7 +91,7 @@
 
 config DVB_BUDGET_PATCH
 	tristate "AV7110 cards with Budget Patch"
-	depends on DVB_BUDGET
+	depends on DVB_BUDGET && HOTPLUG
 	select DVB_AV7110
 	help
 	  Support for Budget Patch (full TS) modification on 

No - kconfig gets it wrong.
When selecting a config option kconfig shall secure that 'depends on' are evaluated also for the selected symbol.

Otherwise we will end up in a mintenace nightmare. Just think of how many places to fix if we add and additional depends on to FW_LOADER.

Roman Zippel added in to:.

Sam

Hi,

Which dependencies? select was more intended to select symbols without a prompt (it's dependency would be simply 'n'). The selected symbol can also have multiple prompts, how should these dependencies be merged? The current select is intentionally simple, so the calculation is straightforward. Anything more complex I have to completely rethink the behaviour between depencies and selects, e.g. something like this:

	A ---selects----> C ---selects----> D
	B --depends on-->   --depends on--> E

If you want to change the behaviour of select how will it change the behaviour of the other dependencies and selects?
I have some ideas for that, but I didn't had the time yet to sit down and completely think it through and a few other changes are more important.

bye, Roman

The current usage of select in the kernel covers many select's of symbols that actually have a prompt.

I assume Sam thinks in the direction to let a symbol inherit the dependencies off all symbols it selects.

E.g. in

config A
depends on B

config C
select A

C should be treated as if it would depend on B.

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

depends on Z

config Z
depends on Y

Correct. But at the same time I miss some functionality to tell me what a given symbol:
1) depends on
2) selects

It would be nice in menuconfig to see what config symbol that has dependencies and/or side effects.

[*] PCI support
Could look like:
[*]d PCI support
The space for the 'd' tag could be misused for other purposes later.

The pressing 'd' would give me the following output:

C "The C prompt"
-> depends on
CONFIG_Z "Prompt for Z"
-> depends on
CONFIG_Y "Prompt for Y"
-> Selects
CONFIG_A "Promtp for A"
-> depends on
CONFIG_B "Prompt for B"

Or something similar.
The idea is to give user an idea of dependencies (both ways) - recursive. Using both CONFIG symbol and prompt will give the user an idea where to locate it.

Something like this is on my wish list.

Sam

Definately agreed. Not all users want/can go and grep the source tree looking for dependencies of a given
symbol.

Showing the dependencies on menuconfig/xconfig is a user friendly feature.

On Wed, 11 Aug 2004 08:15:25 -0300 Marcelo Tosatti wrote:

| On Tue, Aug 10, 2004 at 11:16:57PM +0200, Sam Ravnborg wrote:
| > On Tue, Aug 10, 2004 at 10:44:11AM +0200, Adrian Bunk wrote:
| > > 
| > > I assume Sam thinks in the direction to let a symbol inherit the 
| > > dependencies off all symbols it selects.
| > > 
| > > E.g. in
| > > 
| > > config A
| > > 	depends on B
| > > 
| > > config C
| > > 	select A
| >         depends on Z
| > 
| >   config Z
| >         depends on Y
| > > 
| > > 
| > > C should be treated as if it would depend on B.
| > 
| > Correct. But at the same time I miss some functionality to
| > tell me what a given symbol:
| > 1) depends on
| > 2) selects
| > 
| > It would be nice in menuconfig to see what config symbol
| > that has dependencies and/or side effects. 
| 
| Definately agreed. Not all users want/can go and grep the source 
| tree looking for dependencies of a given 
| symbol.
| 
| Showing the dependencies on menuconfig/xconfig is a user friendly 
| feature. 

xconfig already has a way to do this. Set all Options (5 of them, although fewer would probably work). menuconfig doesn't do this.

What would be even more helpful would be a way to tell *config to jump from <current location, where a symbol is used> to <where a symbol is defined>.

--
~Randy

There are two problems:
1) If A has no prompt it's not visible and so it's dependency is 'n', this means a number of symbols wouldn't be visible anymore.
2) It would change the bahaviour of symbols, which already do multiple selects (e.g. CONFIG_INET_AH), the select of CRYPTO would be useless, as it would only become visible, when CRYPTO is enabled. This means such selects wouldn't be possible anymore.

This really needs a different (but similiar) mechanism, what I have in mind is something like this:

config A
autoselect

config B
depends on A

For the visibility calculation A is set to y and A is automatically selected if any symbol, which depends on A, is enabled.

xconfig has something like this, if you enable 'Debug Info', although it rather dumps the internal representation.
Adding something like this to menuconfig, would mean hacking lxdialog, which is rather at the bottom of the list of things I want to do. :)

bye, Roman

How do you want to handle the following?

<-- snip -->

config FW_LOADER
tristate "Hotplug firmware loading support"
depends on HOTPLUG

config ATMEL

      tristate "Atmel at76c50x chipset  802.11b support"
      depends on NET_RADIO && EXPERIMENTAL
      select FW_LOADER
      select CRC32

<-- snip -->

We've been biten that often by cases like a "select I2C_ALGOBIT" without a dependency on or a select of I2C and such cases are real issues that need a proper handling.

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

Hi,

The basic idea is to mark FW_LOADER as automatically selected and ATMEL would simply depend on it and as soon as ATMEL is selected FW_LOADER is selected. It'll work similiar to select, but it will respect dependencies (since it uses normal depencencies), it will also be easier to force deselection of FW_LOADER by (temporarily) disabling autoselect for it.

bye, Roman

Did a quick hack on this.
When choosing help on "HCI BlueFRITZ! USB driver" menuconfig now displays:

-------------------------------------------------
Depends on (select to enable this option):
BT & USB
Selects (will be enabled by this option):
FW_LOADER

CONFIG_BT_HCIBFUSB

Bluetooth HCI BlueFRITZ! USB driver.
....
-------------------------------------------------

To simplify things I just malloc'ed 'enough' memory for the help screen. Just a quick hack, but something to play with - and no lxdialog hacking :-)

Sam

===== scripts/kconfig/expr.c 1.5 vs edited =====

@@ -1087,3 +1087,42 @@
 {
 	expr_print(e, expr_print_file_helper, out, E_NONE);
 }
+
+void expr_get_dep_txt(struct expr *e, char *t)
+{
+	if (!e)
+		return;
+
+	switch (e->type) {
+	case E_SYMBOL:
+		strcat(t, e->left.sym->name);
+		break;
+	case E_AND:
+		expr_get_dep_txt(e->left.expr, t);
+		strcat(t, " & ");
+		expr_get_dep_txt(e->right.expr, t);
+		break;
+	case E_OR:
+		expr_get_dep_txt(e->left.expr, t);
+		strcat(t, " | ");
+		expr_get_dep_txt(e->right.expr, t);
+		break;
+	case E_NOT:
+		strcat(t, " !");
+		expr_get_dep_txt(e->left.expr, t);
+		break;
+	case E_EQUAL:
+		expr_get_dep_txt(e->left.expr, t);
+		strcat(t, " == ");
+		expr_get_dep_txt(e->right.expr, t);
+		break;
+	case E_UNEQUAL:
+		expr_get_dep_txt(e->left.expr, t);
+		strcat(t, " != ");
+		expr_get_dep_txt(e->right.expr, t);
+		break;
+	default:
+		printf("expr_calc_value: %d?\n", e->type);
+		break;
+	}
+}

===== scripts/kconfig/expr.h 1.11 vs edited =====

@@ -174,6 +174,7 @@
 struct expr *expr_trans_compare(struct expr *e, enum expr_type type, struct symbol *sym);
 
 void expr_fprint(struct expr *e, FILE *out);
+void expr_get_dep_txt(struct expr *e, char *t);
 
 static inline int expr_is_yes(struct expr *e)
 {

===== scripts/kconfig/lkc_proto.h 1.3 vs edited =====

@@ -9,6 +9,8 @@
 
 P(menu_is_visible,bool,(struct menu *menu));
 P(menu_get_prompt,const char *,(struct menu *menu));
+P(menu_get_dep_txt,void,(struct menu *menu, char *t));
+P(menu_get_select_txt,void,(struct menu *menu, char *t));
 P(menu_get_root_menu,struct menu *,(struct menu *menu));
 P(menu_get_parent_menu,struct menu *,(struct menu *menu));
 

===== scripts/kconfig/mconf.c 1.12 vs edited =====

@@ -572,19 +572,29 @@
 static void show_help(struct menu *menu)
 {
 	const char *help;
-	char *helptext;
+	char *helptext = malloc(20000); /* Enough.. */
 	struct symbol *sym = menu->sym;
 
+	*helptext = '\0';
+	strcat(helptext, "Depends on (select to enable this option):\n");
+	menu_get_dep_txt(menu, helptext);
+	strcat(helptext, "\nSelects (will be enabled by this option):\n");
+	menu_get_select_txt(menu, helptext);
+	
 	help = sym->help;
 	if (!help)
 		help = nohelp_text;
 	if (sym->name) {
-		helptext = malloc(strlen(sym->name) + strlen(help) + 16);
-		sprintf(helptext, "CONFIG_%s:\n\n%s", sym->name, help);
-		show_helptext(menu_get_prompt(menu), helptext);
-		free(helptext);
-	} else
-		show_helptext(menu_get_prompt(menu), help);
+		strcat(helptext, "\n\nCONFIG_");
+		strcat(helptext, sym->name);
+		strcat(helptext, "\n\n");
+		strcat(helptext, help);
+	} else {
+		strcat(helptext, "\n\n");
+		strcat(helptext, help);
+	}
+	show_helptext(menu_get_prompt(menu), helptext);
+	free(helptext);
 }
 
 static void show_readme(void)

===== scripts/kconfig/menu.c 1.12 vs edited =====

@@ -372,6 +372,26 @@
 	return NULL;
 }
 
+void menu_get_dep_txt(struct menu *menu, char *t)
+{
+	if (menu->dep)
+		expr_get_dep_txt(menu->dep, t);
+	if (menu->sym && menu->sym->dep)
+		expr_get_dep_txt(menu->sym->dep, t);
+}
+
+void menu_get_select_txt(struct menu *menu, char *t)
+{
+	if (menu->sym)
+	{
+		struct property *prop;
+		for_all_properties(menu->sym, prop, P_SELECT)
+		{
+			expr_get_dep_txt(prop->expr, t);
+		}
+	}
+}
+
 struct menu *menu_get_root_menu(struct menu *menu)
 {
 	return &rootmenu;

On Sat, 14 Aug 2004 09:49:53 +0200 Sam Ravnborg wrote:

| On Thu, Aug 12, 2004 at 01:05:47AM +0200, Roman Zippel wrote:
|  
| > > It would be nice in menuconfig to see what config symbol
| > > that has dependencies and/or side effects. 
| > 
| > xconfig has something like this, if you enable 'Debug Info', although it 
| > rather dumps the internal representation.
| > Adding something like this to menuconfig, would mean hacking lxdialog, 
| > which is rather at the bottom of the list of things I want to do. :)
| 
| Did a quick hack on this.
| When choosing help on "HCI BlueFRITZ! USB driver" menuconfig now displays:
| 
| -------------------------------------------------
| Depends on (select to enable this option):
| BT & USB
| Selects (will be enabled by this option): 
| FW_LOADER
| 
| CONFIG_BT_HCIBFUSB
| 
| Bluetooth HCI BlueFRITZ! USB driver.
| ....
| -------------------------------------------------
| 
| To simplify things I just malloc'ed 'enough' memory for the help screen.
| Just a quick hack, but something to play with - and no lxdialog hacking :-)

Hi Sam,
I started on this yesterday also, but you are ahead of me.

Looking pretty good, but for menu items in the EMBEDDED menu, it misses entries like this one:

config FUTEX
bool "Enable futex support" if EMBEDDED

It doesn't list any Depends data, but it actually depends on EMBEDDED.

I'll look at it more later. Got other stuff to do now.

--
~Randy

Unless I misunderstood Roman, FW_LOADER should be no longer selected.

But if the dependencies of BT_HCIBFUSB were
depends on BT && USB && FW_LOADER
the dependency information was useless since the option itself isn't shown for FW_LOADER=n.

And even if the option was shown:
How should an average sysadmin configuring his own kernel know where on earth the FW_LOADER option is he has to enable?

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

If I ever find time to look into it then my plan is to give the user the possibility to see _all_ menus + symbols with a prompt. And for all symbols with a promt and with dependency information to let the user enable a pop up window giving the possibility to select all "Depends on" symbols.

So consider:
config FOO
depends on BAR

config BAR
select BAZ

config BAZ

Then when pressing 'd' on the FOO symbol (which is marked as invisible) the user will be prompted with:

CONFIG_FOO

Depends on:
CONFIG_BAR: [ ] "Prompt for BAR"

...

Sam

Assuming I'm configuring a kernel for i386, what should I see when pressing 'd' on the following driver?

config OAKNET
tristate "National DP83902AV (Oak ethernet) support" depends on NET_ETHERNET && PPC && BROKEN select CRC32

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

What about:
Depends on:
[ ] "Prompt for NET_ETHERNET" (CONFIG_NET_ETHERNET)
--- "Promt ..." (CONFIG_PPC)
--- " Prompt ... " (CONFIG_BROKEN)

Selects
"Prompt for CRC32)" (CONFIG_CRC32)

That would make sense to me at least.
If I ever look into it I will touch upon the corner cases for sure.

Sam

CONFIG_BROKEN is available.

And in other cases, e.g. I2C_ALGOBIT might be a dependency, but depends itself on I2C which might be disabled.

Showing all !i386 drivers on i386 will make it harder for people to find the drivers they actually need.

Thanks.

BTW:
"make oldconfig" will need a similar treatment before we can start changing selects to depends.

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

Hi,

Use it if you really need to, but just don't use it as convenient replacement for depends.

bye, Roman

And what's the correct handling of dependencies the selected symbol has?

FW_LOADER depends on HOTPLUG, and this was the issue that started the whole thread.

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

Hi,

The use of select is already a crotch here, so there's no real correct handling. There are a few possibilities:
- if you select FW_LOADER, you have to select HOTPLUG too
- if you select FW_LOADER, you have to depend on HOTPLUG
- FW_LOADER itself can select HOTPLUG

bye, Roman

Solution 2 is what my patch tried.

Thinking about them, I'd prefer solution 3. But with solution 1 or 3, I'm sure people like Russell King will scream since this will make it non-trivial to de-select HOTPLUG.

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

Let me make my position over the use of "select" clear: I do not oppose its appropriate use, where that is defined as selecting another configuration option for which the user has no visibility.

In the above case, it _may_ make sense (I haven't looked deeply into it yet) to:

- make _all_ drivers which need FW_LOADER select it
- make _all_ drivers which currently depend on HOTPLUG select it
- make FW_LOADER select HOTPLUG
- remove user questions for FW_LOADER and HOTPLUG

That means that FW_LOADER and HOTPLUG are automatically selected whenever the configuration requires them and are automatically deselected when it doesn't need them, and you don't have to worry about whether you can disable them now or after finding the thousand and one configuration symbols which need to be turned off first.

However, keeping the option user-visible _and_ using select is problematical to say the least.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Currently, I see no good alternative to e.g. USB_STORAGE selecting SCSI. Enhanchements to _all_ kernel configuration tools are required before this can be changed.

But it still leaves the problem that modules not included in the kernel might require the functionality provided by such an option. Even CRC32 is user-visible.

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

Hi,

config CRC32
tristate "CRC32 functions" if SHOW_ME_EVERYTHING default m

bye, Roman

Hi,

What's wrong with expr_print()?

Nothing ;-)
Originally I had in mind expanding string storage using realloc as required. But my hack became a bit simpler than that, so I could have used it.

Should we delete it then?

No.
If I revisit this then I will do so, and use expr_print().

Sam

Hi,

I already thought about changing the argument to expr_print into a structure to e.g. add another function to get more control about how to print a symbol, do realloc would be easier then as well.

bye, Roman

BTW last time I tried to turn CONFIG_FW_LOADER off on 2.6.8-rc3 I was not able to do so... Is that expected?

Yes:
cd linux-2.6.7
find . -name Kconfig\* | xargs grep select | grep FW_LOADER

AFAIK there's currently no way implemented to easyly disable an option that is selected by other options.

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