From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/mode-config: Create drm_mode_config_create_initial_state() Date: Wed, 27 May 2026 14:30:52 +1000 Message-ID: In-Reply-To: <20260526-drm-mode-config-init-v6-16-852346394200@kernel.org> References: <20260526-drm-mode-config-init-v6-0-852346394200@kernel.org> <20260526-drm-mode-config-init-v6-16-852346394200@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the key patch introducing `drm_mode_config_create_initial_state()`. **Main concern =E2=80=94 no cleanup on partial failure:** If, say, plane st= ate creation succeeds for all planes but a CRTC state allocation fails, the= function returns an error but all the plane states that were already alloc= ated are leaked. The caller gets an error with no way to know which objects= got states. The `continue` for objects with existing states helps with re-= entrance, but doesn't solve the forward-leak problem. ```c drm_for_each_plane(plane, dev) { if (plane->state) continue; ret =3D drm_mode_config_plane_create_state(plane); if (ret) return ret; // leaked: all privobj and colorop states allocated ab= ove } ``` In practice, this is called during probe, so a failure here likely means th= e driver fails to load and the device gets torn down entirely (which would = clean up via `drm_mode_config_cleanup()`). So this may be acceptable, but i= t would be cleaner to document this expectation or add cleanup. The `if (privobj->state) continue` / `if (colorop->state) continue` guards = are good for handling the case where `drm_atomic_private_obj_init()` alread= y created the state, or where `drmm_connector_hdmi_init()` already set up a= connector state. The doc update to patch 1's lifetime documentation (splitting reset-time in= to init-time and resume-time) is correct. --- Generated by Claude Code Patch Reviewer