Re: [PATCH v3 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 1 potential issue(s) to consider:
- [Medium] Missing failure guards in multiple KUnit tests (e.g., check_split_boundaries, check_overlap_partial_init, check_all, check_used) trigger kernel assertions when ID reservations fail.
--

commit 7c9d7d30bf5be71855d8dd170fb19ba5954562d9
Author: Piotr Piórkowski <piotr.piorkowski@intel.com>

drm/xe/kunit: Extend GuC ID manager split-pool coverage

Expand the GuC ID manager KUnit coverage to exercise the new
usable/shareable pool model.

> 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..bf5d2beaaeadd 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

[ ... ]

> @@ -94,14 +156,485 @@ static void check_all(struct kunit *test)
>  	mutex_lock(idm_mutex(idm));
>  
>  	for (n = 0; n < idm_total(idm); n++)
> -		KUNIT_EXPECT_LE(test, 0, xe_guc_id_mgr_reserve_usable_locked(idm, 1));
> -	KUNIT_EXPECT_EQ(test, bitmap_weight(idm->bitmap, idm_total(idm)), idm_total(idm));
> +		KUNIT_EXPECT_GE(test, xe_guc_id_mgr_reserve_usable_locked(idm, 1), 0);
> +	KUNIT_EXPECT_EQ(test, test_idm_used_total(idm), idm_total(idm));
>  	for (n = 0; n < idm_total(idm); n++)
>  		idm_release_chunk_locked(idm, n, 1);

[Severity: Medium]
If any single bit reservation fails during the loop, will the subsequent
release loop unconditionally attempt to release every bit index up to
idm_total(idm)? This appears to bypass bounds checks and trigger a kernel WARN
when attempting to release an unreserved bit. Should we add a guard before
releasing these allocations?

[ ... ]

> +static void check_overlap_partial_init(struct kunit *test)
> +{
> +	struct xe_guc_id_mgr *idm = test->priv;
> +	unsigned int pools_size = (GUC_ID_MAX / 3) * 2;
> +	unsigned int shareable_start;
> +	unsigned int rest;
> +	int id;
> +
> +	if (!require_iov_config_or_skip(test))
> +		return;
> +
> +	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, pools_size, pools_size));
> +
> +	mutex_lock(idm_mutex(idm));
> +
> +	shareable_start = idm_shareable_start(idm);
> +	rest = idm_total(idm) - pools_size;
> +
> +	KUNIT_EXPECT_EQ(test, idm_total(idm), GUC_ID_MAX);
> +	KUNIT_EXPECT_EQ(test, xe_guc_id_mgr_max_usable(idm), pools_size);
> +	KUNIT_EXPECT_EQ(test, xe_guc_id_mgr_max_shareable(idm), pools_size);
> +	KUNIT_EXPECT_EQ(test, shareable_start, GUC_ID_MAX - pools_size);
> +
> +	id = xe_guc_id_mgr_reserve_usable_locked(idm, pools_size);
> +	KUNIT_EXPECT_EQ(test, id, 0);
> +
> +	id = xe_guc_id_mgr_reserve_shareable_locked(idm, rest, 0);
> +	KUNIT_EXPECT_EQ(test, id, pools_size);
> +
> +	KUNIT_EXPECT_EQ(test, test_idm_used_total(idm), GUC_ID_MAX);
> +	KUNIT_EXPECT_EQ(test, idm_used_usable(idm), pools_size);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), pools_size);
> +
> +	KUNIT_EXPECT_EQ(test, xe_guc_id_mgr_release_usable_locked(idm, 0, pools_size), 0);

[Severity: Medium]
If the reservation for id fails here, will passing the constant 0 to
xe_guc_id_mgr_release_usable_locked() bypass the initial -ERANGE bounds check?
It looks like this would reach the internal bit-clearing logic and trip the
test_bit() assertion. Would it be better to verify the reservation succeeded
before releasing?

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

[ ... ]

> +static void check_split_boundaries(struct kunit *test)
> +{
> +	struct xe_guc_id_mgr *idm = test->priv;
> +	unsigned int shareable_start;
> +	int id;
> +
> +	if (!require_iov_config_or_skip(test))
> +		return;
> +
> +	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, 4, 4));
> +
> +	mutex_lock(idm_mutex(idm));
> +
> +	shareable_start = idm_shareable_start(idm);
> +
> +	id = xe_guc_id_mgr_reserve_usable_locked(idm, 4);
> +	KUNIT_EXPECT_EQ(test, id, 0);
> +	KUNIT_EXPECT_EQ(test, xe_guc_id_mgr_reserve_usable_locked(idm, 1), -ENOSPC);
> +
> +	id = xe_guc_id_mgr_reserve_shareable_locked(idm, 4, 0);
> +	KUNIT_EXPECT_EQ(test, id, shareable_start);
> +	KUNIT_EXPECT_EQ(test, xe_guc_id_mgr_reserve_shareable_locked(idm, 1, 0), -ENOSPC);
> +
> +	KUNIT_EXPECT_EQ(test, idm_used_usable(idm), 4);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), 4);
> +
> +	idm_release_chunk_locked(idm, 0, 4);
> +	idm_release_chunk_locked(idm, shareable_start, 4);

[Severity: Medium]
If the initial reservations fail, will this unconditional release of constant
ranges 0 and shareable_start trigger idm_assert()? Unlike other tests, there
are no failure guards here before releasing the allocations.

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

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