<html><head><meta name="color-scheme" content="light dark"></head><body><pre style="word-wrap: break-word; white-space: pre-wrap;">ChangeSet 1.1043.1.20, 2003/02/17 10:41:09-08:00, mark@alpha.dyndns.org

[PATCH] USB: ov511 bugfixes/cleanup

This patch updates the 2.5 ov511 driver to version 1.64. This fixes some
longstanding bugs and cleans the code up a bit.

Changes:
 - Eliminate remaining uses of sleep_on()
 - Remove unnecessary (and racy) calls to waitqueue_active()
 - Fix a memory leak in the open() error path
 - Remove some redundant and unused variables
 - Documentation fixes


diff -Nru a/drivers/usb/media/ov511.c b/drivers/usb/media/ov511.c
--- a/drivers/usb/media/ov511.c	Tue Feb 18 16:38:25 2003
+++ b/drivers/usb/media/ov511.c	Tue Feb 18 16:38:25 2003
@@ -1,7 +1,7 @@
 /*
  * OmniVision OV511 Camera-to-USB Bridge Driver
  *
- * Copyright (c) 1999-2002 Mark W. McClelland
+ * Copyright (c) 1999-2003 Mark W. McClelland
  * Original decompression code Copyright 1998-2000 OmniVision Technologies
  * Many improvements by Bret Wallach &lt;bwallac1@san.rr.com&gt;
  * Color fixes by by Orion Sky Lawlor &lt;olawlor@acm.org&gt; (2/26/2000)
@@ -60,7 +60,7 @@
 /*
  * Version Information
  */
-#define DRIVER_VERSION "v1.63 for Linux 2.5"
+#define DRIVER_VERSION "v1.64 for Linux 2.5"
 #define EMAIL "mark@alpha.dyndns.org"
 #define DRIVER_AUTHOR "Mark McClelland &lt;mark@alpha.dyndns.org&gt; &amp; Bret Wallach \
 	&amp; Orion Sky Lawlor &lt;olawlor@acm.org&gt; &amp; Kevin Moore &amp; Charl P. Botha \
@@ -137,7 +137,7 @@
 MODULE_PARM(cams, "i");
 MODULE_PARM_DESC(cams, "Number of simultaneous cameras");
 MODULE_PARM(compress, "i");
-MODULE_PARM_DESC(compress, "Turn on compression (not reliable yet)");
+MODULE_PARM_DESC(compress, "Turn on compression");
 MODULE_PARM(testpat, "i");
 MODULE_PARM_DESC(testpat,
   "Replace image with vertical bar testpattern (only partially working)");
@@ -1349,6 +1349,13 @@
 	return 0;
 }
 
+/* Sleeps until no frames are active. Returns !0 if got signal */
+static int
+ov51x_wait_frames_inactive(struct usb_ov511 *ov)
+{
+	return wait_event_interruptible(ov-&gt;wq, ov-&gt;curframe &lt; 0);
+}
+
 /* Resets the hardware snapshot button */
 static void
 ov51x_clear_snapshot(struct usb_ov511 *ov)
@@ -2121,7 +2128,7 @@
 
 	return 0;
 }
-#endif /* CONFIG_PROC_FS &amp;&amp; CONFIG_VIDEO_PROC_FS */
+#endif /* CONFIG_VIDEO_PROC_FS */
 
 /* Turns on or off the LED. Only has an effect with OV511+/OV518(+) */
 static void
@@ -2486,8 +2493,6 @@
 
 	/******** Clock programming ********/
 
-	// FIXME: Test this with OV6630
-
 	/* The OV6620 needs special handling. This prevents the 
 	 * severe banding that normally occurs */
 	if (ov-&gt;sensor == SEN_OV6620 || ov-&gt;sensor == SEN_OV6630)
@@ -2995,6 +3000,7 @@
 			ov-&gt;frame[i].format = force_palette;
 		else
 			ov-&gt;frame[i].format = VIDEO_PALETTE_YUV420;
+
 		ov-&gt;frame[i].depth = get_depth(ov-&gt;frame[i].format);
 	}
 
