Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Redundant call to drm_connector_unregister() in the connector destroy hook triggers a kernel WARN_ON during driver unload. - [Medium] LVDS connectors are missing the DRM backlight property allocation, leaving them without the new DRM-level brightness control capabilities. Pre-existing issues: - [High] Pre-existing error path Use-After-Free, Double Free, and memory leak when device initialization fails. -- commit 1adb86b656e02ccab8cb2193136ef89ff549cddf Author: Mario Limonciello (AMD) <[email protected]> Subject: drm/amd/display: use drm backlight This commit converts the AMD display driver to use the new DRM backlight infrastructure to allow brightness control via DRM connector properties. It also includes fixes for backlight resource lifecycle management. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c [ ... ] > @@ -8152,6 +8163,10 @@ static void amdgpu_dm_connector_destroy(struct > drm_connector *connector) > aconnector->dc_sink = NULL; > > drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux); > + /* DRM core will handle drm_backlight cleanup via: > + * drm_connector_unregister() -> drm_backlight_unregister() > + * drm_connector_cleanup() -> drm_backlight_free() > + */ > drm_connector_unregister(connector); > drm_connector_cleanup(connector); [Severity: High] Does this redundant unregister trigger a WARN_ON during driver unload? When the driver is removed, drm_dev_unregister() automatically calls drm_connector_unregister_all(), which unregisters the connector and its newly added backlight object via drm_backlight_unregister(). Later, when the DRM device is cleaned up and the connector reference drops to zero, amdgpu_dm_connector_destroy() is called. Because drm_connector_unregister() calls drm_backlight_unregister() before checking if the connector is already unregistered, it will attempt to unregister the backlight a second time. This asserts a warning because the backlight was already removed from the list. [ ... ] > @@ -9402,6 +9417,12 @@ void amdgpu_dm_connector_init_helper(struct > amdgpu_display_manager *dm, > aconnector->base.state->max_bpc = 16; > aconnector->base.state->max_requested_bpc = > aconnector->base.state->max_bpc; > > + if (connector_type == DRM_MODE_CONNECTOR_eDP) { > + int r = drm_backlight_alloc(&aconnector->base); > + > + if (r) > + drm_err(dm->ddev, "Failed to allocate backlight: %d\n", > r); > + } [Severity: Medium] Are LVDS connectors meant to be excluded from the new backlight allocation? The AMD display driver also supports backlight control for legacy LVDS panels, but drm_backlight_alloc() is explicitly restricted to eDP here. Since it is skipped for LVDS, the connector's backlight pointer remains NULL, and LVDS panels will not expose the new DRM luminance properties. [ ... ] While reviewing these changes, I also noticed an issue in the error handling path of amdgpu_dm_initialize_drm_device() in the same file: > fail: > kfree(aencoder); > kfree(aconnector); > > return -EINVAL; [Severity: High] This is a pre-existing issue, but can this error path lead to a use-after-free and double-free? If a late initialization step fails after a connector is successfully created and registered into the global drm_device connector list, the code jumps to the fail label and frees aconnector. This leaves a dangling pointer in the global DRM connector list. During the subsequent error rollback via drm_mode_config_cleanup(), the DRM core will iterate over the connector list and invoke amdgpu_dm_connector_destroy(), which accesses the freed memory and attempts to free it again. Additionally, the newly added backlight allocation would be leaked here. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
