Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Locking `ttm_bo->base.resv` while unlocking `&ttm_bo->base._resv` causes deadlocks and kernel panics for objects with shared reservation locks. - [High] Execution continues unlocked if `dma_resv_trylock()` fails, leading to unsafe fence modification and panics. --
commit 4a41e0d0f5b4b25aaadf25f72a1d222898154844 Author: Christian König <[email protected]> drm/xe: remove workaround for TTM internals This commit removes a workaround for TTM internals in xe_ttm_bo_release_notify(). It replaces the custom xe_ttm_bo_lock_in_destructor() function with a direct call to dma_resv_trylock(). > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 5843f850339c7..34eae56716076 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -1680,8 +1655,11 @@ static void xe_ttm_bo_release_notify(struct > ttm_buffer_object *ttm_bo) > bo = ttm_to_xe_bo(ttm_bo); > xe_assert(xe_bo_device(bo), !(bo->created && > kref_read(&ttm_bo->base.refcount))); > > - if (!xe_ttm_bo_lock_in_destructor(ttm_bo)) > - return; > + /* > + * This should never fail since there are no other references to the BO > + * any more. > + */ > + WARN_ON(!dma_resv_trylock(ttm_bo->base.resv)); [Severity: Critical] Does this cause a locking mismatch between ttm_bo->base.resv and &ttm_bo->base._resv? For imported dma-buf objects, ttm_bo->base.resv points to the exporter's shared reservation lock, while &ttm_bo->base._resv points to the local unused reservation object. The trylock now uses ttm_bo->base.resv, but the subsequent fence replacement loop and the unlock in the same function still operate on &ttm_bo->base._resv: dma_resv_for_each_fence(&cursor, &ttm_bo->base._resv, DMA_RESV_USAGE_BOOKKEEP, fence) { ... } dma_fence_put(replacement); dma_resv_unlock(&ttm_bo->base._resv); Can this lead to the shared exporter lock being successfully acquired but never unlocked (causing a deadlock), followed by a call to unlock the unheld local _resv ww_mutex (triggering an immediate kernel panic)? [Severity: High] Is it safe to continue execution unconditionally if dma_resv_trylock() fails? The previous implementation safely returned early if the lock failed. The new implementation wraps the trylock in a WARN_ON(), which only logs a warning and does not abort control flow. For imported BOs, the reservation lock is shared with the exporter and other drivers, meaning the lock can legitimately be contended and the trylock can fail. Will this allow execution to proceed unconditionally to modify fences and unlock the reservation without actually holding the lock? > > /* > * Scrub the preempt fences if any. The unbind fence is already -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