@@ -3577,12 +3583,8 @@
 		if (frame-&gt;scanstate == STATE_LINES) {
 	    		int nextf;
 
-			frame-&gt;grabstate = FRAME_DONE;	// FIXME: Is this right?
-
-			if (waitqueue_active(&amp;frame-&gt;wq)) {
-				frame-&gt;grabstate = FRAME_DONE;
-				wake_up_interruptible(&amp;frame-&gt;wq);
-			}
+			frame-&gt;grabstate = FRAME_DONE;
+			wake_up_interruptible(&amp;frame-&gt;wq);
 
 			/* If next frame is ready or grabbing,
 			 * point to it */
@@ -3747,12 +3749,8 @@
 	if (frame-&gt;scanstate == STATE_LINES) {
     		int nextf;
 
-		frame-&gt;grabstate = FRAME_DONE;	// FIXME: Is this right?
-
-		if (waitqueue_active(&amp;frame-&gt;wq)) {
-			frame-&gt;grabstate = FRAME_DONE;
-			wake_up_interruptible(&amp;frame-&gt;wq);
-		}
+		frame-&gt;grabstate = FRAME_DONE;
+		wake_up_interruptible(&amp;frame-&gt;wq);
 
 		/* If next frame is ready or grabbing,
 		 * point to it */
@@ -4228,7 +4226,7 @@
 }
 
 static void
-ov51x_dealloc(struct usb_ov511 *ov, int now)
+ov51x_dealloc(struct usb_ov511 *ov)
 {
 	PDEBUG(4, "entered");
 	down(&amp;ov-&gt;buf_lock);
@@ -4258,10 +4256,6 @@
 	if (ov-&gt;user)
 		goto out;
 
-	err = ov51x_alloc(ov);
-	if (err &lt; 0)
-		goto out;
-
 	ov-&gt;sub_flag = 0;
 
 	/* In case app doesn't set them... */
@@ -4283,9 +4277,13 @@
 			goto out;
 	}
 
+	err = ov51x_alloc(ov);
+	if (err &lt; 0)
+		goto out;
+
 	err = ov51x_init_isoc(ov);
 	if (err) {
-		ov51x_dealloc(ov, 0);
+		ov51x_dealloc(ov);
 		goto out;
 	}
 
@@ -4319,7 +4317,7 @@
 		ov51x_led_control(ov, 0);
 
 	if (ov-&gt;dev)
-		ov51x_dealloc(ov, 0);
+		ov51x_dealloc(ov);
 
 	up(&amp;ov-&gt;lock);
 
@@ -4331,7 +4329,7 @@
 		ov-&gt;cbuf = NULL;
 		up(&amp;ov-&gt;cbuf_lock);
 
-		ov51x_dealloc(ov, 1);
+		ov51x_dealloc(ov);
 		kfree(ov);
 		ov = NULL;
 	}
