[RFC PATCH 2.6.17-mm1 5/3] ieee1394: raw1394: remove redundantcounting semaphore

[Stefan Richter]

An already existing wait queue replaces raw1394's complete_sem which was
maintained in parallel to the wait queue.  The role of the semaphore's
counter is taken over by a direct check of what was really counted:  The
presence of items in the list of completed requests.

Notes:

 - raw1394_release() sleeps uninterruptibly until all requests were
   completed.  This is the same behaviour as before the patch.

 - The macros wait_event and wait_event_interruptible are called with a
   condition argument which has a side effect, i.e. manipulation of the
   requests list.  This side effect happens only if the condition is
   true.  The patch relies on the fact that wait_event[_interruptible]
   does not evaluate the condition again after it became true.

 - The diffstat looks unfavorable with respect to added lines of code.
   However 6 of them are comments, and some are due to separation of
   existing code blocks into two small helper functions.

Signed-off-by: Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx>
---
 drivers/ieee1394/raw1394-private.h |    3 -
 drivers/ieee1394/raw1394.c         |   79 ++++++++++++++++-------------
 2 files changed, 46 insertions(+), 36 deletions(-)

This patch won't apply without the -mm patch because of the list_move
patch in -mm.  Variants of this and other 1394 patches for older kernels
can be found at http://me.in-berlin.de/~s5r6/linux1394/updates/.

I tested the patch on 2.6.16.x SMP PREEMPT x86 K7 uniprocessor with the
libraw1394 apps 1394commander and gscanbus, issuing asynchronous
transactions, bus resets, and PHY packets, which trigger raw1394_open,
raw1394_write, raw1394_read, raw1394_poll, raw1394_release.

