Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

8 participants
3611 discussions

[PATCH 6/8] dma-buf: use inline lock for the stub fence v2

by Christian König

Using the inline lock is now the recommended way for dma_fence implementations. So use this approach for the framework's internal fences as well. Also saves about 4 bytes for the external spinlock. v2: drop unnecessary changes Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Philipp Stanner <phasta(a)kernel.org> Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> --- drivers/dma-buf/dma-fence.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 39f0a4d08a2d..133cb9033b2c 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -24,7 +24,6 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_emit); EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal); EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); -static DEFINE_SPINLOCK(dma_fence_stub_lock); static struct dma_fence dma_fence_stub; /* @@ -123,12 +122,9 @@ static const struct dma_fence_ops dma_fence_stub_ops = { static int __init dma_fence_init_stub(void) { - dma_fence_init(&dma_fence_stub, &dma_fence_stub_ops, - &dma_fence_stub_lock, 0, 0); - + dma_fence_init(&dma_fence_stub, &dma_fence_stub_ops, NULL, 0, 0); set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &dma_fence_stub.flags); - dma_fence_signal(&dma_fence_stub); return 0; } @@ -160,11 +156,7 @@ struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp) if (fence == NULL) return NULL; - dma_fence_init(fence, - &dma_fence_stub_ops, - &dma_fence_stub_lock, - 0, 0); - + dma_fence_init(fence, &dma_fence_stub_ops, NULL, 0, 0); set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags); -- 2.43.0

7 months, 2 weeks

[PATCH 5/8] dma-buf/selftests: test RCU ops and inline lock v2

by Christian König

Drop the mock_fence and the kmem_cache, instead use the inline lock and test if the ops are properly dropped after signaling. v2: move the RCU check to the end of the test Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> --- drivers/dma-buf/st-dma-fence.c | 44 ++++++++-------------------------- 1 file changed, 10 insertions(+), 34 deletions(-) diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index 5d0d9abc6e21..0d9d524d79b6 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -14,43 +14,26 @@ #include "selftest.h" -static struct kmem_cache *slab_fences; - -static struct mock_fence { - struct dma_fence base; - struct spinlock lock; -} *to_mock_fence(struct dma_fence *f) { - return container_of(f, struct mock_fence, base); -} - static const char *mock_name(struct dma_fence *f) { return "mock"; } -static void mock_fence_release(struct dma_fence *f) -{ - kmem_cache_free(slab_fences, to_mock_fence(f)); -} - static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name, - .release = mock_fence_release, }; static struct dma_fence *mock_fence(void) { - struct mock_fence *f; + struct dma_fence *f; - f = kmem_cache_alloc(slab_fences, GFP_KERNEL); + f = kmalloc(sizeof(*f), GFP_KERNEL); if (!f) return NULL; - spin_lock_init(&f->lock); - dma_fence_init(&f->base, &mock_ops, &f->lock, 0, 0); - - return &f->base; + dma_fence_init(f, &mock_ops, NULL, 0, 0); + return f; } static int sanitycheck(void *arg) @@ -100,6 +83,11 @@ static int test_signaling(void *arg) goto err_free; } + if (rcu_dereference_protected(f->ops, true)) { + pr_err("Fence ops not cleared on signal\n"); + goto err_free; + } + err = 0; err_free: dma_fence_put(f); @@ -540,19 +528,7 @@ int dma_fence(void) SUBTEST(test_stub), SUBTEST(race_signal_callback), }; - int ret; pr_info("sizeof(dma_fence)=%zu\n", sizeof(struct dma_fence)); - - slab_fences = KMEM_CACHE(mock_fence, - SLAB_TYPESAFE_BY_RCU | - SLAB_HWCACHE_ALIGN); - if (!slab_fences) - return -ENOMEM; - - ret = subtests(tests, NULL); - - kmem_cache_destroy(slab_fences); - - return ret; + return subtests(tests, NULL); } -- 2.43.0

7 months, 2 weeks

[PATCH 4/8] dma-buf: inline spinlock for fence protection v5

by Christian König

