Re: [PATCH RFC 13/13] drm/powervr_sgx: Add KUnit tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/powervr_sgx: link failure when DRM_POWERVR_SGX=y and
  KUnit tests are =m
- [Low] drm/powervr_sgx: missing KUnit cleanup action in
  equiv_test_sgx545()
--

commit 051430874fcdb70b3619ecd6aa282acde598440c
Author: Joseph Telaak <joseph@telaak.dev>

drm/powervr_sgx: Add KUnit tests

This commit adds KUnit test suites for the PowerVR SGX driver covering
firmware parsing, kernel-object teardown, the core table, register and
firmware structure equivalence on SGX545, the CCB ring, render-target
tiling, the USSE validator, and PRIME cache ranges.

> diff --git a/drivers/gpu/drm/powervr_sgx/Kconfig b/drivers/gpu/drm/powervr_sgx/Kconfig
> index dbbba1d68bc69..6c880ffef5dbc 100644
> --- a/drivers/gpu/drm/powervr_sgx/Kconfig
> +++ b/drivers/gpu/drm/powervr_sgx/Kconfig
> @@ -47,3 +47,19 @@ config DRM_POWERVR_SGX_OF
>  	  bring-up. Dma-buf import isn't supported on ARM yet.
>  
>  	  Say N unless this kernel is for such an SoC.
> +
> +config DRM_POWERVR_SGX_KUNIT_TEST
> +	tristate "KUnit tests for the PowerVR SGX5xx driver" if !KUNIT_ALL_TESTS
> +	depends on DRM_POWERVR_SGX && KUNIT
> +	select DRM_KUNIT_TEST_HELPERS
> +	default KUNIT_ALL_TESTS

[Severity: Medium]
Can vmlinux fail to link when CONFIG_DRM_POWERVR_SGX=y and
CONFIG_DRM_POWERVR_SGX_KUNIT_TEST=m?

The driver's .c files include the test sources under
#if IS_ENABLED(CONFIG_DRM_POWERVR_SGX_KUNIT_TEST), which evaluates to true
for =m as well as =y.  When DRM_POWERVR_SGX is built-in and
DRM_POWERVR_SGX_KUNIT_TEST=m (for instance with CONFIG_KUNIT=m and
CONFIG_KUNIT_ALL_TESTS=m), the tests are compiled into the built-in driver
objects while kunit and drm_kunit_helpers are built as loadable modules,
leaving undefined references to symbols such as
drm_kunit_helper_alloc_device() and kunit_vm_mmap().

Since the tests are compiled directly into the driver rather than as a
separate module, making DRM_POWERVR_SGX_KUNIT_TEST a bool that depends on
DRM_POWERVR_SGX && KUNIT && (DRM_POWERVR_SGX=m || KUNIT=y) would avoid
this.

> diff --git a/drivers/gpu/drm/powervr_sgx/tests/pvrsgx_equiv_test.c b/drivers/gpu/drm/powervr_sgx/tests/pvrsgx_equiv_test.c
> new file mode 100644
> index 0000000000000..aa10ee3d37b21
> --- /dev/null
> +++ b/drivers/gpu/drm/powervr_sgx/tests/pvrsgx_equiv_test.c

[ ... ]

> +static struct eq_fx *eq_device(struct kunit *test, unsigned int core, u32 core_rev)
> +{

[ ... ]

> +	INIT_WORK(&sdev->reap_work, eq_noop_work);
> +	/* The restart path queues the reap; the real driver's own workqueue isn't set up here. */
> +	sdev->wq = system_dfl_wq;
> +	sdev->free_wq = system_dfl_wq;
> +	sdev->irq = -1;
> +	sdev->regs = (void __iomem __force *)fx->regs;
> +
> +	sdev->family = pvrsgx_core_family_get(core);
> +	KUNIT_ASSERT_NOT_NULL(test, sdev->family);
> +	sdev->cr = sdev->family->regs;
> +	sdev->core_id = sdev->family->core_id;
> +	sdev->core_rev = core_rev;
> +	sdev->core = pvrsgx_core_lookup(sdev->family, sdev->core_id, sdev->core_rev);
> +	KUNIT_ASSERT_NOT_NULL(test, sdev->core);
> +
> +	sdev->kernel_mmu = pvrsgx_mmu_create(sdev);
> +	sdev->idle_mmu = pvrsgx_mmu_create(sdev);
> +	sdev->reset_mmu = pvrsgx_mmu_create(sdev);
> +	sdev->reset_page = alloc_page(GFP_KERNEL | __GFP_ZERO | __GFP_DMA32);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sdev->kernel_mmu);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sdev->idle_mmu);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sdev->reset_mmu);
> +	KUNIT_ASSERT_NOT_NULL(test, sdev->reset_page);

[ ... ]

> +static void eq_fini(struct eq_fx *fx)
> +{
> +	struct pvrsgx_device *sdev = fx->sdev;
> +
> +	sdev->test_wr = NULL;
> +	eq_cur = NULL;
> +	cancel_work_sync(&sdev->reap_work);
> +	pvrsgx_ccb_inval_reap(sdev, true);
> +	pvrsgx_pb_put(sdev->pb);
> +	sdev->pb = NULL;
> +	pvrsgx_kobj_fini(sdev);
> +	pvrsgx_fw_fini(sdev);
> +	__free_page(sdev->reset_page);
> +	pvrsgx_mmu_destroy(sdev->reset_mmu);
> +	pvrsgx_mmu_destroy(sdev->idle_mmu);
> +	pvrsgx_mmu_destroy(sdev->kernel_mmu);
> +	mutex_destroy(&sdev->pb_lock);
> +	mutex_destroy(&sdev->done_lock);
> +	mutex_destroy(&sdev->ccb_lock);
> +}

[ ... ]

> +static void equiv_test_sgx545(struct kunit *test)
> +{

[ ... ]

> +	pvrsgx_transfer_ctx_fini(sdev, &xctx);
> +	pvrsgx_render_ctx_fini(sdev, &rctx);
> +	pvrsgx_vm_put(vm);
> +	eq_check(fx, "teardown");
> +	eq_fini(fx);
> +}

[Severity: Low]
If a KUNIT_ASSERT_* fails during eq_device() or equiv_test_sgx545(), can
skipping eq_fini() leak the unmanaged allocations and leave sdev->reap_work
queued while KUnit frees sdev?

KUNIT_ASSERT_* aborts the test immediately without reaching eq_fini() at
the end of equiv_test_sgx545().  If an assertion fails after
pvrsgx_fw_start() has queued sdev->reap_work onto system_dfl_wq, KUnit
frees sdev without cancel_work_sync(&sdev->reap_work), which can cause a
use-after-free on sdev->reap_work in the worker in addition to leaking the
MMU page tables, reset_page, and firmware arenas.  Registering cleanup via
kunit_add_action_or_reset() would ensure teardown runs on assertion
failures too.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-powervr-sgx-v1-0-c69b84bac011@telaak.dev?part=13