Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm/dp: stale link training parameters artificially restrict modes after hotplug - [Medium] drm/msm/dp: data race and TOCTOU in mode validation reading link_params --
commit 69ad059161eff4e2e43af0ffa4e037a24d8b918c Author: Jean-Francois Bobier <[email protected]> drm/msm/dp: validate modes against the trained link, not the sink's claim This commit updates msm_dp_display_mode_valid() to validate modes against the trained link parameters rather than the sink's advertised capabilities. This prevents advertising modes that the link cannot carry after a training fallback. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index 6abe30bdff120..d1e111c6a5fb8 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > @@ -793,10 +795,42 @@ enum drm_mode_status msm_dp_display_mode_valid(struct > msm_dp *dp, > mode_bpp, link_pclk_khz); > > mode_rate_khz = link_pclk_khz * mode_bpp; > - supported_rate_khz = link_info->num_lanes * link_info->rate * 8; > > - if (mode_rate_khz > supported_rate_khz) > + /* > + * link_info is what the sink advertises in DPCD, which is not > + * necessarily what we managed to train. msm_dp_ctrl_on_link() walks > + * the rate down HBR3 -> HBR2 -> HBR -> RBR and then halves the lane > + * count until training succeeds, so a cable with two unusable lanes > + * ends up carrying half the bandwidth the sink claims. > + * > + * Sizing modes against the sink's maximum then advertises modes the > + * link cannot carry: training "succeeds" at the reduced parameters, > + * the mode does not fit in them, and the sink shows a black screen > + * with nothing in the log to say why. > + * > + * link->link_params holds the trained values, and is reset to the > + * sink's maximum at the top of every msm_dp_ctrl_on_link(), so taking > + * the lower of the two is accurate after a fallback and a no-op > + * before the first training. > + */ > + num_lanes = link_info->num_lanes; > + rate = link_info->rate; > + > + trained = &msm_dp_display->link->link_params; > + if (trained->num_lanes && trained->num_lanes < num_lanes) > + num_lanes = trained->num_lanes; > + if (trained->rate && trained->rate < rate) > + rate = trained->rate; [Severity: High] Will this improperly restrict modes for new monitors after a hotplug? When a monitor is unplugged, msm_dp_hpd_unplug_handle() and msm_dp_link_reset_data() do not clear or invalidate link_params. If the previous monitor fell back to a lower lane count or rate, does this mean the new monitor's modes will be evaluated against the old monitor's degraded parameters? [Severity: Medium] Is it safe to read trained->num_lanes and trained->rate locklessly here? During an asynchronous non-blocking modeset, msm_dp_ctrl_on_link() can modify these values concurrently: drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_on_link() { ... ctrl->link->link_params.rate = rate; ctrl->link->link_params.num_lanes = panel->link_info.num_lanes; ... } Since userspace can concurrently read the EDID and trigger mode validation, could the lack of locking or READ_ONCE() here lead to a data race or a TOCTOU issue where the condition check and subsequent assignment see different values? > + > + supported_rate_khz = num_lanes * rate * 8; > + > + if (mode_rate_khz > supported_rate_khz) { > + drm_dbg_dp(dp->drm_dev, > + "mode %s rejected: needs %u kHz, link carries %u kHz > (%u lanes @ %u)\n", > + mode->name, mode_rate_khz, supported_rate_khz, > + num_lanes, rate); > return MODE_BAD; > + } > > return MODE_OK; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
