Re: [PATCH v2 02/56] drm/i915/hdmi: Add new data members for FRL configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: NULL pointer dereference in intel_bios_hdmi_max_frl_rate() via devdata
- [Medium] drm/i915: FRL rate incorrectly clamped to 0 when VBT specifies platform limit
--

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

drm/i915/hdmi: Add new data members for FRL configuration

This commit adds new members to crtc_state and intel_hdmi to identify
FRL-supporting HDMI sinks. It also tracks the FRL training information
after a successful link training.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b6fa1a1558b82..69534eea78a15 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
[ ... ]
> @@ -2545,6 +2547,38 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *_connector)
>  	}
>  }
>  
> +static void
> +intel_hdmi_reset_frl_config(struct intel_hdmi *intel_hdmi)
> +{
> +	intel_hdmi->frl.trained = false;
> +	intel_hdmi->frl.lanes = 0;
> +	intel_hdmi->frl.rate_gbps = 0;
> +	intel_hdmi->frl.ffe_level = 0;
> +}
> +
> +static void
> +intel_hdmi_init_frl_config(struct intel_connector *connector)
> +{
> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> +	struct intel_encoder *encoder = &hdmi_to_dig_port(intel_hdmi)->base;
> +	int max_src_frl_rate =
> +		DIV_ROUND_UP(intel_bios_hdmi_max_frl_rate(encoder->devdata), 1000000) * 4;

[Severity: High]
Is there a risk of a NULL pointer dereference here?

When intel_hdmi_set_edid() detects a digital display, it calls
intel_hdmi_init_frl_config() which unconditionally evaluates encoder->devdata
in the call to intel_bios_hdmi_max_frl_rate().

However, the devdata field in struct intel_encoder is documented as:
/* VBT information for this encoder (may be NULL for older platforms) */

If devdata is NULL, intel_bios_hdmi_max_frl_rate() will immediately
dereference it:

int intel_bios_hdmi_max_frl_rate(const struct intel_bios_encoder_data *devdata)
{
	struct intel_display *display = devdata->display;
    ...

Could this cause a crash upon connecting an HDMI display on platforms that
lack VBT port data?

> +	int max_sink_dsc_frl_rate =
> +		intel_hdmi_sink_dsc_max_frl_rate(&connector->base);
> +	int max_sink_frl_rate =
> +		intel_hdmi_sink_max_frl_rate(&connector->base);
> +
> +	intel_hdmi->has_sink_hdmi_21 = max_sink_frl_rate > 0;
> +
> +	intel_hdmi->max_frl_rate = min(max_sink_frl_rate,
> +				       max_src_frl_rate);

[Severity: Medium]
Does this clamp the maximum FRL rate to 0 Gbps for capable platforms?

When the VBT specifies HDMI_MAX_FRL_RATE_PLATFORM, or when no limit is
provided for platforms other than Meteorlake, intel_bios_hdmi_max_frl_rate()
returns 0.

This 0 value is converted to a max_src_frl_rate of 0, which is then passed
into this min() assignment. As a result, intel_hdmi->max_frl_rate becomes 0.

Will this unintentionally disable FRL on capable platforms that rely on the
default platform limit?

> +
> +	intel_hdmi->max_dsc_frl_rate = min(max_sink_dsc_frl_rate,
> +					   max_src_frl_rate);
> +
> +	intel_hdmi_reset_frl_config(intel_hdmi);
> +}

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