[Spca50x-devs] PATCH: gspcav2-0.0.28 initial locking cleanups (check return value of mutex_lock_interruptible

[Hans de Goede <j.w.r.degoede@xxxxxx>]

Hi all,

Here is an initial locking cleanup patch, this patch adds return value checks 
for all calls to mutex_lock_interruptible, and properly returns -ERESTARTSYS 
instead of -EINTR when mutex_lock_interruptible fails.

Regards,

Hans

--- gspcav2-0.0.28/gspca.c	2008-04-20 13:03:08.000000000 +0200
+++ gspcav2-0.0.28.new/gspca.c	2008-04-20 22:44:15.000000000 +0200
@@ -500,9 +500,8 @@
 	struct usb_host_endpoint *ep;
 	int n, ret;
 
-	ret = mutex_lock_interruptible(&gspca_dev->usb_lock);
-	if (ret < 0)
-		return ret;
+	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+		return -ERESTARTSYS;
 
 	/* set the max alternate setting and loop until urb submit succeeds */
 	intf = usb_ifnum_to_if(gspca_dev->dev, gspca_dev->iface);
@@ -554,10 +553,9 @@
 	return ret;
 }
 
+/* Note both the queue and the usb lock should be hold when calling this */
 static void gspca_stream_off(struct gspca_dev *gspca_dev)
 {
-	mutex_lock_interruptible(&gspca_dev->usb_lock);
-	gspca_dev->streaming = 0;
 	if (gspca_dev->present) {
 		gspca_dev->sd_desc->stopN(gspca_dev);
 		gspca_kill_transfer(gspca_dev);
@@ -570,7 +568,6 @@
 		wake_up_interruptible(&gspca_dev->wq);
 		PDEBUG(D_ERR|D_STREAM, "stream off no device ??");
 	}
-	mutex_unlock(&gspca_dev->usb_lock);
 }
 
 static int gspca_set_default_mode(struct gspca_dev *gspca_dev)
@@ -790,9 +787,7 @@
 {
 	int ret;
 
-/*	mutex_lock_interruptible(&gspca_dev->queue_lock); */
 	ret = try_fmt_cap(file, priv, fmt);
-/*	mutex_unlock(&gspca_dev->queue_lock); */
 	if (ret < 0)
 		return ret;
 	return 0;
@@ -811,7 +806,8 @@
 			fmt->fmt.pix.width, fmt->fmt.pix.height);
 	}
 #endif
-	mutex_lock_interruptible(&gspca_dev->queue_lock);
+	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+		return -ERESTARTSYS;
 	ret = try_fmt_cap(file, priv, fmt);
 	if (ret < 0)
 		goto out;
@@ -819,8 +815,14 @@
 	if (ret == gspca_dev->curr_mode)
 		goto out;			/* same mode */
 	was_streaming = gspca_dev->streaming;
-	if (was_streaming != 0)
+	if (was_streaming != 0) {
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock)) {
+			ret = -ERESTARTSYS;
+			goto out;
+		}
 		gspca_stream_off(gspca_dev);
+		mutex_unlock(&gspca_dev->usb_lock);
+	}
 	gspca_dev->width = (int) fmt->fmt.pix.width;
 	gspca_dev->height = (int) fmt->fmt.pix.height;
 	gspca_dev->pixfmt = fmt->fmt.pix.pixelformat;
@@ -839,9 +841,8 @@
 
 	PDEBUG(D_STREAM, "opening");
 	gspca_dev = (struct gspca_dev *) video_devdata(file);
-	ret = mutex_lock_interruptible(&gspca_dev->queue_lock);
-	if (ret < 0)
-		return ret;
+	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+		return -ERESTARTSYS;
 	if (!gspca_dev->present) {
 		ret = -ENODEV;
 		goto out;
@@ -849,9 +850,10 @@
 
 	/* if not done yet, initialize the sensor */
 	if (gspca_dev->users == 0) {
-		ret = mutex_lock_interruptible(&gspca_dev->usb_lock);
-		if (ret < 0)
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock)) {
+			ret = -ERESTARTSYS;
 			goto out;
+		}
 		ret = gspca_dev->sd_desc->open(gspca_dev);
 		mutex_unlock(&gspca_dev->usb_lock);
 		if (ret != 0) {
@@ -885,22 +887,17 @@
 	struct gspca_dev *gspca_dev = file->private_data;
 
 	PDEBUG(D_STREAM, "closing");
-	if (gspca_dev->streaming) {
-		mutex_lock_interruptible(&gspca_dev->queue_lock);
+	
+	/* no need for locking, we only allow one open and no
+	   other fileops can be running when close gets called */
+	if (gspca_dev->streaming)
 		gspca_stream_off(gspca_dev);
-		mutex_unlock(&gspca_dev->queue_lock);
-	}
-	mutex_lock_interruptible(&gspca_dev->usb_lock);
+
 	gspca_dev->sd_desc->close(gspca_dev);
-	mutex_unlock(&gspca_dev->usb_lock);
-	atomic_inc(&gspca_dev->nevent);
-	wake_up_interruptible(&gspca_dev->wq);	/* wake blocked processes */
-	schedule();
-	mutex_lock_interruptible(&gspca_dev->queue_lock);
+
 	frame_free(gspca_dev);
 	file->private_data = NULL;
 	gspca_dev->users--;
-	mutex_unlock(&gspca_dev->queue_lock);
 	PDEBUG(D_STREAM, "closed");
 	return 0;
 }
