Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Concurrent execution of DP link training and hardware programming between hpd_work and DRM atomic commits corrupts hardware state. - [High] Bypassing atomic KMS for link retraining on a cable replug forcefully applies stale display timings to potentially new monitors. - [High] Use-After-Free of the driver's private data structure due to uncanceled delayed work on driver unbind. Pre-existing issues: - [High] The DP lane count is permanently crippled when a display with fewer lanes is connected. -- --- Patch [2]: [PATCH 2/2] drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug --- commit 87e56572108019fd06745308967a4ab03655f77f Author: Yashas D <[email protected]> drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug When a cable is replugged while the upstream display pipeline is still active, the bridge can retrain the DP link and re-enable the video stream directly from the HPD interrupt work handler. > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index d9bd4ef8f0e21..f6f930ca1519d 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c [ ... ] > +static void ti_sn_bridge_hpd_work(struct work_struct *work) > +{ > + struct ti_sn65dsi86 *pdata = > + container_of(work, struct ti_sn65dsi86, hpd_work); [ ... ] > + ret = ti_sn_bridge_link_train(pdata, bpp, NULL); > + if (ret) > + goto notify; > + > + ti_sn_bridge_set_video_timings(pdata, NULL); > + mutex_lock(&pdata->hpd_mutex); > + if (pdata->bridge_enabled) > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > + VSTREAM_ENABLE, VSTREAM_ENABLE); > + mutex_unlock(&pdata->hpd_mutex); [Severity: High] Could this lockless invocation of ti_sn_bridge_link_train() race with a DRM atomic commit? If userspace reacts to the hotplug event and issues a modeset, ti_sn_bridge_atomic_disable() clears the enabled flag but doesn't cancel the running work. Then ti_sn_bridge_atomic_enable() executes link training concurrently without locking. This seems like it could cause both threads to race while programming complex DP timing and PLL registers simultaneously, leading to hardware state machine corruption. [Severity: High] Does bypassing atomic KMS for link retraining here forcefully apply stale display timings to a potentially new monitor? If a user unplugs the current monitor and connects a different one with a different EDID and capabilities, forcing the old cached_mode into the hardware could drive out-of-spec timings to the newly plugged display. Shouldn't we rely on userspace to read the new EDID via hotplug events and orchestrate a proper modeset? [ ... ] > +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_atomic_commit *state) > +{ > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > + struct drm_connector *connector; > + int max_dp_lanes; > + unsigned int bpp; > + int ret; > + > + connector = drm_atomic_get_new_connector_for_encoder(state, > + bridge->encoder); > + if (!connector) { > + dev_err_ratelimited(pdata->dev, "Could not get the > connector\n"); > + return; > + } > + > + max_dp_lanes = ti_sn_get_max_lanes(pdata); > + mutex_lock(&pdata->hpd_mutex); > + pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes); [Severity: High] This is a pre-existing issue, but does this in-place min() reduction permanently mutate the driver's maximum allowed lane capability based on the current peripheral sink? If a 2-lane monitor is connected, pdata->dp_lanes shrinks to 2. Connecting a 4-lane capable monitor afterward would be artificially limited to 2 lanes since the original physical board maximum is lost. [ ... ] > @@ -2050,6 +2192,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client) > > mutex_init(&pdata->hpd_mutex); > mutex_init(&pdata->comms_mutex); > + INIT_WORK(&pdata->hpd_work, ti_sn_bridge_hpd_work); > > pdata->regmap = devm_regmap_init_i2c(client, > &ti_sn65dsi86_regmap_config); [Severity: High] Is there a chance this work item can outlive the driver's private data structure during unbind? Since the work is initialized here but no devm cleanup action is registered to cancel it on teardown, if a hardware interrupt schedules the work just before the driver is unbound, devres will free the pdata memory. The system workqueue could then execute ti_sn_bridge_hpd_work() and dereference the freed memory. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
