[PATCH] dma-buf: fix and rework dma_buf_poll v6

[PATCH] dma-buf: fix and rework dma_buf_poll v6

[Christian König]

Daniel pointed me towards this function and there are multiple obvious problems
in the implementation.

First of all the retry loop is not working as intended. In general the retry
makes only sense if you grab the reference first and then check the sequence
values.

Then we should always also wait for the exclusive fence.

It's also good practice to keep the reference around when installing callbacks
to fences you don't own.

And last the whole implementation was unnecessary complex and rather hard to
understand which could lead to probably unexpected behavior of the IOCTL.

Fix all this by reworking the implementation from scratch. Dropping the
whole RCU approach and taking the lock instead.

Only mildly tested and needs a thoughtful review of the code.

v2: fix the reference counting as well
v3: keep the excl fence handling as is for stable
v4: back to testing all fences, drop RCU
v5: handle in and out separately
v6: add missing clear of events

Signed-off-by: Christian König <christian.koenig@amd.com>
CC: stable@vger.kernel.org
---
 drivers/dma-buf/dma-buf.c | 156 +++++++++++++++++---------------------
 include/linux/dma-buf.h   |   2 +-
 2 files changed, 72 insertions(+), 86 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index eadd1eaa2fb5..39e1ef872829 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -72,7 +72,7 @@ static void dma_buf_release(struct dentry *dentry)
 	 * If you hit this BUG() it means someone dropped their ref to the
 	 * dma-buf while still having pending operation to the buffer.
 	 */
-	BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
+	BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
 
 	dmabuf->ops->release(dmabuf);
 
@@ -202,16 +202,57 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
 	wake_up_locked_poll(dcb->poll, dcb->active);
 	dcb->active = 0;
 	spin_unlock_irqrestore(&dcb->poll->lock, flags);
