base: genlock: Remove genlock_release_lock and associated ioctl

base: genlock: allow synchronization with a single gralloc handle base: genlock: remove BIT macro usage
2012-08-07 00:45:11 +08:00
parent 55ece772cd
commit 65b62e5ee4
2 changed files with 251 additions and 65 deletions
--- a/drivers/base/genlock.c
+++ b/drivers/base/genlock.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2011, Code Aurora Forum. All rights reserved.
+/* Copyright (c) 2011-2012, Code Aurora Forum. All rights reserved.
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 and
@@ -22,7 +22,6 @@
 #include <linux/anon_inodes.h>
 #include <linux/miscdevice.h>
 #include <linux/genlock.h>
-#include <linux/interrupt.h> /* for in_interrupt() */

 /* Lock states - can either be unlocked, held as an exclusive write lock or a
 * shared read lock
@@ -32,6 +31,9 @@
 #define _RDLOCK  GENLOCK_RDLOCK
 #define _WRLOCK GENLOCK_WRLOCK

+#define GENLOCK_LOG_ERR(fmt, args...) \
+pr_err("genlock: %s: " fmt, __func__, ##args)
+
 struct genlock {
 	struct list_head active;  /* List of handles holding lock */
 	spinlock_t lock;          /* Spinlock to protect the lock internals */
@@ -49,12 +51,29 @@ struct genlock_handle {
 				     taken */
 };

+/*
+ * Create a spinlock to protect against a race condition when a lock gets
+ * released while another process tries to attach it
+ */
+
+static DEFINE_SPINLOCK(genlock_file_lock);
+
 static void genlock_destroy(struct kref *kref)
 {
-       struct genlock *lock = container_of(kref, struct genlock,
-                       refcount);
+	struct genlock *lock = container_of(kref, struct genlock,
+			refcount);

-       kfree(lock);
+	/*
+	 * Clear the private data for the file descriptor in case the fd is
+	 * still active after the lock gets released
+	 */
+
+	spin_lock(&genlock_file_lock);
+	if (lock->file)
+		lock->file->private_data = NULL;
+	spin_unlock(&genlock_file_lock);
+
+	kfree(lock);
 }

 /*
@@ -64,6 +83,15 @@ static void genlock_destroy(struct kref *kref)

 static int genlock_release(struct inode *inodep, struct file *file)
 {
+	struct genlock *lock = file->private_data;
+	/*
+	 * Clear the refrence back to this file structure to avoid
+	 * somehow reusing the lock after the file has been destroyed
+	 */
+
+	if (lock)
+		lock->file = NULL;
+
 	return 0;
 }

@@ -82,12 +110,21 @@ struct genlock *genlock_create_lock(struct genlock_handle *handle)
 {
 	struct genlock *lock;

-	if (handle->lock != NULL)
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
 		return ERR_PTR(-EINVAL);
+	}
+
+	if (handle->lock != NULL) {
+		GENLOCK_LOG_ERR("Handle already has a lock attached\n");
+		return ERR_PTR(-EINVAL);
+	}

 	lock = kzalloc(sizeof(*lock), GFP_KERNEL);
-	if (lock == NULL)
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("Unable to allocate memory for a lock\n");
 		return ERR_PTR(-ENOMEM);
+	}

 	INIT_LIST_HEAD(&lock->active);
 	init_waitqueue_head(&lock->queue);
@@ -120,8 +157,10 @@ static int genlock_get_fd(struct genlock *lock)
 {
 	int ret;

-	if (!lock->file)
+	if (!lock->file) {
+		GENLOCK_LOG_ERR("No file attached to the lock\n");
 		return -EINVAL;
+	}

 	ret = get_unused_fd_flags(0);
 	if (ret < 0)
@@ -143,19 +182,37 @@ struct genlock *genlock_attach_lock(struct genlock_handle *handle, int fd)
 	struct file *file;
 	struct genlock *lock;

-	if (handle->lock != NULL)
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
 		return ERR_PTR(-EINVAL);
+	}
+
+	if (handle->lock != NULL) {
+		GENLOCK_LOG_ERR("Handle already has a lock attached\n");
+		return ERR_PTR(-EINVAL);
+	}

 	file = fget(fd);
