ISDN: make ICN not auto-grab port 0x320

PATCH In review Rene Herman 4 participants 13 messages Aug 9, 2008 – Aug 12, 2008

lore

Share & tools

lore

Rene Herman

author

2008-08-09 17:39 UTC #0 lore raw

PATCH 1/1 ISDN: make ICN not auto-grab port 0x320

Hi Andrew.

Stop Breaking Ingo's Randconfig Testing, Part II.

Rene.

Quoted 1 line

From 6f6a39c8dbc42d69d6dd654821cae8214ec382d5 Mon Sep 17 00:00:00 2001

From: Rene Herman <[email protected]>
Date: Sat, 9 Aug 2008 18:12:12 +0200
Subject: [PATCH] ISDN: make ICN not auto-grab port 0x320

Grabbing ISA bus resources without anything or anyone telling us we should can break boot on randconfig/allyesconfig builds by keeping resources that are in fact owned by different hardware busy and does as reported by Ingo Molnar.

Generally it's also dangerous to just poke at random I/O ports and especially those in the range where other old easily confused ISA hardware might live.

For this old I4L ISA ISDN driver, insist that the user specify the port before grabbing it.

Users of this driver are nonexistent or howling mad and a one time

echo "options icn baseport=0x320" >> /etc/modprobe.conf

away from the old behaviour.

This is a deprecated driver (stack, even) but as long as it's in the tree, might as well fix it I guess.

Signed-off-by Rene Herman <[email protected]>

---
 drivers/isdn/icn/icn.c |   41 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 38 insertions(+), 3 deletions(-)

drivers/isdn/icn/icn.c +38 -3

index bf7997a..56fc1bd 100644
@@ -13,8 +13,9 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/sched.h>
+#include <linux/isa.h>
 
-static int portbase = ICN_BASEADDR;
+static int portbase;
 static unsigned long membase = ICN_MEMADDR;
 static char *icn_id = "\0";
 static char *icn_id2 = "\0";

@@ -1623,7 +1624,20 @@ icn_setup(char *line)
 __setup("icn=", icn_setup);
 #endif /* MODULE */
 
-static int __init icn_init(void)
+static int __devinit icn_isa_match(struct device *isa_dev, unsigned int id)
+{
+	int match = portbase != 0;
+
+	if (match)
+		dev_info(isa_dev, "portbase = %#x, membase = %#lx\n",
+			 portbase, membase);
+	else
+		dev_err(isa_dev, "please specify portbase\n");
+
+	return match;
+}
+
+static int __devinit icn_isa_probe(struct device *isa_dev, unsigned int id)
 {
 	char *p;
 	char rev[10];

@@ -1646,7 +1660,7 @@ static int __init icn_init(void)
 	return (icn_addcard(portbase, icn_id, icn_id2));
 }
 
-static void __exit icn_exit(void)
+static int __devexit icn_isa_remove(struct device *isa_dev, unsigned int id)
 {
 	isdn_ctrl cmd;
 	icn_card *card = cards;

@@ -1685,6 +1699,27 @@ static void __exit icn_exit(void)
 		iounmap(dev.shmem);
 		release_mem_region(dev.memaddr, 0x4000);
 	}
+	return 0;
+}
+
+static struct isa_driver icn_isa_driver = {
+	.match		= icn_isa_match,
+	.probe		= icn_isa_probe,
+	.remove		= __devexit_p(icn_isa_remove),
+
+	.driver	= {
+		.name	= "icn"
+	}
+};
+
+static int __init icn_init(void)
+{
+	return isa_register_driver(&icn_isa_driver, 1);
+}
+
+static void __exit icn_exit(void)
+{
+	isa_unregister_driver(&icn_isa_driver);
 	printk(KERN_NOTICE "ICN-ISDN-driver unloaded\n");
 }

Signature · 1 line

1.5.5

Alan Cox

2008-08-09 21:07 UTC #1 lore raw

