Re: [PATCH v10 05/15] drm/amd/display: update KUnit luminance fixtures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/amd/display: wrong connector_type IDA freed in setup_test_connector() user
- [Medium] drm/amd/display: link failure in backlight KUnit test when DRM_LUMINANCE=y
--

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

drm/amd/display: update KUnit luminance fixtures

This commit updates the backlight KUnit test fixtures to initialize DRM mode
configuration, connector state, and the display manager lock so tests work
when the atomic LUMINANCE property is enabled.

> 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 b21850788f36f..8883010e8af9c 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
> @@ -41,12 +41,21 @@ static void setup_test_connector(struct kunit *test,
>  				 struct dm_backlight_connector_fixture *fixture,
>  				 int bl_idx, enum signal_type signal)
>  {
> -	fixture->adev = kunit_kzalloc(test, sizeof(*fixture->adev), GFP_KERNEL);
> -	fixture->aconnector = kunit_kzalloc(test, sizeof(*fixture->aconnector), GFP_KERNEL);
> +	int ret;
> +
> +	fixture->adev = dm_kunit_alloc_adev(test);
> +	ret = drmm_mode_config_init(&fixture->adev->ddev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	fixture->aconnector = dm_kunit_alloc_connector(test, fixture->adev, NULL);
>  	fixture->link = kunit_kzalloc(test, sizeof(*fixture->link), GFP_KERNEL);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->adev);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->aconnector);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->link);
> +	ret = drmm_connector_init(&fixture->adev->ddev,
> +				  &fixture->aconnector->base,
> +				  &dm_backlight_test_connector_funcs,
> +				  DRM_MODE_CONNECTOR_eDP, NULL);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	drm_atomic_helper_connector_reset(&fixture->aconnector->base);

[Severity: Medium]
Does hardcoding DRM_MODE_CONNECTOR_eDP in setup_test_connector() corrupt or
leak the connector_type_id IDA when dm_test_should_create_sysfs_non_edp() runs?

drmm_connector_init() -> drm_connector_init_and_add() ->
drm_connector_init_only() allocates connector->connector_type_id from
drm_connector_enum_list[DRM_MODE_CONNECTOR_eDP].ida and registers
drm_connector_cleanup_action() for managed teardown.

However, dm_test_should_create_sysfs_non_edp() mutates connector_type after
calling setup_test_connector():

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c:dm_test_should_create_sysfs_non_edp() {
	setup_test_connector(test, &fixture, 0, SIGNAL_TYPE_HDMI_TYPE_A);
	fixture.aconnector->base.connector_type = DRM_MODE_CONNECTOR_HDMIA;
	...
}

When the test finishes and drm_connector_cleanup() runs:

drivers/gpu/drm/drm_connector.c:drm_connector_cleanup() {
	...
	ida_free(&drm_connector_enum_list[connector->connector_type].ida,
		 connector->connector_type_id);
	...
}

it frees connector_type_id back to the DRM_MODE_CONNECTOR_HDMIA IDA instead of
DRM_MODE_CONNECTOR_eDP, triggering a warning in ida_free() (or freeing an
active HDMI-A ID) while leaking the eDP connector_type_id.

> @@ -483,12 +492,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);
>  
>  	setup_test_link_service(test, link);
>  	link->dc->link_srv->edp_get_backlight_level = dm_test_get_backlight_level_error;
>  	primary = kunit_kzalloc(test, sizeof(*primary), GFP_KERNEL);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, primary);
>  	adev->ddev.primary = primary;
> +	adev->dm.ddev = &adev->ddev;
>  	adev->dm.backlight_link[0] = link;

[Severity: Medium]
Can this test suite link when CONFIG_DRM_LUMINANCE=y and
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled?

With commit de26f6665aed ("drm/amd: add luminance property support"),
DRM_AMDGPU in drivers/gpu/drm/amd/amdgpu/Kconfig only selects
BACKLIGHT_CLASS_DEVICE when !DRM_LUMINANCE:

	select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE

Neither DRM_AMD_DC_KUNIT_TEST in drivers/gpu/drm/amd/display/Kconfig nor
drivers/gpu/drm/amd/display/amdgpu_dm/tests/.kunitconfig enables
CONFIG_BACKLIGHT_CLASS_DEVICE.

However, dm_test_register_backlight_device_success() and
dm_test_unregister_backlight_device() in amdgpu_dm_backlight_test.c, as well
as amdgpu_dm_register_backlight_device() in amdgpu_dm_backlight.c, still
unconditionally call backlight_device_register() and
backlight_device_unregister():

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c:dm_test_unregister_backlight_device() {
	backlight_device_unregister(data);
}

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c:amdgpu_dm_register_backlight_device() {
	...
	dm->backlight_dev[aconnector->bl_idx] =
		backlight_device_register(bl_name, aconnector->base.kdev, dm,
					  &amdgpu_dm_backlight_ops, &props);
	...
}

Because include/linux/backlight.h does not provide inline stubs for
backlight_device_register() or backlight_device_unregister() when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled, won't building the KUnit tests (or
amdgpu) with CONFIG_DRM_LUMINANCE=y and CONFIG_BACKLIGHT_CLASS_DEVICE=n fail
at link time with undefined references to those symbols?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=5