-	if (file == NULL)
+	if (file == NULL) {
+		GENLOCK_LOG_ERR("Bad file descriptor\n");
 		return ERR_PTR(-EBADF);
+	}

+	/*
+	 * take a spinlock to avoid a race condition if the lock is
+	 * released and then attached
+	 */
+
+	spin_lock(&genlock_file_lock);
 	lock = file->private_data;
+	spin_unlock(&genlock_file_lock);

 	fput(file);

-	if (lock == NULL)
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("File descriptor is invalid\n");
 		return ERR_PTR(-EINVAL);
+	}

 	handle->lock = lock;
 	kref_get(&lock->refcount);
@@ -199,13 +256,16 @@ static int _genlock_unlock(struct genlock *lock, struct genlock_handle *handle)

 	spin_lock_irqsave(&lock->lock, irqflags);

-	if (lock->state == _UNLOCKED)
+	if (lock->state == _UNLOCKED) {
+		GENLOCK_LOG_ERR("Trying to unlock an unlocked handle\n");
 		goto done;
+	}

 	/* Make sure this handle is an owner of the lock */
-	if (!handle_has_lock(lock, handle))
+	if (!handle_has_lock(lock, handle)) {
+		GENLOCK_LOG_ERR("handle does not have lock attached to it\n");
 		goto done;
-
+	}
 	/* If the handle holds no more references to the lock then
 	   release it (maybe) */

