Re: [PATCH v5 1/2] drm/i915/selftests: Fix unlocked mm.stolen in reset selftest

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/i915/selftests: redundant poisoned bitmap in
  __igt_reset_stolen()
--

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

drm/i915/selftests: Fix unlocked mm.stolen in reset selftest

This commit fixes the stolen memory reset selftest to hold
i915->mm.stolen_lock across the poison and verify passes, check against
mm.stolen.head_node, cap the tested range to dsm.usable_size, and track
poisoned pages in a bitmap.

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index 3599c9f101e14..a419dfe9a34fb 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
[ ... ]
> @@ -79,6 +97,8 @@ __igt_reset_stolen(struct intel_gt *gt,
>  		i915_request_add(rq);
>  	}
>  
> +	mutex_lock(&gt->i915->mm.stolen_lock);
> +
>  	for (page = 0; page < num_pages; page++) {
>  		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);
>  		void __iomem *s;
> @@ -95,10 +115,10 @@ __igt_reset_stolen(struct intel_gt *gt,
>  				      ggtt->error_capture.start,
>  				      PAGE_SIZE);
>  
> -		if (!__drm_mm_interval_first(&gt->i915->mm.stolen,
> -					     page << PAGE_SHIFT,
> -					     ((page + 1) << PAGE_SHIFT) - 1))
> +		if (stolen_page_unused(gt, page)) {
>  			memset_io(s, STACK_MAGIC, PAGE_SIZE);
> +			__set_bit(page, poisoned);
> +		}
[ ... ]
> @@ -143,10 +163,13 @@ __igt_reset_stolen(struct intel_gt *gt,
>  			in = tmp;
>  		x = crc32_le(0, in, PAGE_SIZE);
>  
> -		if (x != crc[page] &&
> -		    !__drm_mm_interval_first(&gt->i915->mm.stolen,
> -					     page << PAGE_SHIFT,
> -					     ((page + 1) << PAGE_SHIFT) - 1)) {
> +		/*
> +		 * Only check pages we poisoned: pages in use before the
> +		 * reset (e.g. ring buffers) may legitimately be rewritten
> +		 * and released during the reset.
> +		 */
> +		if (x != crc[page] && test_bit(page, poisoned) &&
> +		    stolen_page_unused(gt, page)) {
>  			pr_debug("unused stolen page %pa modified by GPU reset\n",
>  				 &page);
>  			if (count++ == 0)

[Severity: Low]
Is the poisoned bitmap needed when gt->i915->mm.stolen_lock is held
continuously across the poison pass, the reset, and the verify pass?

Every insertion and removal in gt->i915->mm.stolen (including
i915_gem_object_release_stolen() -> __i915_gem_stolen_remove_node()) takes
stolen_lock.  Because __igt_reset_stolen() never drops stolen_lock between
the two loops, no stolen node can be released during the reset, so
stolen_page_unused(gt, page) in the verify pass always returns the same
value as in the poison pass and matches test_bit(page, poisoned).

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