@@ -4449,7 +4447,7 @@
 	case VIDIOCSPICT:
 	{
 		struct video_picture *p = arg;
-		int i;
+		int i, rc;
 
 		PDEBUG(4, "VIDIOCSPICT");
 
@@ -4469,10 +4467,9 @@
 		if (p-&gt;palette != ov-&gt;frame[0].format) {
 			PDEBUG(4, "Detected format change");
 
-			/* If we're collecting previous frame wait
-			   before changing modes */
-			interruptible_sleep_on(&amp;ov-&gt;wq);
-			if (signal_pending(current)) return -EINTR;
+			rc = ov51x_wait_frames_inactive(ov);
+			if (rc)
+				return rc;
 
 			mode_init_regs(ov, ov-&gt;frame[0].width,
 				ov-&gt;frame[0].height, p-&gt;palette, ov-&gt;sub_flag);
@@ -4530,7 +4527,7 @@
 	case VIDIOCSWIN:
 	{
 		struct video_window *vw = arg;
-		int i, result;
+		int i, rc;
 
 		PDEBUG(4, "VIDIOCSWIN: %dx%d", vw-&gt;width, vw-&gt;height);
 
@@ -4545,15 +4542,14 @@
 			return -EINVAL;
 #endif
 
-		/* If we're collecting previous frame wait
-		   before changing modes */
-		interruptible_sleep_on(&amp;ov-&gt;wq);
-		if (signal_pending(current)) return -EINTR;
+		rc = ov51x_wait_frames_inactive(ov);
+		if (rc)
+			return rc;
 
-		result = mode_init_regs(ov, vw-&gt;width, vw-&gt;height,
+		rc = mode_init_regs(ov, vw-&gt;width, vw-&gt;height,
 			ov-&gt;frame[0].format, ov-&gt;sub_flag);
-		if (result &lt; 0)
-			return result;
+		if (rc &lt; 0)
+			return rc;
 
 		for (i = 0; i &lt; OV511_NUMFRAMES; i++) {
 			ov-&gt;frame[i].width = vw-&gt;width;
@@ -4600,7 +4596,7 @@
 	case VIDIOCMCAPTURE:
 	{
 		struct video_mmap *vm = arg;
-		int ret, depth;
+		int rc, depth;
 		unsigned int f = vm-&gt;frame;
 
 		PDEBUG(4, "VIDIOCMCAPTURE: frame: %d, %dx%d, %s", f, vm-&gt;width,
@@ -4642,14 +4638,14 @@
 		    (ov-&gt;frame[f].depth != depth)) {
 			PDEBUG(4, "VIDIOCMCAPTURE: change in image parameters");
 
-			/* If we're collecting previous frame wait
-			   before changing modes */
-			interruptible_sleep_on(&amp;ov-&gt;wq);
-			if (signal_pending(current)) return -EINTR;
-			ret = mode_init_regs(ov, vm-&gt;width, vm-&gt;height,
+			rc = ov51x_wait_frames_inactive(ov);
+			if (rc)
+				return rc;
+
+			rc = mode_init_regs(ov, vm-&gt;width, vm-&gt;height,
 				vm-&gt;format, ov-&gt;sub_flag);
 #if 0
-			if (ret &lt; 0) {
+			if (rc &lt; 0) {
 				PDEBUG(1, "Got error while initializing regs ");
 				return ret;
 			}
@@ -4702,18 +4698,15 @@
 				return rc;
 
 			if (frame-&gt;grabstate == FRAME_ERROR) {
-				int ret;
-
-				if ((ret = ov51x_new_frame(ov, fnum)) &lt; 0)
-					return ret;
+				if ((rc = ov51x_new_frame(ov, fnum)) &lt; 0)
+					return rc;
 				goto redo;
 			}
 			/* Fall through */
 		case FRAME_DONE:
 			if (ov-&gt;snap_enabled &amp;&amp; !frame-&gt;snapshot) {
-				int ret;
-				if ((ret = ov51x_new_frame(ov, fnum)) &lt; 0)
-					return ret;
+				if ((rc = ov51x_new_frame(ov, fnum)) &lt; 0)
+					return rc;
 				goto redo;
 			}
 
@@ -6089,7 +6082,6 @@
 	return -EBUSY;
 }
 
-
 /****************************************************************************
  *
  *  USB routines
@@ -6097,11 +6089,10 @@
  ***************************************************************************/
 
 static int
-ov51x_probe(struct usb_interface *intf, 
-	    const struct usb_device_id *id)
+ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
 	struct usb_device *dev = interface_to_usbdev(intf);
-	struct usb_interface_descriptor *interface;
+	struct usb_interface_descriptor *idesc;
 	struct usb_ov511 *ov;
 	int i;
 	int registered = 0;
@@ -6112,12 +6103,11 @@
 	if (dev-&gt;descriptor.bNumConfigurations != 1)
 		return -ENODEV;
 
-	interface = &amp;intf-&gt;altsetting[0].desc;
+	idesc = &amp;intf-&gt;altsetting[0].desc;
 
-	/* Checking vendor/product should be enough, but what the hell */
-	if (interface-&gt;bInterfaceClass != 0xFF)
+	if (idesc-&gt;bInterfaceClass != 0xFF)
 		return -ENODEV;
-	if (interface-&gt;bInterfaceSubClass != 0x00)
+	if (idesc-&gt;bInterfaceSubClass != 0x00)
 		return -ENODEV;
 
 	if ((ov = kmalloc(sizeof(*ov), GFP_KERNEL)) == NULL) {
@@ -6128,7 +6118,7 @@
 	memset(ov, 0, sizeof(*ov));
 
 	ov-&gt;dev = dev;
-	ov-&gt;iface = interface-&gt;bInterfaceNumber;
+	ov-&gt;iface = idesc-&gt;bInterfaceNumber;
 	ov-&gt;led_policy = led;
 	ov-&gt;compress = compress;
 	ov-&gt;lightfreq = lightfreq;
@@ -6272,7 +6262,7 @@
 
 error_out:
 	err("Camera initialization failed");
-	return -ENOMEM;
+	return -EIO;
 }
 
 static void
@@ -6284,6 +6274,7 @@
 	PDEBUG(3, "");
 
 	usb_set_intfdata (intf, NULL);
+
 	if (!ov)
 		return;
 
@@ -6298,10 +6289,9 @@
 
 	/* This will cause the process to request another frame */
 	for (n = 0; n &lt; OV511_NUMFRAMES; n++)
-		if (waitqueue_active(&amp;ov-&gt;frame[n].wq))
-			wake_up_interruptible(&amp;ov-&gt;frame[n].wq);
-	if (waitqueue_active(&amp;ov-&gt;wq))
-		wake_up_interruptible(&amp;ov-&gt;wq);
+		wake_up_interruptible(&amp;ov-&gt;frame[n].wq);
+
+	wake_up_interruptible(&amp;ov-&gt;wq);
 
 	ov-&gt;streaming = 0;
 	ov51x_unlink_isoc(ov);
@@ -6317,7 +6307,7 @@
 		ov-&gt;cbuf = NULL;
 		up(&amp;ov-&gt;cbuf_lock);
 
-		ov51x_dealloc(ov, 1);
+		ov51x_dealloc(ov);
 		kfree(ov);
 		ov = NULL;
 	}
@@ -6332,7 +6322,6 @@
 	.probe =	ov51x_probe,
 	.disconnect =	ov51x_disconnect
 };
-
 
 /****************************************************************************
  *
</pre></body></html>