[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 at amd.com>
> CC: stable at 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