@@ -228,7 +288,7 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 {
 	unsigned long irqflags;
 	int ret = 0;
-	unsigned int ticks = msecs_to_jiffies(timeout);
+	unsigned long ticks = msecs_to_jiffies(timeout);

 	spin_lock_irqsave(&lock->lock, irqflags);

@@ -236,8 +296,8 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 	 * succeed to not block, the mere idea is too dangerous to continue
 	 */

-	if (in_interrupt() && !(flags & GENLOCK_NOBLOCK))
-		BUG();
+//	if (in_interrupt() && !(flags & GENLOCK_NOBLOCK))
+//		BUG();

 	/* Fast path - the lock is unlocked, so go do the needful */

@@ -247,12 +307,15 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 	if (handle_has_lock(lock, handle)) {

 		/*
-		 * If the handle already holds the lock and the type matches,
-		 * then just increment the active pointer. This allows the
-		 * handle to do recursive locks
+		 * If the handle already holds the lock and the lock type is
+		 * a read lock then just increment the active pointer. This
+		 * allows the handle to do recursive read locks. Recursive
+		 * write locks are not allowed in order to support
+		 * synchronization within a process using a single gralloc
+		 * handle.
 		 */

-		if (lock->state == op) {
+		if (lock->state == _RDLOCK && op == _RDLOCK) {
 			handle->active++;
 			goto done;
 		}
@@ -261,32 +324,46 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 		 * If the handle holds a write lock then the owner can switch
 		 * to a read lock if they want. Do the transition atomically
 		 * then wake up any pending waiters in case they want a read
-		 * lock too.
+		 * lock too. In order to support synchronization within a
+		 * process the caller must explicity request to convert the
+		 * lock type with the GENLOCK_WRITE_TO_READ flag.
 		 */

-		if (op == _RDLOCK && handle->active == 1) {
-			lock->state = _RDLOCK;
-			wake_up(&lock->queue);
+		if (flags & GENLOCK_WRITE_TO_READ) {
+			if (lock->state == _WRLOCK && op == _RDLOCK) {
+				lock->state = _RDLOCK;
+				wake_up(&lock->queue);
+				goto done;
+			} else {
+				GENLOCK_LOG_ERR("Invalid state to convert"
+					"write to read\n");
+				ret = -EINVAL;
+				goto done;
+			}
+		}
+	} else {
+
+		/*
+		 * Check to ensure the caller has not attempted to convert a
+		 * write to a read without holding the lock.
+		 */
+
+		if (flags & GENLOCK_WRITE_TO_READ) {
+			GENLOCK_LOG_ERR("Handle must have lock to convert"
+				"write to read\n");
+			ret = -EINVAL;
 			goto done;
 		}

 		/*
-		 * Otherwise the user tried to turn a read into a write, and we
-		 * don't allow that.
+		 * If we request a read and the lock is held by a read, then go
+		 * ahead and share the lock
 		 */

-		ret = -EINVAL;
-		goto done;
+		if (op == GENLOCK_RDLOCK && lock->state == _RDLOCK)
+			goto dolock;
 	}

-	/*
-	 * If we request a read and the lock is held by a read, then go
-	 * ahead and share the lock
-	 */
-
-	if (op == GENLOCK_RDLOCK && lock->state == _RDLOCK)
-		goto dolock;
-
 	/* Treat timeout 0 just like a NOBLOCK flag and return if the
 	   lock cannot be aquired without blocking */

@@ -295,15 +372,26 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 		goto done;
 	}

-	/* Wait while the lock remains in an incompatible state */
+	/*
+	 * Wait while the lock remains in an incompatible state
+	 * state    op    wait
+	 * -------------------
+	 * unlocked n/a   no
+	 * read     read  no
+	 * read     write yes
+	 * write    n/a   yes
+	 */

-	while (lock->state != _UNLOCKED) {
-		unsigned int elapsed;
+	while ((lock->state == _RDLOCK && op == _WRLOCK) ||
+			lock->state == _WRLOCK) {
+		signed long elapsed;

 		spin_unlock_irqrestore(&lock->lock, irqflags);

 		elapsed = wait_event_interruptible_timeout(lock->queue,
-			lock->state == _UNLOCKED, ticks);
+			lock->state == _UNLOCKED ||
+			(lock->state == _RDLOCK && op == _RDLOCK),
+			ticks);

 		spin_lock_irqsave(&lock->lock, irqflags);

@@ -312,7 +400,7 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 			goto done;
 		}

-		ticks = elapsed;
+		ticks = (unsigned long) elapsed;
 	}

 dolock:
@@ -320,7 +408,7 @@ dolock:

 	list_add_tail(&handle->entry, &lock->active);
 	lock->state = op;
-	handle->active = 1;
+	handle->active++;

 done:
 	spin_unlock_irqrestore(&lock->lock, irqflags);
@@ -329,7 +417,7 @@ done:
 }

 /**
- * genlock_lock - Acquire or release a lock
+ * genlock_lock - Acquire or release a lock (depreciated)
 * @handle - pointer to the genlock handle that is requesting the lock
 * @op - the operation to perform (RDLOCK, WRLOCK, UNLOCK)
 * @flags - flags to control the operation
@@ -341,11 +429,76 @@ done:
 int genlock_lock(struct genlock_handle *handle, int op, int flags,
 	uint32_t timeout)
 {
-	struct genlock *lock = handle->lock;
+	struct genlock *lock;
+	unsigned long irqflags;
+
 	int ret = 0;

-	if (lock == NULL)
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
 		return -EINVAL;
+	}
+
+	lock = handle->lock;
+
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("Handle does not have a lock attached\n");
+		return -EINVAL;
+	}
+
+	switch (op) {
+	case GENLOCK_UNLOCK:
+		ret = _genlock_unlock(lock, handle);
+		break;
+	case GENLOCK_RDLOCK:
+		spin_lock_irqsave(&lock->lock, irqflags);
+		if (handle_has_lock(lock, handle)) {
+			/* request the WRITE_TO_READ flag for compatibility */
+			flags |= GENLOCK_WRITE_TO_READ;
+		}
+		spin_unlock_irqrestore(&lock->lock, irqflags);
+		/* fall through to take lock */
+	case GENLOCK_WRLOCK:
+		ret = _genlock_lock(lock, handle, op, flags, timeout);
+		break;
+	default:
+		GENLOCK_LOG_ERR("Invalid lock operation\n");
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(genlock_lock);
+
+/**
+ * genlock_dreadlock - Acquire or release a lock
+ * @handle - pointer to the genlock handle that is requesting the lock
+ * @op - the operation to perform (RDLOCK, WRLOCK, UNLOCK)
+ * @flags - flags to control the operation
+ * @timeout - optional timeout to wait for the lock to come free
+ *
+ * Returns: 0 on success or error code on failure
+ */
+
+int genlock_dreadlock(struct genlock_handle *handle, int op, int flags,
+	uint32_t timeout)
+{
+	struct genlock *lock;
+
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
+		return -EINVAL;
+	}
+
+	lock = handle->lock;
+
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("Handle does not have a lock attached\n");
+		return -EINVAL;
+	}

 	switch (op) {
 	case GENLOCK_UNLOCK:
@@ -356,13 +509,14 @@ int genlock_lock(struct genlock_handle *handle, int op, int flags,
 		ret = _genlock_lock(lock, handle, op, flags, timeout);
 		break;
 	default:
+		GENLOCK_LOG_ERR("Invalid lock operation\n");
 		ret = -EINVAL;
 		break;
 	}

 	return ret;
 }
