Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Medium] The brightness quirk mask is not applied to `backlight_millinits` for AMD AUX panels, causing firmware to write unmasked values during autonomous operations Pre-existing issues: - [High] Unprotected traversal of drm_mode_config.connector_list - [High] Unprotected access to `dm->dc->current_state` leading to potential Use-After-Free - [High] Potential division by zero in brightness conversion functions when firmware capabilities are anomalous - [High] Incomplete bounds validation during power module creation allows multi-display setups to trigger a division by zero -- commit 8608a59a12eb05fd14ffc0e7cb1af582e44dade9 Author: Andrei Rusu de Castro <[email protected]> Date: Wed Sep 2 22:31:59 2026 +0000 drm/amd/display: pass userspace brightness to power module The patch adjusts how brightness is passed to the power module in amdgpu_dm, keeping it as userspace percentage (millipercent) instead of converting to PWM domain first. This fixes domain mismatch issues and handles panel-specific brightness quirks for AMD AUX writes. Closes: https://sashiko.dev/#/patchset/[email protected]?part=3 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c [ ... ] > @@ -218,6 +218,39 @@ u32 convert_brightness_to_user(const struct > amdgpu_dm_backlight_caps *caps, > } > EXPORT_IF_KUNIT(convert_brightness_to_user); > > +static u32 convert_brightness_to_millipercent(const struct > amdgpu_dm_backlight_caps *caps, > + u32 brightness) > +{ > + unsigned int min, max; > + > + if (!get_brightness_range(caps, &min, &max) || max <= min) > + return 0; > + > + if (brightness >= max) > + return 100 * 1000; > + > + return DIV_ROUND_CLOSEST_ULL((u64)brightness * 100 * 1000, max); > +} > + > +STATIC_IFN_KUNIT > +u32 convert_brightness_for_power_module(const struct > amdgpu_dm_backlight_caps *caps, > + u32 user_brightness) > +{ > + u32 brightness; > + > + if (!caps) > + return user_brightness; > + > + if (!caps->aux_support) > + return convert_brightness_to_millipercent(caps, > user_brightness); > + > + brightness = convert_brightness_from_user(caps, user_brightness); [Severity: High] This is a pre-existing issue, but while convert_brightness_to_millipercent() properly guards against max <= min to avoid division by zero, the convert_brightness_from_user() function called here lacks these same guards. Does convert_brightness_from_user() unconditionally divide by max, risking a kernel panic if anomalous firmware sets max to zero? Similarly, does convert_brightness_to_user() risk dividing by zero when evaluating max - min? > + > + return brightness; > +} [ ... ] > @@ -262,7 +295,6 @@ void amdgpu_dm_backlight_set_level(struct > amdgpu_display_manager *dm, > bool rc = false, reallow_idle = false; > struct drm_connector *connector; > struct dc_stream_state *stream; > - unsigned int min, max; > > list_for_each_entry(connector, &dm->ddev->mode_config.connector_list, > head) { [Severity: High] This isn't a bug introduced by this patch, but is it safe to traverse the DRM connector_list using the raw list_for_each_entry macro without holding mode_config.mutex or using the drm_connector_list_iter API? Can a concurrent hotplug event removing a connector leave this loop traversing a freed pointer, resulting in a use-after-free? [ ... ] > @@ -285,12 +317,9 @@ void amdgpu_dm_backlight_set_level(struct > amdgpu_display_manager *dm, > /* update scratch register */ > if (bl_idx == 0) > amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, > dm->brightness[bl_idx]); > - brightness = convert_brightness_from_user(caps, dm->brightness[bl_idx]); > link = (struct dc_link *)dm->backlight_link[bl_idx]; > - > - /* Apply brightness quirk */ > - if (caps->brightness_mask) > - brightness |= caps->brightness_mask; > + brightness = convert_brightness_for_power_module(caps, > + > dm->brightness[bl_idx]); [Severity: High] This is a pre-existing issue, but further down in this function, dm_find_stream_with_link() is called before acquiring dc_lock. Inside dm_find_stream_with_link(), the dm->dc->current_state pointer is dereferenced and iterated over. Because this sysfs callback runs concurrently with atomic commits (which free the old state), is there a risk of a use-after-free crash by reading a stale current_state pointer before the lock is held? [ ... ] > --- a/drivers/gpu/drm/amd/display/modules/power/power.c > +++ b/drivers/gpu/drm/amd/display/modules/power/power.c > @@ -157,6 +157,8 @@ struct mod_power *mod_power_create(struct dc *dc, > init_params[inst].use_custom_backlight_caps; > core_power->bl_prop[inst].custom_backlight_caps_config_no = > > init_params[inst].custom_backlight_caps_config_no; > + core_power->bl_prop[inst].brightness_mask = > + init_params[inst].brightness_mask; > > // Do not allow less than 101 backlight levels > if (init_params[inst].num_backlight_levels < 101) [Severity: High] This isn't a bug introduced by this patch, but further down in mod_power_create(), a loop validates the brightness ranges for eDP instances: for (inst = 0; inst < edp_num; inst++) { if (core_power->bl_prop[inst].nits_range != 0 && ...) { is_brightness_range_valid = true; break; } } Does this premature break cause the loop to exit without verifying remaining instances? If panel 1 has a zero nits_range while panel 0 is valid, could this lead to a division by zero later in backlight_millinit_to_pwm_legacy() when it evaluates panel 1? [ ... ] > --- a/drivers/gpu/drm/amd/display/modules/power/power_abm.c > +++ b/drivers/gpu/drm/amd/display/modules/power/power_abm.c [ ... ] > @@ -1020,6 +1050,11 @@ void fill_backlight_level_params(struct core_power > *core_power, > > if (backlight_control_type == BACKLIGHT_CONTROL_AMD_AUX && !is_hdr) > backlight_level_params->control_type = BACKLIGHT_CONTROL_PWM; > + > + if (backlight_level_params->control_type == BACKLIGHT_CONTROL_PWM) > + backlight_level_params->backlight_pwm_u16_16 = > + backlight_apply_source_mask(core_power, backlight_pwm, > + panel_inst); > } [Severity: Medium] When backlight_level_params->control_type remains BACKLIGHT_CONTROL_AMD_AUX (for instance, during HDR playback), the brightness_mask is not applied to backlight_level_params->backlight_millinits here. Because these parameters are subsequently passed to the DMCUB firmware for autonomous operations like PSR/Replay exits, will this allow the firmware to write unmasked brightness values directly to the panel, bypassing the quirk? Should backlight_millinits also be masked here when the control type is AMD AUX? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
