| Message ID | 20201113044731.570886-2-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix noise lines on i915 + Xorg | expand |
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@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Ok, with the additional info that there was a revert of this and an update for it in Focal (just the revert was not done forward, and we probably should think about how we could avoid this to happen again) it sounds less dangerous. You want to make sure that this info goes into the bug reports regression potential, too. -Stefan > 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; >
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@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@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; >
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;
BugLink: https://bugs.launchpad.net/bugs/1896091 This reverts commit c1484c993957fe6b366ab453fbb7b50c0d2916de. Signed-off-by: Kai-Heng Feng <kai.heng.feng@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(-)