Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Flawed logical OR (||) for LTTPR ALPM capabilities and incomplete feature integration. - [Medium] DPCD capability registers are evaluated without applying capability bitmasks. --
--- Patch [12]: [PATCH v5 12/15] drm/i915/alpm: Replace is_edp() with alpm_is_possible() --- commit 7013cc56cbeba3729d838fd8bd915ecee9e1de24 Author: Animesh Manna <[email protected]> drm/i915/alpm: Replace is_edp() with alpm_is_possible() Add a separate alpm_is_possible() which will check for both edp and dp. Cc: Jouni Högander <[email protected]> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index d23f3bdf13399..f3f6f40f96244 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -72,6 +72,16 @@ void intel_alpm_init(struct intel_dp *intel_dp) > mutex_init(&intel_dp->alpm.lock); > } > > +bool intel_alpm_is_possible(struct intel_dp *intel_dp) > +{ > + struct intel_display *display = to_intel_display(intel_dp); > + > + return (DISPLAY_VER(display) >= 12 && intel_dp->alpm_dpcd) || > + (DISPLAY_VER(display) >= 35 && > + intel_dp->lttpr_common_caps[DP_LTTPR_ALPM_CAPABILITIES - > + > DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV]); > +} [Severity: High] Should this logical OR (||) be an AND (&&) for LTTPR capabilities? By using OR, if the LTTPR supports ALPM but the sink does not (or vice versa), this function will return true. Could this lead to mismatched configurations? For example, if the LTTPR supports ALPM but the sink doesn't, intel_alpm_enable_sink() will write DP_ALPM_MODE_AUX_LESS to the sink's DP_RECEIVER_ALPM_CONFIG register even though the sink lacks support. Additionally, downstream functions like intel_alpm_lobf_compute_config() exclusively check the sink's capability. Does using a logical OR here effectively render the LTTPR check dead code in those paths? [Severity: Medium] Is it safe to evaluate the raw DPCD capability bytes (intel_dp->alpm_dpcd and lttpr_common_caps) directly as booleans? Since DPCD 0x02e is a bitfield containing multiple distinct bits (like DP_ALPM_PM_STATE_2A_SUPPORT), a non-zero value doesn't necessarily mean basic ALPM capability is supported. Should this use existing helpers like intel_alpm_aux_wake_supported() or explicitly apply the DP_ALPM_CAP bitmasks to prevent falsely detecting ALPM support on reserved bits? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
