Re: [PATCHv2] drm/i915/pps: reconcile a BIOS-enabled VDD without leaking its wakeref

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Early return in intel_pps_vdd_off_sync_unlocked() skips wakeref cleanup and permanently leaks vdd_wakeref if the hardware VDD bit is reset externally.
--

commit 74c15e0ea248699dfcba4c3a0009676b770c8241
Author: Arun R Murthy <arun.r.murthy@intel.com>
Subject: drm/i915/pps: reconcile a BIOS-enabled VDD without leaking its wakeref

When the BIOS leaves the eDP VDD force bit enabled at boot, pps_vdd_init()
adopts that state by taking an AUX power-domain reference into vdd_wakeref.
This commit explicitly tracks this handover to prevent blindly reusing or
leaking the wakeref when the HW force bit is reset under the driver.

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..4152880774395 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -758,9 +758,22 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp)
>  	if (edp_have_panel_vdd(intel_dp))
>  		return need_to_disable;
>  
> -	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));
> +	/*
> +	 * pps_vdd_init() may have taken a reference at the BIOS->driver handover
> +	 * for an already-on VDD whose HW force bit was since reset under us (DC
> +	 * states, DMC, BIOS). Only in that boot/resume handover case reuse the
> +	 * held reference instead of overwriting and leaking it. At runtime a
> +	 * held wakeref here is a real imbalance, so keep asserting it and depend
> +	 * on the HW read.
> +	 */
> +	if (intel_dp->pps.vdd_wakeref_boot) {
> +		intel_dp->pps.vdd_wakeref_boot = false;
> +	} else {
> +		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));

[Severity: High]
This is a pre-existing issue, but does this overwrite and permanently leak
the runtime vdd_wakeref if the hardware VDD bit is reset externally?

As the commit message acknowledges, the hardware VDD bit can be cleared
externally by DMC, DC states, or BIOS independently of the driver.

If this happens while a runtime wakeref is held, and the delayed VDD off
timer expires, intel_pps_vdd_off_sync_unlocked() appears to return early
without dropping the wakeref:

drivers/gpu/drm/i915/display/intel_pps.c:intel_pps_vdd_off_sync_unlocked() {
    ...
    if (!edp_have_panel_vdd(intel_dp))
        return;
    ...
}

If intel_pps_vdd_on_unlocked() is subsequently called, it triggers this
WARN_ON and overwrites the non-NULL wakeref pointer with a new allocation.
This makes the original leaked wakeref unrecoverable.

Is there a way to safely clean up the existing runtime wakeref before
allocating a new one?

> +	}
>  
>  	pp_stat_reg = _pp_stat_reg(intel_dp);
>  	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);

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