Re: [PATCH v8 10/14] drm/amd/display: Update KUnit backlight tests for luminance property and fixtures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mock connectors bypass standard DRM initialization, leading to missing cleanup and leaving dangling pointers on the global `drm_backlight_list`.
- [Medium] Double initialization of `mode_config` via `drmm_mode_config_init()` overwrites lists, leaks DRM standard properties, and double-registers cleanup actions.
--

commit a56dca4aad815c6c0a1a6a861da4fc03fba94e83
Author: Mario Limonciello (AMD) <superm1@kernel.org>

    drm/amd/display: Update KUnit backlight tests for luminance property and fixtures

    Initialize backlight connector state and display manager dc_lock in
    KUnit connector and adev allocators, and update property count
    expectations in tests to account for the newly attached LUMINANCE property.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> index e03173d4e5bd8..c5f65e9a90ca8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> @@ -52,6 +52,7 @@ static void setup_test_connector(struct kunit *test,
>  	fixture->aconnector->dc_link = fixture->link;
>  	fixture->aconnector->base.dev = &fixture->adev->ddev;
>  	fixture->link->connector_signal = signal;
> +	drm_backlight_connector_init(&fixture->aconnector->base);
>  }

[Severity: High]
Does this bypass standard DRM initialization? By manually initializing the
backlight fields here (and in dm_kunit_alloc_connector()) without registering
a standard DRM lifecycle cleanup action, the mock connector avoids
drm_connector_cleanup(). 

When tests like dm_test_register_backlight_device_success() later call
amdgpu_dm_register_backlight_device() and link it into drm_backlight_list, does
the lack of cleanup leave a dangling pointer on the global list after the KUnit
test finishes and frees the mock connector?

[ ... ]
> @@ -483,12 +484,17 @@ static void dm_test_register_backlight_device_success(struct kunit *test)
>  	struct dc_link *link = dm_kunit_alloc_link(test);
>  	struct drm_minor *primary;
>  	unsigned int max;
> +	int ret;
> +
> +	ret = drmm_mode_config_init(&adev->ddev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
Is this call to drmm_mode_config_init() redundant and potentially harmful?

The DRM device adev is allocated via dm_kunit_alloc_adev(), which uses
__drm_kunit_helper_alloc_drm_device() with DRIVER_MODESET. This implies
the mode config is already initialized. Calling it again zeroes out lists
like property_list (using INIT_LIST_HEAD) without freeing previously
allocated standard properties, which leads to a memory leak. 

Furthermore, does it register a duplicate drm_mode_config_init_release action,
leading to double teardown attempts on the same structures?

A similarly problematic duplicate call also appears to be present in
dm_test_setup_backlight_device_attaches_abm_property().

[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
> index aad122c7229f5..ed5045c948fe9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
[ ... ]
> @@ -172,6 +177,7 @@ struct amdgpu_dm_connector *dm_kunit_alloc_connector(struct kunit *test,
>  	if (adev)
>  		aconnector->base.dev = &adev->ddev;
>  	aconnector->dc_link = link;
> +	drm_backlight_connector_init(&aconnector->base);
>  
>  	return aconnector;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908044035.62093-1-mario.limonciello@amd.com?part=10