Implement per-fence spinlocks, allowing implementations to not give an external spinlock to protect the fence internal state. Instead a spinlock embedded into the fence structure itself is used in this case. Shared spinlocks have the problem that implementations need to guarantee that the lock lives at least as long all fences referencing them. Using a per-fence spinlock allows completely decoupling spinlock producer and consumer life times, simplifying the handling in most use cases. v2: improve naming, coverage and function documentation v3: fix one additional locking in the selftests v4: separate out some changes to make the patch smaller, fix one amdgpu crash found by CI systems v5: improve comments Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> --- drivers/dma-buf/dma-fence.c | 21 ++++++++++++++++----- drivers/dma-buf/sync_debug.h | 2 +- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_writeback.c | 2 +- drivers/gpu/drm/nouveau/nouveau_fence.c | 3 ++- drivers/gpu/drm/qxl/qxl_release.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 3 ++- drivers/gpu/drm/xe/xe_hw_fence.c | 3 ++- include/linux/dma-fence.h | 19 +++++++++++++------ 9 files changed, 40 insertions(+), 18 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index bd4ec7e26dae..39f0a4d08a2d 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -343,7 +343,6 @@ void __dma_fence_might_wait(void) } #endif - /** * dma_fence_signal_timestamp_locked - signal completion of a fence * @fence: the fence to signal @@ -1068,7 +1067,6 @@ static void __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) { - BUG_ON(!lock); BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name); kref_init(&fence->refcount); @@ -1080,10 +1078,15 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, */ RCU_INIT_POINTER(fence->ops, ops); INIT_LIST_HEAD(&fence->cb_list); - fence->lock = lock; fence->context = context; fence->seqno = seqno; fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT); + if (lock) { + fence->extern_lock = lock; + } else { + spin_lock_init(&fence->inline_lock); + fence->flags |= BIT(DMA_FENCE_FLAG_INLINE_LOCK_BIT); + } fence->error = 0; trace_dma_fence_init(fence); @@ -1093,7 +1096,7 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, * dma_fence_init - Initialize a custom fence. * @fence: the fence to initialize * @ops: the dma_fence_ops for operations on this fence - * @lock: the irqsafe spinlock to use for locking this fence + * @lock: optional irqsafe spinlock to use for locking this fence * @context: the execution context this fence is run on * @seqno: a linear increasing sequence number for this context * @@ -1103,6 +1106,10 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, * * context and seqno are used for easy comparison between fences, allowing * to check which fence is later by simply using dma_fence_later(). + * + * It is strongly discouraged to provide an external lock because this couples + * lock and fence life time. This is only allowed for legacy use cases when + * multiple fences need to be prevented from signaling out of order. */ void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, @@ -1116,7 +1123,7 @@ EXPORT_SYMBOL(dma_fence_init); * dma_fence_init64 - Initialize a custom fence with 64-bit seqno support. * @fence: the fence to initialize * @ops: the dma_fence_ops for operations on this fence - * @lock: the irqsafe spinlock to use for locking this fence + * @lock: optional irqsafe spinlock to use for locking this fence * @context: the execution context this fence is run on * @seqno: a linear increasing sequence number for this context * @@ -1126,6 +1133,10 @@ EXPORT_SYMBOL(dma_fence_init); * * Context and seqno are used for easy comparison between fences, allowing * to check which fence is later by simply using dma_fence_later(). + * + * It is strongly discouraged to provide an external lock because this couples + * lock and fence life time. This is only allowed for legacy use cases when + * multiple fences need to be prevented from signaling out of order. */ void dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h index 02af347293d0..c49324505b20 100644 --- a/drivers/dma-buf/sync_debug.h +++ b/drivers/dma-buf/sync_debug.h @@ -47,7 +47,7 @@ struct sync_timeline { static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence) { - return container_of(fence->lock, struct sync_timeline, lock); + return container_of(fence->extern_lock, struct sync_timeline, lock); } /** diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a7797d260f1e..17472915842f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -159,7 +159,7 @@ static const struct dma_fence_ops drm_crtc_fence_ops; static struct drm_crtc *fence_to_crtc(struct dma_fence *fence) { BUG_ON(fence->ops != &drm_crtc_fence_ops); - return container_of(fence->lock, struct drm_crtc, fence_lock); + return container_of(fence->extern_lock, struct drm_crtc, fence_lock); } static const char *drm_crtc_fence_get_driver_name(struct dma_fence *fence) diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 95b8a2e4bda6..624a4e8b6c99 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -81,7 +81,7 @@ * From userspace, this property will always read as zero. */ -#define fence_to_wb_connector(x) container_of(x->lock, \ +#define fence_to_wb_connector(x) container_of(x->extern_lock, \ struct drm_writeback_connector, \ fence_lock) diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 4a193b7d6d9e..c282c94138b2 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -41,7 +41,8 @@ static const struct dma_fence_ops nouveau_fence_ops_legacy; static inline struct nouveau_fence_chan * nouveau_fctx(struct nouveau_fence *fence) { - return container_of(fence->base.lock, struct nouveau_fence_chan, lock); + return container_of(fence->base.extern_lock, struct nouveau_fence_chan, + lock); } static bool diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 06b0b2aa7953..37d4ae0faf0d 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -62,7 +62,8 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr, struct qxl_device *qdev; unsigned long cur, end = jiffies + timeout; - qdev = container_of(fence->lock, struct qxl_device, release_lock); + qdev = container_of(fence->extern_lock, struct qxl_device, + release_lock); if (!wait_event_timeout(qdev->release_event, (dma_fence_is_signaled(fence) || diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c index 85795082fef9..d251eec57df9 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c @@ -47,7 +47,8 @@ struct vmw_event_fence_action { static struct vmw_fence_manager * fman_from_fence(struct vmw_fence_obj *fence) { - return container_of(fence->base.lock, struct vmw_fence_manager, lock); + return container_of(fence->base.extern_lock, struct vmw_fence_manager, + lock); } static void vmw_fence_obj_destroy(struct dma_fence *f) diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c index ae8ed15b64c5..14720623ad00 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.c +++ b/drivers/gpu/drm/xe/xe_hw_fence.c @@ -124,7 +124,8 @@ static struct xe_hw_fence *to_xe_hw_fence(struct dma_fence *fence); static struct xe_hw_fence_irq *xe_hw_fence_irq(struct xe_hw_fence *fence) { - return container_of(fence->dma.lock, struct xe_hw_fence_irq, lock); + return container_of(fence->dma.extern_lock, struct xe_hw_fence_irq, + lock); } static const char *xe_hw_fence_get_driver_name(struct dma_fence *dma_fence) diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 086324af96c9..9ed359e38d4e 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -34,7 +34,8 @@ struct seq_file; * @ops: dma_fence_ops associated with this fence * @rcu: used for releasing fence with kfree_rcu * @cb_list: list of all callbacks to call - * @lock: spin_lock_irqsave used for locking + * @extern_lock: external spin_lock_irqsave used for locking (deprecated) + * @inline_lock: alternative internal spin_lock_irqsave used for locking * @context: execution context this fence belongs to, returned by * dma_fence_context_alloc() * @seqno: the sequence number of this fence inside the execution context, @@ -49,6 +50,7 @@ struct seq_file; * of the time. * * DMA_FENCE_FLAG_INITIALIZED_BIT - fence was initialized + * DMA_FENCE_FLAG_INLINE_LOCK_BIT - use inline spinlock instead of external one * DMA_FENCE_FLAG_SIGNALED_BIT - fence is already signaled * DMA_FENCE_FLAG_TIMESTAMP_BIT - timestamp recorded for fence signaling * DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called @@ -66,7 +68,10 @@ struct seq_file; * been completed, or never called at all. */ struct dma_fence { - spinlock_t *lock; + union { + spinlock_t *extern_lock; + spinlock_t inline_lock; + }; const struct dma_fence_ops __rcu *ops; /* * We clear the callback list on kref_put so that by the time we @@ -100,6 +105,7 @@ struct dma_fence { enum dma_fence_flag_bits { DMA_FENCE_FLAG_INITIALIZED_BIT, + DMA_FENCE_FLAG_INLINE_LOCK_BIT, DMA_FENCE_FLAG_SEQNO64_BIT, DMA_FENCE_FLAG_SIGNALED_BIT, DMA_FENCE_FLAG_TIMESTAMP_BIT, @@ -381,11 +387,12 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep) * dma_fence_spinlock - return pointer to the spinlock protecting the fence * @fence: the fence to get the lock from * - * Return the pointer to the extern lock. + * Return either the pointer to the embedded or the external spin lock. */ static inline spinlock_t *dma_fence_spinlock(struct dma_fence *fence) { - return fence->lock; + return test_bit(DMA_FENCE_FLAG_INLINE_LOCK_BIT, &fence->flags) ? + &fence->inline_lock : fence->extern_lock; } /** @@ -396,7 +403,7 @@ static inline spinlock_t *dma_fence_spinlock(struct dma_fence *fence) * Lock the fence, preventing it from changing to the signaled state. */ #define dma_fence_lock_irqsave(fence, flags) \ - spin_lock_irqsave(fence->lock, flags) + spin_lock_irqsave(dma_fence_spinlock(fence), flags) /** * dma_fence_unlock_irqrestore - unlock the fence and irqrestore @@ -406,7 +413,7 @@ static inline spinlock_t *dma_fence_spinlock(struct dma_fence *fence) * Unlock the fence, allowing it to change it's state to signaled again. */ #define dma_fence_unlock_irqrestore(fence, flags) \ - spin_unlock_irqrestore(fence->lock, flags) + spin_unlock_irqrestore(dma_fence_spinlock(fence), flags) /** * dma_fence_assert_held - lockdep assertion that fence is locked -- 2.43.0

