Re: [PATCH v5 2/4] drm/xe/kunit: Extend GuC ID manager split-pool coverage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] test: xe_guc_id_mgr: check_used triggers idm_assert on allocation failure
- [Medium] test: xe_guc_id_mgr: missing early returns on failed ID reservations
--

commit e5671ad31dee2fc19148c70082f71c4bcc2e8f21
Author: Piotr Piórkowski <piotr.piorkowski@intel.com>
Subject: drm/xe/kunit: Extend GuC ID manager split-pool coverage

This commit expands the GuC ID manager KUnit tests to cover the new
usable/shareable pool model. It introduces tests for boundary conditions,
overlap, and partial initialization.

>     v3: Guard failed ID reservations before releasing test allocations.
>     v4: Avoid releasing unreserved ID ranges after failed test reservations.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c b/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c
> index 78d3de46b7572..5fe71eac2e380 100644
> --- a/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c
> +++ b/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c

[ ... ]

> @@ -62,13 +93,13 @@ static void check_used(struct kunit *test)
>  
>  	for (n = 0; n < idm_total(idm); n++) {
>  		kunit_info(test, "n=%u", n);
> -		KUNIT_EXPECT_EQ(test, bitmap_weight(idm->bitmap, idm_total(idm)), n);
> +		KUNIT_EXPECT_EQ(test, test_idm_used_total(idm), n);
>  		KUNIT_EXPECT_GE(test, xe_guc_id_mgr_reserve_usable_locked(idm, 1), 0);
> -		KUNIT_EXPECT_EQ(test, bitmap_weight(idm->bitmap, idm_total(idm)), n + 1);
> +		KUNIT_EXPECT_EQ(test, test_idm_used_total(idm), n + 1);
>  	}
> -	KUNIT_EXPECT_EQ(test, bitmap_weight(idm->bitmap, idm_total(idm)), idm_total(idm));
> -	idm_release_chunk_locked(idm, 0, idm_total(idm));
> -	KUNIT_EXPECT_EQ(test, bitmap_weight(idm->bitmap, idm_total(idm)), 0);
> +	KUNIT_EXPECT_EQ(test, test_idm_used_total(idm), idm_total(idm));
> +	idm_release_chunk_locked(idm, 0, test_idm_used_total(idm));

[Severity: Medium]
If xe_guc_id_mgr_reserve_usable_locked() fails inside the loop above, the
KUNIT_EXPECT_GE() will record a failure, but execution continues. When the
loop finishes, test_idm_used_total(idm) will evaluate to 0.

Does this cause idm_release_chunk_locked() to be called with a count of 0?

Looking at xe_guc_id_mgr.c, idm_release_chunk_locked() starts with:
    idm_assert(idm, count);

Would calling this cleanup path with a count of 0 trigger this internal
assertion and result in a drm_WARN during a failed test?

> +	KUNIT_EXPECT_EQ(test, test_idm_used_total(idm), 0);
>  
>  	mutex_unlock(idm_mutex(idm));
>  }

[ ... ]

> +static void check_overlap_alloc(struct kunit *test)
> +{
> +	struct xe_guc_id_mgr *idm = test->priv;
> +	unsigned int total;
> +	int usable_id, shareable_id;
> +
> +	if (!require_iov_config_or_skip(test))
> +		return;
> +
> +	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, GUC_ID_MAX, GUC_ID_MAX));
> +
> +	mutex_lock(idm_mutex(idm));
> +	total = idm_total(idm);
> +
> +	usable_id = xe_guc_id_mgr_reserve_usable_locked(idm, 2);
> +	KUNIT_EXPECT_EQ(test, usable_id, 0);

[Severity: Medium]
The commit message notes "v3: Guard failed ID reservations before releasing
test allocations" and "v4: Avoid releasing unreserved ID ranges after failed
test reservations." 

However, tests like check_overlap_alloc() and 
check_overlap_partial_shared_range_blocking() appear to be missing the early
return guards that were added to check_quota().

If xe_guc_id_mgr_reserve_usable_locked() fails, could negative IDs cascade
into subsequent test logic and release functions? Should there be an early
return guard like "if (usable_id < 0)" added here as well?

> +
> +	shareable_id = xe_guc_id_mgr_reserve_shareable_locked(idm, 3, 0);
> +	KUNIT_EXPECT_EQ(test, shareable_id, total - 3);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005085132.3720685-1-piotr.piorkowski@intel.com?part=2