PATCH: gspcav2-0.0.28 initial locking cleanups (check return value of mutex_lock_interruptible

Hans de Goede <[email protected]>
Newsgroups gmane.linux.drivers.spca50x.devel
Message-ID <[email protected]> 
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

-------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/spca50x-devs
gspcav2-0.0.28-lock-interruptible-check-return.patch (text/plain, 7.7 KB) 
--- 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 */
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.