Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/amd/display: partial init and NULL deref when atomic_create_state() fails - [Medium] drm/amd/display: ERR_PTR deref on cleanup in connector KUnit tests --
commit a996d78c707775b6da542d46e144536aa2b44c5d Author: Maxime Ripard <[email protected]> drm/amdgpu: dm: Convert to atomic_create_state This commit converts the amdgpu_dm connector state initialization from the drm_connector_funcs.reset hook to drm_connector_funcs.atomic_create_state across connectors, MST connectors, writeback connectors, and KUnit tests. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > index 3a4e98b08ac1f..e59595b5e39b2 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c [ ... ] > @@ -3199,8 +3195,16 @@ void amdgpu_dm_connector_init_helper(struct > amdgpu_display_manager *dm, > * Some of the properties below require access to state, like bpc. > * Allocate some default initial connector state with our reset helper. > */ > - if (aconnector->base.funcs->reset) > - aconnector->base.funcs->reset(&aconnector->base); > + if (!aconnector->base.state && > + aconnector->base.funcs->atomic_create_state) { > + struct drm_connector_state *conn_state; > + > + conn_state = > aconnector->base.funcs->atomic_create_state(&aconnector->base); > + if (IS_ERR(conn_state)) > + return; > + > + aconnector->base.state = conn_state; > + } > > aconnector->connector_id = link_index; > aconnector->bl_idx = -1; [Severity: High] What happens in the callers of amdgpu_dm_connector_init_helper() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c when atomic_create_state() fails with ERR_PTR(-ENOMEM) and returns early? Returning early from this void function skips initializing aconnector->dc_link, aconnector->bl_idx = -1, aconnector->audio_inst = -1, aconnector->hpd_lock, aconnector->handle_mst_msg_ready, and connector properties, while leaving aconnector->base.state as NULL without reporting an error to the caller. In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c, amdgpu_dm_connector_init() continues execution assuming the connector was initialized: amdgpu_dm_connector_init_helper( dm, aconnector, connector_type, link, link_index); ... if (dc_is_dp_signal(link->connector_signal)) amdgpu_dm_initialize_dp_connector(dm, aconnector, link->link_index); For DP or eDP connectors, amdgpu_dm_connector_init() -> amdgpu_dm_initialize_dp_connector() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c immediately dereferences aconnector->dc_link while it is still NULL: aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc; For other connectors, amdgpu_dm_mode_config_init() proceeds to call amdgpu_dm_update_connector_after_detect(), which also dereferences aconnector->dc_link->local_sink. In addition, if amdgpu_dm_connector_destroy() runs on this partially initialized connector, aconnector->bl_idx is still 0 rather than -1, so it will unregister dm->backlight_dev[0]. Similarly, in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c, dm_dp_add_mst_connector() calls amdgpu_dm_connector_init_helper() and then calls drm_connector_attach_max_bpc_property(connector, 8, 16), which dereferences connector->state while it is NULL: amdgpu_dm_connector_init_helper( &adev->dm, aconnector, DRM_MODE_CONNECTOR_DisplayPort, master->dc_link, master->connector_id); ... connector->max_bpc_property = master->base.max_bpc_property; if (connector->max_bpc_property) drm_connector_attach_max_bpc_property(connector, 8, 16); Along the same lines, in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c, amdgpu_dm_wb_connector_init() ignores an ERR_PTR return from amdgpu_dm_connector_atomic_create_state() and returns 0 with connector->state left as NULL: conn_state = amdgpu_dm_connector_atomic_create_state(connector); if (!IS_ERR(conn_state)) { if (connector->state) connector->funcs->atomic_destroy_state(connector, connector->state); connector->state = conn_state; } Should amdgpu_dm_connector_init_helper() and amdgpu_dm_wb_connector_init() propagate the allocation error to their callers so connector initialization can fail cleanly? [ ... ] > diff --git > a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c > index 8117aaf41f220..ebf8155326cb1 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c > @@ -1468,19 +1468,19 @@ static void > dm_test_decide_crtc_timing_no_crtc_clock(struct kunit *test) > KUNIT_EXPECT_EQ(test, drm_mode.crtc_hdisplay, 0); > } > > -/* Tests for amdgpu_dm_connector_funcs_reset() */ > +/* Tests for amdgpu_dm_connector_atomic_create_state() */ > > static const struct drm_connector_funcs dm_test_connector_funcs = { > - .reset = amdgpu_dm_connector_funcs_reset, > + .atomic_create_state = amdgpu_dm_connector_atomic_create_state, > .atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; > > /** > - * dm_test_funcs_reset_sets_defaults - Test funcs_reset sets defaults > + * dm_test_atomic_create_sets_defaults - Test funcs_reset sets defaults > * @test: The KUnit test context > */ > -static void dm_test_funcs_reset_sets_defaults(struct kunit *test) > +static void dm_test_atomic_create_sets_defaults(struct kunit *test) > { > struct device *dev; > struct drm_device *drm; > @@ -1501,9 +1501,9 @@ static void dm_test_funcs_reset_sets_defaults(struct > kunit *test) > drmm_connector_init(drm, connector, &dm_test_connector_funcs, > DRM_MODE_CONNECTOR_DisplayPort, NULL); > > - amdgpu_dm_connector_funcs_reset(connector); > + connector->state = amdgpu_dm_connector_atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state); > > - KUNIT_ASSERT_NOT_NULL(test, connector->state); > dm_state = to_dm_connector_state(connector->state); > KUNIT_EXPECT_EQ(test, (int)dm_state->scaling, (int)RMX_OFF); > KUNIT_EXPECT_FALSE(test, dm_state->underscan_enable); > @@ -1515,10 +1515,10 @@ static void dm_test_funcs_reset_sets_defaults(struct > kunit *test) > } > > /** > - * dm_test_funcs_reset_edp_abm_level - Test funcs_reset eDP sets ABM > + * dm_test_atomic_create_edp_abm_level - Test funcs_reset eDP sets ABM > * @test: The KUnit test context > */ > -static void dm_test_funcs_reset_edp_abm_level(struct kunit *test) > +static void dm_test_atomic_create_edp_abm_level(struct kunit *test) > { > struct device *dev; > struct drm_device *drm; > @@ -1542,9 +1542,10 @@ static void dm_test_funcs_reset_edp_abm_level(struct > kunit *test) > > /* Test with abm_level > 0 */ > amdgpu_dm_set_abm_level_param(3); > - amdgpu_dm_connector_funcs_reset(connector); > > - KUNIT_ASSERT_NOT_NULL(test, connector->state); > + connector->state = amdgpu_dm_connector_atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state); > + > dm_state = to_dm_connector_state(connector->state); > KUNIT_EXPECT_EQ(test, (int)dm_state->abm_level, 3); > > @@ -1552,11 +1553,11 @@ static void dm_test_funcs_reset_edp_abm_level(struct > kunit *test) > } > > /** > - * dm_test_funcs_reset_edp_abm_disabled - Test funcs_reset eDP ABM > + * dm_test_atomic_create_edp_abm_disabled - Test funcs_reset eDP ABM > * disabled > * @test: The KUnit test context > */ > -static void dm_test_funcs_reset_edp_abm_disabled(struct kunit *test) > +static void dm_test_atomic_create_edp_abm_disabled(struct kunit *test) > { > struct device *dev; > struct drm_device *drm; > @@ -1580,9 +1581,10 @@ static void > dm_test_funcs_reset_edp_abm_disabled(struct kunit *test) > > /* Test with abm_level <= 0 → immediate disable */ > amdgpu_dm_set_abm_level_param(-1); > - amdgpu_dm_connector_funcs_reset(connector); > > - KUNIT_ASSERT_NOT_NULL(test, connector->state); > + connector->state = amdgpu_dm_connector_atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state); > + > dm_state = to_dm_connector_state(connector->state); > KUNIT_EXPECT_EQ(test, (int)dm_state->abm_level, > (int)ABM_LEVEL_IMMEDIATE_DISABLE); > @@ -1620,8 +1622,8 @@ static void > dm_test_atomic_dup_state_copies_fields(struct kunit *test) > drmm_connector_init(drm, connector, &dm_test_connector_funcs, > DRM_MODE_CONNECTOR_HDMIA, NULL); > > - amdgpu_dm_connector_funcs_reset(connector); > - KUNIT_ASSERT_NOT_NULL(test, connector->state); > + connector->state = amdgpu_dm_connector_atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state); > > /* Modify original state fields */ > dm_state = to_dm_connector_state(connector->state); [Severity: Medium] In dm_test_atomic_create_sets_defaults(), dm_test_atomic_create_edp_abm_level(), dm_test_atomic_create_edp_abm_disabled(), and dm_test_atomic_dup_state_copies_fields() in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c, can assigning directly to connector->state before KUNIT_ASSERT_NOT_ERR_OR_NULL() cause an error pointer dereference during test teardown? Each of these tests initializes the connector with drmm_connector_init(), which registers drm_connector_cleanup() as a managed cleanup action. If amdgpu_dm_connector_atomic_create_state() returns ERR_PTR(-ENOMEM), connector->state holds that non-NULL error pointer when KUNIT_ASSERT_NOT_ERR_OR_NULL() aborts the test, and drm_connector_cleanup() in drivers/gpu/drm/drm_connector.c will then pass it to atomic_destroy_state(): if (connector->state && connector->funcs->atomic_destroy_state) connector->funcs->atomic_destroy_state(connector, connector->state); Would it be safer to store the return value in a local conn_state variable and assert KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state) before assigning connector->state, as done in the other tests in this file? [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-no-more-connector-reset-v2-0-0b7e4e85b...@kernel.org?part=53