Quoted 4 lines · On Sat, 09 Aug 2008 19:39:53 +0200 Rene Herman <[email protected]> wrote:

Grabbing ISA bus resources without anything or anyone telling us we should can break boot on randconfig/allyesconfig builds by keeping resources that are in fact owned by different hardware busy and does as reported by Ingo Molnar.

Not an interesting case, and also wrong in the modular case where loading the module is a direct user action indicating clear intent to use the functionality *as is*.

NAK this one too.

At the very least make the requirement to say "like please run for real" dependant on it not being built modular - which is what was done for the last ones that were twiddled to keep Ingo amused.

Alan

Andrew Morton

2008-08-11 23:49 UTC #2 lore raw

Quoted 12 lines · On Sat, 9 Aug 2008 21:50:20 +0100 Alan Cox <[email protected]> wrote:

Quoted 4 lines · On Sat, 09 Aug 2008 19:39:53 +0200 Rene Herman <[email protected]> wrote:
Grabbing ISA bus resources without anything or anyone telling us we should can break boot on randconfig/allyesconfig builds by keeping resources that are in fact owned by different hardware busy and does as reported by Ingo Molnar.
Not an interesting case, and also wrong in the modular case where loading the module is a direct user action indicating clear intent to use the functionality *as is*.

Quoted 6 lines

NAK this one too.
At the very least make the requirement to say "like please run for real" dependant on it not being built modular - which is what was done for the last ones that were twiddled to keep Ingo amused.

otoh it's a bit sad to break allyesconfig kernels - there is regression-testing value in being able to run such kernels.

I wonder if we can add a new boot option `allyesconfig-test' or something like that, and then, within the offending drivers, test that flag and take suitable avoiding action.

Or we could do it at compile-time - define
CONFIG_ALLYESCONFIG_TESTING in some fashion.

Rene Herman

author

2008-08-12 05:02 UTC #3 lore raw

Quoted 9 lines · Andrew Morton

otoh it's a bit sad to break allyesconfig kernels - there is regression-testing value in being able to run such kernels.
I wonder if we can add a new boot option `allyesconfig-test' or something like that, and then, within the offending drivers, test that flag and take suitable avoiding action.
Or we could do it at compile-time - define
CONFIG_ALLYESCONFIG_TESTING in some fashion.

Yes, latter I'd feel with the thing most against it that there aren't actually many that need it. Here's the list that was posted:

http://lkml.org/lkml/2008/8/1/96

Three legacy ISA driver from that list can now be stricken. Hurrah.

There's always going to be at least a few that'll remain though. For example, what are you going to do with:

drivers/ide/Kconfig +5 -0

@@ -9,6 +9,11 @@ config HAVE_IDE
 menuconfig IDE
 	tristate "ATA/ATAPI/MFM/RLL support"
 	depends on HAVE_IDE
+
+	# my test box expects /dev/sda, not /dev/hda
+	depends on BROKEN_BOOT_ALLOWED
+	select BROKEN_BOOT if IDE = y
+
 	depends on BLOCK
 	---help---
 	  If you say Y here, your kernel will be able to manage low cost

The hda->sda thing was just a disaster -- personally I have to remember to change things around till this very day before I boot into my distro kernel to test things. I wouldn't be the one suggesting a patch to drivers/ide to "fix" that though including really a patch to its Kconfig marking it as breaking anything in a global way rather than a user-specific way as was done here.

Maybe the test boxes should always boot with ide_core.noprobe=<all> for a suitable value of <all> or some such...

Others are the video ones. FB_VESA, FB_VGA16FB and even MDA_CONSOLE are in there which seems to imply some decidly non-legacy hardware and I sort of doubt willingness to assume global validity of such a setup.

But yes, there is value in the testing so perhaps still worh something generic.

(I'll continue some specific looking through the list first although I don't believe there is much else that really wants specific fixing).

Rene.

Andrew Morton

2008-08-12 05:31 UTC #4 lore raw

