Re: [PATCH 1/9] drm/i915/de: Introduce dislay->mmio and intel_de_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915/de: NULL display->mmio.uncore when intel_de_init() is skipped
--

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

drm/i915/de: Introduce dislay->mmio and intel_de_init()

This commit adds a display->mmio struct and intel_de_init() to initialize
display->mmio.uncore, and updates __to_uncore() to return display->mmio.uncore
instead of calling to_intel_uncore(display->drm).

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index d0a239a369807..f1135ee5071b4 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -14,9 +14,11 @@
>  
>  static inline struct intel_uncore *__to_uncore(struct intel_display *display)
>  {
> -	return to_intel_uncore(display->drm);
> +	return display->mmio.uncore;
>  }

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> index c74256359c253..f1bd1252a9602 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -1798,6 +1798,13 @@ static void __intel_display_device_info_runtime_init(struct intel_display *displ
>  	struct intel_display_runtime_info *display_runtime = DISPLAY_RUNTIME_INFO(display);
>  	enum pipe pipe;
>  
> +	/*
> +	 * TODO end of intel_display_device_probe() would be a more
> +	 * logical place, but at the moment i915 calls that super
> +	 * early when things aren't fully set up yet.
> +	 */
> +	intel_de_init(display);

[Severity: High]
Can this leave display->mmio.uncore NULL and cause a NULL pointer dereference
when HAS_DISPLAY(display) is already false before runtime init?

In drivers/gpu/drm/i915/display/intel_display_device.c,
intel_display_device_info_runtime_init() only calls
__intel_display_device_info_runtime_init() when HAS_DISPLAY(display) is true:

void intel_display_device_info_runtime_init(struct intel_display *display)
{
	if (HAS_DISPLAY(display))
		__intel_display_device_info_runtime_init(display);

On devices where intel_display_device_probe() sets DISPLAY_INFO() to
no_display—such as Ivybridge Quanta server SKUs (INTEL_IVB_Q_IDS via
has_no_display()), PCI IDs without an entry in intel_display_ids like Arctic
Sound-M (INTEL_ATS_M_IDS where find_platform_desc() returns NULL), or GMD_ID
platforms (INTEL_MTL_IDS / INTEL_ARL_IDS) where probe_gmdid_display() reads 0
from GMD_ID_DISPLAY—HAS_DISPLAY(display) is false and
__intel_display_device_info_runtime_init() is skipped.

Because __to_uncore(display) in drivers/gpu/drm/i915/display/intel_de.h now
returns display->mmio.uncore instead of computing
to_intel_uncore(display->drm), any subsequent unconditional intel_de_*() call
will pass a NULL struct intel_uncore pointer to intel_uncore_read*() or
intel_uncore_write*():

1. On INTEL_IVB_Q_IDS, ilk_irq_handler() in drivers/gpu/drm/i915/i915_irq.c
   unconditionally calls ilk_display_irq_master_disable() and
   ilk_display_irq_master_enable() in
   drivers/gpu/drm/i915/display/intel_display_irq.c on every interrupt, which
   call intel_de_read_fw(display, DEIER) and intel_de_write_fw(display, DEIER,
   ...).

2. On INTEL_ATS_M_IDS and headless GMD_ID platforms, gen11_irq_handler() and
   dg1_irq_handler() unconditionally call gen11_gu_misc_irq_ack() in
   drivers/gpu/drm/i915/display/intel_display_irq.c when GEN11_GU_MISC_IRQ is
   set, which calls intel_de_read_fw(display, GEN11_GU_MISC_IIR) and
   intel_de_write_fw(display, GEN11_GU_MISC_IIR, iir).

3. During system resume, i915_drm_resume() in
   drivers/gpu/drm/i915/i915_driver.c unconditionally calls
   intel_gmbus_reset(display) in drivers/gpu/drm/i915/display/intel_gmbus.c,
   which calls intel_de_write_fw(display, GMBUS0(display), 0) and
   intel_de_write_fw(display, GMBUS4(display), 0).

Should intel_de_init(display) be called unconditionally at the start of
intel_display_device_info_runtime_init() before the HAS_DISPLAY(display)
check?

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