V4L1: make PMS not auto-grab port 0x250

Hi Andrew.

Stop Randomising Ingo's Breakconfig Builds, Part III.

Rene

From: Rene Herman <[email protected]>
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.

---
 drivers/media/video/pms.c |   77 +++++++++++++++++++++++++++++---------------
 1 files changed, 51 insertions(+), 26 deletions(-)

index 00425d7..ce63205 100644
@@ -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

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

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Tsss. Lots of people still have doom...

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

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:

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.

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)

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

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

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.