Quoted 9 lines · On Tue, 12 Aug 2008 07:02:10 +0200 Rene Herman <[email protected]> wrote:

Quoted 7 lines · Andrew Morton
otoh it's a bit sad to break allyesconfig kernels - there is regression-testing value in being able to run such kernels.
I wonder if we can add a new boot option `allyesconfig-test' or something like that, and then, within the offending drivers, test that flag and take suitable avoiding action.

Quoted 5 lines

Quoted 2 lines
Or we could do it at compile-time - define
CONFIG_ALLYESCONFIG_TESTING in some fashion.
Yes, latter I'd feel with the thing most against it that there aren't actually many that need it.

I think the boot option is the way, if at all.

Because the config option isn't very usable. What's to stop someone from doing `make allyesconfig' and then menually editing the .config so it's no longer truly an allyesconfig .config?

otoh, if is't purely a manual setting rather than some automagic thing then it might be workable. CONFIG_INGO :)

Adrian Bunk

2008-08-12 13:10 UTC #5 lore raw

Quoted 17 lines · Andrew Morton

Quoted 9 lines · On Tue, 12 Aug 2008 07:02:10 +0200 Rene Herman <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
otoh it's a bit sad to break allyesconfig kernels - there is regression-testing value in being able to run such kernels.
I wonder if we can add a new boot option `allyesconfig-test' or something like that, and then, within the offending drivers, test that flag and take suitable avoiding action.
Quoted 5 lines
Quoted 2 lines
Or we could do it at compile-time - define
CONFIG_ALLYESCONFIG_TESTING in some fashion.
Yes, latter I'd feel with the thing most against it that there aren't actually many that need it.

Quoted 8 lines

I think the boot option is the way, if at all.
Because the config option isn't very usable. What's to stop someone from doing `make allyesconfig' and then menually editing the .config so it's no longer truly an allyesconfig .config?
otoh, if is't purely a manual setting rather than some automagic thing then it might be workable. CONFIG_INGO :)

Write the required settings into a file and use KCONFIG_ALLCONFIG.

Our kconfig files are already complicated enough, and needlessly adding more complexity only increases the problems (which would in turn result in this Hungarian guy spreading his kconfig FUD even more often).

Tomorrow someone might want to boot an allyesconfig kernel on an old Pentium-MMX, and the CONFIG_M686=y allyesconfig gives might break his boot - another special case in kconfig for an even more exotic use case?

Everyone doing randconfig builds knows that CONFIG_STANDALONE=n is not a good idea, and similarly someone trying to boot allyesconfig kernels might need a list of options he mustn't enable.

After all, we are not talking about stuff for normal users.

cu
Adrian

Signature · 4 lines

       "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

Rene Herman

author

2008-08-12 13:26 UTC #6 lore raw

Quoted 4 lines · Adrian Bunk

Quoted 2 lines · Andrew Morton
I think the boot option is the way, if at all.

Quoted 8 lines

Quoted 6 lines
Because the config option isn't very usable. What's to stop someone from doing `make allyesconfig' and then menually editing the .config so it's no longer truly an allyesconfig .config?
otoh, if is't purely a manual setting rather than some automagic thing then it might be workable. CONFIG_INGO :)
Write the required settings into a file and use KCONFIG_ALLCONFIG.

Found those, but seems unable to express the "depends on BROKEN_BOOT if FOO=y" method that's currently used.

Yes, might admittedly not be considered a huge problem and perhaps we could just ship an allyes.config (and same allrandom.config) but it's not nice to spread information about a single symbol over various files like that.

Currently, we have a tristate that turns into a y/n bool if !MODULES. What would be real nice here is a tristate that turns into a m/n bool if !RANDOM, where allyesconfig and randconfig would pre-select RANDOM.

If I'm not mistaken, that's currently not possible...

Rene

Adrian Bunk

2008-08-12 13:45 UTC #7 lore raw

Quoted 15 lines · Rene Herman

Quoted 4 lines · Adrian Bunk
Quoted 2 lines · Andrew Morton
I think the boot option is the way, if at all.
Quoted 8 lines
Quoted 6 lines
Because the config option isn't very usable. What's to stop someone from doing `make allyesconfig' and then menually editing the .config so it's no longer truly an allyesconfig .config?
otoh, if is't purely a manual setting rather than some automagic thing then it might be workable. CONFIG_INGO :)
Write the required settings into a file and use KCONFIG_ALLCONFIG.

