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) <[email protected]> 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-b2a6f2413...@kernel.org?part=5
