Re: [PATCH 7/9] drm/i915/de: Move unclaimed mmio detection into display code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/i915/de: NULL deref on displayless devices in unclaimed MMIO checks
- [Medium] drm/i915/de: intel_de_init() enables FPGA_DBG check on Ivy Bridge
- [Medium] drm/i915/de: unclaimed MMIO status is not cleared on initialization
- [Low] drm/i915/display: unused mmio.mmio_debug field in struct intel_display
--

commit 74c572411ef669251a5e2e347d14a7771407e433
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm/i915/de: Move unclaimed mmio detection into display code

This commit moves the FPGA_DBG and CLAIM_ER unclaimed MMIO access detection
from intel_uncore into the display code, protecting the tracking state with
display->mmio.lock.

> diff --git a/drivers/gpu/drm/i915/display/intel_de.c b/drivers/gpu/drm/i915/display/intel_de.c
> index 8b7a4e696ff96..0a51105e0fbad 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.c
> +++ b/drivers/gpu/drm/i915/display/intel_de.c

[ ... ]

> @@ -163,8 +164,127 @@ u16 intel_de_read16(struct intel_display *display, intel_reg_t reg)

[ ... ]

> +bool intel_de_unclaimed_mmio_detected(struct intel_display *display)
> +{
> +	bool ret;
> +
> +	if (!display->mmio.funcs->check_for_unclaimed_mmio)
> +		return false;
> +
> +	spin_lock_irq(&display->mmio.lock);
> +	ret = display->mmio.funcs->check_for_unclaimed_mmio(display);
> +	spin_unlock_irq(&display->mmio.lock);
> +
> +	return ret;
> +}
> +
> +bool intel_de_arm_unclaimed_mmio_detection(struct intel_display *display)
> +{
> +	bool ret = false;
> +
> +	if (!display->mmio.funcs->check_for_unclaimed_mmio)
> +		return false;

[Severity: High]
Can intel_de_unclaimed_mmio_detected() and
intel_de_arm_unclaimed_mmio_detection() dereference a NULL display->mmio.funcs
pointer on devices without display?

On displayless devices where intel_display_device_probe() takes the no_display
path (such as INTEL_IVB_Q_IDS, INTEL_ATS_M_IDS, or GMD_ID platforms where
GMD_ID_DISPLAY reads 0), HAS_DISPLAY(display) is false. During probe,
intel_display_device_info_runtime_init() skips
__intel_display_device_info_runtime_init():

drivers/gpu/drm/i915/display/intel_display_device.c:
intel_display_device_info_runtime_init() {
	if (HAS_DISPLAY(display))
		__intel_display_device_info_runtime_init(display);

Because intel_de_init() is only called from
__intel_display_device_info_runtime_init(), display->mmio.funcs remains NULL
and display->mmio.lock is never initialized.

Later, i915_drm_resume_early(), i915_pm_runtime_suspend(), and
i915_pm_runtime_resume() unconditionally call
intel_display_driver_mmio_resume_early() and
intel_display_driver_mmio_suspend_late(), which call
intel_de_unclaimed_mmio_detected() and intel_de_arm_unclaimed_mmio_detection()
and dereference display->mmio.funcs.

Additionally, if HAS_DISPLAY(display) is initially true so intel_de_init()
sets display->mmio.funcs = &ivb_mmio_funcs, and
__intel_display_device_info_runtime_init() subsequently jumps to
display_fused_off, display->mmio.funcs remains set while HAS_DISPLAY(display)
becomes false, causing suspend and resume to access FPGA_DBG even though
display is fused off.

Should intel_de_init() be called unconditionally (or display->mmio.funcs be
checked / reset when display is fused off), and should
intel_display_driver_mmio_suspend_late() and
intel_display_driver_mmio_resume_early() check HAS_DISPLAY(display)?

[ ... ]

> @@ -304,7 +470,11 @@ void intel_de_init(struct intel_display *display)
>  	spin_lock_init(&display->mmio.lock);
>  
>  	if (driver_is_i915()) {
> -		if (display->platform.valleyview || display->platform.cherryview)
> +		if (display->platform.valleyview || display->platform.cherryview)
> +			display->mmio.funcs = &vlv_mmio_funcs;
> +		else if (DISPLAY_VER(display) >= 7)
> +			display->mmio.funcs = &ivb_mmio_funcs;
> +		else if (DISPLAY_VER(display) >= 6)
>  			display->mmio.funcs = &snb_mmio_funcs;
>  		else
>  			display->mmio.funcs = &i9xx_mmio_funcs;

[Severity: Medium]
Does checking DISPLAY_VER(display) >= 7 in intel_de_init() unintentionally
enable FPGA_DBG unclaimed MMIO checks on Ivy Bridge?

Previously, intel_uncore_init_mmio() gated FPGA_DBG unclaimed MMIO detection
on HAS_FPGA_DBG_UNCLAIMED(display). In
drivers/gpu/drm/i915/display/intel_display_device.c, has_fpga_dbg is set on
hsw_desc and bdw_desc onwards, whereas ivb_display has ip.ver = 7 and
has_fpga_dbg = 0. Checking DISPLAY_VER(display) >= 7 assigns ivb_mmio_funcs to
Ivy Bridge as well, and leaves HAS_FPGA_DBG_UNCLAIMED(display) unused.

Should this check HAS_FPGA_DBG_UNCLAIMED(display) instead?

> @@ -314,4 +484,7 @@ void intel_de_init(struct intel_display *display)
>  		else
>  			display->mmio.funcs = &i9xx_mmio_funcs;
>  	}
> +
> +	if (display->mmio.funcs->check_for_unclaimed_mmio)
> +		display->mmio.unclaimed_mmio_check = 1;
>  }

[Severity: Medium]
Previously, intel_uncore_init_mmio() cleared any stale unclaimed MMIO bit left
set by BIOS or pre-boot firmware during initialization:

	/* clear out unclaimed reg detection bit */
	if (intel_uncore_unclaimed_mmio(uncore))
		drm_dbg(&i915->drm, "unclaimed mmio detected on uncore init, clearing\n");

With that removed and not replaced in intel_de_init(), if mmio_debug is
disabled by default, won't a stale FPGA_DBG_RM_NOCLAIM or CLAIM_ER bit from
boot firmware remain latched until intel_de_arm_unclaimed_mmio_detection()
runs at the end of the first modeset or in
intel_display_driver_mmio_suspend_late(), triggering a false-positive error or
falsely arming oneshot mmio_debug reporting?

> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index aded2af8c92a6..cb18ee040b5d6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -539,11 +539,18 @@ struct intel_display {
>  		 * serializes all display register accesses done
>  		 * using intel_de_{read,write}(), but not those
>  		 * done using intel_de_{read,write}_fw().
> +		 *
> +		 * The lock also protects mmio.mmio_debug and
> +		 * mmio.unclaimed_mmio_check.
>  		 */
>  		spinlock_t lock;
>  
>  		struct intel_uncore *uncore;
>  		const struct intel_mmio_funcs *funcs;
> +
> +		/* protected by mmio.lock */
> +		int mmio_debug;
> +		int unclaimed_mmio_check;
>  	} mmio;

[Severity: Low]
This isn't a bug, but display->mmio.mmio_debug is never used since
check_unclaimed_mmio_pre(), check_unclaimed_mmio_post(), and
intel_de_arm_unclaimed_mmio_detection() all use display->params.mmio_debug.
Should mmio.mmio_debug be dropped from struct intel_display?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104221842.18799-1-ville.syrjala@linux.intel.com?part=7