Quoted 7 lines

Found those, but seems unable to express the "depends on BROKEN_BOOT if FOO=y" method that's currently used.
Yes, might admittedly not be considered a huge problem and perhaps we could just ship an allyes.config (and same allrandom.config) but it's not nice to spread information about a single symbol over various files like that.

The dependencies we express in kconfig are already pretty complicated, and that is continuously causing problems.

Stuffing more stuff into kconfig for such an exotic developer-only use case is not a good idea.

And as already said, tomorrow someone might want to boot an allyesconfig kernel on a Pentium-MMX...

Quoted 3 lines

Currently, we have a tristate that turns into a y/n bool if !MODULES. What would be real nice here is a tristate that turns into a m/n bool if !RANDOM, where allyesconfig and randconfig would pre-select RANDOM.

allyesconfig is not random.

Quoted 3 lines

If I'm not mistaken, that's currently not possible...
Rene

cu
Adrian

Signature · 4 lines

       "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

Rene Herman

author

2008-08-12 13:53 UTC #8 lore raw

Quoted 3 lines · Adrian Bunk

Quoted 1 line
Currently, we have a tristate that turns into a y/n bool if !MODULES. What would be real nice here is a tristate that turns into a m/n bool if !RANDOM, where allyesconfig and randconfig would pre-select RANDOM.
allyesconfig is not random.

Oh, how very, very important. s/RANDOM/!SPECIFIC/ then (and I meant "if RANDOM" ofcourse).

The point is just that such a tristate would be a one-stop mark for drivers/options that you want to be specifically selected for builtin use since they're not necessarily expected to boot on general PCs.

Rene.

Adrian Bunk

2008-08-12 14:05 UTC #9 lore raw

Quoted 7 lines · Rene Herman

Quoted 5 lines · Adrian Bunk
Quoted 4 lines
Currently, we have a tristate that turns into a y/n bool if !MODULES.
What would be real nice here is a tristate that turns into a m/n
bool if !RANDOM, where allyesconfig and randconfig would pre-select RANDOM.

Quoted 8 lines

Quoted 1 line
allyesconfig is not random.
Oh, how very, very important. s/RANDOM/!SPECIFIC/ then (and I meant "if RANDOM" ofcourse).
The point is just that such a tristate would be a one-stop mark for drivers/options that you want to be specifically selected for builtin use since they're not necessarily expected to boot on general PCs.

The part of what you want that is AFAIK not possible with the current kconfig is the "allyesconfig and randconfig would pre-select".

The actual dependencies on such an option could trivially be expressed the way you want it.

But I'm trying to make the kconfig dependencies more robust by making them less complex, and making them more complex for this developer-only use case is nonsense.

Even more considering that the developer who gave the initial list of variables is the same who always spreads FUD about kconfig when the complex dependencies break...

Quoted 1 line

Rene.

cu
Adrian

Signature · 4 lines

       "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

Rene Herman

author

2008-08-12 14:45 UTC #10 lore raw

Quoted 6 lines · Adrian Bunk

Quoted 5 lines · Rene Herman
Quoted 3 lines · Adrian Bunk
Quoted 3 lines
Currently, we have a tristate that turns into a y/n bool if !MODULES.
What would be real nice here is a tristate that turns into a m/n
bool if !RANDOM, where allyesconfig and randconfig would pre-select RANDOM.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
allyesconfig is not random.
Oh, how very, very important. s/RANDOM/!SPECIFIC/ then (and I meant "if RANDOM" ofcourse).
The point is just that such a tristate would be a one-stop mark for drivers/options that you want to be specifically selected for builtin use since they're not necessarily expected to boot on general PCs.
The part of what you want that is AFAIK not possible with the current kconfig is the "allyesconfig and randconfig would pre-select".
The actual dependencies on such an option could trivially be expressed the way you want it.

