Re: [PATCH RFC v2 2/4] drm/i915/display/dp: Adopt dp_connector helpers to expose link training state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Kernel panic in drm_connector_cleanup() due to an uninitialized connector when kmemdup_array fails.
- [High] Calling DRM DP helpers from atomic commit tail violates locking contracts and introduces a data race.
- [Medium] The current negotiated link parameters are incorrectly reported as the maximum link parameters.
- [Medium] The source maximum lane count capability is incorrectly hardcoded to 4, misrepresenting actual port capabilities.
- [Medium] The return value of drm_connector_dp_init_with_ddc() is ignored, potentially leaving the connector with missing source capabilities on allocation failure.
- [Medium] The failure path for the rates allocation misses the cleanup for intel_dp_aux_init(), leaking resources.
--

commit 77ebe85e37823b9a8f351840bfca5631c84a3dac
Author: Kory Maincent <kory.maincent@bootlin.com>

drm/i915/display/dp: Adopt dp_connector helpers to expose link training state

This commit switches the i915 DP connector initialization to use
drm_connector_dp_init_with_ddc() to provide source link capabilities.
It also adds intel_dp_report_link_train() to collect and expose
negotiated link parameters via the DRM DP helpers.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5c3e816b01352..5f934257771a7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c

[ ... ]

> @@ -7174,10 +7178,12 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  			struct intel_connector *connector)
>  {

[ ... ]

> +	intel_dp_set_source_rates(intel_dp);
> +	link_caps.nlanes = 4;

[Severity: Medium]
Is it intentional to hardcode the maximum lane count to 4 here?

Some digital encoder ports may be restricted to 1 or 2 lanes depending on
the hardware platform or Type-C port configuration. The actual capability
is stored in dig_port->max_lanes. Hardcoding this to 4 might misrepresent
the actual port capabilities.

> +	link_caps.nlink_rates = intel_dp->num_source_rates;
> +	rates = kmemdup_array(intel_dp->source_rates, intel_dp->num_source_rates,
> +			      sizeof(*rates), GFP_KERNEL);
> +	if (!rates)
> +		goto fail;

[Severity: Critical]
Does this error path trigger a NULL pointer dereference?

At this point in the initialization, drm_connector_dp_init_with_ddc()
hasn't been called yet, so connector->base.dev is still NULL.

The jump to the fail label calls drm_connector_cleanup(&connector->base).
This function unconditionally dereferences the device pointer, which will
cause a kernel panic.

[Severity: Medium]
Does this error path also leak the resources allocated by intel_dp_aux_init()
earlier in this function?

The direct jump to the fail label bypasses the intel_dp_aux_fini(intel_dp)
call, which means the aux channel name allocation and the PM QoS request
will permanently leak.

> +
> +	link_caps.link_rates = rates;
> +	link_caps.dsc = HAS_DSC(display);
> +
> +	drm_connector_dp_init_with_ddc(dev, &connector->base, &intel_dp_connector_funcs,
> +				       &link_caps, type, &intel_dp->aux.ddc);

[Severity: Medium]
Should we check the return value here?

drm_connector_dp_init_with_ddc() can fail with -ENOMEM when allocating the
link capabilities array internally. If it fails, the connector will proceed
partially initialized without its source capabilities.

> +	kfree(rates);
> +
>  	drm_connector_helper_add(&connector->base, &intel_dp_connector_helper_funcs);

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index 54c585c59b900..aaa6eba058af5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1116,6 +1116,18 @@ intel_dp_128b132b_intra_hop(struct intel_dp *intel_dp,
>  	return sink_status & DP_INTRA_HOP_AUX_REPLY_INDICATION ? 1 : 0;
>  }
>  
> +static void intel_dp_report_link_train(struct intel_dp *intel_dp)
> +{
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +
> +	drm_dp_set_max_link_params(&connector->base, intel_dp->link_rate,
> +				   intel_dp->lane_count);

[Severity: Medium]
Should this report the architectural maximums instead of the currently
negotiated parameters?

intel_dp->link_rate and intel_dp->lane_count reflect the current link
configuration, which might have been downgraded due to training failures
or signal degradation. The hardware maximum capabilities are stored in
intel_dp->link.max_rate and intel_dp->link.max_lane_count.

[Severity: High]
Can this helper be safely called from this context?

drm_dp_set_max_link_params() and drm_dp_set_cur_link_params() both require
the connection_mutex to be held. However, intel_dp_stop_link_train()
(which calls this) executes during the hardware enable phase (commit_tail).

For non-blocking commits, this runs asynchronously in a workqueue after
the modeset locks have already been released. This will trigger a lockdep
warning and could race against concurrent userspace sysfs readers.

> +
> +	drm_dp_set_cur_link_params(&connector->base, intel_dp->link_rate,
> +				   intel_dp->lane_count,
> +				   connector->dp.dsc_decompression_enabled);
> +}

[ ... ]

> @@ -1652,6 +1667,8 @@ void intel_dp_start_link_train(struct intel_atomic_state *state,
>  	 */
>  	int lttpr_count;
>  
> +	drm_dp_sink_set_link_caps(&intel_dp->attached_connector->base, &intel_dp->aux);

[Severity: High]
Similar to the helpers above, drm_dp_sink_set_link_caps() explicitly warns
if the connection_mutex is not locked.

Since intel_dp_start_link_train() is executed from the asynchronous
commit_tail for non-blocking commits, the modeset locks are already
released. Can this cause a lockdep warning and data race during asynchronous
commits?

> +
>  	intel_hpd_block(encoder);
>  
>  	lttpr_count = intel_dp_init_lttpr_and_dprx_caps(intel_dp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-feat_link_cap-v2-0-a3dec4c02ad9@bootlin.com?part=2