[PATCH] V4L1: make PMS not auto-grab port 0x250

[PATCH] V4L1: make PMS not auto-grab port 0x250

[Rene Herman]

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

Hi Andrew.

Stop Randomising Ingo's Breakconfig Builds, Part III.

Rene

[-- Attachment #2: 0001-V4L1-make-PMS-not-auto-grab-port-0x250.patch --]
[-- Type: text/plain, Size: 4372 bytes --]

>From cb369dfe174a66bd4d815950c23c1d30234ea8d2 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@gmail.com>
Date: Sat, 9 Aug 2008 20:39:19 +0200
Subject: [PATCH] V4L1: make PMS not auto-grab port 0x250

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.

This specific V4L1 driver does probe for its hardware and releases
resources when not found but was still found to hang the boot when
booting a randconfig kernel. Just insist that the user specify the
port before going ahead and poking at it.

Users of this driver are nonexistent and/or a one time

echo "options pms io_port=0x250" >> /etc/modprobe.conf

away from the old behaviour.

This is a deprecated driver 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/media/video/pms.c |   77 +++++++++++++++++++++++++++++---------------
 1 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/media/video/pms.c b/drivers/media/video/pms.c
index 00425d7..ce63205 100644
--- a/drivers/media/video/pms.c
+++ b/drivers/media/video/pms.c
@@ -27,14 +27,13 @@
 #include <linux/mm.h>
 #include <linux/ioport.h>
 #include <linux/init.h>
-#include <asm/io.h>
 #include <linux/videodev.h>
-#include <media/v4l2-common.h>
-#include <media/v4l2-ioctl.h>
 #include <linux/mutex.h>
-
+#include <linux/isa.h>
+#include <asm/io.h>
 #include <asm/uaccess.h>
-
+#include <media/v4l2-common.h>
+#include <media/v4l2-ioctl.h>
 
 #define MOTOROLA	1
 #define PHILIPS2	2
@@ -68,13 +67,12 @@ static int standard;	/* 0 - auto 1 - ntsc 2 - pal 3 - secam */
  *	I/O ports and Shared Memory
  */
 
-static int io_port		=	0x250;
-static int data_port		=	0x251;
-static int mem_base		=	0xC8000;
-static void __iomem *mem;
-static int video_nr             =       -1;
-
+static int io_port;
+static int data_port;
+static int mem_base = 0xC8000;
+static int video_nr = -1;
 
+static void __iomem *mem;
 
 static inline void mvv_write(u8 index, u8 value)
 {
@@ -1019,10 +1017,26 @@ static int init_mediavision(void)
  *	Initialization and module stuff
  */
 
-static int __init init_pms_cards(void)
+module_param(io_port, int, 0);
+module_param(mem_base, int, 0);
+module_param(video_nr, int, 0);
+MODULE_LICENSE("GPL");
+
+static int __devinit pms_isa_match(struct device *dev, unsigned int id)
 {
-	printk(KERN_INFO "Mediavision Pro Movie Studio driver 0.02\n");
+	int match = io_port != 0;
 
+	if (match)
+		dev_info(dev, "io_port = %#x, mem_base = %#x\n",
+			 io_port, mem_base);
+	else
+		dev_err(dev, "please specify io_port\n");
+
+	return match;
+}
+
+static int __devinit pms_isa_probe(struct device *dev, unsigned int id)
+{
 	data_port = io_port +1;
 
 	if(init_mediavision())
@@ -1039,25 +1053,36 @@ static int __init init_pms_cards(void)
 	return video_register_device((struct video_device *)&pms_device, VFL_TYPE_GRABBER, video_nr);
 }
 
-module_param(io_port, int, 0);
-module_param(mem_base, int, 0);
-module_param(video_nr, int, 0);
-MODULE_LICENSE("GPL");
+static int __devexit pms_isa_remove(struct device *dev, unsigned int id)
+{
+	release_region(io_port, 3);
+	release_region(0x9A01, 1);
+	video_unregister_device((struct video_device *)&pms_device);
+	iounmap(mem);
+	return 0;
+}
+
+static struct isa_driver pms_isa_driver = {
+	.match		= pms_isa_match,
+	.probe		= pms_isa_probe,
+	.remove		= __devexit_p(pms_isa_remove),
 
+	.driver		= {
+		.name	= "pms"
+	}
+};
 
-static void __exit shutdown_mediavision(void)
+static int __init pms_init(void)
 {
-	release_region(io_port,3);
-	release_region(0x9A01, 1);
+	printk(KERN_INFO "Mediavision Pro Movie Studio driver 0.02\n");
+	return isa_register_driver(&pms_isa_driver, 1);
 }
 
-static void __exit cleanup_pms_module(void)
+static void __exit pms_exit(void)
 {
-	shutdown_mediavision();
-	video_unregister_device((struct video_device *)&pms_device);
-	iounmap(mem);
+	isa_unregister_driver(&pms_isa_driver);
 }
 
-module_init(init_pms_cards);
-module_exit(cleanup_pms_module);
+module_init(pms_init);
+module_exit(pms_exit);
 
-- 
1.5.5

Re: [PATCH] V4L1: make PMS not auto-grab port 0x250

[Alan Cox]

I have a PMS card. It was the hot technology of 199x about the same time
as doom came out. I'm probably the only person who still has one ;)

I'm going to NAK this however because passing in a port is a really dumb
interface. The PMS card can only be at port 0x250 so if you load it there
is no doubt and confusion involved.

The code is fine, the behaviour is correct. Ingo should fix his config
stuff.

Just apply a tiny bit of rational thought here. There is exactly ONE
Ingo. He's a smart cookie and can add exception lists to his tester.
There are millions of users some of whom are brilliant, others are not
computer wizards. The code should be optimised for them not for Ingo -
Ingo is an optimisation for the special case not the normal workload!

Alan

Re: [PATCH] V4L1: make PMS not auto-grab port 0x250

[Rene Herman]

On 09-08-08 22:46, Alan Cox wrote:

> I have a PMS card. It was the hot technology of 199x about the same
> time as doom came out. I'm probably the only person who still has one
> ;)

Tsss. Lots of people still have doom...

> I'm going to NAK this however because passing in a port is a really
> dumb interface. The PMS card can only be at port 0x250 so if you load
> it there is no doubt and confusion involved.
> 
> The code is fine, the behaviour is correct. Ingo should fix his
> config stuff.

He already did. The deep legacy ones such as this though I myself feel 
are better of just not doing what they do.

> Just apply a tiny bit of rational thought here. There is exactly ONE 
> Ingo.

And as you say yourself -- close to exactly 1 person who still has this 
hardware and closer still to 0 who use it. Really, you contradict yourself:

> He's a smart cookie and can add exception lists to his tester. There
> are millions of users some of whom are brilliant, others are not
> computer wizards. The code should be optimised for them not for Ingo
> - Ingo is an optimisation for the special case not the normal 
> workload!

Millions of users using PMS? I expect you are still going to NAK this 
anyway out of a theoretical standpoint but please stop contradicting 
yourself ;-)