7 months, 2 weeks

[PATCH 3/8] dma-buf: abstract fence locking v2

by Christian König

Add dma_fence_lock_irqsafe() and dma_fence_unlock_irqrestore() wrappers and mechanically apply them everywhere. Just a pre-requisite cleanup for a follow up patch. v2: add some missing i915 bits, add abstraction for lockdep assertion as well v3: one more suggestion by Tvrtko Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> --- drivers/dma-buf/dma-fence.c | 48 ++++++++++----------- drivers/dma-buf/st-dma-fence.c | 6 ++- drivers/dma-buf/sw_sync.c | 14 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 +- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 2 +- drivers/gpu/drm/i915/i915_active.c | 19 ++++---- drivers/gpu/drm/nouveau/nouveau_drm.c | 5 ++- drivers/gpu/drm/scheduler/sched_fence.c | 6 +-- drivers/gpu/drm/xe/xe_sched_job.c | 4 +- include/linux/dma-fence.h | 38 ++++++++++++++++ 12 files changed, 96 insertions(+), 56 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ae77f900c267..bd4ec7e26dae 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -365,7 +365,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, struct dma_fence_cb *cur, *tmp; struct list_head cb_list; - lockdep_assert_held(fence->lock); + dma_fence_assert_held(fence); if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))) @@ -412,9 +412,9 @@ void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp) if (WARN_ON(!fence)) return; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); dma_fence_signal_timestamp_locked(fence, timestamp); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_signal_timestamp); @@ -473,9 +473,9 @@ bool dma_fence_check_and_signal(struct dma_fence *fence) unsigned long flags; bool ret; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); ret = dma_fence_check_and_signal_locked(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); return ret; } @@ -501,9 +501,9 @@ void dma_fence_signal(struct dma_fence *fence) tmp = dma_fence_begin_signalling(); - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); dma_fence_signal_timestamp_locked(fence, ktime_get()); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); dma_fence_end_signalling(tmp); } @@ -604,10 +604,10 @@ void dma_fence_release(struct kref *kref) * don't leave chains dangling. We set the error flag first * so that the callbacks know this signal is due to an error. */ - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); fence->error = -EDEADLK; dma_fence_signal_locked(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); } ops = rcu_dereference(fence->ops); @@ -637,7 +637,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence) const struct dma_fence_ops *ops; bool was_set; - lockdep_assert_held(fence->lock); + dma_fence_assert_held(fence); was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags); @@ -673,9 +673,9 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) { unsigned long flags; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); __dma_fence_enable_signaling(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_enable_sw_signaling); @@ -715,8 +715,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, return -ENOENT; } - spin_lock_irqsave(fence->lock, flags); - + dma_fence_lock_irqsave(fence, flags); if (__dma_fence_enable_signaling(fence)) { cb->func = func; list_add_tail(&cb->node, &fence->cb_list); @@ -724,8 +723,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, INIT_LIST_HEAD(&cb->node); ret = -ENOENT; } - - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); return ret; } @@ -748,9 +746,9 @@ int dma_fence_get_status(struct dma_fence *fence) unsigned long flags; int status; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); status = dma_fence_get_status_locked(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); return status; } @@ -780,13 +778,11 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb) unsigned long flags; bool ret; - spin_lock_irqsave(fence->lock, flags); - + dma_fence_lock_irqsave(fence, flags); ret = !list_empty(&cb->node); if (ret) list_del_init(&cb->node); - - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); return ret; } @@ -825,7 +821,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) unsigned long flags; signed long ret = timeout ? timeout : 1; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (dma_fence_test_signaled_flag(fence)) goto out; @@ -849,11 +845,11 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) __set_current_state(TASK_INTERRUPTIBLE); else __set_current_state(TASK_UNINTERRUPTIBLE); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); ret = schedule_timeout(ret); - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (ret > 0 && intr && signal_pending(current)) ret = -ERESTARTSYS; } @@ -863,7 +859,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) __set_current_state(TASK_RUNNING); out: - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); return ret; } EXPORT_SYMBOL(dma_fence_default_wait); diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index 73ed6fd48a13..5d0d9abc6e21 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -410,8 +410,10 @@ struct race_thread { static void __wait_for_callbacks(struct dma_fence *f) { - spin_lock_irq(f->lock); - spin_unlock_irq(f->lock); + unsigned long flags; + + dma_fence_lock_irqsave(f, flags); + dma_fence_unlock_irqrestore(f, flags); } static int thread_signal_callback(void *arg) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 6f09d13be6b6..4c81a37dd682 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -156,12 +156,12 @@ static void timeline_fence_release(struct dma_fence *fence) struct sync_timeline *parent = dma_fence_parent(fence); unsigned long flags; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (!list_empty(&pt->link)) { list_del(&pt->link); rb_erase(&pt->node, &parent->pt_tree); } - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); sync_timeline_put(parent); dma_fence_free(fence); @@ -179,7 +179,7 @@ static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadlin struct sync_pt *pt = dma_fence_to_sync_pt(fence); unsigned long flags; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { if (ktime_before(deadline, pt->deadline)) pt->deadline = deadline; @@ -187,7 +187,7 @@ static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadlin pt->deadline = deadline; __set_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags); } - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); } static const struct dma_fence_ops timeline_fence_ops = { @@ -431,13 +431,13 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a goto put_fence; } - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (!test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { ret = -ENOENT; goto unlock; } data.deadline_ns = ktime_to_ns(pt->deadline); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); dma_fence_put(fence); @@ -450,7 +450,7 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return 0; unlock: - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); put_fence: dma_fence_put(fence); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c index b82357c65723..1404e1fe62a4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c @@ -479,10 +479,10 @@ bool amdgpu_ring_soft_recovery(struct amdgpu_ring *ring, unsigned int vmid, if (amdgpu_sriov_vf(ring->adev) || !ring->funcs->soft_recovery || !fence) return false; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (!dma_fence_is_signaled_locked(fence)) dma_fence_set_error(fence, -ENODATA); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); while (!dma_fence_is_signaled(fence) && ktime_to_ns(ktime_sub(deadline, ktime_get())) > 0) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 6a2ea200d90c..4761e7486811 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2802,8 +2802,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm) dma_fence_put(vm->last_unlocked); dma_fence_wait(vm->last_tlb_flush, false); /* Make sure that all fence callbacks have completed */ - spin_lock_irqsave(vm->last_tlb_flush->lock, flags); - spin_unlock_irqrestore(vm->last_tlb_flush->lock, flags); + dma_fence_lock_irqsave(vm->last_tlb_flush, flags); + dma_fence_unlock_irqrestore(vm->last_tlb_flush, flags); dma_fence_put(vm->last_tlb_flush); list_for_each_entry_safe(mapping, tmp, &vm->freed, list) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 139642eacdd0..d5c41e24fb51 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -638,7 +638,7 @@ static inline uint64_t amdgpu_vm_tlb_seq(struct amdgpu_vm *vm) * sure that the dma_fence structure isn't freed up. */ rcu_read_lock(); - lock = vm->last_tlb_flush->lock; + lock = dma_fence_spinlock(vm->last_tlb_flush); rcu_read_unlock(); spin_lock_irqsave(lock, flags); diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c index bf6117d5fc57..78ea2d9ccedf 100644 --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c @@ -148,7 +148,7 @@ __dma_fence_signal__notify(struct dma_fence *fence, { struct dma_fence_cb *cur, *tmp; - lockdep_assert_held(fence->lock); + dma_fence_assert_held(fence); list_for_each_entry_safe(cur, tmp, list, node) { INIT_LIST_HEAD(&cur->node); diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c index 6b0c1162505a..9d41e052ab65 100644 --- a/drivers/gpu/drm/i915/i915_active.c +++ b/drivers/gpu/drm/i915/i915_active.c @@ -1045,9 +1045,10 @@ __i915_active_fence_set(struct i915_active_fence *active, * nesting rules for the fence->lock; the inner lock is always the * older lock. */ - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (prev) - spin_lock_nested(prev->lock, SINGLE_DEPTH_NESTING); + spin_lock_nested(dma_fence_spinlock(prev), + SINGLE_DEPTH_NESTING); /* * A does the cmpxchg first, and so it sees C or NULL, as before, or @@ -1061,17 +1062,18 @@ __i915_active_fence_set(struct i915_active_fence *active, */ while (cmpxchg(__active_fence_slot(active), prev, fence) != prev) { if (prev) { - spin_unlock(prev->lock); + spin_unlock(dma_fence_spinlock(prev)); dma_fence_put(prev); } - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); prev = i915_active_fence_get(active); GEM_BUG_ON(prev == fence); - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (prev) - spin_lock_nested(prev->lock, SINGLE_DEPTH_NESTING); + spin_lock_nested(dma_fence_spinlock(prev), + SINGLE_DEPTH_NESTING); } /* @@ -1088,10 +1090,11 @@ __i915_active_fence_set(struct i915_active_fence *active, */ if (prev) { __list_del_entry(&active->cb.node); - spin_unlock(prev->lock); /* serialise with prev->cb_list */ + /* serialise with prev->cb_list */ + spin_unlock(dma_fence_spinlock(prev)); } list_add_tail(&active->cb.node, &fence->cb_list); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); return prev; } diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index f2e04a048ac2..e6805b8b9ffa 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -156,12 +156,13 @@ nouveau_name(struct drm_device *dev) static inline bool nouveau_cli_work_ready(struct dma_fence *fence) { + unsigned long flags; bool ret = true; - spin_lock_irq(fence->lock); + dma_fence_lock_irqsave(fence, flags); if (!dma_fence_is_signaled_locked(fence)) ret = false; - spin_unlock_irq(fence->lock); + dma_fence_unlock_irqrestore(fence, flags); if (ret == true) dma_fence_put(fence); diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 9391d6f0dc01..724d77694246 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -156,19 +156,19 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, struct dma_fence *parent; unsigned long flags; - spin_lock_irqsave(&fence->lock, flags); + dma_fence_lock_irqsave(f, flags); /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags); return; } fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags); /* * smp_load_aquire() to ensure that if we are racing another diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c index 3927666fe556..ae5b38b2a884 100644 --- a/drivers/gpu/drm/xe/xe_sched_job.c +++ b/drivers/gpu/drm/xe/xe_sched_job.c @@ -190,11 +190,11 @@ static bool xe_fence_set_error(struct dma_fence *fence, int error) unsigned long irq_flags; bool signaled; - spin_lock_irqsave(fence->lock, irq_flags); + dma_fence_lock_irqsave(fence, irq_flags); signaled = test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); if (!signaled) dma_fence_set_error(fence, error); - spin_unlock_irqrestore(fence->lock, irq_flags); + dma_fence_unlock_irqrestore(fence, irq_flags); return signaled; } diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 80db7ede91de..086324af96c9 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -377,6 +377,44 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep) } while (1); } +/** + * dma_fence_spinlock - return pointer to the spinlock protecting the fence + * @fence: the fence to get the lock from + * + * Return the pointer to the extern lock. + */ +static inline spinlock_t *dma_fence_spinlock(struct dma_fence *fence) +{ + return fence->lock; +} + +/** + * dma_fence_lock_irqsave - irqsave lock the fence + * @fence: the fence to lock + * @flags: where to store the CPU flags. + * + * Lock the fence, preventing it from changing to the signaled state. + */ +#define dma_fence_lock_irqsave(fence, flags) \ + spin_lock_irqsave(fence->lock, flags) + +/** + * dma_fence_unlock_irqrestore - unlock the fence and irqrestore + * @fence: the fence to unlock + * @flags the CPU flags to restore + * + * Unlock the fence, allowing it to change it's state to signaled again. + */ +#define dma_fence_unlock_irqrestore(fence, flags) \ + spin_unlock_irqrestore(fence->lock, flags) + +/** + * dma_fence_assert_held - lockdep assertion that fence is locked + * @fence: the fence which should be locked + */ +#define dma_fence_assert_held(fence) \ + lockdep_assert_held(dma_fence_spinlock(fence)); + #ifdef CONFIG_LOCKDEP bool dma_fence_begin_signalling(void); void dma_fence_end_signalling(bool cookie); -- 2.43.0

