ACK: [PATCH 1/1] Revert "UBUNTU: SAUCE: drm/i915: Synchronize active and retire callbacks"
Kleber Souza
kleber.souza at canonical.com
Fri Nov 20 10:09:14 UTC 2020
On 13.11.20 05:47, Kai-Heng Feng wrote:
> BugLink: https://bugs.launchpad.net/bugs/1896091
>
> This reverts commit c1484c993957fe6b366ab453fbb7b50c0d2916de.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
I'm also affected by this and it's pretty annoying. Thanks for finding
a fix!
Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> ---
> drivers/gpu/drm/i915/i915_active.c | 52 ++++--------------------
> drivers/gpu/drm/i915/i915_active.h | 10 +++--
> drivers/gpu/drm/i915/i915_active_types.h | 2 -
> 3 files changed, 14 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 768713657e85..d960d0be5bd2 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -148,22 +148,8 @@ __active_retire(struct i915_active *ref)
> spin_unlock_irqrestore(&ref->tree_lock, flags);
>
> /* After the final retire, the entire struct may be freed */
> - if (ref->retire) {
> - if (ref->active) {
> - bool freed = false;
> -
> - /* Don't race with the active callback, and avoid UaF */
> - down_write(&ref->rwsem);
> - ref->freed = &freed;
> - ref->retire(ref);
> - if (!freed) {
> - ref->freed = NULL;
> - up_write(&ref->rwsem);
> - }
> - } else {
> - ref->retire(ref);
> - }
> - }
> + if (ref->retire)
> + ref->retire(ref);
>
> /* ... except if you wait on it, you must manage your own references! */
> wake_up_var(ref);
> @@ -293,8 +279,7 @@ void __i915_active_init(struct i915_active *ref,
> int (*active)(struct i915_active *ref),
> void (*retire)(struct i915_active *ref),
> struct lock_class_key *mkey,
> - struct lock_class_key *wkey,
> - struct lock_class_key *rkey)
> + struct lock_class_key *wkey)
> {
> unsigned long bits;
>
> @@ -303,13 +288,8 @@ void __i915_active_init(struct i915_active *ref,
> ref->flags = 0;
> ref->active = active;
> ref->retire = ptr_unpack_bits(retire, &bits, 2);
> - ref->freed = NULL;
> - if (ref->active && ref->retire) {
> - __init_rwsem(&ref->rwsem, "i915_active.rwsem", rkey);
> + if (bits & I915_ACTIVE_MAY_SLEEP)
> ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
> - } else if (bits & I915_ACTIVE_MAY_SLEEP) {
> - ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
> - }
>
> spin_lock_init(&ref->tree_lock);
> ref->tree = RB_ROOT;
> @@ -448,20 +428,8 @@ int i915_active_acquire(struct i915_active *ref)
> return err;
>
> if (likely(!i915_active_acquire_if_busy(ref))) {
> - if (ref->active) {
> - if (ref->retire) {
> - /*
> - * This can be a recursive call, and the mutex
> - * above already protects from concurrent active
> - * callbacks, so a read lock fits best.
> - */
> - down_read(&ref->rwsem);
> - err = ref->active(ref);
> - up_read(&ref->rwsem);
> - } else {
> - err = ref->active(ref);
> - }
> - }
> + if (ref->active)
> + err = ref->active(ref);
> if (!err) {
> spin_lock_irq(&ref->tree_lock); /* __active_retire() */
> debug_active_activate(ref);
> @@ -683,20 +651,16 @@ int i915_sw_fence_await_active(struct i915_sw_fence *fence,
> return await_active(ref, flags, sw_await_fence, fence, fence);
> }
>
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
> void i915_active_fini(struct i915_active *ref)
> {
> - if (ref->freed) {
> - *ref->freed = true;
> - up_write(&ref->rwsem);
> - }
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
> debug_active_fini(ref);
> GEM_BUG_ON(atomic_read(&ref->count));
> GEM_BUG_ON(work_pending(&ref->work));
> GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
> mutex_destroy(&ref->mutex);
> -#endif
> }
> +#endif
>
> static inline bool is_idle_barrier(struct active_node *node, u64 idx)
> {
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index a2e3742b1090..cf4058150966 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -153,16 +153,14 @@ void __i915_active_init(struct i915_active *ref,
> int (*active)(struct i915_active *ref),
> void (*retire)(struct i915_active *ref),
> struct lock_class_key *mkey,
> - struct lock_class_key *wkey,
> - struct lock_class_key *rkey);
> + struct lock_class_key *wkey);
>
> /* Specialise each class of i915_active to avoid impossible lockdep cycles. */
> #define i915_active_init(ref, active, retire) do { \
> static struct lock_class_key __mkey; \
> static struct lock_class_key __wkey; \
> - static struct lock_class_key __rkey; \
> \
> - __i915_active_init(ref, active, retire, &__mkey, &__wkey, &__rkey); \
> + __i915_active_init(ref, active, retire, &__mkey, &__wkey); \
> } while (0)
>
> int i915_active_ref(struct i915_active *ref,
> @@ -215,7 +213,11 @@ i915_active_is_idle(const struct i915_active *ref)
> return !atomic_read(&ref->count);
> }
>
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
> void i915_active_fini(struct i915_active *ref);
> +#else
> +static inline void i915_active_fini(struct i915_active *ref) { }
> +#endif
>
> int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> struct intel_engine_cs *engine);
> diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
> index aaee2548cb19..6360c3e4b765 100644
> --- a/drivers/gpu/drm/i915/i915_active_types.h
> +++ b/drivers/gpu/drm/i915/i915_active_types.h
> @@ -32,8 +32,6 @@ struct active_node;
> struct i915_active {
> atomic_t count;
> struct mutex mutex;
> - struct rw_semaphore rwsem;
> - bool *freed;
>
> spinlock_t tree_lock;
> struct active_node *cache;
>
More information about the kernel-team
mailing list