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.koe...@amd.com>
> CC: sta...@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
