Re: [PATCH] drm/i915/psr: clear the Panel Replay error status register

[sashiko-bot@kernel.org]

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 <j@metarealtyinc.ca>
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/20260901203633.92628-1-j@metarealtyinc.ca?part=1