7 months, 2 weeks

Independence for dma_fences! v7

by Christian König

Hi everyone, dma_fences have ever lived under the tyranny dictated by the module lifetime of their issuer, leading to crashes should anybody still holding a reference to a dma_fence when the module of the issuer was unloaded. The basic problem is that when buffer are shared between drivers dma_fence objects can leak into external drivers and stay there even after they are signaled. The dma_resv object for example only lazy releases dma_fences. So what happens is that when the module who originally created the dma_fence unloads the dma_fence_ops function table becomes unavailable as well and so any attempt to release the fence crashes the system. Previously various approaches have been discussed, including changing the locking semantics of the dma_fence callbacks (by me) as well as using the drm scheduler as intermediate layer (by Sima) to disconnect dma_fences from their actual users, but none of them are actually solving all problems. Tvrtko did some really nice prerequisite work by protecting the returned strings of the dma_fence_ops by RCU. This way dma_fence creators where able to just wait for an RCU grace period after fence signaling before they could be save to free those data structures. Now this patch set here goes a step further and protects the whole dma_fence_ops structure by RCU, so that after the fence signals the pointer to the dma_fence_ops is set to NULL when there is no wait nor release callback given. All functionality which use the dma_fence_ops reference are put inside an RCU critical section, except for the deprecated issuer specific wait and of course the optional release callback. Additional to the RCU changes the lock protecting the dma_fence state previously had to be allocated external. This set here now changes the functionality to make that external lock optional and allows dma_fences to use an inline lock and be self contained. v4: Rebases the whole set on upstream changes, especially the cleanup from Philip in patch "drm/amdgpu: independence for the amdkfd_fence!". Adding two patches which brings the DMA-fence self tests up to date. The first selftest changes removes the mock_wait and so actually starts testing the default behavior instead of some hacky implementation in the test. This one got upstreamed independent of this set. The second drops the mock_fence as well and tests the new RCU and inline spinlock functionality. v5: Rebase on top of drm-misc-next instead of drm-tip, leave out all driver changes for now since those should go through the driver specific paths anyway. Address a few more review comments, especially some rebase mess and typos. And finally fix one more bug found by AMDs CI system. v6: Minor style changes, re-ordered patch #1, dropped the scheduler fence change for now v7: The patch adding the dma_fence_was_initialized() function was pushed upstream individually since that is really an independent cleanup. Fixed some missing i915 bits in patch "dma-buf: abstract fence locking". Please review and comment, Christian.

