Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/i915/alpm: DP 2.1 ALPM LTTPR logic is unreachable dead code - [Medium] drm/i915/alpm: Unchecked negative return from drm_dp_lttpr_count() causes register corruption - [Low] drm/i915/alpm: Dead code in lttpr_count error check - [Low] drm/i915/alpm: Comment contradicts LTTPR revision code check --
commit a317ca0f7af6803f7b3932311f82447dfda1b648 Author: Animesh Manna <[email protected]> drm/i915/alpm: Program LTTPR count for DP 2.1 ALPM This commit programs the total number of LTTPRs for DP 2.1 ALPM before link training by issuing an AUX write transaction to DP_TOTAL_LTTPR_CNT. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index da653a407e97e..06c1a59aa2c51 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > @@ -726,7 +726,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp, > { > struct intel_display *display = to_intel_display(intel_dp); > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > - u32 alpm_ctl; > + u32 alpm_ctl, alpm_ctl2, lttpr_count; > [ ... ] > @@ -762,6 +762,11 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp, > > alpm_ctl |= > ALPM_CTL_ALPM_ENTRY_CHECK(crtc_state->alpm_state.check_entry_lines); > > + lttpr_count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps); > + > + alpm_ctl2 = ALPM_CTL2_NUMBER_OF_LTTPR(lttpr_count); [Severity: Medium] Does this assignment to a u32 variable cause error codes to go unhandled? The function drm_dp_lttpr_count() can return negative error codes like -ERANGE or -EINVAL on failure. If this happens, assigning it to the unsigned lttpr_count variable strips the negative sign and results in a large value being passed into ALPM_CTL2_NUMBER_OF_LTTPR(). This could corrupt the register programming by injecting invalid bits via REG_FIELD_PREP. > + > + intel_de_write(display, ALPM_CTL2(display, cpu_transcoder), alpm_ctl2); > intel_de_write(display, ALPM_CTL(display, cpu_transcoder), alpm_ctl); > mutex_unlock(&intel_dp->alpm.lock); > } > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > index cb92cff906146..37c8306a1fa0d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -2265,6 +2265,25 @@ intel_dp_128b132b_link_train(struct intel_dp *intel_dp, > return passed; > } > > +static void intel_dp_update_lttpr_count(struct intel_dp *intel_dp, > + int lttpr_count) > +{ > + u8 val; > + > + /* > + * Program only for DP2.1 and return if LTTPR revison is less than 0x20 > + * where 0:3 represents minor rev and 4:7 represent major rev > + */ [Severity: Low] This isn't a bug, but the comment specifies "less than 0x20", while the code below implements a less than or equal to 0x20 (<= 0x20) check. > + if (!intel_dp_is_edp(intel_dp) || > + lttpr_count < 0 || [Severity: Low] Is this lttpr_count < 0 check reachable? In intel_dp_start_link_train(), the lttpr_count is clamped to 0 on error before being passed to this function: if (lttpr_count < 0) /* Still continue with enabling the port and link training. */ lttpr_count = 0; intel_dp_prepare_link_train(intel_dp, crtc_state); intel_dp_update_lttpr_count(intel_dp, lttpr_count); > + intel_dp->lttpr_common_caps[0] <= 0x20) [Severity: High] Can this LTTPR programming ever be reached for either DP or eDP configurations? For standard DP connections, the function returns immediately because of the !intel_dp_is_edp(intel_dp) check above. For eDP connections, intel_dp_init_lttpr_and_dprx_caps() explicitly skips LTTPR capabilities initialization, leaving intel_dp->lttpr_common_caps[0] as 0. That causes the function to also return immediately due to this <= 0x20 check, rendering the feature unreachable. > + return; > + > + val = intel_dp->lttpr_common_caps[DP_PHY_REPEATER_CNT - > + > DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV]; > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TOTAL_LTTPR_CNT, val); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
