Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/amdgpu: tests store ERR_PTR in connector->state leading to cleanup crash - [Low] drm: WARN_ON triggered during test connector init due to missing atomic hooks --
commit 4e29a193c910088812846e00f2f613afc6ebffa4 Author: Maxime Ripard <[email protected]> drm/amdgpu: dm: Switch to drm_atomic_helper_connector_create_state The amdgpu: dm connector implementation uses the deprecated drm_atomic_helper_connector_reset() as its reset hook. Switch to drm_atomic_helper_connector_create_state() instead. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_crc_test.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_crc_test.c > index 29a513dab9cd8..e47b4dd210b8d 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_crc_test.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_crc_test.c [ ... ] > @@ -654,8 +654,9 @@ static void > dm_test_crtc_set_crc_source_dprx_no_connector(struct kunit *test) > DRM_MODE_CONNECTOR_WRITEBACK), > 0); > KUNIT_ASSERT_EQ(test, kunit_add_action_or_reset(test, > dm_test_crc_connector_cleanup, wb_conn), 0); > - drm_atomic_helper_connector_reset(wb_conn); > - KUNIT_ASSERT_NOT_NULL(test, wb_conn->state); > + > + wb_conn->state = drm_atomic_helper_connector_create_state(wb_conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, wb_conn->state); [Severity: Medium] Can this assignment introduce a regression by causing a crash in the deferred KUnit cleanup path if memory allocation fails? In dm_test_crtc_set_crc_source_dprx_no_connector(), if drm_atomic_helper_connector_create_state() fails, it returns ERR_PTR(-ENOMEM). Because it's directly assigned to wb_conn->state, the assertion KUNIT_ASSERT_NOT_ERR_OR_NULL(test, wb_conn->state) will fail, aborting the test. This test abortion jumps to KUnit's deferred cleanup routines. The previously registered cleanup action dm_test_crc_connector_cleanup() invokes drm_connector_cleanup(wb_conn). Since wb_conn->state contains ERR_PTR(-ENOMEM), which is not NULL, drm_connector_cleanup() will consider it a valid pointer and pass it to drm_atomic_helper_connector_destroy_state(). This will blindly dereference the error pointer, leading to an invalid memory access. Should the return value be validated in a local variable before being assigned to wb_conn->state to ensure it remains NULL on failure? > diff --git > a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_helpers_test.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_helpers_test.c > index 51c3995de76d2..a369a3cbd4afa 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_helpers_test.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_helpers_test.c [ ... ] > @@ -4106,7 +4106,7 @@ static void > dm_test_is_dp_sink_present_queries_link_service(struct kunit *test) > * device pointer, so build a real one here. > */ > static const struct drm_connector_funcs dm_test_connector_funcs = { > - .reset = drm_atomic_helper_connector_reset, > + .atomic_create_state = drm_atomic_helper_connector_create_state, > }; [Severity: Low] Will this missing hook introduce a regression by triggering a kernel warning during test connector initialization? The KUnit test allocates a DRM device with the DRIVER_ATOMIC feature flag. When dm_test_alloc_real_connector() initializes the connector using drmm_connector_init(), it calls into drm_connector_init_only(). Because dm_test_connector_funcs omits both the .atomic_destroy_state and .atomic_duplicate_state hooks, this explicitly violates the core DRM check: WARN_ON(drm_drv_uses_atomic_modeset(dev) && (!funcs->atomic_destroy_state || !funcs->atomic_duplicate_state)) This causes a warning to fire unconditionally during KUnit test execution. Should we add the required hooks to satisfy the API contract for this test connector? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261014-drm-no-more-connector-reset-v1-0-586c9c026...@kernel.org?part=4