Obviously, and that's what is already being done. The point is that as a kconfig intrinsic it doesn't add complexity.

Quoted 2 lines

But I'm trying to make the kconfig dependencies more robust by making them less complex, and making them more complex for this developer-only use case is nonsense.

There's nothing complex about

config FOO
modstate "Dafttech Boot Breaker 2000 support"

with a modstate being;

- a tristate when !CONFIG_RANDOM
- an m/n choice otherwise

And note that it's not "single developer use case". Feel how you want about anyone, but it's about useful testing being done. It's about developers getting bugs reported to them as a result.

Rene.

Adrian Bunk

2008-08-12 15:25 UTC #11 lore raw

Quoted 28 lines · Rene Herman

Quoted 16 lines · Adrian Bunk
Quoted 6 lines · Rene Herman
Quoted 4 lines · Adrian Bunk
Quoted 4 lines
Currently, we have a tristate that turns into a y/n bool if !MODULES. What would be real nice here is a tristate that turns into a m/n bool if !RANDOM, where allyesconfig and randconfig would pre-select RANDOM.
Quoted 8 lines
Quoted 1 line
allyesconfig is not random.
Oh, how very, very important. s/RANDOM/!SPECIFIC/ then (and I meant "if RANDOM" ofcourse).
The point is just that such a tristate would be a one-stop mark for drivers/options that you want to be specifically selected for builtin use since they're not necessarily expected to boot on general PCs.
Quoted 5 lines
The part of what you want that is AFAIK not possible with the current kconfig is the "allyesconfig and randconfig would pre-select".
The actual dependencies on such an option could trivially be expressed the way you want it.
Obviously, and that's what is already being done. The point is that as a kconfig intrinsic it doesn't add complexity.

Quoted 8 lines

Quoted 3 lines
But I'm trying to make the kconfig dependencies more robust by making them less complex, and making them more complex for this developer-only use case is nonsense.
There's nothing complex about
config FOO
modstate "Dafttech Boot Breaker 2000 support"

What happens if another option selects FOO?

As soon as you mix select's with dependencies and tristates you are at the heart of all kconfig complexity...

Quoted 4 lines

with a modstate being;
- a tristate when !CONFIG_RANDOM
- an m/n choice otherwise

You don't need to add a new syntax for that.

Quoted 3 lines

And note that it's not "single developer use case". Feel how you want about anyone, but it's about useful testing being done. It's about developers getting bugs reported to them as a result.

It is something that would only be used by a handful of kernel developers.

It's like randconfig, where having to force CONFIG_STANDALONE=y never was a real obstable.

Quoted 1 line

Rene.

cu
Adrian

Signature · 4 lines

       "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

Rene Herman

author

2008-08-12 13:47 UTC #12 lore raw

In reply to Rene Herman

Quoted 9 lines · Rene Herman

Quoted 1 line · Adrian Bunk
Write the required settings into a file and use KCONFIG_ALLCONFIG.
Found those, but seems unable to express the "depends on BROKEN_BOOT if FOO=y" method that's currently used.
Yes, might admittedly not be considered a huge problem and perhaps we could just ship an allyes.config (and same allrandom.config) but it's not nice to spread information about a single symbol over various files like that.
Currently, we have a tristate that turns into a y/n bool if !MODULES. What would be real nice here is a tristate that turns into a m/n bool if !RANDOM, where allyesconfig and randconfig would pre-select RANDOM.

(I ofcourse mean "if RANDOM" here)

Quoted 1 line

If I'm not mistaken, that's currently not possible...

Rene.