Re: [PATCH] drm/i915/pps: don't orphan the VDD wakeref when the HW force bit is reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch incompletely fixes the wakeref leak by failing to update the VDD off-paths to use the software wakeref as the authoritative record, leaving them vulnerable to the exact same asynchronous hardware state reset.
--

commit 18ac414fc1cf2489e00ff31b05c939f81f2c6a00
Author: Arun R Murthy <arun.r.murthy@intel.com>
drm/i915/pps: don't orphan the VDD wakeref when the HW force bit is reset

This patch attempts to fix a wakeref leak by ensuring the software wakeref
is not overwritten if the volatile hardware bit clears while we still hold
a reference. It updates intel_pps_vdd_on_unlocked to track the wakeref
authoritatively.

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/8477

> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index d4c98b150fa2b..43e89aaf419f6 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -758,9 +758,10 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp)
>  	if (edp_have_panel_vdd(intel_dp))
>  		return need_to_disable;

[Severity: High]
This is a pre-existing issue, but does relying on edp_have_panel_vdd()
here leave vdd_wakeref unbalanced if the hardware bit is spuriously set?

If the hardware EDP_FORCE_VDD bit is set asynchronously (e.g., by DMC, DC
states, or BIOS), edp_have_panel_vdd() will return true and bypass
acquiring the wakeref into intel_dp->pps.vdd_wakeref. When the panel is
later turned off, the off-paths might unconditionally drop a reference we
never took.

Additionally, does this fix leave the VDD off-paths vulnerable to the
exact same race with volatile hardware state?

For example, in intel_pps_vdd_off_sync_unlocked():

    if (!edp_have_panel_vdd(intel_dp))
        return;
        
If the hardware bit was reset asynchronously, this early return completely
skips calling intel_display_power_put(), permanently leaking vdd_wakeref.

Similarly, in intel_pps_init_late():

    if (edp_have_panel_vdd(intel_dp))
        edp_panel_vdd_schedule_off(intel_dp);

If the hardware bit cleared, this fails to schedule the delayed wakeref
cleanup, causing another leak.

Should the VDD off-paths also be updated to use vdd_wakeref as the
authoritative record rather than the volatile hardware bit?

> -	drm_WARN_ON(display->drm, intel_dp->pps.vdd_wakeref);
> -	intel_dp->pps.vdd_wakeref = intel_display_power_get(display,
> -							    intel_aux_power_domain(dig_port));
> +	if (!intel_dp->pps.vdd_wakeref)
> +		intel_dp->pps.vdd_wakeref =
> +			intel_display_power_get(display,
> +						intel_aux_power_domain(dig_port));
>  
>  	pp_stat_reg = _pp_stat_reg(intel_dp);
>  	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831044036.1236136-1-arun.r.murthy@intel.com?part=1