Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/i915/dp: UAF and panel resource leak on kmemdup_array() failure in intel_dp_init_connector() - [Medium] drm/i915/display/dp: missing cur_link_info update on fastboot state readout - [Medium] drm/i915/dp: inaccurate source and sink lane count caps in sysfs - [Medium] drm/i915/dp: intel_dp_report_link_train() reports wrong DSC state on MST --
commit cf7507ada926b43d170f5de858f0ce21a02b59f4 Author: Kory Maincent <[email protected]> drm/i915/display/dp: Use dp_connector helpers to expose link training state This commit wires up the DRM core DisplayPort sysfs helpers in the i915 driver to report source capabilities, sink capabilities, and active link training parameters under dp_link/. It also stores the unclamped DPCD sink rates in dpcd_sink_rates and num_dpcd_sink_rates so the raw sink rates can be exposed to userspace. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index bbd76c0ff04b6..925d0ec939f55 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c [ ... ] > @@ -6141,6 +6150,20 @@ void intel_dp_dpcd_set_probe(struct intel_dp > *intel_dp, bool force_on_external) > intel_dp_needs_dpcd_probe(intel_dp, > force_on_external)); > } > > +static void intel_dp_set_sink_caps(struct intel_connector *connector, > + struct intel_dp *intel_dp) > +{ > + struct drm_connector_dp_link_caps link_caps; > + > + link_caps.nlanes = drm_dp_get_max_lane_count(intel_dp->dpcd, > + > intel_dp->lttpr_common_caps); > + link_caps.link_rates = intel_dp->dpcd_sink_rates; > + link_caps.nlink_rates = intel_dp->num_dpcd_sink_rates; > + link_caps.dsc = drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd); > + > + drm_dp_sink_set_caps(&connector->base, &link_caps); > +} > + > static int > intel_dp_detect(struct drm_connector *_connector, > struct drm_modeset_acquire_ctx *ctx, [ ... ] > @@ -6243,6 +6268,8 @@ intel_dp_detect(struct drm_connector *_connector, > > intel_dp_print_rates(intel_dp); > > + intel_dp_set_sink_caps(connector, intel_dp); > + > if (intel_dp->is_mst) { > /* > * If we are in MST mode then this connector > @@ -6990,11 +7017,14 @@ intel_dp_init_connector(struct intel_digital_port > *dig_port, > struct intel_connector *connector) > { > struct intel_display *display = to_intel_display(dig_port); > + struct drm_connector_dp_link_caps link_caps; > struct intel_dp *intel_dp = &dig_port->dp; > struct intel_encoder *encoder = &dig_port->base; > struct drm_device *dev = encoder->base.dev; > enum port port = encoder->port; > + int *rates; > int type; > + int ret; > > if (drm_WARN(dev, dig_port->max_lanes < 1, > "Not enough lanes (%d) for DP on [ENCODER:%d:%s]\n", > @@ -7069,6 +7099,27 @@ intel_dp_init_connector(struct intel_digital_port > *dig_port, > intel_dp_set_common_link_params(intel_dp); > intel_dp_reset_link_params(intel_dp); > > + rates = kmemdup_array(intel_dp->source_rates, > + intel_dp->num_source_rates, > + sizeof(*rates), > + GFP_KERNEL); > + if (!rates) { > + intel_dp_aux_fini(intel_dp); > + goto fail; > + } [Severity: High] Can this error path cause a use-after-free of dig_port and leak panel resources when kmemdup_array() fails on an eDP connector? At this point in drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_init_connector(), intel_edp_init_connector() has already succeeded. For eDP, intel_edp_init_connector() enables PPS VDD, initializes connector->panel, and calls intel_pps_init_late() in drivers/gpu/drm/i915/display/intel_pps.c, which schedules intel_dp->pps.panel_vdd_work: drivers/gpu/drm/i915/display/intel_dp.c:intel_edp_init_connector() intel_panel_init(connector, drm_edid); intel_edp_backlight_setup(intel_dp, connector); intel_edp_add_properties(intel_dp); intel_pps_init_late(intel_dp); edp_panel_vdd_schedule_off(intel_dp); Jumping to fail only flushes display->power.domains.async_put_work and cleans up the base DRM connector without cancelling panel_vdd_work or tearing down the panel: drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_init_connector() fail: intel_display_power_flush_work(display); drm_connector_cleanup(&connector->base); return false; When intel_dp_init_connector() returns false, callers such as intel_ddi_init() in drivers/gpu/drm/i915/display/intel_ddi.c and g4x_dp_init() in drivers/gpu/drm/i915/display/g4x_dp.c immediately free connector and dig_port while dig_port->dp.pps.panel_vdd_work is still queued on display->wq.unordered, and intel_dp->pps.vdd_wakeref along with connector->panel resources (fixed_edid, fixed_modes, VBT panel data, and backlight) are leaked. Also, since drm_dp_source_set_caps() already duplicates link_caps.link_rates internally via drm_dp_copy_caps() and its failure is treated as non-fatal below, could link_caps.link_rates pass intel_dp->source_rates directly (or treat the allocation failure as non-fatal) instead of aborting connector init? > + > + link_caps.nlanes = dig_port->max_lanes; > + link_caps.nlink_rates = intel_dp->num_source_rates; > + link_caps.link_rates = rates; > + link_caps.dsc = HAS_DSC(display); > + > + ret = drm_dp_source_set_caps(&connector->base, &link_caps); [Severity: Medium] Should source and sink lane count capabilities account for VBT lane limits and DP tunnel bandwidth allocation mode? In drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_init_connector(), intel_dp->source_rates is clamped to VBT limits by intel_dp_set_source_rates(), but link_caps.nlanes uses raw dig_port->max_lanes instead of intel_dp_max_source_lane_count(dig_port): drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_max_source_lane_count() { int vbt_max_lanes = intel_bios_dp_max_lane_count(dig_port->base.devdata); int max_lanes = dig_port->max_lanes; if (vbt_max_lanes) max_lanes = min(max_lanes, vbt_max_lanes); return max_lanes; } Similarly, in drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_set_sink_caps(), link_caps.link_rates uses intel_dp->dpcd_sink_rates (populated via drm_dp_read_dpcd_sink_rates() with intel_dp->tunnel), whereas link_caps.nlanes calls drm_dp_get_max_lane_count(intel_dp->dpcd, intel_dp->lttpr_common_caps) directly. When USB4/Thunderbolt DP tunnel bandwidth allocation mode is active, intel_dp->dpcd[DP_MAX_LANE_COUNT] can be masked by the tunnel, unlike max_dprx_lane_count(intel_dp) / intel_dp->max_sink_lane_count which queries drm_dp_tunnel_max_dprx_lane_count(intel_dp->tunnel). Would using intel_dp_max_source_lane_count(dig_port) for source_link_caps and intel_dp->max_sink_lane_count (or max_dprx_lane_count(intel_dp)) for sink_link_caps report more accurate lane counts in dp_link/ sysfs? > + if (ret) > + drm_dbg_kms(display->drm, > + "failed to set source link rates, skipping.\n"); > + > + kfree(rates); > + > /* init MST on ports that can support it */ > intel_dp_mst_encoder_init(dig_port, connector->base.base.id); > > 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 cb92cff906146..c77cb124f1419 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -1758,6 +1758,18 @@ static void link_recovery_reset(struct > intel_dp_link_training *link_training) > link_training->recovery_state = INTEL_DP_LINK_RECOVERY_IDLE; > } > > +static void intel_dp_report_link_train(struct intel_dp *intel_dp) > +{ > + struct intel_connector *connector = intel_dp->attached_connector; > + > + if (!connector) > + return; > + > + drm_dp_set_cur_link_params(&connector->base, intel_dp->link_rate, > + intel_dp->lane_count, > + connector->dp.dsc_decompression_enabled); > +} [Severity: Medium] How is cur_link_info_dsc supposed to behave when DisplayPort MST is enabled? In MST mode, drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_stream_pre_enable() enables DSC decompression on the dynamic MST stream connector rather than on intel_dp->attached_connector, and only calls primary_encoder->pre_enable() (which reaches intel_dp_stop_link_train()) for the first stream: drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_stream_pre_enable() intel_dp_sink_enable_decompression(state, connector, pipe_config); if (first_mst_stream) { primary_encoder->pre_enable(state, primary_encoder, pipe_config, NULL); Meanwhile, primary_encoder->pre_enable() skips intel_dp_sink_enable_decompression() when is_mst is true, so intel_dp->attached_connector->dp.dsc_decompression_enabled remains false. As a result, in drivers/gpu/drm/i915/display/intel_dp_link_training.c:intel_dp_report_link_train(), connector->dp.dsc_decompression_enabled is always false for MST links, while the root intel_dp->attached_connector is marked connector_status_disconnected in drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_detect() and the active MST stream connectors have unpopulated dp_link/ sysfs attributes. > + > /** > * intel_dp_stop_link_train - stop link training > * @intel_dp: DP struct > @@ -1787,6 +1799,8 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp, > intel_dp_program_link_training_pattern(intel_dp, crtc_state, > DP_PHY_DPRX, > DP_TRAINING_PATTERN_DISABLE); > > + intel_dp_report_link_train(intel_dp); [Severity: Medium] What happens to cur_link_info when an active DP or eDP link is inherited from firmware via fastboot without a full modeset? During driver probe or resume, hardware state readout runs intel_modeset_setup_hw_state() -> intel_modeset_readout_hw_state(), which calls intel_dp_sync_state() and intel_dp_connector_sync_state() in drivers/gpu/drm/i915/display/intel_dp.c: drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_sync_state() if (crtc_state) { intel_dp_reset_link_params(intel_dp); intel_dp_set_link_params(intel_dp, crtc_state->port_clock, crtc_state->lane_count); intel_dp->link.active = true; } drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_connector_sync_state() if (crtc_state && crtc_state->dsc.compression_enable) { drm_WARN_ON(display->drm, !connector->dp.dsc_decompression_aux); connector->dp.dsc_decompression_enabled = true; } else { connector->dp.dsc_decompression_enabled = false; } When subsequent commits take the fastset path instead of a full modeset, intel_dp_stop_link_train() in drivers/gpu/drm/i915/display/intel_dp_link_training.c is never called. Consequently, drm_dp_set_cur_link_params() is never invoked, leaving connector->dp.cur_link_info.nlanes at 0 and hiding the cur_link_info_* sysfs attributes on an active display. Should drm_dp_set_cur_link_params() also be called during hardware state readout when crtc_state is active? > + > if (intel_dp_is_uhbr(crtc_state)) { > ret = poll_timeout_us(ret = > intel_dp_128b132b_intra_hop(intel_dp, crtc_state), > ret == 0, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
