[PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4)

[Dave Airlie <airlied@gmail.com>]

From: Dave Airlie <airlied@redhat.com>

This patch allows the underlying fence in a sync_file to be changed
or set to NULL. This isn't currently required but for Vulkan
semaphores we need to be able to swap and reset the fence.

In order to faciliate this, it uses rcu to protect the fence,
along with a new mutex. The mutex also protects the callback.
It also checks for NULL when retrieving the rcu protected
fence in case it has been reset.

v1.1: fix the locking (Julia Lawall).
v2: use rcu try one
v3: fix poll to use proper rcu, fixup merge/fill ioctls
to not crash on NULL fence cases.
v4: use rcu in even more places, add missing fput

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c | 135 +++++++++++++++++++++++++++++++++++---------
 include/linux/sync_file.h   |   6 +-
 2 files changed, 112 insertions(+), 29 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 153bf03..6376f6f 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,6 +28,10 @@
 
 static const struct file_operations sync_file_fops;
 
+#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
+#define sync_file_assert_held(obj) \
+	lockdep_assert_held(&(obj)->lock)
+
 /**
  * sync_file_validate_type_flags - validate type/flags for support
  * @type: type of sync file object
@@ -81,6 +85,10 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
 
 	sync_file->type = type;
 	sync_file->flags = flags;
+
+	RCU_INIT_POINTER(sync_file->fence, NULL);
+
+	mutex_init(&sync_file->lock);
 	return sync_file;
 
 err:
@@ -117,7 +125,9 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
 	if (!sync_file)
 		return NULL;
 
-	sync_file->fence = dma_fence_get(fence);
+	dma_fence_get(fence);
+
+	RCU_INIT_POINTER(sync_file->fence, fence);
 
 	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
 		 fence->ops->get_driver_name(fence),
@@ -168,13 +178,28 @@ struct dma_fence *sync_file_get_fence(int fd)
 	if (!sync_file)
 		return NULL;
 
-	fence = dma_fence_get(sync_file->fence);
+	if (!rcu_access_pointer(sync_file->fence)) {
+		fput(sync_file->file);
+		return NULL;
+	}
+
+	rcu_read_lock();
+	fence = dma_fence_get_rcu_safe(&sync_file->fence);
+	rcu_read_unlock();
+
 	fput(sync_file->file);
 
 	return fence;
 }
 EXPORT_SYMBOL(sync_file_get_fence);
 
+static inline struct dma_fence *
+sync_file_get_fence_locked(struct sync_file *sync_file)
+{
+	return rcu_dereference_protected(sync_file->fence,
+					 sync_file_held(sync_file));
+}
+
 static int sync_file_set_fence(struct sync_file *sync_file,
 			       struct dma_fence **fences, int num_fences)
 {
@@ -187,7 +212,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 	 * we own the reference of the dma_fence_array creation.
 	 */
 	if (num_fences == 1) {
-		sync_file->fence = fences[0];
+		RCU_INIT_POINTER(sync_file->fence, fences[0]);
 		kfree(fences);
 	} else {
 		array = dma_fence_array_create(num_fences, fences,
@@ -196,24 +221,30 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 		if (!array)
 			return -ENOMEM;
 
-		sync_file->fence = &array->base;
+		RCU_INIT_POINTER(sync_file->fence, &array->base);
 	}
 
 	return 0;
 }
 