-EXPORT_SYMBOL(genlock_lock);
+EXPORT_SYMBOL(genlock_dreadlock);

 /**
 * genlock_wait - Wait for the lock to be released
@@ -372,13 +526,22 @@ EXPORT_SYMBOL(genlock_lock);

 int genlock_wait(struct genlock_handle *handle, uint32_t timeout)
 {
-	struct genlock *lock = handle->lock;
+	struct genlock *lock;
 	unsigned long irqflags;
 	int ret = 0;
-	unsigned int ticks = msecs_to_jiffies(timeout);
+	unsigned long ticks = msecs_to_jiffies(timeout);

-	if (lock == NULL)
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
 		return -EINVAL;
+	}
+
+	lock = handle->lock;
+
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("Handle does not have a lock attached\n");
+		return -EINVAL;
+	}

 	spin_lock_irqsave(&lock->lock, irqflags);

@@ -393,7 +556,7 @@ int genlock_wait(struct genlock_handle *handle, uint32_t timeout)
 	}

 	while (lock->state != _UNLOCKED) {
-		unsigned int elapsed;
+		signed long elapsed;

 		spin_unlock_irqrestore(&lock->lock, irqflags);

@@ -407,7 +570,7 @@ int genlock_wait(struct genlock_handle *handle, uint32_t timeout)
 			break;
 		}

-		ticks = elapsed;
+		ticks = (unsigned long) elapsed;
 	}

 done:
@@ -415,12 +578,7 @@ done:
 	return ret;
 }

-/**
- * genlock_release_lock - Release a lock attached to a handle
- * @handle - Pointer to the handle holding the lock
- */
-
-void genlock_release_lock(struct genlock_handle *handle)
+static void genlock_release_lock(struct genlock_handle *handle)
 {
 	unsigned long flags;

@@ -441,7 +599,6 @@ void genlock_release_lock(struct genlock_handle *handle)
 	handle->lock = NULL;
 	handle->active = 0;
 }