7 months, 2 weeks

Best Crypto Recovery Services Providers; Top Firm to Retrieve Lost Cryptocurrency

by harrisonhall0088＠gmail.com

I want to share my experience with OPTIMISTIC HACKER GAIUS after I was scammed of my cryptocurrency. Feeling helpless, I reached out and received clear, professional guidance from the start. I explained the process, communicated consistently, and handled the situation with care. Through his expertise and persistence, my scammed crypto was successfully recovered. The support restored my confidence and eased the stress of the order. I’m grateful for the help and would recommend his services. Website: optimistichackargaius .c o m Email: support @ optimistichackargaius .c om WhatsApp: +44 737 674 0569

7 months, 3 weeks

HIRE AN AUTHORIZED CRYPTOCURRENCY RECOVERING HACKER / CONTACT SALVAGE ASSET RECOVERY

by gregl4368＠gmail.com

Cryptocurrency investing has always been a passion of mine but that passion quickly turned into panic when I became the victim of a fraudulent SOL trading platform. The platform promised a fifteen percent daily return which seemed like an incredible opportunity I could not miss. I was drawn in by the sleek interface, positive reviews and supposed user testimonials. Believing I had found a breakthrough I went ahead and deposited 92000 in SOL.To my horror the platform shut down the very next day. My account was wiped clean and I had no access to my funds. Just like that my entire investment vanished and I was left in financial ruin. The days that followed were some of the darkest I have ever experienced. I struggled to pay my bills, skipped meals and found myself in a deep emotional and financial pit.Feeling defeated and ashamed I turned to a close friend and confided in them. That conversation changed everything. My friend referred me to SALVAGE ASSET RECOVERY , a specialized team that deals with cryptocurrency scam recovery. I was skeptical at first. After all, I had already been scammed once and was hesitant to trust anyone again. But desperation pushed me to give it a try. SALVAGE ASSET RECOVERY impressed me. They listened carefully to my story, explained the process in clear detail and reassured me that they would do everything possible to help. Using advanced blockchain tools and digital tracing methods they were able to locate the flow of my stolen funds.Weeks later I received the news I never thought I would hear. SALVAGE ASSET RECOVERY had successfully recovered my entire 92000 in SOL. I was overwhelmed with relief and gratitude. I could finally breathe again knowing that my hard earned money had not been lost forever. If something sounds too good to be true in the crypto world it almost always is. While I still believe in the future of blockchain I now invest with much more caution and diligence.To anyone who has fallen victim to a similar scam know that all hope is not lost. Reach out to people who can truly help. SALVAGE ASSET RECOVERY gave me a second chance and for that I will always be thankful. you can use the details provided below. Whats app : +18476547096 Telegram : @Salvageasset