Index: linux-2.6.17-mm2/drivers/ieee1394/raw1394-private.h
===================================================================
--- linux-2.6.17-mm2.orig/drivers/ieee1394/raw1394-private.h	2006-06-25 21:10:39.000000000 +0200
+++ linux-2.6.17-mm2/drivers/ieee1394/raw1394-private.h	2006-06-25 21:11:07.000000000 +0200
@@ -29,9 +29,8 @@ struct file_info {
 
         struct list_head req_pending;
         struct list_head req_complete;
-        struct semaphore complete_sem;
         spinlock_t reqlists_lock;
-        wait_queue_head_t poll_wait_complete;
+        wait_queue_head_t wait_complete;
 
         struct list_head addr_list;
 
Index: linux-2.6.17-mm2/drivers/ieee1394/raw1394.c
===================================================================
--- linux-2.6.17-mm2.orig/drivers/ieee1394/raw1394.c	2006-06-25 21:10:39.000000000 +0200
+++ linux-2.6.17-mm2/drivers/ieee1394/raw1394.c	2006-06-25 21:11:41.000000000 +0200
@@ -132,10 +132,9 @@ static void free_pending_request(struct 
 static void __queue_complete_req(struct pending_request *req)
 {
 	struct file_info *fi = req->file_info;
-	list_move_tail(&req->list, &fi->req_complete);
 
-	up(&fi->complete_sem);
-	wake_up_interruptible(&fi->poll_wait_complete);
+	list_move_tail(&req->list, &fi->req_complete);
+ 	wake_up(&fi->wait_complete);
 }
 
 static void queue_complete_req(struct pending_request *req)
@@ -463,13 +462,36 @@ raw1394_compat_read(const char __user *b
 
 #endif
 
+/* get next completed request  (caller must hold fi->reqlists_lock) */
+static inline struct pending_request *__next_complete_req(struct file_info *fi)
+{
+	struct list_head *lh;
+	struct pending_request *req = NULL;
+
+	if (!list_empty(&fi->req_complete)) {
+		lh = fi->req_complete.next;
+		list_del(lh);
+		req = list_entry(lh, struct pending_request, list);
+	}
+	return req;
+}
+
+/* atomically get next completed request */
+static struct pending_request *next_complete_req(struct file_info *fi)
+{
+	unsigned long flags;
+	struct pending_request *req;
+
+	spin_lock_irqsave(&fi->reqlists_lock, flags);
+	req = __next_complete_req(fi);
+	spin_unlock_irqrestore(&fi->reqlists_lock, flags);
+	return req;
+}
 
 static ssize_t raw1394_read(struct file *file, char __user * buffer,
 			    size_t count, loff_t * offset_is_ignored)
 {
-	unsigned long flags;
 	struct file_info *fi = (struct file_info *)file->private_data;
-	struct list_head *lh;
 	struct pending_request *req;
 	ssize_t ret;
 
@@ -487,22 +509,14 @@ static ssize_t raw1394_read(struct file 
 	}
 
 	if (file->f_flags & O_NONBLOCK) {
-		if (down_trylock(&fi->complete_sem)) {
+		if (!(req = next_complete_req(fi)))
 			return -EAGAIN;
-		}
 	} else {
-		if (down_interruptible(&fi->complete_sem)) {
+		if (wait_event_interruptible(fi->wait_complete,
+					     (req = next_complete_req(fi))))
 			return -ERESTARTSYS;
-		}
 	}
 
-	spin_lock_irqsave(&fi->reqlists_lock, flags);
-	lh = fi->req_complete.next;
-	list_del(lh);
-	spin_unlock_irqrestore(&fi->reqlists_lock, flags);
-
-	req = list_entry(lh, struct pending_request, list);
-
 	if (req->req.length) {
 		if (copy_to_user(int2ptr(req->req.recvb), req->data,
 				 req->req.length)) {
@@ -2744,7 +2758,7 @@ static unsigned int raw1394_poll(struct 
 	unsigned int mask = POLLOUT | POLLWRNORM;
 	unsigned long flags;
 
-	poll_wait(file, &fi->poll_wait_complete, pt);
+	poll_wait(file, &fi->wait_complete, pt);
 
 	spin_lock_irqsave(&fi->reqlists_lock, flags);
 	if (!list_empty(&fi->req_complete)) {
@@ -2769,9 +2783,8 @@ static int raw1394_open(struct inode *in
 	fi->state = opened;
 	INIT_LIST_HEAD(&fi->req_pending);
 	INIT_LIST_HEAD(&fi->req_complete);
-	sema_init(&fi->complete_sem, 0);
 	spin_lock_init(&fi->reqlists_lock);
-	init_waitqueue_head(&fi->poll_wait_complete);
+	init_waitqueue_head(&fi->wait_complete);
 	INIT_LIST_HEAD(&fi->addr_list);
 
 	file->private_data = fi;
@@ -2784,7 +2797,7 @@ static int raw1394_release(struct inode 
 	struct file_info *fi = file->private_data;
 	struct list_head *lh;
 	struct pending_request *req;
-	int done = 0, i, fail = 0;
+	int i, fail;
 	int retval = 0;
 	struct list_head *entry;
 	struct arm_addr *addr = NULL;
@@ -2864,25 +2877,23 @@ static int raw1394_release(struct inode 
 		       "error(s) occurred \n");
 	}
 
-	while (!done) {
+	for (;;) {
+		/* This locked section guarantees that neither
+		 * complete nor pending requests exist once i!=0 */
 		spin_lock_irqsave(&fi->reqlists_lock, flags);
-
-		while (!list_empty(&fi->req_complete)) {
-			lh = fi->req_complete.next;
-			list_del(lh);
-
-			req = list_entry(lh, struct pending_request, list);
-
+		while ((req = __next_complete_req(fi)))
 			free_pending_request(req);
-		}
-
-		if (list_empty(&fi->req_pending))
-			done = 1;
 
+		i = list_empty(&fi->req_pending);
 		spin_unlock_irqrestore(&fi->reqlists_lock, flags);
 
-		if (!done)
-			down_interruptible(&fi->complete_sem);
+		if (i)
+			break;
+
+		/* Sleep until more requests can be freed.
+		 * FIXME?  This sleeps uninterruptibly. */
+		wait_event(fi->wait_complete, (req = next_complete_req(fi)));
+		free_pending_request(req);
 	}
 
 	/* Remove any sub-trees left by user space programs */


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/