-static struct dma_fence **get_fences(struct sync_file *sync_file,
+/* must be called with rcu read lock taken */
+static struct dma_fence **get_fences(struct dma_fence **fence,
 				     int *num_fences)
 {
-	if (dma_fence_is_array(sync_file->fence)) {
-		struct dma_fence_array *array = to_dma_fence_array(sync_file->fence);
+	if (!*fence) {
+		*num_fences = 0;
+		return NULL;
+	}
+
+	if (dma_fence_is_array(*fence)) {
+		struct dma_fence_array *array = to_dma_fence_array(*fence);
 
 		*num_fences = array->num_fences;
 		return array->fences;
 	}
 
 	*num_fences = 1;
-	return &sync_file->fence;
+	return fence;
 }
 
 static void add_fence(struct dma_fence **fences,
@@ -243,18 +274,31 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	struct sync_file *sync_file;
 	struct dma_fence **fences, **nfences, **a_fences, **b_fences;
 	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
+	struct dma_fence *a_fence, *b_fence;
 
 	if (a->type != b->type)
 		return NULL;
 
-	sync_file = sync_file_alloc(a->type, a->flags);
-	if (!sync_file)
+	if (!rcu_access_pointer(a->fence) ||
+	    !rcu_access_pointer(b->fence))
 		return NULL;
 
-	a_fences = get_fences(a, &a_num_fences);
-	b_fences = get_fences(b, &b_num_fences);
+	rcu_read_lock();
+	a_fence = dma_fence_get_rcu_safe(&a->fence);
+	b_fence = dma_fence_get_rcu_safe(&b->fence);
+	rcu_read_unlock();
+
+	a_fences = get_fences(&a_fence, &a_num_fences);
+	b_fences = get_fences(&b_fence, &b_num_fences);
+	if (!a_num_fences || !b_num_fences)
+		goto put_src_fences;
+
 	if (a_num_fences > INT_MAX - b_num_fences)
-		return NULL;
+		goto put_src_fences;
+
+	sync_file = sync_file_alloc(a->type, a->flags);
+	if (!sync_file)
+		goto put_src_fences;
 
 	num_fences = a_num_fences + b_num_fences;
 
@@ -315,11 +359,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 		goto err;
 	}
 
+	dma_fence_put(a_fence);
+	dma_fence_put(b_fence);
 	strlcpy(sync_file->name, name, sizeof(sync_file->name));
 	return sync_file;
 
 err:
 	fput(sync_file->file);
+put_src_fences:
+	dma_fence_put(a_fence);
+	dma_fence_put(b_fence);
 	return NULL;
 
 }
@@ -328,10 +377,15 @@ static void sync_file_free(struct kref *kref)
 {
 	struct sync_file *sync_file = container_of(kref, struct sync_file,
 						     kref);
+	struct dma_fence *fence;
+
+	fence = rcu_dereference_protected(sync_file->fence, 1);
+	if (fence) {
+		if (test_bit(POLL_ENABLED, &fence->flags))
+			dma_fence_remove_callback(fence, &sync_file->cb);
+		dma_fence_put(fence);
+	}
 
-	if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
-		dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
-	dma_fence_put(sync_file->fence);
 	kfree(sync_file);
 }
 
@@ -346,16 +400,25 @@ static int sync_file_release(struct inode *inode, struct file *file)
 static unsigned int sync_file_poll(struct file *file, poll_table *wait)
 {
 	struct sync_file *sync_file = file->private_data;
+	unsigned int ret_val = 0;
+	struct dma_fence *fence;
 
 	poll_wait(file, &sync_file->wq, wait);
 
-	if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
-		if (dma_fence_add_callback(sync_file->fence, &sync_file->cb,
-					   fence_check_cb_func) < 0)
-			wake_up_all(&sync_file->wq);
+	mutex_lock(&sync_file->lock);
+
+	fence = sync_file_get_fence_locked(sync_file);
+	if (fence) {
+		if (!test_and_set_bit(POLL_ENABLED, &fence->flags)) {
+			if (dma_fence_add_callback(fence, &sync_file->cb,
+						   fence_check_cb_func) < 0)
+				wake_up_all(&sync_file->wq);
+		}
+		ret_val = dma_fence_is_signaled(fence) ? POLLIN : 0;
 	}
+	mutex_unlock(&sync_file->lock);
 
-	return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+	return ret_val;
 }
 
 static long sync_file_ioctl_merge(struct sync_file *sync_file,
@@ -431,6 +494,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	struct sync_file_info info;
 	struct sync_fence_info *fence_info = NULL;
 	struct dma_fence **fences;
+	struct dma_fence *fence;
 	__u32 size;
 	int num_fences, ret, i;
 
@@ -440,7 +504,15 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (info.flags || info.pad)
 		return -EINVAL;
 
-	fences = get_fences(sync_file, &num_fences);
+	rcu_read_lock();
+	fence = dma_fence_get_rcu_safe(&sync_file->fence);
+	rcu_read_unlock();
+
+	fences = get_fences(&fence, &num_fences);
+
+	/* if there are no fences in the sync_file just return */
+	if (!num_fences)
+		goto no_fences;
 
 	/*
 	 * Passing num_fences = 0 means that userspace doesn't want to
@@ -451,13 +523,17 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (!info.num_fences)
 		goto no_fences;
 
-	if (info.num_fences < num_fences)
-		return -EINVAL;
+	if (info.num_fences < num_fences) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	size = num_fences * sizeof(*fence_info);
 	fence_info = kzalloc(size, GFP_KERNEL);
-	if (!fence_info)
-		return -ENOMEM;
+	if (!fence_info) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	for (i = 0; i < num_fences; i++)
 		sync_fill_fence_info(fences[i], &fence_info[i]);
@@ -470,7 +546,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 
 no_fences:
 	strlcpy(info.name, sync_file->name, sizeof(info.name));
-	info.status = dma_fence_is_signaled(sync_file->fence);
+	if (num_fences)
+		info.status = dma_fence_is_signaled(sync_file->fence);
+	else
+		info.status = -ENOENT;
 	info.num_fences = num_fences;
 
 	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
@@ -480,7 +559,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 
 out:
 	kfree(fence_info);
-
+	dma_fence_put(fence);
 	return ret;
 }
 
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index e683dd1..4bf661b 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -34,6 +34,7 @@
  * @cb:			fence callback information
  * @type:               sync file type
  * @flags:              flags used to create sync file
+ * @lock:               mutex to protect fence/cb - used for semaphores
  */
 struct sync_file {
 	struct file		*file;
@@ -45,10 +46,13 @@ struct sync_file {
 
 	wait_queue_head_t	wq;
 
-	struct dma_fence	*fence;
+	struct dma_fence __rcu	*fence;
 	struct dma_fence_cb cb;
 	uint32_t type;
 	uint32_t flags;
+
+	/* protects the fence pointer and cb */
+	struct mutex lock;
 };
 
 #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel