Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The patch fails to clear the Panel Replay error status because it evaluates a state variable (`psr->panel_replay_enabled`) that has already been cleared upon error detection. Pre-existing issues: - [High] If `drm_dp_dpcd_readb` reads 0 bytes, the driver falsely assumes success and uses uninitialized stack variables. -- commit 1da2d4e9ad55e4f8b758998afbe0f6630bae226f Author: Jake Steinman <[email protected]> drm/i915/psr: clear the Panel Replay error status register This commit aims to fix an issue where the Panel Replay error status register is never cleared on the sink. It updates intel_psr_short_pulse() to use the same conditional offset for clearing the error as is used for reading it. > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index 52836496040ff..1d0372c5f895f 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -3891,7 +3891,10 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp) > "PSR_ERROR_STATUS unhandled errors %x\n", > error_status & ~errors); [Severity: High] This is a pre-existing issue, but if drm_dp_dpcd_readb() reads 0 bytes, it returns 0. In psr_get_status_and_error_status(), the check if (ret != 1) return ret; will then return 0. Does this cause intel_psr_short_pulse() to wrongly treat the read as a success and proceed to use uninitialized stack variables status and error_status? > /* clear status register */ > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, error_status); > + drm_dp_dpcd_writeb(&intel_dp->aux, > + psr->panel_replay_enabled ? > + DP_PANEL_REPLAY_ERROR_STATUS : DP_PSR_ERROR_STATUS, > + error_status); [Severity: High] Does this code still write to the wrong register during an error condition? If an error is detected, the earlier check in intel_psr_short_pulse() calls intel_psr_disable_locked(): intel_psr_short_pulse() if ((!psr->panel_replay_enabled && status == DP_PSR_SINK_INTERNAL_ERROR) || (error_status & errors)) { intel_psr_disable_locked(intel_dp); ... } intel_psr_disable_locked() unconditionally clears the flag: intel_dp->psr.panel_replay_enabled = false; When we later attempt to clear the status register, psr->panel_replay_enabled will evaluate to false, causing the error status to be incorrectly written to DP_PSR_ERROR_STATUS instead of DP_PANEL_REPLAY_ERROR_STATUS. > > if (!psr->panel_replay_enabled) { > psr_alpm_check(intel_dp); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
