Re: [PATCH v2 15/56] drm/i915/intel_hdmi: Add helper to disable FRL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915: NULL pointer deref in intel_hdmi_frl_ltsl() on systems with broken VBT
- [Low] drm/i915: unused crtc_state parameter in intel_hdmi_frl_ltsl()
--

commit bf532cf416aeac573dbb2724089b2e009ada86f1
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

drm/i915/intel_hdmi: Add helper to disable FRL

This commit adds a function to reset the SCDC FRL configuration and
Transcoder FRL configuration registers.

> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
[ ... ]
> @@ -3824,3 +3824,31 @@ int intel_hdmi_start_frl(struct intel_encoder *encoder,
>  
>  	return -EINVAL;
>  }
> +
> +static void intel_hdmi_frl_ltsl(struct intel_encoder *encoder,
> +				const struct intel_crtc_state *crtc_state)

[Severity: Low]
Is the crtc_state parameter needed here? Looking at the function body, it
appears to be passed in but never referenced. Removing it might help clean
up the function signature.

> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	struct i2c_adapter *adapter =
> +		intel_gmbus_get_adapter(display, intel_hdmi_ddc_pin(encoder));
> +
> +	drm_scdc_disable_frl(adapter);

[Severity: High]
Could this lead to a NULL pointer dereference on systems with a misconfigured
VBT?

If a system has a missing or conflicting DDC pin, intel_hdmi_ddc_pin() can
return 0. Consequently, intel_hdmi_init_connector() fails and no HDMI
connector is registered.

However, if the BIOS left the port enabled in HDMI FRL mode, the driver's
hardware state readout (intel_hdmi_frl_get_config()) will still set
crtc_state->frl.enable = true because it unconditionally trusts the hardware
register state.

When intel_sanitize_encoder() later cleans up this connector-less encoder,
it calls the newly added intel_hdmi_disable_frl(). Since frl.enable is true,
this invokes intel_hdmi_frl_ltsl(), which fetches the adapter for pin 0
(GMBUS_PIN_DISABLED), receiving a NULL pointer.

Passing this NULL adapter to drm_scdc_disable_frl() will then cause a crash
when it is subsequently dereferenced in i2c_transfer(). Should there be a
NULL check for the adapter before calling drm_scdc_disable_frl()?

> +	drm_scdc_clear_update_flags(adapter, SCDC_FLT_UPDATE);
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012035806.3361439-1-ankit.k.nautiyal@intel.com?part=15