Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The refactoring silently drops error propagation when reading the ALPM DPCD capability, altering eDP initialization failure behavior despite the 'Non functional change' claim. - [Medium] The uninitialized stack variable `dpcd` can be assigned to `intel_dp->alpm_dpcd` if `drm_dp_dpcd_readb()` returns 0. --
--- Patch [2]: [PATCH v5 02/15] drm/i915/alpm: Move alpm sink capabality readout in separate function --- commit 7a8fa9f136849fe0967523fc134415ba1067f31f Author: Animesh Manna <[email protected]> drm/i915/alpm: Move alpm sink capabality readout in separate function This commit refactors the ALPM sink capability readout logic into a new standalone function, intel_alpm_init_dpcd(). > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index f1383764b7028..542900e29a014 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -43,6 +43,16 @@ bool intel_alpm_is_alpm_aux_less(struct intel_dp *intel_dp, > (crtc_state->has_lobf && > intel_alpm_aux_less_wake_supported(intel_dp)); > } > > +void intel_alpm_init_dpcd(struct intel_dp *intel_dp) > +{ > + u8 dpcd; > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, &dpcd) < 0) > + return; > + > + intel_dp->alpm_dpcd = dpcd; [Severity: Medium] If drm_dp_dpcd_readb() performs a short read and returns 0, the error check will evaluate to false. Does this result in assigning uninitialized stack memory from dpcd to intel_dp->alpm_dpcd? The previous API used here (drm_dp_dpcd_read_byte) explicitly checked for short reads and returned a negative error, which avoided this scenario. > +} > + [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 113d767e62e9c..bf626a1e189b1 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c [ ... ] > @@ -4810,11 +4809,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp, struct > intel_connector *connector > */ > intel_dp_init_source_oui(intel_dp); > > - /* Read the ALPM DPCD caps */ > - ret = drm_dp_dpcd_read_byte(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, > - &intel_dp->alpm_dpcd); > - if (ret < 0) > - return false; > + intel_alpm_init_dpcd(intel_dp); [Severity: Medium] The commit message states this is a non-functional change, but the previous code returned false and aborted eDP initialization if the AUX read failed. Since the new intel_alpm_init_dpcd() function returns void, does this inadvertently swallow the error and change the initialization failure behavior for eDP? > > /* > * This has to be called after intel_dp->edp_dpcd is filled, PSR checks -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