-EXPORT_SYMBOL(genlock_release_lock);

 /*
 * Release function called when all references to a handle are released
@@ -468,8 +625,10 @@ static const struct file_operations genlock_handle_fops = {
 static struct genlock_handle *_genlock_get_handle(void)
 {
 	struct genlock_handle *handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-	if (handle == NULL)
+	if (handle == NULL) {
+		GENLOCK_LOG_ERR("Unable to allocate memory for the handle\n");
 		return ERR_PTR(-ENOMEM);
+	}

 	return handle;
 }
@@ -531,6 +690,9 @@ static long genlock_dev_ioctl(struct file *filep, unsigned int cmd,
 	struct genlock *lock;
 	int ret;

+	if (IS_ERR_OR_NULL(handle))
+		return -EINVAL;
+
 	switch (cmd) {
 	case GENLOCK_IOC_NEW: {
 		lock = genlock_create_lock(handle);
@@ -540,8 +702,11 @@ static long genlock_dev_ioctl(struct file *filep, unsigned int cmd,
 		return 0;
 	}
 	case GENLOCK_IOC_EXPORT: {
-		if (handle->lock == NULL)
+		if (handle->lock == NULL) {
+			GENLOCK_LOG_ERR("Handle does not have a lock"
+					"attached\n");
 			return -EINVAL;
+		}

 		ret = genlock_get_fd(handle->lock);
 		if (ret < 0)
@@ -574,6 +739,14 @@ static long genlock_dev_ioctl(struct file *filep, unsigned int cmd,
 		return genlock_lock(handle, param.op, param.flags,
 			param.timeout);
 	}
+	case GENLOCK_IOC_DREADLOCK: {
+		if (copy_from_user(&param, (void __user *) arg,
+		sizeof(param)))
+			return -EFAULT;
+
+		return genlock_dreadlock(handle, param.op, param.flags,
+			param.timeout);
+	}
 	case GENLOCK_IOC_WAIT: {
 		if (copy_from_user(&param, (void __user *) arg,
 		sizeof(param)))
@@ -582,10 +755,16 @@ static long genlock_dev_ioctl(struct file *filep, unsigned int cmd,
 		return genlock_wait(handle, param.timeout);
 	}
 	case GENLOCK_IOC_RELEASE: {
-		genlock_release_lock(handle);
-		return 0;
+		/*
+		 * Return error - this ioctl has been deprecated.
+		 * Locks should only be released when the handle is
+		 * destroyed
+		 */
+		GENLOCK_LOG_ERR("Deprecated RELEASE ioctl called\n");
+		return -EINVAL;
 	}
 	default:
+		GENLOCK_LOG_ERR("Invalid ioctl\n");
 		return -EINVAL;
 	}
 }
--- a/include/linux/genlock.h
+++ b/include/linux/genlock.h
@@ -12,7 +12,7 @@ void genlock_put_handle(struct genlock_handle *handle);
 struct genlock *genlock_create_lock(struct genlock_handle *);
 struct genlock *genlock_attach_lock(struct genlock_handle *, int fd);
 int genlock_wait(struct genlock_handle *handle, u32 timeout);
-void genlock_release_lock(struct genlock_handle *);
+/* genlock_release_lock was deprecated */
 int genlock_lock(struct genlock_handle *handle, int op, int flags,
 	u32 timeout);
 #endif
@@ -21,7 +21,8 @@ int genlock_lock(struct genlock_handle *handle, int op, int flags,
 #define GENLOCK_WRLOCK 1
 #define GENLOCK_RDLOCK 2

-#define GENLOCK_NOBLOCK (1 << 0)
+#define GENLOCK_NOBLOCK       (1 << 0)
+#define GENLOCK_WRITE_TO_READ (1 << 1)

 struct genlock_lock {
 	int fd;
@@ -37,9 +38,15 @@ struct genlock_lock {
 	struct genlock_lock)
 #define GENLOCK_IOC_ATTACH _IOW(GENLOCK_IOC_MAGIC, 2, \
 	struct genlock_lock)
+
+/* Deprecated */
 #define GENLOCK_IOC_LOCK _IOW(GENLOCK_IOC_MAGIC, 3, \
 	struct genlock_lock)
+
+/* Deprecated */
 #define GENLOCK_IOC_RELEASE _IO(GENLOCK_IOC_MAGIC, 4)
 #define GENLOCK_IOC_WAIT _IOW(GENLOCK_IOC_MAGIC, 5, \
 	struct genlock_lock)
+#define GENLOCK_IOC_DREADLOCK _IOW(GENLOCK_IOC_MAGIC, 6, \
+	struct genlock_lock)
 #endif