+	dma_fence_put(fence);
+}
+
+static bool dma_buf_poll_shared(struct dma_resv *resv,
+				struct dma_buf_poll_cb_t *dcb)
+{
+	struct dma_resv_list *fobj = dma_resv_get_list(resv);
+	struct dma_fence *fence;
+	int i, r;
+
+	if (!fobj)
+		return false;
+
+	for (i = 0; i < fobj->shared_count; ++i) {
+		fence = rcu_dereference_protected(fobj->shared[i],
+						  dma_resv_held(resv));
+		dma_fence_get(fence);
+		r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
+		if (!r)
+			return true;
+		dma_fence_put(fence);
+	}
+
+	return false;
+}
+
+static bool dma_buf_poll_excl(struct dma_resv *resv,
+			      struct dma_buf_poll_cb_t *dcb)
+{
+	struct dma_fence *fence = dma_resv_get_excl(resv);
+	int r;
+
+	if (!fence)
+		return false;
+
+	dma_fence_get(fence);
+	r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
+	if (!r)
+		return true;
+	dma_fence_put(fence);
+
+	return false;
 }
 
 static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
 {
 	struct dma_buf *dmabuf;
 	struct dma_resv *resv;
-	struct dma_resv_list *fobj;
-	struct dma_fence *fence_excl;
+	unsigned shared_count;
 	__poll_t events;
-	unsigned shared_count, seq;
+	int r, i;
 
 	dmabuf = file->private_data;
 	if (!dmabuf || !dmabuf->resv)
@@ -225,101 +266,46 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
 	if (!events)
 		return 0;
 
-retry:
-	seq = read_seqcount_begin(&resv->seq);
-	rcu_read_lock();
-
-	fobj = rcu_dereference(resv->fence);
-	if (fobj)
-		shared_count = fobj->shared_count;
-	else
-		shared_count = 0;
-	fence_excl = rcu_dereference(resv->fence_excl);
-	if (read_seqcount_retry(&resv->seq, seq)) {
-		rcu_read_unlock();
-		goto retry;
-	}
+	dma_resv_lock(resv, NULL);
 
-	if (fence_excl && (!(events & EPOLLOUT) || shared_count == 0)) {
-		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl;
-		__poll_t pevents = EPOLLIN;
-
-		if (shared_count == 0)
-			pevents |= EPOLLOUT;
+	if (events & EPOLLOUT) {
+		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_out;
 
+		/* Check that callback isn't busy */
 		spin_lock_irq(&dmabuf->poll.lock);
-		if (dcb->active) {
-			dcb->active |= pevents;
-			events &= ~pevents;
-		} else
-			dcb->active = pevents;
+		if (dcb->active)
+			events &= ~EPOLLOUT;
+		else
+			dcb->active = EPOLLOUT;
 		spin_unlock_irq(&dmabuf->poll.lock);
 
-		if (events & pevents) {
-			if (!dma_fence_get_rcu(fence_excl)) {
-				/* force a recheck */
-				events &= ~pevents;
-				dma_buf_poll_cb(NULL, &dcb->cb);
-			} else if (!dma_fence_add_callback(fence_excl, &dcb->cb,
-							   dma_buf_poll_cb)) {
-				events &= ~pevents;
-				dma_fence_put(fence_excl);
-			} else {
-				/*
-				 * No callback queued, wake up any additional
-				 * waiters.
-				 */
-				dma_fence_put(fence_excl);
-				dma_buf_poll_cb(NULL, &dcb->cb);
-			}
-		}
+		if (events & EPOLLOUT && !dma_buf_poll_shared(resv, dcb) &&
+		    !dma_buf_poll_excl(resv, dcb))
+			/* No callback queued, wake up any other waiters */
+			dma_buf_poll_cb(NULL, &dcb->cb);
+		else
+			events &= ~EPOLLOUT;
 	}
 
-	if ((events & EPOLLOUT) && shared_count > 0) {
-		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_shared;
-		int i;
+	if (events & EPOLLIN) {
+		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_in;
 
-		/* Only queue a new callback if no event has fired yet */
+		/* Check that callback isn't busy */
 		spin_lock_irq(&dmabuf->poll.lock);
 		if (dcb->active)
-			events &= ~EPOLLOUT;
+			events &= ~EPOLLIN;
 		else
-			dcb->active = EPOLLOUT;
+			dcb->active = EPOLLIN;
 		spin_unlock_irq(&dmabuf->poll.lock);
 
-		if (!(events & EPOLLOUT))
-			goto out;
-
-		for (i = 0; i < shared_count; ++i) {
-			struct dma_fence *fence = rcu_dereference(fobj->shared[i]);
-
-			if (!dma_fence_get_rcu(fence)) {
-				/*
-				 * fence refcount dropped to zero, this means
-				 * that fobj has been freed
-				 *
-				 * call dma_buf_poll_cb and force a recheck!
-				 */
-				events &= ~EPOLLOUT;
-				dma_buf_poll_cb(NULL, &dcb->cb);
-				break;
-			}
-			if (!dma_fence_add_callback(fence, &dcb->cb,
-						    dma_buf_poll_cb)) {
-				dma_fence_put(fence);
-				events &= ~EPOLLOUT;
-				break;
-			}
-			dma_fence_put(fence);
-		}
-
-		/* No callback queued, wake up any additional waiters. */
-		if (i == shared_count)
+		if (events & EPOLLIN && !dma_buf_poll_excl(resv, dcb))
+			/* No callback queued, wake up any other waiters */
 			dma_buf_poll_cb(NULL, &dcb->cb);
+		else
+			events &= ~EPOLLIN;
 	}
 
-out:
-	rcu_read_unlock();
+	dma_resv_unlock(resv);
 	return events;
 }
 
@@ -562,8 +548,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
 	dmabuf->owner = exp_info->owner;
 	spin_lock_init(&dmabuf->name_lock);
 	init_waitqueue_head(&dmabuf->poll);
-	dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
-	dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
+	dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
+	dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
 
 	if (!resv) {
 		resv = (struct dma_resv *)&dmabuf[1];
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index efdc56b9d95f..7e747ad54c81 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -329,7 +329,7 @@ struct dma_buf {
 		wait_queue_head_t *poll;
 
 		__poll_t active;
-	} cb_excl, cb_shared;
+	} cb_in, cb_out;
 };
 
 /**
-- 
2.25.1

Re: [PATCH] dma-buf: fix and rework dma_buf_poll v6

[Christian König]

Just a gentle ping. Or have I missed your reply?

Thanks,
Christian.

Am 09.07.21 um 14:07 schrieb Christian König:
> Daniel pointed me towards this function and there are multiple obvious problems
> in the implementation.
>
> First of all the retry loop is not working as intended. In general the retry
> makes only sense if you grab the reference first and then check the sequence
> values.
>
> Then we should always also wait for the exclusive fence.
>
> It's also good practice to keep the reference around when installing callbacks
> to fences you don't own.
>
> And last the whole implementation was unnecessary complex and rather hard to
> understand which could lead to probably unexpected behavior of the IOCTL.
>
> Fix all this by reworking the implementation from scratch. Dropping the
> whole RCU approach and taking the lock instead.
>
> Only mildly tested and needs a thoughtful review of the code.
>
> v2: fix the reference counting as well
> v3: keep the excl fence handling as is for stable
> v4: back to testing all fences, drop RCU
> v5: handle in and out separately
> v6: add missing clear of events
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> CC: stable@vger.kernel.org
> ---
>   drivers/dma-buf/dma-buf.c | 156 +++++++++++++++++---------------------
>   include/linux/dma-buf.h   |   2 +-
>   2 files changed, 72 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index eadd1eaa2fb5..39e1ef872829 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -72,7 +72,7 @@ static void dma_buf_release(struct dentry *dentry)
>   	 * If you hit this BUG() it means someone dropped their ref to the
>   	 * dma-buf while still having pending operation to the buffer.
>   	 */
> -	BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
> +	BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
>   
>   	dmabuf->ops->release(dmabuf);
>   
> @@ -202,16 +202,57 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
>   	wake_up_locked_poll(dcb->poll, dcb->active);
>   	dcb->active = 0;
>   	spin_unlock_irqrestore(&dcb->poll->lock, flags);
> +	dma_fence_put(fence);
> +}
> +
> +static bool dma_buf_poll_shared(struct dma_resv *resv,
> +				struct dma_buf_poll_cb_t *dcb)
> +{
> +	struct dma_resv_list *fobj = dma_resv_get_list(resv);
> +	struct dma_fence *fence;
> +	int i, r;
> +
> +	if (!fobj)
> +		return false;
> +
> +	for (i = 0; i < fobj->shared_count; ++i) {
> +		fence = rcu_dereference_protected(fobj->shared[i],
> +						  dma_resv_held(resv));
> +		dma_fence_get(fence);
> +		r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> +		if (!r)
> +			return true;
> +		dma_fence_put(fence);
> +	}
> +
> +	return false;
> +}
> +
> +static bool dma_buf_poll_excl(struct dma_resv *resv,
> +			      struct dma_buf_poll_cb_t *dcb)
> +{
> +	struct dma_fence *fence = dma_resv_get_excl(resv);
> +	int r;
> +
> +	if (!fence)
> +		return false;
> +
> +	dma_fence_get(fence);
> +	r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> +	if (!r)
> +		return true;
> +	dma_fence_put(fence);
> +
> +	return false;
>   }
>   
>   static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>   {
>   	struct dma_buf *dmabuf;
>   	struct dma_resv *resv;
> -	struct dma_resv_list *fobj;
> -	struct dma_fence *fence_excl;
> +	unsigned shared_count;
>   	__poll_t events;
> -	unsigned shared_count, seq;
> +	int r, i;
>   
>   	dmabuf = file->private_data;
>   	if (!dmabuf || !dmabuf->resv)
> @@ -225,101 +266,46 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>   	if (!events)
>   		return 0;
>   
> -retry:
> -	seq = read_seqcount_begin(&resv->seq);
> -	rcu_read_lock();
> -
> -	fobj = rcu_dereference(resv->fence);
> -	if (fobj)
> -		shared_count = fobj->shared_count;
> -	else
> -		shared_count = 0;
> -	fence_excl = rcu_dereference(resv->fence_excl);
> -	if (read_seqcount_retry(&resv->seq, seq)) {
> -		rcu_read_unlock();
> -		goto retry;
> -	}
> +	dma_resv_lock(resv, NULL);
>   
> -	if (fence_excl && (!(events & EPOLLOUT) || shared_count == 0)) {
> -		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl;
> -		__poll_t pevents = EPOLLIN;
> -
> -		if (shared_count == 0)
> -			pevents |= EPOLLOUT;
> +	if (events & EPOLLOUT) {
> +		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_out;
>   
> +		/* Check that callback isn't busy */
>   		spin_lock_irq(&dmabuf->poll.lock);
> -		if (dcb->active) {
> -			dcb->active |= pevents;
> -			events &= ~pevents;
> -		} else
> -			dcb->active = pevents;
> +		if (dcb->active)
> +			events &= ~EPOLLOUT;
> +		else
> +			dcb->active = EPOLLOUT;
>   		spin_unlock_irq(&dmabuf->poll.lock);
>   
> -		if (events & pevents) {
> -			if (!dma_fence_get_rcu(fence_excl)) {
> -				/* force a recheck */
> -				events &= ~pevents;
> -				dma_buf_poll_cb(NULL, &dcb->cb);
> -			} else if (!dma_fence_add_callback(fence_excl, &dcb->cb,
> -							   dma_buf_poll_cb)) {
> -				events &= ~pevents;
> -				dma_fence_put(fence_excl);
> -			} else {
> -				/*
> -				 * No callback queued, wake up any additional
> -				 * waiters.
> -				 */
> -				dma_fence_put(fence_excl);
> -				dma_buf_poll_cb(NULL, &dcb->cb);
> -			}
> -		}
> +		if (events & EPOLLOUT && !dma_buf_poll_shared(resv, dcb) &&
> +		    !dma_buf_poll_excl(resv, dcb))
> +			/* No callback queued, wake up any other waiters */
> +			dma_buf_poll_cb(NULL, &dcb->cb);
> +		else
> +			events &= ~EPOLLOUT;
>   	}
>   
> -	if ((events & EPOLLOUT) && shared_count > 0) {
> -		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_shared;
> -		int i;
> +	if (events & EPOLLIN) {
> +		struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_in;
>   
> -		/* Only queue a new callback if no event has fired yet */
> +		/* Check that callback isn't busy */
>   		spin_lock_irq(&dmabuf->poll.lock);
>   		if (dcb->active)
> -			events &= ~EPOLLOUT;
> +			events &= ~EPOLLIN;
>   		else
> -			dcb->active = EPOLLOUT;
> +			dcb->active = EPOLLIN;
>   		spin_unlock_irq(&dmabuf->poll.lock);
>   
> -		if (!(events & EPOLLOUT))
> -			goto out;
> -
> -		for (i = 0; i < shared_count; ++i) {
> -			struct dma_fence *fence = rcu_dereference(fobj->shared[i]);
> -
> -			if (!dma_fence_get_rcu(fence)) {
> -				/*
> -				 * fence refcount dropped to zero, this means
> -				 * that fobj has been freed
> -				 *
> -				 * call dma_buf_poll_cb and force a recheck!
> -				 */
> -				events &= ~EPOLLOUT;
> -				dma_buf_poll_cb(NULL, &dcb->cb);
> -				break;
> -			}
> -			if (!dma_fence_add_callback(fence, &dcb->cb,
> -						    dma_buf_poll_cb)) {
> -				dma_fence_put(fence);
> -				events &= ~EPOLLOUT;
> -				break;
> -			}
> -			dma_fence_put(fence);
> -		}
> -
> -		/* No callback queued, wake up any additional waiters. */
> -		if (i == shared_count)
> +		if (events & EPOLLIN && !dma_buf_poll_excl(resv, dcb))
> +			/* No callback queued, wake up any other waiters */
>   			dma_buf_poll_cb(NULL, &dcb->cb);
> +		else
> +			events &= ~EPOLLIN;
>   	}
>   
> -out:
> -	rcu_read_unlock();
> +	dma_resv_unlock(resv);
>   	return events;
>   }
>   
> @@ -562,8 +548,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
>   	dmabuf->owner = exp_info->owner;
>   	spin_lock_init(&dmabuf->name_lock);
>   	init_waitqueue_head(&dmabuf->poll);
> -	dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
> -	dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
> +	dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
> +	dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
>   
>   	if (!resv) {
>   		resv = (struct dma_resv *)&dmabuf[1];
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index efdc56b9d95f..7e747ad54c81 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -329,7 +329,7 @@ struct dma_buf {
>   		wait_queue_head_t *poll;
>   
>   		__poll_t active;
> -	} cb_excl, cb_shared;
> +	} cb_in, cb_out;
>   };
>   
>   /**

Re: [PATCH] dma-buf: fix and rework dma_buf_poll v6

[Michel Dänzer]

On 2021-07-09 2:07 p.m., Christian König wrote:
> Daniel pointed me towards this function and there are multiple obvious problems
> in the implementation.
> 
> First of all the retry loop is not working as intended. In general the retry
> makes only sense if you grab the reference first and then check the sequence
> values.
> 
> Then we should always also wait for the exclusive fence.
> 
> It's also good practice to keep the reference around when installing callbacks
> to fences you don't own.
> 
> And last the whole implementation was unnecessary complex and rather hard to
> understand which could lead to probably unexpected behavior of the IOCTL.
> 
> Fix all this by reworking the implementation from scratch. Dropping the
> whole RCU approach and taking the lock instead.
> 
> Only mildly tested and needs a thoughtful review of the code.
> 
> v2: fix the reference counting as well
> v3: keep the excl fence handling as is for stable
> v4: back to testing all fences, drop RCU
> v5: handle in and out separately
> v6: add missing clear of events
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> CC: stable@vger.kernel.org
> ---
>  drivers/dma-buf/dma-buf.c | 156 +++++++++++++++++---------------------
>  include/linux/dma-buf.h   |   2 +-
>  2 files changed, 72 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index eadd1eaa2fb5..39e1ef872829 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
[...]
>  
>  static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>  {
>  	struct dma_buf *dmabuf;
>  	struct dma_resv *resv;
> -	struct dma_resv_list *fobj;
> -	struct dma_fence *fence_excl;
> +	unsigned shared_count;
>  	__poll_t events;
> -	unsigned shared_count, seq;
> +	int r, i;

shared_count, r & i are unused with this patch.



> +		if (events & EPOLLOUT && !dma_buf_poll_shared(resv, dcb) &&
> +		    !dma_buf_poll_excl(resv, dcb))
> +			/* No callback queued, wake up any other waiters */
> +			dma_buf_poll_cb(NULL, &dcb->cb);
> +		else
> +			events &= ~EPOLLOUT;

Something like this might be clearer:

		if (events & EPOLLOUT) {
			if (!dma_buf_poll_shared(resv, dcb) &&
			    !dma_buf_poll_excl(resv, dcb))
				/* No callback queued, wake up any other waiters */
				dma_buf_poll_cb(NULL, &dcb->cb);
			else
				events &= ~EPOLLOUT;
		}


> +		if (events & EPOLLIN && !dma_buf_poll_excl(resv, dcb))
> +			/* No callback queued, wake up any other waiters */
>  			dma_buf_poll_cb(NULL, &dcb->cb);
> +		else
> +			events &= ~EPOLLIN;

Similarly:

		if (events & EPOLLIN) {
			if (!dma_buf_poll_excl(resv, dcb))
				/* No callback queued, wake up any other waiters */
				dma_buf_poll_cb(NULL, &dcb->cb);
			else
				events &= ~EPOLLIN;
		}


Other than that, looks good to me, can't say anything about the locking though.


Haven't been able to test this yet, hopefully later this week.


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] dma-buf: fix and rework dma_buf_poll v6

