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