@@ -963,7 +960,8 @@
 		    && ctrl->value > ctrls->qctrl.maximum)
 			return -ERANGE;
 		PDEBUG(D_CONF, "set ctrl [%08x] = %d", ctrl->id, ctrl->value);
-		mutex_lock_interruptible(&gspca_dev->usb_lock);
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+			return -ERESTARTSYS;
 		ret = ctrls->set(gspca_dev, ctrl->value);
 		mutex_unlock(&gspca_dev->usb_lock);
 		return ret;
@@ -984,7 +982,8 @@
 	     i++, ctrls++) {
 		if (ctrl->id != ctrls->qctrl.id)
 			continue;
-		mutex_lock_interruptible(&gspca_dev->usb_lock);
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+			return -ERESTARTSYS;
 		ret = ctrls->get(gspca_dev, &ctrl->value);
 		mutex_unlock(&gspca_dev->usb_lock);
 		return ret;
@@ -1046,9 +1045,8 @@
 	frsz = gspca_get_buff_size(gspca_dev);
 	if (frsz < 0)
 		return frsz;
-	ret = mutex_lock_interruptible(&gspca_dev->queue_lock);
-	if (ret < 0)
-		return ret;
+	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+		return -ERESTARTSYS;
 	ret = frame_alloc(gspca_dev,
 				rb->count,
 				(unsigned int) frsz,
@@ -1086,9 +1084,8 @@
 	PDEBUG(D_STREAM, "stream on");
 	if (buf_type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
-	ret = mutex_lock_interruptible(&gspca_dev->queue_lock);
-	if (ret < 0)
-		return ret;
+	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+		return -ERESTARTSYS;
 	if (!gspca_dev->present) {
 		ret = -ENODEV;
 		goto out;
@@ -1124,8 +1121,14 @@
 	if (buf_type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
 	if (gspca_dev->streaming) {
-		mutex_lock_interruptible(&gspca_dev->queue_lock);
+		if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+			return -ERESTARTSYS;
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock)) {
+			mutex_unlock(&gspca_dev->queue_lock);
+			return -ERESTARTSYS;
+		}
 		gspca_stream_off(gspca_dev);
+		mutex_unlock(&gspca_dev->usb_lock);
 		mutex_unlock(&gspca_dev->queue_lock);
 	}
 	return 0;
@@ -1139,7 +1142,8 @@
 
 	if (!gspca_dev->sd_desc->get_jcomp)
 		return -EINVAL;
-	mutex_lock_interruptible(&gspca_dev->usb_lock);
+	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+		return -ERESTARTSYS;
 	ret = gspca_dev->sd_desc->get_jcomp(gspca_dev, jpegcomp);
 	mutex_unlock(&gspca_dev->usb_lock);
 	return ret;
@@ -1151,7 +1155,8 @@
 	struct gspca_dev *gspca_dev = priv;
 	int ret;
 
-	mutex_lock_interruptible(&gspca_dev->usb_lock);
+	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+		return -ERESTARTSYS;
 	if (!gspca_dev->sd_desc->set_jcomp)
 		return -EINVAL;
 	ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
@@ -1176,13 +1181,12 @@
 	struct gspca_dev *gspca_dev = priv;
 	int n;
 
-	mutex_lock_interruptible(&gspca_dev->usb_lock);
 	n = parm->parm.capture.readbuffers;
 	if (n == 0 || n > GSPCA_MAX_FRAMES)
 		parm->parm.capture.readbuffers = gspca_dev->nbufread;
 	else
 		gspca_dev->nbufread = n;
-	mutex_unlock(&gspca_dev->usb_lock);
+
 	return 0;
 }
 
@@ -1229,9 +1233,8 @@
 	size = vma->vm_end - vma->vm_start;
 	PDEBUG(D_STREAM, "mmap start:%08x size:%d", (int) start, (int) size);
 
-	ret = mutex_lock_interruptible(&gspca_dev->queue_lock);
-	if (ret < 0)
-		return ret;
+	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+		return -ERESTARTSYS;
 /* sanity check disconnect, in use, no memory available */
 	if (!gspca_dev->present) {
 		ret = -ENODEV;
@@ -1441,9 +1444,9 @@
 		return -EINVAL;
 	}
 
-	ret = mutex_lock_interruptible(&gspca_dev->queue_lock);
-	if (ret < 0)
-		return ret;
+	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+		return -ERESTARTSYS;
+
 	if (frame->v4l2_buf.flags
 			& (V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE)) {
 		PDEBUG(D_STREAM, "qbuf bad state");
@@ -1714,11 +1717,11 @@
 	if (!gspca_dev)
 		return;
 	gspca_dev->present = 0;
-	mutex_lock_interruptible(&gspca_dev->queue_lock);
-	mutex_lock_interruptible(&gspca_dev->usb_lock);
+	mutex_lock(&gspca_dev->queue_lock);
+	mutex_lock(&gspca_dev->usb_lock);
 	gspca_kill_transfer(gspca_dev);
-	mutex_unlock(&gspca_dev->queue_lock);
 	mutex_unlock(&gspca_dev->usb_lock);
+	mutex_unlock(&gspca_dev->queue_lock);
 	while (gspca_dev->users != 0) {		/* wait until fully closed */
 		atomic_inc(&gspca_dev->nevent);
 		wake_up_interruptible(&gspca_dev->wq);	/* wake processes */

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

_______________________________________________
Spca50x-devs mailing list
Spca50x-devs@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/spca50x-devs