7 months, 3 weeks

HIRE AN AUTHORIZED CRYPTOCURRENCY RECOVERING HACKER / CONTACT SALVAGE ASSET RECOVERY

by gregl4368＠gmail.com

7 months, 3 weeks

HIRE A LICENSED BTC / CRYPTOCURRENCY RECOVERY HACKER —Reposolsa Recovery.

by amandahaynes109＠gmail.com

The team called (Reposolsa Recovery) quickly took action, filing my case and working tirelessly on my behalf. Within just four days, I received the surprising news that my 40,000 CAD had been successfully refunded and deposited back into my bank account. I was overjoyed and relieved to see the money returned, especially after the stressful experience. Thanks to (Reposolsa Recovery) professionalism and dedication, they were able to recover my funds. This experience taught me an important lesson about being cautious with online investments and the importance of seeking expert help when dealing with scams. I am truly grateful to (Reposolsa Recovery) to reach out to the recovery team here are the inform. EMAIL: reposolsarecovery(a)email.com WHATSAPP: +12092944699 WEBSITE: https://reposolsa.com Reposolsa Recovery

7 months, 3 weeks

[PATCH AUTOSEL 6.19-6.18] drm/amdgpu: Refactor amdgpu_gem_va_ioctl for Handling Last Fence Update and Timeline Management v4

by Sasha Levin

