[PATCH 1/2] drm/i915: Fix ref->mutex deadlock in i915_active_wait()

[Sultan Alsawaf]

On Thu, Apr 09, 2020 at 02:56:04PM -0500, Seth Forshee wrote:
> I think this patch essentially does what is intended, though I wish that
> were more obviously the case. Going through the code makes me dizzy --
> various structures with their own retire callbacks, and trying to keep
> track of what's being retired.
> 
> The i915_active_request struct has a retire callback that you are no
> longer using. It seems that would almost always be set to node_retire(),
> in which case having the callback in the first place seems pretty
> pointless. But it's the "almost" which gives me pause -- in a couple of
> cases I see INIT_ACTIVE_REQUEST() being used, which would set the
> callback to i915_active_retire_noop(). I _think_ that the requests will
> never be retired in that state, but I'm not 100% certain.
> 
> Are you 100% certain that the requests are never retired with the
> callback set to i915_active_retire_noop()? If so, then I think the patch
> accomplished its purpose, even if it is making some already confusing
> code even more confusing.

The trick is that only `struct active_node` objects are placed onto the
`&ref->tree` rbtree, which is what is being retired:
> 	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {

`struct active_node` objects are only ever coupled with `node_retire()` when
they are allocated in `i915_active_acquire_preallocate_barrier()` and
`active_instance()`, so its retire callback always points to `node_retire()`.

The callback isn't exactly pointless because `struct active_node` objects can be
retired through other generic code paths. It's just that in this instance,
`i915_active_wait()` specifically only retires `struct active_node` objects.

Thanks,
Sultan