Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Data race and unconditional kernel WARN_ON due to dynamic DRM property creation after connector registration. - [Medium] Luminance property updates are silently dropped when an atomic commit includes a modeset. Pre-existing issues: - [High] Atomic check and commit paths dereference a NULL CRTC state if a connector property is updated without the associated CRTC being pulled into the atomic state. -- commit 3fc3b85f7e3897f97fa8465e7158825ddd3e0dd7 Author: Mario Limonciello <[email protected]> drm/amd/display: use drm backlight Convert the AMD display driver to the DRM backlight infrastructure so brightness can be controlled through the connector LUMINANCE property. Link the registered backlight_device to the eDP connector and unlink it on teardown; the DRM core owns the embedded backlight state and its property, so no explicit allocation or failure handling is needed. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 284aac4d96bce..b5254c775a778 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -5048,6 +5048,9 @@ static void amdgpu_dm_atomic_commit_tail(struct > drm_atomic_commit *state) > if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state)) [Severity: High] This is a pre-existing issue, but does this code dereference a NULL pointer if a connector-only property is updated? When a connector-only property is updated, the DRM core stages the property but does not add the CRTC to the atomic state. The drm_atomic_get_new_crtc_state call returns NULL when the CRTC is not in the state. If userspace submits an atomic commit that updates only the newly exposed LUMINANCE property without modifying the CRTC, new_crtc_state will be NULL and immediately dereferenced inside drm_atomic_crtc_needs_modeset(). > continue; > > + drm_connector_update_privacy_screen(new_con_state); > + drm_atomic_helper_connector_apply_luminance(new_con_state); [Severity: Medium] Will this cause luminance property updates to be silently dropped when an atomic commit includes a modeset? The continue statement immediately above skips the newly added drm_atomic_helper_connector_apply_luminance() call if the CRTC needs a modeset (e.g., when changing display resolution). Since amdgpu_dm does not use standard DRM modeset helpers, the skipped luminance update would never be applied to the hardware, and the backlight would remain at its previous value. [ ... ] > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > index ed1f85650e280..0174339f0ee79 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > @@ -496,6 +497,9 @@ amdgpu_dm_register_backlight_device(struct > amdgpu_dm_connector *aconnector) > dm->actual_brightness[aconnector->bl_idx] = > real_brightness; > dm->brightness[aconnector->bl_idx] = real_brightness; > } > + /* Link the registered backlight device to the DRM connector. */ > + drm_backlight_link(&aconnector->base, > dm->backlight_dev[aconnector->bl_idx]); [Severity: High] Does this trigger a kernel WARN_ON and data race by dynamically creating the LUMINANCE property after connector registration? Because amdgpu_dm_register_backlight_device() is executed during the late registration phase (amdgpu_dm_connector_late_register), the connector has already been exposed to userspace. The drm_backlight_link() call creates the LUMINANCE property dynamically, but adding properties after registration is prohibited by the DRM core. Could this also cause a data race if userspace iterates the connector's properties array (e.g., reacting to a sysfs KOBJ_ADD uevent by issuing DRM_IOCTL_MODE_GETCONNECTOR) locklessly while it is being reallocated? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