From: Srinivasan Shanmugam <srinivasan.shanmugam(a)amd.com> [ Upstream commit bd8150a1b3370a9f7761c5814202a3fe5a79f44f ] This commit simplifies the amdgpu_gem_va_ioctl function, key updates include: - Moved the logic for managing the last update fence directly into amdgpu_gem_va_update_vm. - Introduced checks for the timeline point to enable conditional replacement or addition of fences. v2: Addressed review comments from Christian. v3: Updated comments (Christian). v4: The previous version selected the fence too early and did not manage its reference correctly, which could lead to stale or freed fences being used. This resulted in refcount underflows and could crash when updating GPU timelines. The fence is now chosen only after the VA mapping work is completed, and its reference is taken safely. After exporting it to the VM timeline syncobj, the driver always drops its local fence reference, ensuring balanced refcounting and avoiding use-after-free on dma_fence. Crash signature: [ 205.828135] refcount_t: underflow; use-after-free. [ 205.832963] WARNING: CPU: 30 PID: 7274 at lib/refcount.c:28 refcount_warn_saturate+0xbe/0x110 ... [ 206.074014] Call Trace: [ 206.076488] <TASK> [ 206.078608] amdgpu_gem_va_ioctl+0x6ea/0x740 [amdgpu] [ 206.084040] ? __pfx_amdgpu_gem_va_ioctl+0x10/0x10 [amdgpu] [ 206.089994] drm_ioctl_kernel+0x86/0xe0 [drm] [ 206.094415] drm_ioctl+0x26e/0x520 [drm] [ 206.098424] ? __pfx_amdgpu_gem_va_ioctl+0x10/0x10 [amdgpu] [ 206.104402] amdgpu_drm_ioctl+0x4b/0x80 [amdgpu] [ 206.109387] __x64_sys_ioctl+0x96/0xe0 [ 206.113156] do_syscall_64+0x66/0x2d0 ... [ 206.553351] BUG: unable to handle page fault for address: ffffffffc0dfde90 ... [ 206.553378] RIP: 0010:dma_fence_signal_timestamp_locked+0x39/0xe0 ... [ 206.553405] Call Trace: [ 206.553409] <IRQ> [ 206.553415] ? __pfx_drm_sched_fence_free_rcu+0x10/0x10 [gpu_sched] [ 206.553424] dma_fence_signal+0x30/0x60 [ 206.553427] drm_sched_job_done.isra.0+0x123/0x150 [gpu_sched] [ 206.553434] dma_fence_signal_timestamp_locked+0x6e/0xe0 [ 206.553437] dma_fence_signal+0x30/0x60 [ 206.553441] amdgpu_fence_process+0xd8/0x150 [amdgpu] [ 206.553854] sdma_v4_0_process_trap_irq+0x97/0xb0 [amdgpu] [ 206.554353] edac_mce_amd(E) ee1004(E) [ 206.554270] amdgpu_irq_dispatch+0x150/0x230 [amdgpu] [ 206.554702] amdgpu_ih_process+0x6a/0x180 [amdgpu] [ 206.555101] amdgpu_irq_handler+0x23/0x60 [amdgpu] [ 206.555500] __handle_irq_event_percpu+0x4a/0x1c0 [ 206.555506] handle_irq_event+0x38/0x80 [ 206.555509] handle_edge_irq+0x92/0x1e0 [ 206.555513] __common_interrupt+0x3e/0xb0 [ 206.555519] common_interrupt+0x80/0xa0 [ 206.555525] </IRQ> [ 206.555527] <TASK> ... [ 206.555650] RIP: 0010:dma_fence_signal_timestamp_locked+0x39/0xe0 ... [ 206.555667] Kernel panic - not syncing: Fatal exception in interrupt Link: https://patchwork.freedesktop.org/patch/654669/ Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Christian König <christian.koenig(a)amd.com> Suggested-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam(a)amd.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Alex Deucher <alexander.deucher(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- LLM Generated explanations, may be completely bogus: I now have a thorough understanding of this commit. Let me compile my complete analysis. --- ## Complete Analysis ### 1. COMMIT MESSAGE ANALYSIS The commit title says "Refactor" but the v4 description reveals this is a **critical bug fix**: > "The previous version selected the fence too early and did not manage its reference correctly, which could lead to stale or freed fences being used. This resulted in refcount underflows and could crash when updating GPU timelines." The commit includes a **concrete crash signature** showing: - `refcount_t: underflow; use-after-free` in `amdgpu_gem_va_ioctl` - `BUG: unable to handle page fault` in `dma_fence_signal_timestamp_locked` from IRQ context - `Kernel panic - not syncing: Fatal exception in interrupt` This is a **kernel panic** triggered via a userspace ioctl path (`amdgpu_gem_va_ioctl`). ### 2. CODE CHANGE ANALYSIS - THE BUG The bug is in `amdgpu_gem_update_bo_mapping()`, which was introduced by commit 70773bef4e091 ("drm/amdgpu: update userqueue BOs and PDs") in v6.16-rc1. **Bug mechanism** in the old code at lines 115-154 of the current file: ```132:154:drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c /* Find the last update fence */ switch (operation) { case AMDGPU_VA_OP_MAP: case AMDGPU_VA_OP_REPLACE: if (bo && (bo->tbo.base.resv == vm->root.bo->tbo.base.resv)) last_update = vm->last_update; else last_update = bo_va->last_pt_update; break; case AMDGPU_VA_OP_UNMAP: case AMDGPU_VA_OP_CLEAR: last_update = fence; break; default: return; } /* Add fence to timeline */ if (!point) drm_syncobj_replace_fence(syncobj, last_update); else drm_syncobj_add_point(syncobj, chain, last_update, point); ``` **Problem 1 - Missing dma_fence references**: `last_update = vm->last_update` and `last_update = bo_va->last_pt_update` read fence pointers **without taking a reference** via `dma_fence_get()`. These fences can be replaced and their references dropped by concurrent VM operations or fence signaling. The bare pointer is then passed to `drm_syncobj_replace_fence()` or `drm_syncobj_add_point()`, which attempt `dma_fence_get()` on a potentially freed fence - **use-after- free**. I verified that `vm->last_update` is replaced via `swap()` in `amdgpu_vm_sdma_commit()` (line 106-146 of `amdgpu_vm_sdma.c`), which puts the old fence. If the fence was already signaled and no other holders exist, it's freed. **Problem 2 - Fence leak**: In the calling code (lines 971-983 of 6.16.y), when `timeline_syncobj` is set and the operation is MAP/REPLACE, the `fence` (clear_freed fence) returned from `amdgpu_gem_va_update_vm()` is **never put** - it's passed to `amdgpu_gem_update_bo_mapping()` which ignores it for MAP/REPLACE operations. **The fix** properly addresses both issues: - Moves fence selection into `amdgpu_gem_va_update_vm()`, which runs immediately after the VM operations complete - Takes explicit references with `dma_fence_get()` on the selected fence - Returns the properly referenced fence to the caller - The caller **always** calls `dma_fence_put(fence)` regardless of whether a timeline syncobj is used ### 3. CLASSIFICATION This is a **use-after-free / refcount underflow bug fix** disguised as a "refactor." The crash is a kernel panic from interrupt context - one of the most severe possible outcomes. It's reachable from userspace via the `amdgpu_gem_va_ioctl` ioctl. ### 4. AFFECTED VERSIONS - Feature introduced: commit 70773bef4e091, first in **v6.16-rc1** (not in v6.15 or earlier) - Partial memleak fix: commit ad6c120f68880, also in v6.16-rc1 - **Stable trees affected**: 6.16.y, 6.17.y, 6.18.y (all confirmed to have the buggy code) - **NOT affected**: 6.12.y LTS, 6.6.y LTS, 6.1.y LTS, 5.15.y LTS (no timeline syncobj code) ### 5. SCOPE AND RISK - **Lines changed**: ~+70/-41 (net ~+30 lines, mostly restructuring) - **Files changed**: 1 (`amdgpu_gem.c`) - **Risk**: Moderate - it's a structural change, but well-contained and well-reviewed - **Review quality**: Reviewed-by Christian König (the top DRM fence/amdgpu expert), Suggested-by Christian König - **Dependency**: Uses `amdgpu_vm_is_bo_always_valid()` which exists since v6.11 - available in all affected trees - **Backport concern**: The diff may need minor adaptation for 6.16.y/6.17.y due to `va_flags` changes (6716a823d18d4 in v6.18-rc1) ### 6. USER IMPACT - **Severity**: Kernel panic (fatal exception in interrupt handler) - **Trigger**: Userspace GPU applications using the VM timeline syncobj feature (usermode queues) - **Who is affected**: AMD GPU users on 6.16+ kernels using MESA/AMDVLK with timeline synchronization ### 7. VERDICT Despite being titled "Refactor," this commit fixes: 1. A **use-after-free** on `dma_fence` objects (accessing freed memory) 2. A **refcount underflow** leading to premature fence destruction 3. A **kernel panic** in interrupt context (`dma_fence_signal_timestamp_locked`) 4. A **fence memory leak** (clear_freed fence never put when timeline is used) The fix is well-reviewed by the subsystem expert, self-contained to one file, and addresses a real crash that users hit. The affected code exists in 6.16.y, 6.17.y, and 6.18.y stable trees. **YES** drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 135 ++++++++++++++---------- 1 file changed, 82 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 3e38c5db29871..c5570a9698ed2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -112,47 +112,6 @@ amdgpu_gem_update_timeline_node(struct drm_file *filp, return 0; } -static void -amdgpu_gem_update_bo_mapping(struct drm_file *filp, - struct amdgpu_bo_va *bo_va, - uint32_t operation, - uint64_t point, - struct dma_fence *fence, - struct drm_syncobj *syncobj, - struct dma_fence_chain *chain) -{ - struct amdgpu_bo *bo = bo_va ? bo_va->base.bo : NULL; - struct amdgpu_fpriv *fpriv = filp->driver_priv; - struct amdgpu_vm *vm = &fpriv->vm; - struct dma_fence *last_update; - - if (!syncobj) - return; - - /* Find the last update fence */ - switch (operation) { - case AMDGPU_VA_OP_MAP: - case AMDGPU_VA_OP_REPLACE: - if (bo && (bo->tbo.base.resv == vm->root.bo->tbo.base.resv)) - last_update = vm->last_update; - else - last_update = bo_va->last_pt_update; - break; - case AMDGPU_VA_OP_UNMAP: - case AMDGPU_VA_OP_CLEAR: - last_update = fence; - break; - default: - return; - } - - /* Add fence to timeline */ - if (!point) - drm_syncobj_replace_fence(syncobj, last_update); - else - drm_syncobj_add_point(syncobj, chain, last_update, point); -} - static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf) { struct ttm_buffer_object *bo = vmf->vma->vm_private_data; @@ -764,16 +723,19 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va, uint32_t operation) { - struct dma_fence *fence = dma_fence_get_stub(); + struct dma_fence *clear_fence = dma_fence_get_stub(); + struct dma_fence *last_update = NULL; int r; if (!amdgpu_vm_ready(vm)) - return fence; + return clear_fence; - r = amdgpu_vm_clear_freed(adev, vm, &fence); + /* First clear freed BOs and get a fence for that work, if any. */ + r = amdgpu_vm_clear_freed(adev, vm, &clear_fence); if (r) goto error; + /* For MAP/REPLACE we also need to update the BO mappings. */ if (operation == AMDGPU_VA_OP_MAP || operation == AMDGPU_VA_OP_REPLACE) { r = amdgpu_vm_bo_update(adev, bo_va, false); @@ -781,13 +743,59 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev, goto error; } + /* Always update PDEs after we touched the mappings. */ r = amdgpu_vm_update_pdes(adev, vm, false); + if (r) + goto error; + + /* + * Decide which fence represents the "last update" for this VM/BO: + * + * - For MAP/REPLACE we want the PT update fence, which is tracked as + * either vm->last_update (for always-valid BOs) or bo_va->last_pt_update + * (for per-BO updates). + * + * - For UNMAP/CLEAR we rely on the fence returned by + * amdgpu_vm_clear_freed(), which already covers the page table work + * for the removed mappings. + */ + switch (operation) { + case AMDGPU_VA_OP_MAP: + case AMDGPU_VA_OP_REPLACE: + if (bo_va && bo_va->base.bo) { + if (amdgpu_vm_is_bo_always_valid(vm, bo_va->base.bo)) { + if (vm->last_update) + last_update = dma_fence_get(vm->last_update); + } else { + if (bo_va->last_pt_update) + last_update = dma_fence_get(bo_va->last_pt_update); + } + } + break; + case AMDGPU_VA_OP_UNMAP: + case AMDGPU_VA_OP_CLEAR: + if (clear_fence) + last_update = dma_fence_get(clear_fence); + break; + default: + break; + } error: if (r && r != -ERESTARTSYS) DRM_ERROR("Couldn't update BO_VA (%d)\n", r); - return fence; + /* + * If we managed to pick a more specific last-update fence, prefer it + * over the generic clear_fence and drop the extra reference to the + * latter. + */ + if (last_update) { + dma_fence_put(clear_fence); + return last_update; + } + + return clear_fence; } int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, @@ -813,6 +821,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, uint64_t vm_size; int r = 0; + /* Validate virtual address range against reserved regions. */ if (args->va_address < AMDGPU_VA_RESERVED_BOTTOM) { dev_dbg(dev->dev, "va_address 0x%llx is in reserved area 0x%llx\n", @@ -846,6 +855,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, return -EINVAL; } + /* Validate operation type. */ switch (args->operation) { case AMDGPU_VA_OP_MAP: case AMDGPU_VA_OP_UNMAP: @@ -869,6 +879,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, abo = NULL; } + /* Add input syncobj fences (if any) for synchronization. */ r = amdgpu_gem_add_input_fence(filp, args->input_fence_syncobj_handles, args->num_syncobj_handles); @@ -891,6 +902,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, goto error; } + /* Resolve the BO-VA mapping for this VM/BO combination. */ if (abo) { bo_va = amdgpu_vm_bo_find(&fpriv->vm, abo); if (!bo_va) { @@ -903,6 +915,11 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, bo_va = NULL; } + /* + * Prepare the timeline syncobj node if the user requested a VM + * timeline update. This only allocates/looks up the syncobj and + * chain node; the actual fence is attached later. + */ r = amdgpu_gem_update_timeline_node(filp, args->vm_timeline_syncobj_out, args->vm_timeline_point, @@ -934,18 +951,30 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, default: break; } + + /* + * Once the VA operation is done, update the VM and obtain the fence + * that represents the last relevant update for this mapping. This + * fence can then be exported to the user-visible VM timeline. + */ if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) && !adev->debug_vm) { fence = amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va, args->operation); - if (timeline_syncobj) - amdgpu_gem_update_bo_mapping(filp, bo_va, - args->operation, - args->vm_timeline_point, - fence, timeline_syncobj, - timeline_chain); - else - dma_fence_put(fence); + if (timeline_syncobj && fence) { + if (!args->vm_timeline_point) { + /* Replace the existing fence when no point is given. */ + drm_syncobj_replace_fence(timeline_syncobj, + fence); + } else { + /* Attach the last-update fence at a specific point. */ + drm_syncobj_add_point(timeline_syncobj, + timeline_chain, + fence, + args->vm_timeline_point); + } + } + dma_fence_put(fence); } -- 2.51.0

7 months, 3 weeks

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig