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

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.