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ä <[email protected]> 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/[email protected]?part=7