We know this driver breaks the boot during useful kernel work. We know 
that changing it has about a 0.0001% percent change of mattering to 
anyone and then only as long as all those person can't be bothered to 
setup a value in his modprobe.conf.

Now, mind you, I don't care really deeply or anything but this is the 
second time today that I get a comment that places something theoretical 
over something actual. I had deluded myself into thinking that was not 
the way things were done here. Silly me.

Rene.

Re: [PATCH] V4L1: make PMS not auto-grab port 0x250

[Alan Cox]

> > Just apply a tiny bit of rational thought here. There is exactly ONE 
> > Ingo.
> 
> And as you say yourself -- close to exactly 1 person who still has this 
> hardware and closer still to 0 who use it. Really, you contradict yourself:

Consider the posibility that I might be talking about the general case
here. And if there are two people with the PMS card thats the general
case 8)

> We know this driver breaks the boot during useful kernel work. We know 
> that changing it has about a 0.0001% percent change of mattering to 
> anyone and then only as long as all those person can't be bothered to 
> setup a value in his modprobe.conf.

It never breaks anything for anyone as a module, only compiled in. Which
despite your moans about 'factual' things is a fact.

Can I suggest a rather more elegant solution would be to add a
"CONFIG_UNSAFE_PROBES" tristate (so you can decide if you want no unsafe
probing devices, unsafe probing devices only as modules, or anything goes)

Alan

Re: [PATCH] V4L1: make PMS not auto-grab port 0x250

[Rene Herman]

On 09-08-08 23:55, Alan Cox wrote:

> It never breaks anything for anyone as a module, only compiled in.

Exactly, such as during a randconfig which is the issue. If I change 
things to not autograb when builtin, autograb when modular, would you 
still object?

Rene.