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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 22, 2021 at 01:08:44PM +0200, Christian König wrote:
> Totally forgotten to ping once more about that.
> 
> Michel has tested this now and I think we should push it ASAP. So can I get
> an rb?

		spin_lock_irq(&dmabuf->poll.lock);
		if (dcb->active)
			events &= ~EPOLLIN;
		else
			dcb->active = EPOLLIN;
		spin_unlock_irq(&dmabuf->poll.lock);


This pattern (and same for EPOLLOUT) makes no sense to me. I guess the
intent is that this filters out events for which we're already listening,
but:

- it checks for any active event, not the specific one
- if we're waiting already and new fences have been added, wont we miss
  them?

Or does this all work because the poll machinery restarts everything
again?

I'm totally confused here for sure. The other changes in the patch look
good though.
-Daniel

> 
> Thanks,
> Christian.
> 
> Am 23.07.21 um 10:04 schrieb Michel Dänzer:
> > On 2021-07-20 3:11 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
> > > v7: change coding style as suggested by Michel, drop unused variables
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > CC: stable@vger.kernel.org
> > Working fine with https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880
> > 
> > Tested-by: Michel Dänzer <mdaenzer@redhat.com>
> > 
> > 
> 

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