[Daniel Vetter]

On Wed, Jul 14, 2021 at 3:21 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
> Just a gentle ping. Or have I missed your reply?

Nah just got overwhelmed with dma-resv discussion and took a break. I
still think some igt (or reviewing what we have) would be good. We
could also just merge Jason's import/export series, since he's typed
the igts for that which us dma_buf poll.
-Daniel

>
> Thanks,
> Christian.
>
> Am 09.07.21 um 14:07 schrieb Christian König:
> > Daniel pointed me towards this function and there are multiple obvious problems
> > in the implementation.
> >
> > First of all the retry loop is not working as intended. In general the retry
> > makes only sense if you grab the reference first and then check the sequence
> > values.
> >
> > Then we should always also wait for the exclusive fence.
> >
> > It's also good practice to keep the reference around when installing callbacks
> > to fences you don't own.
> >
> > And last the whole implementation was unnecessary complex and rather hard to
> > understand which could lead to probably unexpected behavior of the IOCTL.
> >
> > Fix all this by reworking the implementation from scratch. Dropping the
> > whole RCU approach and taking the lock instead.
> >
> > Only mildly tested and needs a thoughtful review of the code.
> >
> > v2: fix the reference counting as well
> > v3: keep the excl fence handling as is for stable
> > v4: back to testing all fences, drop RCU
> > v5: handle in and out separately
> > v6: add missing clear of events
> >
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > CC: stable@vger.kernel.org
> > ---
> >   drivers/dma-buf/dma-buf.c | 156 +++++++++++++++++---------------------
> >   include/linux/dma-buf.h   |   2 +-
> >   2 files changed, 72 insertions(+), 86 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index eadd1eaa2fb5..39e1ef872829 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -72,7 +72,7 @@ static void dma_buf_release(struct dentry *dentry)
> >        * If you hit this BUG() it means someone dropped their ref to the
> >        * dma-buf while still having pending operation to the buffer.
> >        */
> > -     BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
> > +     BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
> >
> >       dmabuf->ops->release(dmabuf);
> >
> > @@ -202,16 +202,57 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> >       wake_up_locked_poll(dcb->poll, dcb->active);
> >       dcb->active = 0;
> >       spin_unlock_irqrestore(&dcb->poll->lock, flags);
> > +     dma_fence_put(fence);
> > +}
> > +
> > +static bool dma_buf_poll_shared(struct dma_resv *resv,
> > +                             struct dma_buf_poll_cb_t *dcb)
> > +{
> > +     struct dma_resv_list *fobj = dma_resv_get_list(resv);
> > +     struct dma_fence *fence;
> > +     int i, r;
> > +
> > +     if (!fobj)
> > +             return false;
> > +
> > +     for (i = 0; i < fobj->shared_count; ++i) {
> > +             fence = rcu_dereference_protected(fobj->shared[i],
> > +                                               dma_resv_held(resv));
> > +             dma_fence_get(fence);
> > +             r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> > +             if (!r)
> > +                     return true;
> > +             dma_fence_put(fence);
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static bool dma_buf_poll_excl(struct dma_resv *resv,
> > +                           struct dma_buf_poll_cb_t *dcb)
> > +{
> > +     struct dma_fence *fence = dma_resv_get_excl(resv);
> > +     int r;
> > +
> > +     if (!fence)
> > +             return false;
> > +
> > +     dma_fence_get(fence);
> > +     r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> > +     if (!r)
> > +             return true;
> > +     dma_fence_put(fence);
> > +
> > +     return false;
> >   }
> >
> >   static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> >   {
> >       struct dma_buf *dmabuf;
> >       struct dma_resv *resv;
> > -     struct dma_resv_list *fobj;
> > -     struct dma_fence *fence_excl;
> > +     unsigned shared_count;
> >       __poll_t events;
> > -     unsigned shared_count, seq;
> > +     int r, i;
> >
> >       dmabuf = file->private_data;
> >       if (!dmabuf || !dmabuf->resv)
> > @@ -225,101 +266,46 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> >       if (!events)
> >               return 0;
> >
> > -retry:
> > -     seq = read_seqcount_begin(&resv->seq);
> > -     rcu_read_lock();
> > -
> > -     fobj = rcu_dereference(resv->fence);
> > -     if (fobj)
> > -             shared_count = fobj->shared_count;
> > -     else
> > -             shared_count = 0;
> > -     fence_excl = rcu_dereference(resv->fence_excl);
> > -     if (read_seqcount_retry(&resv->seq, seq)) {
> > -             rcu_read_unlock();
> > -             goto retry;
> > -     }
> > +     dma_resv_lock(resv, NULL);
> >
> > -     if (fence_excl && (!(events & EPOLLOUT) || shared_count == 0)) {
> > -             struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl;
> > -             __poll_t pevents = EPOLLIN;
> > -
> > -             if (shared_count == 0)
> > -                     pevents |= EPOLLOUT;
> > +     if (events & EPOLLOUT) {
> > +             struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_out;
> >
> > +             /* Check that callback isn't busy */
> >               spin_lock_irq(&dmabuf->poll.lock);
> > -             if (dcb->active) {
> > -                     dcb->active |= pevents;
> > -                     events &= ~pevents;
> > -             } else
> > -                     dcb->active = pevents;
> > +             if (dcb->active)
> > +                     events &= ~EPOLLOUT;
> > +             else
> > +                     dcb->active = EPOLLOUT;
> >               spin_unlock_irq(&dmabuf->poll.lock);
> >
> > -             if (events & pevents) {
> > -                     if (!dma_fence_get_rcu(fence_excl)) {
> > -                             /* force a recheck */
> > -                             events &= ~pevents;
> > -                             dma_buf_poll_cb(NULL, &dcb->cb);
> > -                     } else if (!dma_fence_add_callback(fence_excl, &dcb->cb,
> > -                                                        dma_buf_poll_cb)) {
> > -                             events &= ~pevents;
> > -                             dma_fence_put(fence_excl);
> > -                     } else {
> > -                             /*
> > -                              * No callback queued, wake up any additional
> > -                              * waiters.
> > -                              */
> > -                             dma_fence_put(fence_excl);
> > -                             dma_buf_poll_cb(NULL, &dcb->cb);
> > -                     }
> > -             }
> > +             if (events & EPOLLOUT && !dma_buf_poll_shared(resv, dcb) &&
> > +                 !dma_buf_poll_excl(resv, dcb))
> > +                     /* No callback queued, wake up any other waiters */
> > +                     dma_buf_poll_cb(NULL, &dcb->cb);
> > +             else
> > +                     events &= ~EPOLLOUT;
> >       }
> >
> > -     if ((events & EPOLLOUT) && shared_count > 0) {
> > -             struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_shared;
> > -             int i;
> > +     if (events & EPOLLIN) {
> > +             struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_in;
> >
> > -             /* Only queue a new callback if no event has fired yet */
> > +             /* Check that callback isn't busy */
> >               spin_lock_irq(&dmabuf->poll.lock);
> >               if (dcb->active)
> > -                     events &= ~EPOLLOUT;
> > +                     events &= ~EPOLLIN;
> >               else
> > -                     dcb->active = EPOLLOUT;
> > +                     dcb->active = EPOLLIN;
> >               spin_unlock_irq(&dmabuf->poll.lock);
> >
> > -             if (!(events & EPOLLOUT))
> > -                     goto out;
> > -
> > -             for (i = 0; i < shared_count; ++i) {
> > -                     struct dma_fence *fence = rcu_dereference(fobj->shared[i]);
> > -
> > -                     if (!dma_fence_get_rcu(fence)) {
> > -                             /*
> > -                              * fence refcount dropped to zero, this means
> > -                              * that fobj has been freed
> > -                              *
> > -                              * call dma_buf_poll_cb and force a recheck!
> > -                              */
> > -                             events &= ~EPOLLOUT;
> > -                             dma_buf_poll_cb(NULL, &dcb->cb);
> > -                             break;
> > -                     }
> > -                     if (!dma_fence_add_callback(fence, &dcb->cb,
> > -                                                 dma_buf_poll_cb)) {
> > -                             dma_fence_put(fence);
> > -                             events &= ~EPOLLOUT;
> > -                             break;
> > -                     }
> > -                     dma_fence_put(fence);
> > -             }
> > -
> > -             /* No callback queued, wake up any additional waiters. */
> > -             if (i == shared_count)
> > +             if (events & EPOLLIN && !dma_buf_poll_excl(resv, dcb))
> > +                     /* No callback queued, wake up any other waiters */
> >                       dma_buf_poll_cb(NULL, &dcb->cb);
> > +             else
> > +                     events &= ~EPOLLIN;
> >       }
> >
> > -out:
> > -     rcu_read_unlock();
> > +     dma_resv_unlock(resv);
> >       return events;
> >   }
> >
> > @@ -562,8 +548,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
> >       dmabuf->owner = exp_info->owner;
> >       spin_lock_init(&dmabuf->name_lock);
> >       init_waitqueue_head(&dmabuf->poll);
> > -     dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
> > -     dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
> > +     dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
> > +     dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
> >
> >       if (!resv) {
> >               resv = (struct dma_resv *)&dmabuf[1];
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index efdc56b9d95f..7e747ad54c81 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -329,7 +329,7 @@ struct dma_buf {
> >               wait_queue_head_t *poll;
> >
> >               __poll_t active;
> > -     } cb_excl, cb_shared;
> > +     } cb_in, cb_out;
> >   };
> >
> >   /**
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch