Re: [PATCH v5 2/2] drm/i915/selftests: take error_mutex in stolen reset test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/i915/selftests: lock inversion holding error_mutex in
  __igt_reset_stolen()
- [Medium] drm/i915/selftests: lockdep inversion in __igt_reset_stolen()
  insert_page()
--

commit c33d0f8d797f0fad1f041bc0438964e9aab4e35c
Author: Sebastian Brzezinka <sebastian.brzezinka@intel.com>

drm/i915/selftests: take error_mutex in stolen reset test

This commit acquires ggtt->error_mutex around the stolen memory page
sampling and GPU reset in __igt_reset_stolen() to serialize access to the
ggtt->error_capture slot.

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index a419dfe9a34fb..386741637e45d 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
> @@ -98,6 +98,7 @@ __igt_reset_stolen(struct intel_gt *gt,
>  	}
>  
>  	mutex_lock(&gt->i915->mm.stolen_lock);
> +	mutex_lock(&ggtt->error_mutex);
>  
>  	for (page = 0; page < num_pages; page++) {
>  		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);

[Severity: Medium]
Does calling ggtt->vm.insert_page() in the two sampling loops while
holding ggtt->error_mutex introduce a lockdep circular dependency on
Cherryview and Broxton with VT-d?

On platforms where intel_vm_no_concurrent_access_wa() is true,
gen8_gmch_probe() sets ggtt->vm.insert_page to
bxt_vtd_ggtt_insert_page__BKL(), which calls stop_machine() and acquires
cpu_hotplug_lock.  Because i915_vma_coredump_create() acquires
ggtt->error_mutex inside a dma_fence_begin_signalling() section (after
i915_vma_resource_hold()), lockdep already tracks cpu_hotplug_lock ->
dma_fence_map -> ggtt->error_mutex.  Calling ggtt->vm.insert_page() under
ggtt->error_mutex inverts that order.

Using ggtt->vm.raw_insert_page when non-NULL, the same way
i915_vma_coredump_create() does under ggtt->error_mutex, avoids
stop_machine().

> @@ -182,6 +183,7 @@ __igt_reset_stolen(struct intel_gt *gt,
>  	mb();
>  	ggtt->vm.clear_range(&ggtt->vm, ggtt->error_capture.start, PAGE_SIZE);
>  
> +	mutex_unlock(&ggtt->error_mutex);
>  	mutex_unlock(&gt->i915->mm.stolen_lock);
>  
>  	if (count > 0) {

[Severity: Medium]
Can keeping ggtt->error_mutex held across intel_gt_reset() cause an ABBA
deadlock with GuC error capture?

CPU0                                  kworker (guc->ct.requests.worker)
----                                  ---------------------------------
__igt_reset_stolen()
  mutex_lock(&ggtt->error_mutex)
  intel_gt_reset()
    mutex_lock(&gt->reset.mutex)
    ...
    flush_work(&guc->ct.requests.worker)
                                      ct_incoming_request_worker_func()
                                        ...
                                        i915_vma_coredump_create()
                                          mutex_lock(&ggtt->error_mutex)

 *** DEADLOCK ***

Holding ggtt->error_mutex across intel_gt_reset() also inverts the lock
order against i_mmap_rwsem (acquired in gt_revoke() ->
unmap_mapping_range()) and dma_fence_map, since i915_vma_coredump_create()
takes ggtt->error_mutex inside a dma_fence_begin_signalling() section.

Since ggtt->error_capture is cleared before the reset and
gt->i915->mm.stolen_lock stays held across it, ggtt->error_mutex could be
unlocked before the reset and re-locked for the post-reset loop.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112115907.16263-1-sebastian.brzezinka@intel.com?part=2