[PATCH] ISDN: make ICN not auto-grab port 0x320

[PATCH] ISDN: make ICN not auto-grab port 0x320

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 69 bytes --]

Hi Andrew.

Stop Breaking Ingo's Randconfig Testing, Part II.

Rene.

[-- Attachment #2: 0001-ISDN-make-ICN-not-auto-grab-port-0x320.patch --]
[-- Type: text/plain, Size: 2928 bytes --]

>From 6f6a39c8dbc42d69d6dd654821cae8214ec382d5 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@gmail.com>
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 <rene.herman@gmail.com>
---
 drivers/isdn/icn/icn.c |   41 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/icn/icn.c b/drivers/isdn/icn/icn.c
index bf7997a..56fc1bd 100644
--- a/drivers/isdn/icn/icn.c
+++ b/drivers/isdn/icn/icn.c
@@ -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");
 }
 
-- 
1.5.5

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Alan Cox]

On Sat, 09 Aug 2008 19:39:53 +0200
Rene Herman <rene.herman@keyaccess.nl> 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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Andrew Morton]

On Sat, 9 Aug 2008 21:50:20 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Sat, 09 Aug 2008 19:39:53 +0200
> Rene Herman <rene.herman@keyaccess.nl> 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.
> 

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.

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Rene Herman]

On 12-08-08 01:48, Andrew Morton wrote:

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

--- linux.orig/drivers/ide/Kconfig
+++ linux/drivers/ide/Kconfig
@@ -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.

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Andrew Morton]

On Tue, 12 Aug 2008 07:02:10 +0200 Rene Herman <rene.herman@keyaccess.nl> wrote:

> On 12-08-08 01:48, Andrew Morton wrote:
> 
> > 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.

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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Adrian Bunk]

On Mon, Aug 11, 2008 at 10:30:43PM -0700, Andrew Morton wrote:
> On Tue, 12 Aug 2008 07:02:10 +0200 Rene Herman <rene.herman@keyaccess.nl> wrote:
> 
> > On 12-08-08 01:48, Andrew Morton wrote:
> > 
> > > 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.
> 
> 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

-- 

       "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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Rene Herman]

On 12-08-08 15:08, Adrian Bunk wrote:

> On Mon, Aug 11, 2008 at 10:30:43PM -0700, Andrew Morton wrote:

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

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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Adrian Bunk]

On Tue, Aug 12, 2008 at 03:26:19PM +0200, Rene Herman wrote:
> On 12-08-08 15:08, Adrian Bunk wrote:
>
>> On Mon, Aug 11, 2008 at 10:30:43PM -0700, Andrew Morton wrote:
>
>>> 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.
>
> 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...

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

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

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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Rene Herman]

On 12-08-08 15:26, Rene Herman wrote:

> On 12-08-08 15:08, Adrian Bunk wrote:

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

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

Rene.

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Rene Herman]

On 12-08-08 15:43, Adrian Bunk wrote:

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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Adrian Bunk]

On Tue, Aug 12, 2008 at 03:53:33PM +0200, Rene Herman wrote:
> On 12-08-08 15:43, Adrian Bunk wrote:
>
>>> 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.

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

> Rene.

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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Rene Herman]

On 12-08-08 16:03, Adrian Bunk wrote:

> On Tue, Aug 12, 2008 at 03:53:33PM +0200, Rene Herman wrote:
>> On 12-08-08 15:43, Adrian Bunk wrote:
>>
>>>> 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.
> 
> 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.

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

Re: [PATCH] ISDN: make ICN not auto-grab port 0x320

[Adrian Bunk]

On Tue, Aug 12, 2008 at 04:45:14PM +0200, Rene Herman wrote:
>
> On 12-08-08 16:03, Adrian Bunk wrote:
>
>> On Tue, Aug 12, 2008 at 03:53:33PM +0200, Rene Herman wrote:
>>> On 12-08-08 15:43, Adrian Bunk wrote:
>>>
>>>>> 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.
>>
>> 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.
>
>> 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...

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

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

> Rene.

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